Skip to content
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

Instantiation expression can be followed by line break or binary operator #49353

Merged
merged 9 commits into from Jun 10, 2022

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Jun 2, 2022

This PR adjusts our parsing of instantiation expressions. When a potential type argument list is followed by

  • a line break,
  • an ( token,
  • a template literal string, or
  • any token except < or > that isn't the start of an expression,

we consider that construct to be a type argument list. Otherwise we consider the construct to be a < relational expression followed by a '>` relational expression.

Fixes #49348.
Fixes #49362.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 2, 2022
Comment on lines 5743 to 5744
case SyntaxKind.InterfaceKeyword:
case SyntaxKind.LetKeyword:
Copy link
Member

Choose a reason for hiding this comment

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

You need to cover more than just these cases and have tests for them - for example

class C {
    static specialFoo = f<string>
    static bar = 123;
}

class D {
    public specialFoo = f<string>
    public bar = 123;
}

class E {
    private specialFoo = f<string>
    private bar = 123;
}

class F {
    protected specialFoo = f<string>
    protected bar = 123;
}

@DanielRosenwasser
Copy link
Member

CC @nicolo-ribaudo @evanw @kdy1 the idea of this PR and issue is that an instantiation expression cannot be followed by a newline and some set of strict-mode reserved words.

@nicolo-ribaudo
Copy link

Other than strict-mode keywords, you might also want to special-case await (which is only a keyword in modules, and not in general in strict mode).

@DanielRosenwasser
Copy link
Member

We just discussed the nuances of async. We're considering a rule where if something that could be parsed as an instantiation expression is followed by an identifier or keyword of any sort, then we will favor the instantiation expression (and effectively act as if a user had added a semicolon).

@nicolo-ribaudo
Copy link

nicolo-ribaudo commented Jun 2, 2022

I hate any new "valid JavaScript has different semantics when parsed as TypeScript" rule, but it's so uncommon to see a < b > [LineTerminator] c in JS code that probably no one will never notice it.

Maybe OT Since you are fine-tuning how instantiation expressions are parsed: it's weird that a<b> ?? c is not valid code, from a syntactic point of view.

@DanielRosenwasser
Copy link
Member

Maybe OT Since you are fine-tuning how instantiation expressions are parsed: it's weird that a<b> ?? c is not valid code, from a syntactic point of view.

😬

Opened #49362

@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 9, 2022
@ahejlsberg ahejlsberg changed the title Instantiation expression can be followed by let or interface on new line Instantiation expression can be followed by line break or binary operator Jun 9, 2022
@sandersn
Copy link
Member

sandersn commented Jun 9, 2022

@typescript-bot test top100

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2022

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

@sandersn
Copy link
Member

sandersn commented Jun 9, 2022

Ran out of memory trying to compile angular ☠️
I'm going to try again, especially since I thought angular was part of the user tests too. Maybe it's an old version though.

@typescript-bot test top100

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2022

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

@sandersn
Copy link
Member

sandersn commented Jun 9, 2022

Let's try again:
@typescript-bot test top100

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2022

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

Update: The results are in!

@ahejlsberg
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2022

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

@typescript-bot
Copy link
Collaborator

@sandersn
The results of the user tests run you requested are in!

Here they are:

Comparison Report - main..refs/pull/49353/merge

cheeriojs/cheerio

1 of 2 projects failed to build with the old tsc

tsconfig.eslint.json

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..49353

Metric main 49353 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 334,181k (± 0.01%) 334,167k (± 0.00%) -14k (- 0.00%) 334,123k 334,201k
Parse Time 2.05s (± 0.67%) 2.05s (± 0.70%) +0.00s (+ 0.24%) 2.02s 2.09s
Bind Time 0.87s (± 0.55%) 0.88s (± 0.68%) +0.01s (+ 1.04%) 0.87s 0.89s
Check Time 5.70s (± 0.28%) 5.72s (± 0.53%) +0.02s (+ 0.33%) 5.64s 5.78s
Emit Time 6.35s (± 0.39%) 6.38s (± 0.71%) +0.03s (+ 0.43%) 6.29s 6.48s
Total Time 14.96s (± 0.26%) 15.02s (± 0.50%) +0.07s (+ 0.43%) 14.89s 15.23s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,818k (± 0.02%) 192,851k (± 0.01%) +33k (+ 0.02%) 192,780k 192,901k
Parse Time 0.85s (± 0.99%) 0.86s (± 0.70%) +0.01s (+ 0.71%) 0.84s 0.87s
Bind Time 0.56s (± 0.59%) 0.56s (± 0.67%) -0.01s (- 0.89%) 0.55s 0.56s
Check Time 7.58s (± 0.52%) 7.59s (± 0.34%) +0.00s (+ 0.04%) 7.55s 7.66s
Emit Time 2.51s (± 0.76%) 2.50s (± 0.56%) -0.01s (- 0.20%) 2.48s 2.54s
Total Time 11.50s (± 0.47%) 11.50s (± 0.25%) 0.00s ( 0.00%) 11.45s 11.56s
Monaco - node (v14.15.1, x64)
Memory used 325,852k (± 0.01%) 325,850k (± 0.01%) -3k (- 0.00%) 325,818k 325,909k
Parse Time 1.57s (± 0.56%) 1.57s (± 0.60%) -0.00s (- 0.19%) 1.54s 1.58s
Bind Time 0.77s (± 0.44%) 0.77s (± 0.67%) -0.00s (- 0.26%) 0.76s 0.78s
Check Time 5.60s (± 0.28%) 5.60s (± 0.30%) +0.00s (+ 0.05%) 5.57s 5.65s
Emit Time 3.34s (± 0.94%) 3.33s (± 0.92%) -0.02s (- 0.54%) 3.28s 3.41s
Total Time 11.29s (± 0.37%) 11.27s (± 0.30%) -0.02s (- 0.20%) 11.20s 11.34s
TFS - node (v14.15.1, x64)
Memory used 289,213k (± 0.01%) 289,218k (± 0.01%) +5k (+ 0.00%) 289,174k 289,257k
Parse Time 1.36s (± 1.30%) 1.37s (± 0.41%) +0.01s (+ 0.66%) 1.36s 1.38s
Bind Time 0.72s (± 0.55%) 0.73s (± 0.76%) +0.01s (+ 0.97%) 0.72s 0.74s
Check Time 5.28s (± 0.42%) 5.29s (± 0.64%) +0.01s (+ 0.17%) 5.24s 5.38s
Emit Time 3.54s (± 2.18%) 3.55s (± 2.12%) +0.01s (+ 0.14%) 3.40s 3.69s
Total Time 10.90s (± 0.88%) 10.93s (± 0.84%) +0.03s (+ 0.29%) 10.78s 11.16s
material-ui - node (v14.15.1, x64)
Memory used 446,624k (± 0.00%) 446,649k (± 0.00%) +26k (+ 0.01%) 446,591k 446,688k
Parse Time 1.87s (± 0.52%) 1.88s (± 0.36%) +0.01s (+ 0.32%) 1.86s 1.89s
Bind Time 0.69s (± 0.52%) 0.70s (± 0.67%) +0.01s (+ 0.86%) 0.69s 0.71s
Check Time 13.11s (± 0.54%) 13.17s (± 0.65%) +0.06s (+ 0.46%) 13.03s 13.37s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.68s (± 0.50%) 15.75s (± 0.58%) +0.07s (+ 0.45%) 15.58s 15.96s
xstate - node (v14.15.1, x64)
Memory used 537,476k (± 0.00%) 537,497k (± 0.00%) +21k (+ 0.00%) 537,452k 537,529k
Parse Time 2.57s (± 0.45%) 2.58s (± 0.52%) +0.01s (+ 0.39%) 2.55s 2.62s
Bind Time 1.15s (± 1.06%) 1.16s (± 0.96%) +0.01s (+ 0.61%) 1.14s 1.18s
Check Time 1.52s (± 0.29%) 1.52s (± 0.60%) -0.00s (- 0.20%) 1.51s 1.55s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.32s (± 0.32%) 5.33s (± 0.30%) +0.01s (+ 0.19%) 5.30s 5.37s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 49353 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here.

@typescript-bot
Copy link
Collaborator

@ahejlsberg
Great news! no new errors were found between main..refs/pull/49353/merge

@ahejlsberg
Copy link
Member Author

All test runs are clean and performance is unaffected.

@ahejlsberg ahejlsberg merged commit e6808c4 into main Jun 10, 2022
@ahejlsberg ahejlsberg deleted the fix49348 branch June 10, 2022 17:26
@nicolo-ribaudo
Copy link

nicolo-ribaudo commented Jun 10, 2022

Does this PR also change the behavior of new expressions? e.g.

new A<B>
C

does ASI affect this code?

(or as an alternative, it there a playground where I can test it myself? 😅)

EDIT: Yes, ASI inserts a semicolon there: https://www.typescriptlang.org/play?ts=4.8.0-dev.20220611#code%2FHYUw7gBAggPAQgPgFAGEg=

@sosukesuzuki
Copy link

Hi, will this PR be released in the near future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
7 participants