Improve Recovery of Unterminated Regular Expressions #58289
Merged
Conversation
This file contains 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
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
graphemecluster
commented
Apr 22, 2024
| @@ -2389,7 +2390,8 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean | |||
| function reScanSlashToken(): SyntaxKind { | |||
| if (token === SyntaxKind.SlashToken || token === SyntaxKind.SlashEqualsToken) { | |||
| // Quickly get to the end of regex such that we know the flags | |||
| let p = tokenStart + 1; | |||
There was a problem hiding this comment.
This is eliminated for readability
graphemecluster
commented
Apr 22, 2024
There was a problem hiding this comment.
I know this is bad. Are there any solutions that does not affect pressing enter?
graphemecluster
commented
Apr 22, 2024
| @@ -49,5 +49,5 @@ file.tsx(11,20): error TS1161: Unterminated regular expression literal. | |||
| !!! error TS2304: Cannot find name 'data'. | |||
| ~ | |||
| !!! error TS1005: ';' expected. | |||
| ~~ | |||
There was a problem hiding this comment.
Should we include angle brackets (in addition to parens and braces) to the logic?
There was a problem hiding this comment.
With respect to treating them as unterminated? I don't think that's necessary. (?<) will still produce an error during the grammar check pass while still maintaining a proper bound for the RegExp body.
Also, Annex B does not treat /\k</ as an error since the RegExp does not define a named capture group, which indicates that <> handling is not a lexical syntax error.
There was a problem hiding this comment.
I'm not too terribly concerned about this case, to be honest. Parsing falls off the rails here because of malformed JSX, not a malformed RegExp.
graphemecluster
commented
Apr 22, 2024
graphemecluster
commented
Apr 22, 2024
This is now recovered.
The change is no longer necessary since it’s moved to `checker.ts` in microsoft#58295. The partially reverts microsoft@1a5228d.
|
Comments marked “Outdated” are not actually outdated, they’re just due to the merge |
|
Please inform me if you would like me to move the last commit to #58320 or a new PR. |
It seems unrelated, right? So should probably be separate? |
|
#58339 also addressed some of this. Can you update your PR with the latest changes from |
OK, I’ll do so if you don’t mind the number of PRs. |
rbuckton
requested changes
May 16, 2024
| @@ -49,5 +49,5 @@ file.tsx(11,20): error TS1161: Unterminated regular expression literal. | |||
| !!! error TS2304: Cannot find name 'data'. | |||
| ~ | |||
| !!! error TS1005: ';' expected. | |||
| ~~ | |||
There was a problem hiding this comment.
With respect to treating them as unterminated? I don't think that's necessary. (?<) will still produce an error during the grammar check pass while still maintaining a proper bound for the RegExp body.
Also, Annex B does not treat /\k</ as an error since the RegExp does not define a named capture group, which indicates that <> handling is not a lexical syntax error.
rbuckton
requested changes
May 20, 2024
This reverts commit e67692a.
|
@jakebailey I’ve moved the changes related to flags scanning to #58612. After either PR is merged I will resolve the conflict of the other one. |
rbuckton
reviewed
May 23, 2024
|
@typescript-bot perf test |
|
Starting jobs; this comment will be updated as builds start and complete.
|
|
@rbuckton Here are the results of running the user tests comparing Everything looks good! |
|
Hey @rbuckton, the results of running the DT tests are ready. Everything looks the same! |
|
@rbuckton Here are the results of running the user tests comparing Everything looks good! |
|
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
reviewed
May 23, 2024
|
@rbuckton Here are the results of running the top 400 repos comparing Everything looks good! |
|
@rbuckton Here are the results of running the top 200 repos comparing Something interesting changed - please have a look. DetailsServer exited prematurely with code unknown and signal SIGABRTAffected reposbackstage/backstageRaw error text:RepoResults8/backstage.backstage.rawError.txt in the artifact folder Replay commands: RepoResults8/backstage.backstage.replay.txt in the artifact folder
Last few requests{"seq":38,"type":"request","command":"definitionAndBoundSpan","arguments":{"file":"@PROJECT_ROOT@/scripts/create-github-release.js","line":93,"offset":42}}
{"seq":39,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/scripts/create-github-release.js","line":193,"offset":4,"includeExternalModuleExports":false,"triggerKind":1}}
{"seq":40,"type":"request","command":"completionEntryDetails","arguments":{"file":"@PROJECT_ROOT@/scripts/create-github-release.js","line":193,"offset":4,"entryNames":["arguments"]}}
{"seq":41,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/packages/techdocs-cli/cli-e2e-test.config.js","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps#!/bin/bash
git clone https://github.com/backstage/backstage --recurse-submodules
git -C "./backstage" reset --hard 024b530575d5d9d4d28d53db56d245d81a8c413c
# Install packages (exact steps are below, but it might be easier to follow the repo readme)
yarn --cwd "/mnt/ts_downloads/base/backstage" install --no-immutable --mode=skip-build
yarn --cwd "/mnt/ts_downloads/base/backstage/storybook" install --no-immutable --mode=skip-build
yarn --cwd "/mnt/ts_downloads/base/backstage/microsite" install --no-immutable --mode=skip-build
downloadUrl=$(curl -s "https://typescript.visualstudio.com/TypeScript/_apis/build/builds/161891/artifacts?artifactName=RepoResults8&api-version=7.0" | jq -r ".resource.downloadUrl")
wget -O RepoResults8.zip "$downloadUrl"
unzip -p RepoResults8.zip RepoResults8/backstage.backstage.replay.txt > backstage.backstage.replay.txt
npm install --no-save @typescript/server-replayTo run the repro: # `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.
npx tsreplay ./backstage ./backstage.backstage.replay.txt <PATH_TO_tsserver.js>Server exited prematurely with code unknown and signal SIGABRTAffected reposcalcom/cal.comRaw error text:RepoResults7/calcom.cal.com.rawError.txt in the artifact folder Replay commands: RepoResults7/calcom.cal.com.replay.txt in the artifact folder
Last few requests{"seq":54,"type":"request","command":"navbar","arguments":{"file":"@PROJECT_ROOT@/packages/trpc/index.ts"}}
{"seq":55,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/packages/trpc/index.ts","line":1,"offset":7,"includeExternalModuleExports":false,"triggerKind":2,"triggerCharacter":" "}}
{"seq":56,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/checkly.config.ts"],"openFiles":[]}}
{"seq":57,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/packages/prisma/zod-utils.test.ts","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps#!/bin/bash
git clone https://github.com/calcom/cal.com --recurse-submodules
git -C "./cal.com" reset --hard 64845c670da99c16c12a794ef5695dade0d7488a
yarn --cwd "/mnt/ts_downloads/base/cal.com" install --no-immutable --mode=skip-build
downloadUrl=$(curl -s "https://typescript.visualstudio.com/TypeScript/_apis/build/builds/161891/artifacts?artifactName=RepoResults7&api-version=7.0" | jq -r ".resource.downloadUrl")
wget -O RepoResults7.zip "$downloadUrl"
unzip -p RepoResults7.zip RepoResults7/calcom.cal.com.replay.txt > calcom.cal.com.replay.txt
npm install --no-save @typescript/server-replayTo run the repro: # `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.
npx tsreplay ./cal.com ./calcom.cal.com.replay.txt <PATH_TO_tsserver.js>Server exited prematurely with code unknown and signal SIGABRTAffected reposelastic/kibanaRaw error text:RepoResults14/elastic.kibana.rawError.txt in the artifact folder Replay commands: RepoResults14/elastic.kibana.replay.txt in the artifact folder
Last few requests{"seq":805,"type":"request","command":"getOutliningSpans","arguments":{"file":"@PROJECT_ROOT@/x-pack/plugins/security_solution/common/translations.ts"}}
{"seq":806,"type":"request","command":"navtree","arguments":{"file":"@PROJECT_ROOT@/x-pack/plugins/security_solution/common/translations.ts"}}
{"seq":807,"type":"request","command":"navbar","arguments":{"file":"@PROJECT_ROOT@/x-pack/plugins/security_solution/common/translations.ts"}}
{"seq":808,"type":"request","command":"navto","arguments":{"searchValue":"a","maxResultCount":256}}
Repro steps#!/bin/bash
git clone https://github.com/elastic/kibana --recurse-submodules
git -C "./kibana" reset --hard b1916090d03cb4038d0ae25507968dff0109a964
downloadUrl=$(curl -s "https://typescript.visualstudio.com/TypeScript/_apis/build/builds/161891/artifacts?artifactName=RepoResults14&api-version=7.0" | jq -r ".resource.downloadUrl")
wget -O RepoResults14.zip "$downloadUrl"
unzip -p RepoResults14.zip RepoResults14/elastic.kibana.replay.txt > elastic.kibana.replay.txt
npm install --no-save @typescript/server-replayTo run the repro: # `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.
npx tsreplay ./kibana ./elastic.kibana.replay.txt <PATH_TO_tsserver.js> |
graphemecluster
force-pushed
the
regex-recovery
branch
from
ad3611e
to
77235de
Compare
rbuckton
approved these changes
May 24, 2024
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.
Since unterminated regexes are invalid that they don’t even form a complete
RegularExpressionLiteral, we can do everything at our discretion as long as a syntax error is produced. But we should do all our best to parse the remaining parts with fewer subsequent syntax errors.Instead of consuming all until the end of the line starting from the slash character, if we encounter an unbalanced bracket, which is likely a bracket balancing something before the regex, we stop scanning the regex here so as to increase the chance that a bracket forms a pair of syntax characters with the corresponding bracket before the regex.
Now that unterminated regexes are handled separately from the well-terminated ones and are no longer scanned (actually, parsed) by
scanRegularExpressionWorker. This should make movingscanRegularExpressionWorkeraway to the parser or the checker in the future much easier.#55600 (comment) no longer applies.