Skip to content

Report grammar errors for invalid decorator grammar - #57749

Merged
Ron Buckton (rbuckton) merged 3 commits into
mainfrom
fix-55336-decorator-grammar
Mar 13, 2024
Merged

Report grammar errors for invalid decorator grammar#57749
Ron Buckton (rbuckton) merged 3 commits into
mainfrom
fix-55336-decorator-grammar

Conversation

@rbuckton

@rbucktonRon Buckton (rbuckton) commented Mar 12, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 12, 2024

Copy link
Copy Markdown
Contributor

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

CommandStatusResults
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

TypeScript Bot (typescript-bot) commented Mar 12, 2024

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@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
Copy Markdown
Contributor

Jake Bailey (@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
Copy Markdown
Contributor

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

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Angular - node (v18.15.0, x64)
Memory used295,599k (± 0.01%)295,602k (± 0.01%)~295,566k295,626kp=0.689 n=6
Parse Time2.66s (± 0.15%)2.66s (± 0.24%)~2.65s2.67sp=0.673 n=6
Bind Time0.83s (± 1.08%)0.83s (± 0.99%)~0.82s0.84sp=0.550 n=6
Check Time8.21s (± 0.27%)8.21s (± 0.34%)~8.17s8.24sp=0.625 n=6
Emit Time7.15s (± 0.50%)7.13s (± 0.39%)~7.08s7.15sp=0.288 n=6
Total Time18.85s (± 0.27%)18.83s (± 0.24%)~18.78s18.89sp=0.747 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used193,787k (± 0.94%)193,308k (± 0.93%)~191,422k195,121kp=0.810 n=6
Parse Time1.35s (± 1.01%)1.36s (± 1.14%)~1.35s1.39sp=0.800 n=6
Bind Time0.72s (± 0.00%)0.72s (± 0.00%)~0.72s0.72sp=1.000 n=6
Check Time9.28s (± 0.22%)9.32s (± 0.48%)~9.28s9.38sp=0.053 n=6
Emit Time2.60s (± 0.53%)2.61s (± 0.86%)~2.59s2.65sp=0.867 n=6
Total Time13.96s (± 0.20%)14.01s (± 0.47%)~13.94s14.10sp=0.107 n=6
Monaco - node (v18.15.0, x64)
Memory used347,332k (± 0.00%)347,343k (± 0.01%)~347,316k347,382kp=0.810 n=6
Parse Time2.49s (± 0.16%)2.49s (± 0.34%)~2.47s2.49sp=0.527 n=6
Bind Time0.92s (± 0.59%)0.93s (± 0.44%)~0.92s0.93sp=0.282 n=6
Check Time6.95s (± 0.28%)6.96s (± 0.20%)~6.94s6.98sp=0.222 n=6
Emit Time4.08s (± 0.55%)4.06s (± 0.40%)~4.04s4.08sp=0.142 n=6
Total Time14.44s (± 0.26%)14.43s (± 0.24%)~14.38s14.47sp=0.872 n=6
TFS - node (v18.15.0, x64)
Memory used302,764k (± 0.01%)302,761k (± 0.01%)~302,724k302,786kp=0.936 n=6
Parse Time2.02s (± 0.79%)2.01s (± 0.96%)~1.98s2.03sp=0.560 n=6
Bind Time1.00s (± 1.03%)1.00s (± 0.63%)~0.99s1.01sp=0.654 n=6
Check Time6.33s (± 0.31%)6.32s (± 0.49%)~6.29s6.37sp=0.570 n=6
Emit Time3.60s (± 0.23%)3.61s (± 0.48%)~3.59s3.63sp=0.134 n=6
Total Time12.95s (± 0.33%)12.94s (± 0.28%)~12.87s12.97sp=0.803 n=6
material-ui - node (v18.15.0, x64)
Memory used511,265k (± 0.01%)511,254k (± 0.01%)~511,212k511,310kp=0.689 n=6
Parse Time2.66s (± 0.37%)2.66s (± 0.39%)~2.64s2.67sp=0.788 n=6
Bind Time0.98s (± 0.85%)0.97s (± 1.41%)~0.95s0.99sp=0.210 n=6
Check Time17.32s (± 0.23%)17.29s (± 0.40%)~17.19s17.37sp=0.746 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time20.96s (± 0.16%)20.93s (± 0.31%)~20.82s20.98sp=0.687 n=6
mui-docs - node (v18.15.0, x64)
Memory used1,789,413k (± 0.00%)1,789,404k (± 0.00%)~1,789,369k1,789,453kp=0.575 n=6
Parse Time6.63s (± 0.25%)6.63s (± 0.52%)~6.61s6.70sp=0.681 n=6
Bind Time2.39s (± 0.26%)2.39s (± 0.49%)~2.38s2.41sp=1.000 n=6
Check Time58.97s (± 0.36%)58.94s (± 0.34%)~58.78s59.30sp=0.810 n=6
Emit Time0.16s (± 3.16%)0.16s (± 2.52%)~0.16s0.17sp=0.595 n=6
Total Time68.14s (± 0.32%)68.12s (± 0.31%)~67.94s68.47sp=1.000 n=6
self-build-src - node (v18.15.0, x64)
Memory used2,395,362k (± 0.01%)2,395,399k (± 0.03%)~2,394,342k2,396,222kp=0.936 n=6
Parse Time5.08s (± 0.89%)5.07s (± 1.21%)~4.97s5.13sp=0.936 n=6
Bind Time1.89s (± 0.64%)1.90s (± 0.70%)~1.88s1.91sp=0.507 n=6
Check Time33.58s (± 0.25%)33.62s (± 0.32%)~33.47s33.76sp=0.376 n=6
Emit Time2.67s (± 1.21%)2.68s (± 0.94%)~2.64s2.71sp=0.689 n=6
Total Time43.24s (± 0.18%)43.29s (± 0.28%)~43.14s43.51sp=0.470 n=6
self-compiler - node (v18.15.0, x64)
Memory used414,595k (± 0.01%)414,684k (± 0.01%)+89k (+ 0.02%)414,638k414,721kp=0.005 n=6
Parse Time2.81s (± 0.77%)2.82s (± 0.91%)~2.78s2.86sp=0.466 n=6
Bind Time1.06s (± 0.49%)1.06s (± 0.71%)~1.05s1.07sp=0.784 n=6
Check Time15.15s (± 0.32%)15.18s (± 0.27%)~15.13s15.24sp=0.260 n=6
Emit Time1.12s (± 1.26%)1.13s (± 1.04%)~1.12s1.15sp=0.357 n=6
Total Time20.15s (± 0.19%)20.19s (± 0.25%)~20.13s20.25sp=0.148 n=6
vscode - node (v18.15.0, x64)
Memory used2,868,527k (± 0.00%)2,868,526k (± 0.00%)~2,868,465k2,868,591kp=1.000 n=6
Parse Time10.77s (± 0.14%)10.77s (± 0.17%)~10.75s10.80sp=0.807 n=6
Bind Time3.45s (± 0.37%)3.45s (± 0.63%)~3.42s3.48sp=1.000 n=6
Check Time60.91s (± 0.33%)60.82s (± 0.40%)~60.49s61.16sp=0.630 n=6
Emit Time16.33s (± 0.51%)16.34s (± 0.36%)~16.29s16.45sp=0.936 n=6
Total Time91.46s (± 0.20%)91.39s (± 0.31%)~91.08s91.75sp=0.689 n=6
webpack - node (v18.15.0, x64)
Memory used404,894k (± 0.01%)404,877k (± 0.02%)~404,799k405,010kp=0.298 n=6
Parse Time3.24s (± 0.49%)3.25s (± 0.34%)~3.23s3.26sp=0.280 n=6
Bind Time1.39s (± 0.76%)1.40s (± 1.63%)~1.38s1.44sp=0.250 n=6
Check Time14.09s (± 0.16%)14.07s (± 0.35%)~14.00s14.13sp=0.459 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time18.72s (± 0.14%)18.72s (± 0.27%)~18.64s18.77sp=1.000 n=6
xstate - node (v18.15.0, x64)
Memory used513,150k (± 0.01%)513,196k (± 0.01%)~513,154k513,267kp=0.078 n=6
Parse Time3.27s (± 0.32%)3.27s (± 0.36%)~3.26s3.29sp=0.619 n=6
Bind Time1.54s (± 0.49%)1.54s (± 0.34%)~1.53s1.54sp=0.784 n=6
Check Time2.86s (± 0.82%)2.86s (± 0.53%)~2.85s2.88sp=0.935 n=6
Emit Time0.08s (± 4.99%)0.08s (± 4.99%)~0.08s0.09sp=1.000 n=6
Total Time7.76s (± 0.46%)7.75s (± 0.16%)~7.74s7.77sp=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)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@typescript-bot

Copy link
Copy Markdown
Contributor

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

Everything looks good!

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 12, 2024

Copy link
Copy Markdown
Contributor

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

CommandStatusResults
pack this✅ Started✅ Results

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 12, 2024

Copy link
Copy Markdown
Contributor

Hey Ron Buckton (@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
Ron Buckton (rbuckton) deleted the fix-55336-decorator-grammar branch March 13, 2024 18:50
@ef4

Copy link
Copy Markdown

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

StNekroman (StNekroman) commented Sep 24, 2025

Copy link
Copy Markdown

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

@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Mar 23, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Milestone BugPRs 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

7 participants

@rbuckton@jakebailey@typescript-bot@ef4@StNekroman@weswigham@andrewbranch