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

Report grammar errors for invalid decorator grammar #57749

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Mar 12, 2024

This reports a grammar error when invalid syntax is used in a decorator expression that does not match the proposed syntax for Stage 3 Decorators. In addition, this adds quick fixes to add parentheses around the expression to make it valid.

This is a somewhat aggressive error as we previously were far more flexible for decorator expressions under --experimentalDecorators, which could mean new errors reported in existing code. Since parse and emit do not change, and since there is a quick-fix available, I'd like to try to go ahead with this change as is. However, if we feel it is too much of break, I can change the grammar check to only apply when targeting ES decorators.

Fixes #55336

@jakebailey
Copy link
Member

@typescript-bot test top400
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot perf test this faster
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 12, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
test top400 ✅ Started ✅ Results
user test this ✅ Started 👀 Results
run dt ✅ Started ✅ Results
perf test this faster ✅ Started 👀 Results
pack this ✅ Started ✅ Results
@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 12, 2024

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/160317/artifacts?artifactName=tgz&fileId=7078521EA8FE0E59B92E23275A8C67308DE31F636FF34B76148571F54C2FB85402&fileName=/typescript-5.5.0-insiders.20240312.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.5.0-pr-57749-2".;

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

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

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

@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.

@typescript-bot
Copy link
Collaborator

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

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,599k (± 0.01%) 295,602k (± 0.01%) ~ 295,566k 295,626k p=0.689 n=6
Parse Time 2.66s (± 0.15%) 2.66s (± 0.24%) ~ 2.65s 2.67s p=0.673 n=6
Bind Time 0.83s (± 1.08%) 0.83s (± 0.99%) ~ 0.82s 0.84s p=0.550 n=6
Check Time 8.21s (± 0.27%) 8.21s (± 0.34%) ~ 8.17s 8.24s p=0.625 n=6
Emit Time 7.15s (± 0.50%) 7.13s (± 0.39%) ~ 7.08s 7.15s p=0.288 n=6
Total Time 18.85s (± 0.27%) 18.83s (± 0.24%) ~ 18.78s 18.89s p=0.747 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,787k (± 0.94%) 193,308k (± 0.93%) ~ 191,422k 195,121k p=0.810 n=6
Parse Time 1.35s (± 1.01%) 1.36s (± 1.14%) ~ 1.35s 1.39s p=0.800 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.00%) ~ 0.72s 0.72s p=1.000 n=6
Check Time 9.28s (± 0.22%) 9.32s (± 0.48%) ~ 9.28s 9.38s p=0.053 n=6
Emit Time 2.60s (± 0.53%) 2.61s (± 0.86%) ~ 2.59s 2.65s p=0.867 n=6
Total Time 13.96s (± 0.20%) 14.01s (± 0.47%) ~ 13.94s 14.10s p=0.107 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,332k (± 0.00%) 347,343k (± 0.01%) ~ 347,316k 347,382k p=0.810 n=6
Parse Time 2.49s (± 0.16%) 2.49s (± 0.34%) ~ 2.47s 2.49s p=0.527 n=6
Bind Time 0.92s (± 0.59%) 0.93s (± 0.44%) ~ 0.92s 0.93s p=0.282 n=6
Check Time 6.95s (± 0.28%) 6.96s (± 0.20%) ~ 6.94s 6.98s p=0.222 n=6
Emit Time 4.08s (± 0.55%) 4.06s (± 0.40%) ~ 4.04s 4.08s p=0.142 n=6
Total Time 14.44s (± 0.26%) 14.43s (± 0.24%) ~ 14.38s 14.47s p=0.872 n=6
TFS - node (v18.15.0, x64)
Memory used 302,764k (± 0.01%) 302,761k (± 0.01%) ~ 302,724k 302,786k p=0.936 n=6
Parse Time 2.02s (± 0.79%) 2.01s (± 0.96%) ~ 1.98s 2.03s p=0.560 n=6
Bind Time 1.00s (± 1.03%) 1.00s (± 0.63%) ~ 0.99s 1.01s p=0.654 n=6
Check Time 6.33s (± 0.31%) 6.32s (± 0.49%) ~ 6.29s 6.37s p=0.570 n=6
Emit Time 3.60s (± 0.23%) 3.61s (± 0.48%) ~ 3.59s 3.63s p=0.134 n=6
Total Time 12.95s (± 0.33%) 12.94s (± 0.28%) ~ 12.87s 12.97s p=0.803 n=6
material-ui - node (v18.15.0, x64)
Memory used 511,265k (± 0.01%) 511,254k (± 0.01%) ~ 511,212k 511,310k p=0.689 n=6
Parse Time 2.66s (± 0.37%) 2.66s (± 0.39%) ~ 2.64s 2.67s p=0.788 n=6
Bind Time 0.98s (± 0.85%) 0.97s (± 1.41%) ~ 0.95s 0.99s p=0.210 n=6
Check Time 17.32s (± 0.23%) 17.29s (± 0.40%) ~ 17.19s 17.37s p=0.746 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.96s (± 0.16%) 20.93s (± 0.31%) ~ 20.82s 20.98s p=0.687 n=6
mui-docs - node (v18.15.0, x64)
Memory used 1,789,413k (± 0.00%) 1,789,404k (± 0.00%) ~ 1,789,369k 1,789,453k p=0.575 n=6
Parse Time 6.63s (± 0.25%) 6.63s (± 0.52%) ~ 6.61s 6.70s p=0.681 n=6
Bind Time 2.39s (± 0.26%) 2.39s (± 0.49%) ~ 2.38s 2.41s p=1.000 n=6
Check Time 58.97s (± 0.36%) 58.94s (± 0.34%) ~ 58.78s 59.30s p=0.810 n=6
Emit Time 0.16s (± 3.16%) 0.16s (± 2.52%) ~ 0.16s 0.17s p=0.595 n=6
Total Time 68.14s (± 0.32%) 68.12s (± 0.31%) ~ 67.94s 68.47s p=1.000 n=6
self-build-src - node (v18.15.0, x64)
Memory used 2,395,362k (± 0.01%) 2,395,399k (± 0.03%) ~ 2,394,342k 2,396,222k p=0.936 n=6
Parse Time 5.08s (± 0.89%) 5.07s (± 1.21%) ~ 4.97s 5.13s p=0.936 n=6
Bind Time 1.89s (± 0.64%) 1.90s (± 0.70%) ~ 1.88s 1.91s p=0.507 n=6
Check Time 33.58s (± 0.25%) 33.62s (± 0.32%) ~ 33.47s 33.76s p=0.376 n=6
Emit Time 2.67s (± 1.21%) 2.68s (± 0.94%) ~ 2.64s 2.71s p=0.689 n=6
Total Time 43.24s (± 0.18%) 43.29s (± 0.28%) ~ 43.14s 43.51s p=0.470 n=6
self-compiler - node (v18.15.0, x64)
Memory used 414,595k (± 0.01%) 414,684k (± 0.01%) +89k (+ 0.02%) 414,638k 414,721k p=0.005 n=6
Parse Time 2.81s (± 0.77%) 2.82s (± 0.91%) ~ 2.78s 2.86s p=0.466 n=6
Bind Time 1.06s (± 0.49%) 1.06s (± 0.71%) ~ 1.05s 1.07s p=0.784 n=6
Check Time 15.15s (± 0.32%) 15.18s (± 0.27%) ~ 15.13s 15.24s p=0.260 n=6
Emit Time 1.12s (± 1.26%) 1.13s (± 1.04%) ~ 1.12s 1.15s p=0.357 n=6
Total Time 20.15s (± 0.19%) 20.19s (± 0.25%) ~ 20.13s 20.25s p=0.148 n=6
vscode - node (v18.15.0, x64)
Memory used 2,868,527k (± 0.00%) 2,868,526k (± 0.00%) ~ 2,868,465k 2,868,591k p=1.000 n=6
Parse Time 10.77s (± 0.14%) 10.77s (± 0.17%) ~ 10.75s 10.80s p=0.807 n=6
Bind Time 3.45s (± 0.37%) 3.45s (± 0.63%) ~ 3.42s 3.48s p=1.000 n=6
Check Time 60.91s (± 0.33%) 60.82s (± 0.40%) ~ 60.49s 61.16s p=0.630 n=6
Emit Time 16.33s (± 0.51%) 16.34s (± 0.36%) ~ 16.29s 16.45s p=0.936 n=6
Total Time 91.46s (± 0.20%) 91.39s (± 0.31%) ~ 91.08s 91.75s p=0.689 n=6
webpack - node (v18.15.0, x64)
Memory used 404,894k (± 0.01%) 404,877k (± 0.02%) ~ 404,799k 405,010k p=0.298 n=6
Parse Time 3.24s (± 0.49%) 3.25s (± 0.34%) ~ 3.23s 3.26s p=0.280 n=6
Bind Time 1.39s (± 0.76%) 1.40s (± 1.63%) ~ 1.38s 1.44s p=0.250 n=6
Check Time 14.09s (± 0.16%) 14.07s (± 0.35%) ~ 14.00s 14.13s p=0.459 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 18.72s (± 0.14%) 18.72s (± 0.27%) ~ 18.64s 18.77s p=1.000 n=6
xstate - node (v18.15.0, x64)
Memory used 513,150k (± 0.01%) 513,196k (± 0.01%) ~ 513,154k 513,267k p=0.078 n=6
Parse Time 3.27s (± 0.32%) 3.27s (± 0.36%) ~ 3.26s 3.29s p=0.619 n=6
Bind Time 1.54s (± 0.49%) 1.54s (± 0.34%) ~ 1.53s 1.54s p=0.784 n=6
Check Time 2.86s (± 0.82%) 2.86s (± 0.53%) ~ 2.85s 2.88s p=0.935 n=6
Emit Time 0.08s (± 4.99%) 0.08s (± 4.99%) ~ 0.08s 0.09s p=1.000 n=6
Total Time 7.76s (± 0.46%) 7.75s (± 0.16%) ~ 7.74s 7.77s p=0.625 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Should we also test @import.meta.decorator, since we have tests for super and this?

@jakebailey
Copy link
Member

All of the user / top tests seem to be okay with this, which is good news.

@rbuckton
Copy link
Member Author

Should we also test @import.meta.decorator, since we have tests for super and this?

I'm not sure why we would. Those tests specifically test which this is in scope.

@rbuckton
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 12, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results
@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 12, 2024

Hey @rbuckton, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/160330/artifacts?artifactName=tgz&fileId=7D1FCABBA39743080FCFCD40B8981DDBE7F6538EB6E6517B682DA7FDF8AD76F102&fileName=/typescript-5.5.0-insiders.20240312.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.5.0-pr-57749-11".;

@rbuckton rbuckton merged commit 5e8f900 into main Mar 13, 2024
19 checks passed
@rbuckton rbuckton deleted the fix-55336-decorator-grammar branch March 13, 2024 18:50
@ef4
Copy link

ef4 commented Jul 5, 2024

However, if we feel it is too much of break, I can change the grammar check to only apply when targeting ES decorators.

That would be a lot better, speaking as a framework maintainer that's trying to help shepherd a big ecosystem of community maintained code across the migration from experimental decorators to stage3 decorators. As experimental decorators are clearly an end-of-the-road feature that's maintained only for compatibility, anything you can do to not destabilize them unnecessarily would be helpful.

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
6 participants