Report RegExp errors in grammar check, use Annex B grammar #58295
Conversation
rbuckton
requested review from
DanielRosenwasser,
andrewbranch,
jakebailey and
RyanCavanaugh
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Theoretically this should net some "missing" errors @typescript-bot user test this |
|
@jakebailey Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
|
FYI the cause of the CI error was modified here: |
|
In general, I think there might be a happy medium between general specification strictness and Annex B looseness. For example, we probably want to treat Cases like Most of the other Annex B checks we may want to just leave out in favor of stricter parsing as they generally indicate invalid syntax that you're less likely to actually come across in practice. The upside of doing this during the grammar check pass is that if a user really wants to use that syntax then they have the option to annotate the line with There are a few other things I want to explore either in this PR or another follow up PR to cut down on the number |
|
I'll discuss whether we should use stricter rules than Annex B in the next Design Meeting, with any changes coming in a follow-up PR. |
|
@typescript-bot perf test |
|
Hey @rbuckton, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: codemirror |
|
Hm, crash on the DT code, it looks. Definitely relevant. |
|
@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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Not source mapped, so that's less than helpful :/ |
|
The assert is from: $ NODE_OPTIONS=--enable-source-maps pnpm dtslint --localTs ~/work/TypeScript/built/local types codemirror
dtslint@0.2.20
Error: Debug Failure. False expression.
Occurred while linting /home/jabaile/work/DefinitelyTyped/types/codemirror/test/addon/mode/simple.ts:1
Rule: "@definitelytyped/expect"
at checkGrammarRegularExpressionLiteral (/home/jabaile/work/TypeScript/src/compiler/checker.ts:31382:23)
at <anonymous> (/home/jabaile/work/TypeScript/src/compiler/checker.ts:31397:37)
at addLazyDiagnostic (/home/jabaile/work/TypeScript/src/compiler/checker.ts:47223:35)
at checkRegularExpressionLiteral (/home/jabaile/work/TypeScript/src/compiler/checker.ts:31397:13)
at checkExpressionWorker (/home/jabaile/work/TypeScript/src/compiler/checker.ts:39712:24)
at checkExpression (/home/jabaile/work/TypeScript/src/compiler/checker.ts:39621:36)
at checkExpressionForMutableLocation (/home/jabaile/work/TypeScript/src/compiler/checker.ts:39357:22)
at checkPropertyAssignment (/home/jabaile/work/TypeScript/src/compiler/checker.ts:39371:16)
at checkObjectLiteral (/home/jabaile/work/TypeScript/src/compiler/checker.ts:31658:80)
at checkExpressionWorker (/home/jabaile/work/TypeScript/src/compiler/checker.ts:39716:24)Repro instructions are: $ git clone --filter=blob:none https://github.com/DefinitelyTyped/DefinitelyTyped.git
$ cd DefinitelyTyped
$ pnpm i --filter . --filter '...{./types/codemirror}...'
$ NODE_OPTIONS=--enable-source-maps pnpm dtslint --localTs ~/work/TypeScript/built/local types codemirror |
I was able to repro it locally. I was lamenting the lack of source maps, but I should have tagged the assert with an appropriate message anyways. The assert was overaggressive since we can also rescan |
|
@typescript-bot run dt |
|
Hey @rbuckton, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham, @DanielRosenwasser, any other comments? |
|
|
||
| function checkRegularExpressionLiteral(node: RegularExpressionLiteral) { | ||
| const nodeLinks = getNodeLinks(node); | ||
| if (!nodeLinks.grammarChecked) { |
There was a problem hiding this comment.
I think you can actually reuse NodeCheckFlags.TypeChecked for this, if you'd like. It's how we do similar marking for getter/setter pairs, and nodes with a checkNodeDeferred component.
Does that mean we have no test with |
Apparently not. I'll add one. |
|
@weswigham why does it have to be a lazy diagnostic though? Where does the order-checking impact anything else here? |
It allows us to completely skip the rescan when, eg, requesting completions in the file. |
We will eventually want to always perform the scan if we want to leverage information like group names or capture group counting to infer stronger types for |
|
Okay, I guess don't have a strong opinion about it. It does feel a little premature since ideally a scan should be fast enough, but it's no big deal for now. Are we good with merging? |
I'm mostly waiting on the macos tests to catch up, but they're lagging about 4hrs behind right now. Nothing in this PR should be OS dependent, so I'm fine with merging if we don't want to wait. |
|
After this PR, these lines no longer work and might happen to break something as the early exit causes the token span and value to be different depending on |
The change is no longer necessary since it’s moved to `checker.ts` in microsoft#58295. The partially reverts microsoft@1a5228d.
This moves extended regular expression syntax checks to a grammar check. This ensures that RegExp grammar checks don't prevent type checking and allows them to be overridden via
// @ts-ignorewhen necessary.This also weakens the grammar check to use the productions and semantics in Annex B. This uses a
booleanvalue to choose whether to use Annex B grammar in case we want to make this configurable, but for now the only place this is defined is guaranteed to betrue.Related #58287