- Notifications
You must be signed in to change notification settings - Fork768
Suggested changes to #2026 (Implement RegExp literal syntax checking)#2107
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:jabaile/regexp-errors
Are you sure you want to change the base?
Suggested changes to #2026 (Implement RegExp literal syntax checking)#2107
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Just for consistencyPartially portsmicrosoft/TypeScript@c44a057
Fixes: no error on `/((?<foo>))(?<foo>)/`Portsmicrosoft/TypeScript@c435526#diff-65d73a953f046cf34092e70dd4a376ad5c885da833e750f88afeca31b6d63926
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 file is frommicrosoft/TypeScript#60249; I should've extract the changes out as a separate Strada PR
| }else { | ||
| v.error(diagnostics.Unexpected_0_Did_you_mean_to_escape_it_with_backslash,v.pos,1,string(ch)) |
graphemeclusterNov 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 is removed in order to partially get rid ofmicrosoft/TypeScript#62707 (not a full fix). I won't portmicrosoft/TypeScript#62716 until it gets a minimal review
| atomStart:=v.pos | ||
| atom:=v.scanClassAtom() | ||
| ifv.charAtOffset(0)=='-'&&v.charAtOffset(1)!=']'{ | ||
| ifv.charAtOffset(0)=='-' { |
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.
Redundant check
| } | ||
| ifv.charAtOffset(0)=='}' { | ||
| v.pos++ | ||
| }elseifhasDigits{ |
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.
Redundant check
| ifv.anyUnicodeModeOrNonAnnexB { | ||
| v.error(diagnostics.X_c_must_be_followed_by_an_ASCII_letter,v.pos-2,2) | ||
| }elseifatomEscape{ | ||
| }else { |
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 is weird, but in Annex B,\c are really two characters regardless of whether it is an atom escape. I believe it is due to theClassAtomNoDash production.
Also some changes frommicrosoft/TypeScript#60249; didn't file a separate PR due to insignificance.
| ifch!=0&&(scanner.IsIdentifierStart(ch)||ch=='_'||ch=='$') { | ||
| ifch!=0&&scanner.IsIdentifierStart(ch) { | ||
| v.pos++ | ||
| forv.pos<v.end { | ||
| ch=v.charAtOffset(0) | ||
| ifscanner.IsIdentifierPart(ch)||ch=='_'||ch=='$'{ | ||
| ifscanner.IsIdentifierPart(ch) { |
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.
Again, some redundant checks
| propertyNameOrValueStart:=v.pos | ||
| v.scanIdentifier(v.charAtOffset(0)) | ||
| v.scanWordCharacters(v.charAtOffset(0)) |
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.
I'm not sure if your original intention in relaxing the checks was to suppress the "Expected '}'" error when the} appears after a several exotic identifier characters. If so, you might want to revert that commit.
Uh oh!
There was an error while loading.Please reload this page.
The merge target of this PR is
jabaile/regexp-errors, notmain. It is intended as a supplement of#2026.Feel free to fast-forward/cherry-pick/rebase some of the commits here.