Skip to content

fix(54760): Report error for 'declare type' followed by newline #54761

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

Merged
merged 6 commits into from
Jul 20, 2023

Conversation

strager
Copy link
Contributor

@strager strager commented Jun 24, 2023

The following TypeScript code does not report a diagnostic. Instead, the TypeScript copmiler parses the code as if ASI was applied between two tokens on the same line:

var declare, type, T;

declare /*ASI*/ type /*ASI*/
T = null;

I think this behavior is confusing. TypeScript v4.3.5 used to report a diagnostic for this code, which I think is better than the current behavior of allowing this code without any diagnostic.

Instead of performing ASI between 'declare' and 'type', report a diagnostic telling the user that a newline shouldn't be written after 'type'.

Fixes #54760

The following TypeScript code does not report a diagnostic. Instead,
the TypeScript copmiler parses the code as if ASI was applied between
two tokens on the same line:

    var declare, type, T;

    declare /*ASI*/ type /*ASI*/
    T = null;

I think this behavior is confusing. TypeScript v4.3.5 used to report a
diagnostic for this code, which I think is better than the current
behavior of allowing this code without any diagnostic.

Instead of performing ASI between 'declare' and 'type', report a
diagnostic telling the user that a newline shouldn't be written after
'type'.
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jun 24, 2023
Comment on lines +7 to +10
declare type /*unexpected newline*/
T1 = null;
~~
!!! error TS1142: Line break not permitted here.
Copy link
Contributor Author

@strager strager Jun 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for reviewers: I don't like the placement of the diagnostic, but I don't know TypeScript diagnostic conventions. Is here fine or should it point to the type keyword on the previous line?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can give a specialized diagnostic, but I think this is actually fine as-is.

@DanielRosenwasser
Copy link
Member

This looks good but...shouldn't there be a .types and .symbols baseline generated? Any ideas @weswigham @sandersn?

@jakebailey
Copy link
Member

This looks good but...shouldn't there be a .types and .symbols baseline generated?

The test has // @noTypesAndSymbols: true set since it's just testing the parser.

@DanielRosenwasser
Copy link
Member

Ah - maybe I'm being a stickler, but if we turn that off and switch the line to

- var declare, type, T;
+ var declare: string, type: number, T: boolean;

we can get a slightly better sense of how TypeScript itself is parsing.

It would also be good to detect whether or not T1 and T2 can be resolved correctly.

+ var x: T1, y: T2;

@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 27, 2023
@strager
Copy link
Contributor Author

strager commented Jun 27, 2023

It would also be good to detect whether or not T1 and T2 can be resolved correctly.

Good idea. I updated this pull request. I also included an explanation for each test case.

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Though you'll need to accept baselines.

@@ -7093,13 +7093,20 @@ namespace Parser {
case SyntaxKind.PrivateKeyword:
case SyntaxKind.ProtectedKeyword:
case SyntaxKind.PublicKeyword:
case SyntaxKind.ReadonlyKeyword:
case SyntaxKind.ReadonlyKeyword: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I know it helps with scope hygiene, but since the rest of the codebase avoids it, I'd rather we didn't add braces unless necessary.

Suggested change
case SyntaxKind.ReadonlyKeyword: {
case SyntaxKind.ReadonlyKeyword:

continue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}

@strager
Copy link
Contributor Author

strager commented Jul 18, 2023

It looks like this pull request was approved. When can I expect it to be merged?

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I would probably consider making that change suggested by Daniel.

Usually it's the assignee that drives a PR to completion but I'm happy to click the button for parser changes like this.

@jakebailey
Copy link
Member

@strager If you don't have the time to make the change, I'm happy to push it onto the PR.

@jakebailey
Copy link
Member

Almost forgot

@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 19, 2023

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at f2190c4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 19, 2023

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at f2190c4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 19, 2023

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at f2190c4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 19, 2023

Heya @jakebailey, I've started to run the abridged perf test suite on this PR at f2190c4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 19, 2023

Heya @jakebailey, I've started to run the extended test suite on this PR at f2190c4. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..54761

Metric main 54761 Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 366,178k (± 0.00%) 366,200k (± 0.00%) +22k (+ 0.01%) 366,178k 366,214k p=0.031 n=6
Parse Time 3.56s (± 0.37%) 3.55s (± 0.68%) ~ 3.52s 3.58s p=0.935 n=6
Bind Time 1.19s (± 0.63%) 1.18s (± 0.69%) ~ 1.17s 1.19s p=0.383 n=6
Check Time 9.64s (± 0.15%) 9.70s (± 0.59%) ~ 9.63s 9.79s p=0.076 n=6
Emit Time 7.92s (± 0.47%) 7.99s (± 0.61%) +0.07s (+ 0.86%) 7.94s 8.08s p=0.024 n=6
Total Time 22.31s (± 0.14%) 22.42s (± 0.40%) +0.11s (+ 0.50%) 22.31s 22.54s p=0.036 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 192,911k (± 0.04%) 194,027k (± 0.87%) ~ 192,889k 196,314k p=0.261 n=6
Parse Time 1.58s (± 1.44%) 1.60s (± 0.65%) ~ 1.58s 1.61s p=0.249 n=6
Bind Time 0.82s (± 0.92%) 0.83s (± 0.66%) ~ 0.82s 0.83s p=0.476 n=6
Check Time 10.11s (± 0.96%) 10.16s (± 0.52%) ~ 10.07s 10.21s p=0.335 n=6
Emit Time 3.00s (± 0.83%) 3.01s (± 0.78%) ~ 2.98s 3.04s p=0.871 n=6
Total Time 15.52s (± 0.60%) 15.59s (± 0.32%) ~ 15.53s 15.66s p=0.108 n=6
Monaco - node (v16.17.1, x64)
Memory used 346,172k (± 0.00%) 346,185k (± 0.00%) ~ 346,160k 346,207k p=0.128 n=6
Parse Time 2.76s (± 0.46%) 2.77s (± 0.38%) ~ 2.76s 2.79s p=0.071 n=6
Bind Time 1.08s (± 0.48%) 1.08s (± 0.51%) ~ 1.07s 1.08s p=0.640 n=6
Check Time 7.98s (± 0.24%) 8.01s (± 0.46%) ~ 7.97s 8.07s p=0.222 n=6
Emit Time 4.43s (± 0.22%) 4.45s (± 0.24%) ~ 4.43s 4.46s p=0.066 n=6
Total Time 16.25s (± 0.19%) 16.30s (± 0.26%) ~ 16.25s 16.36s p=0.106 n=6
TFS - node (v16.17.1, x64)
Memory used 300,242k (± 0.01%) 300,227k (± 0.01%) ~ 300,193k 300,255k p=0.423 n=6
Parse Time 2.19s (± 0.37%) 2.21s (± 0.37%) +0.03s (+ 1.22%) 2.20s 2.22s p=0.006 n=6
Bind Time 1.20s (± 0.97%) 1.20s (± 1.18%) ~ 1.18s 1.22s p=0.934 n=6
Check Time 7.30s (± 0.37%) 7.30s (± 0.44%) ~ 7.26s 7.35s p=1.000 n=6
Emit Time 4.34s (± 0.43%) 4.36s (± 0.78%) ~ 4.32s 4.40s p=0.182 n=6
Total Time 15.03s (± 0.26%) 15.08s (± 0.37%) ~ 15.02s 15.17s p=0.077 n=6
material-ui - node (v16.17.1, x64)
Memory used 481,669k (± 0.01%) 481,645k (± 0.01%) ~ 481,564k 481,712k p=0.810 n=6
Parse Time 3.24s (± 0.53%) 3.25s (± 0.13%) ~ 3.24s 3.25s p=0.731 n=6
Bind Time 0.95s (± 1.23%) 0.96s (± 0.85%) ~ 0.95s 0.97s p=0.666 n=6
Check Time 18.37s (± 1.16%) 18.34s (± 0.45%) ~ 18.26s 18.49s p=0.936 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 22.57s (± 1.00%) 22.55s (± 0.40%) ~ 22.46s 22.71s p=0.872 n=6
xstate - node (v16.17.1, x64)
Memory used 561,279k (± 0.02%) 561,303k (± 0.02%) ~ 561,137k 561,493k p=0.689 n=6
Parse Time 4.01s (± 0.30%) 4.02s (± 0.34%) ~ 4.01s 4.04s p=0.176 n=6
Bind Time 1.74s (± 4.52%) 1.71s (± 5.88%) ~ 1.58s 1.79s p=0.742 n=6
Check Time 3.09s (± 2.76%) 3.09s (± 1.26%) ~ 3.06s 3.14s p=0.567 n=6
Emit Time 0.09s (± 8.51%) 0.09s (± 4.62%) ~ 0.08s 0.09s p=0.730 n=6
Total Time 8.94s (± 0.15%) 8.92s (± 0.82%) ~ 8.82s 8.99s p=1.000 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-148-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current 54761 6
Baseline main 6

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/54761/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Unknown failure"
  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

  • [NEW] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-54761/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • [MISSING] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/54761/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, seems good to me; hopefully this isn't too bandaid-y, given the very similar issue #54760 bisected to the same problematic PR (#43005), but I suspect they wouldn't have similar fixes anyway.

@jakebailey jakebailey merged commit e607c8e into microsoft:main Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Incorrect ASI inside 'declare type'
5 participants