Skip to content

Conversation

@rbuckton
Copy link
Contributor

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

@StNekroman
Copy link

StNekroman commented Sep 24, 2025

Why this touches legacy experimental decorators , despite PR's description was about "Stage 3 Decorators" ?
Any good reason for breaking backward compatibility?

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

Development

Successfully merging this pull request may close these issues.

TS 5.0 decorators don't follow the DecoratorCallExpression grammar

8 participants