Restore switch expression in EscapeIdentifier and add test coverage - #151
Merged
Merged
Conversation
Follow-up to #142, which fixed the crash on identifiers named after C# keywords (#147) but converted the switch expression to a switch statement. Convert it back to a switch expression with the new cases included, with the `or` patterns wrapped across lines for readability. The keyword set is unchanged (verified identical, 77 keywords). Add EscapeIdentifierTests covering: - every reserved keyword Roslyn knows about, asserting both that it gets escaped and that the result parses back as an IdentifierToken, so the list cannot silently drift from the language - non-keywords, including contextual keywords like `var` and `record`, which must not be escaped - end-to-end conversion of method, constructor and lambda parameters and field accesses named after keywords, plus a check that the emitted C# parses without diagnostics The undocumented typed-reference keywords (__arglist, __makeref, __reftype, __refvalue) are excluded from the exhaustive check, as they are not valid Java identifiers and so cannot reach EscapeIdentifier from parsed source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Follow-up to #142 (thanks @lordmilko!), which fixed the crash on identifiers named after C# keywords (#147) but converted the switch expression into a switch statement.
Changes
Restore the switch expression. Same 77 keywords as merged in #142 — verified identical by diffing the extracted keyword sets — just written back as a switch expression with the
orpatterns split across lines so the list stays readable.Add
EscapeIdentifierTests. #142 had no tests, so this adds them:SyntaxFacts.GetReservedKeywordKinds()rather than a hand-maintained list, asserting each one is escaped and that the result parses back as anIdentifierToken. This is the test that matters: it fails if the language ever gains a keyword we don't escape, so the list can't silently drift.var,recordandvalue, which are legal identifiers and must not be escaped.Structure structcrash), constructor parameters, lambda parameters and field accesses named after keywords, plus a check that the emitted C# parses with zero diagnostics.Confirmed the tests are meaningful: against the pre-#142 keyword list they produce 113 failures; against master they all pass.
Note
The exhaustive check excludes the four undocumented typed-reference keywords (
__arglist,__makeref,__reftype,__refvalue). Roslyn reports them as reserved, but they aren't on the MS keywords page and aren't valid Java identifiers, so they can't reachEscapeIdentifierfrom a parsed source file.@-escaping them doesn't produce an identifier token either, so treating them as escapable would be wrong.Full suite: 247 passing.
🤖 Generated with Claude Code