Add Token.isLiteralIdentifier - #2662
Merged
Merged
Conversation
The tokenizer represents `true`, `false`, and `nil` as identifiers rather than
keywords, so any rule that treats an identifier as a name referring to something
has to remember to exclude them. That check was spelled out by hand in several
places, including inside ParsingHelpers itself.
Names the fact once, next to `isNumber` and the other token predicates, so it is
discoverable rather than rediscovered — the `preferLazyMap` rule initially missed
it and rejected every closure body containing a boolean literal.
Adopted at the two sites where it is a drop-in. The remaining sites match these
literals as `switch` patterns (`case .identifier("true"), ...`), where spelling
them out reads at least as clearly as a `where` clause would, so they are left
alone. `redundantType` deliberately keeps its own `true`/`false`-only check,
since `nil` has no inferable type and must not be folded in.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2662 +/- ##
===========================================
- Coverage 95.47% 95.46% -0.01%
===========================================
Files 178 178
Lines 27186 27189 +3
===========================================
+ Hits 25955 25956 +1
- Misses 1231 1233 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Small readability change: names the fact that
true,false, andnilare tokenized as identifiers rather than keywords, so rules don't each have to remember it.Reasoning
Any rule that treats an identifier as a name referring to something has to exclude these three, and that check was written out by hand in a handful of places — including inside
ParsingHelpersitself:This is easy to miss, which is the motivation: while writing
preferLazyMapI did miss it, and the rule initially rejected any closure body containing a boolean literal. Putting the predicate next toisNumberand the other token checks makes it discoverable instead of rediscovered.Adopted at the two sites where it's a drop-in replacement. The other sites match these literals as
switchpatterns (case .identifier("true"), .identifier("false"), ...), where spelling them out reads at least as clearly as awhereclause would, so I left them alone.redundantTypekeeps its owntrue/false-only check on purpose —nilhas no inferable type and shouldn't be folded in.Full test suite passes.