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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve escape sequence handling in private names #50856
Improve escape sequence handling in private names #50856
Changes from all commits
77d7109
3f8b934
69195a9
fec0977
6fccba5
83b3ea3
82d0fc9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks consistent with unicode escape handling elsewhere in the scanner, but I'm not sure I understand why
scanIdentifier
doesn't handle them. Is it illegal to start an identifier with an escape sequence? (Maybe that would introduce an ambiguity, but none jumps to mind.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I reading this correctly? It looks like any identifier can start with a unicode escape. If so, why wouldn't the fix be in
scanIdentifier
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielRosenwasser Last question ⬆️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scanIdentifier
could handle them if we just checked if the first character was either an identifier start or a\
, but right now the logic is to not advance if we can't get at least one complete identifier start.This was meant to be consistent with identifiers. Right now the handling for any of the following incomplete escape sequences...
is to not munch up these characters, and identify the
\
as an unknown token, followed by whatever.As an extension, what the current code does with private fields is to make each of
an incomplete private field with the name
#
, followed by an unknown token\
followed by whatever.Arguably, private fields could diverge here for some better errors.