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.

Sign up for GitHub

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

Jump to bottom

Fix bug#56997 - Parenthesized SatisfiesExpressions with comments are not unwrapped consistently in emitted JS #57281

Merged
merged 13 commits into from
Apr 19, 2024
Merged

Fix bug#56997 - Parenthesized SatisfiesExpressions with comments are not unwrapped consistently in emitted JS #57281

merged 13 commits into from
Apr 19, 2024

Conversation

jean-michelet
Copy link
Contributor

Fixes #56997

Hi, this is my first time contributing here 👋

I don't know if I've done anything relevant, but I will follow your feedback carefully.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Feb 3, 2024
@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

@jean-michelet
Copy link
Contributor Author

@microsoft-github-policy-service agree

@jean-michelet jean-michelet changed the title Bug#56997 Fix bug#56997 - Parenthesized SatisfiesExpressions with comments are not unwrapped consistently in emitted JS Feb 3, 2024
jean-michelet
jean-michelet commented Feb 5, 2024
View reviewed changes
src/compiler/checker.ts Outdated Show resolved Hide resolved
Andarist
Andarist reviewed Feb 5, 2024
View reviewed changes
src/compiler/types.ts Outdated Show resolved Hide resolved
jakebailey
jakebailey reviewed Feb 5, 2024
View reviewed changes
src/compiler/types.ts Outdated Show resolved Hide resolved
@jean-michelet
Copy link
Contributor Author

Thanks for the review @jakebailey @Andarist

I've edited what was requested, let me know if it looks good to you.

@jakebailey
Copy link
Member

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

@typescript-bot perf test this faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 6, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 6, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 6, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 6, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

@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,695k (± 0.01%) 295,663k (± 0.01%) -31k (- 0.01%) 295,643k 295,694k p=0.037 n=6
Parse Time 2.66s (± 0.37%) 2.66s (± 0.28%) ~ 2.65s 2.67s p=0.858 n=6
Bind Time 0.82s (± 0.50%) 0.83s (± 0.66%) +0.01s (+ 1.62%) 0.83s 0.84s p=0.006 n=6
Check Time 8.21s (± 0.30%) 8.21s (± 0.31%) ~ 8.18s 8.24s p=0.569 n=6
Emit Time 7.11s (± 0.41%) 7.09s (± 0.40%) ~ 7.06s 7.13s p=0.572 n=6
Total Time 18.80s (± 0.28%) 18.79s (± 0.21%) ~ 18.72s 18.82s p=1.000 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 192,530k (± 1.25%) 194,453k (± 1.66%) ~ 191,460k 197,478k p=0.810 n=6
Parse Time 1.37s (± 1.37%) 1.36s (± 1.47%) ~ 1.35s 1.40s p=0.618 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.00%) ~ 0.72s 0.72s p=1.000 n=6
Check Time 9.39s (± 0.86%) 9.40s (± 0.38%) ~ 9.35s 9.44s p=0.418 n=6
Emit Time 2.61s (± 1.15%) 2.62s (± 0.52%) ~ 2.60s 2.64s p=0.566 n=6
Total Time 14.09s (± 0.59%) 14.10s (± 0.37%) ~ 14.04s 14.17s p=0.688 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,464k (± 0.01%) 347,466k (± 0.00%) ~ 347,453k 347,486k p=0.630 n=6
Parse Time 2.47s (± 0.47%) 2.49s (± 0.49%) ~ 2.47s 2.50s p=0.096 n=6
Bind Time 0.93s (± 0.81%) 0.92s (± 0.56%) -0.01s (- 1.26%) 0.91s 0.92s p=0.020 n=6
Check Time 6.96s (± 0.34%) 6.95s (± 0.28%) ~ 6.94s 6.99s p=0.622 n=6
Emit Time 4.05s (± 0.29%) 4.05s (± 0.13%) ~ 4.04s 4.05s p=0.542 n=6
Total Time 14.41s (± 0.19%) 14.40s (± 0.17%) ~ 14.37s 14.44s p=0.808 n=6
TFS - node (v18.15.0, x64)
Memory used 302,849k (± 0.00%) 302,849k (± 0.00%) ~ 302,837k 302,862k p=1.000 n=6
Parse Time 2.00s (± 1.17%) 2.01s (± 0.70%) ~ 1.99s 2.03s p=0.807 n=6
Bind Time 1.00s (± 1.47%) 1.00s (± 0.89%) ~ 0.99s 1.01s p=0.932 n=6
Check Time 6.34s (± 0.22%) 6.33s (± 0.29%) ~ 6.32s 6.37s p=0.564 n=6
Emit Time 3.59s (± 0.38%) 3.59s (± 0.74%) ~ 3.55s 3.63s p=0.934 n=6
Total Time 12.94s (± 0.18%) 12.94s (± 0.25%) ~ 12.90s 12.99s p=1.000 n=6
material-ui - node (v18.15.0, x64)
Memory used 511,362k (± 0.00%) 511,366k (± 0.00%) ~ 511,334k 511,390k p=0.630 n=6
Parse Time 2.66s (± 0.51%) 2.66s (± 0.70%) ~ 2.63s 2.68s p=0.932 n=6
Bind Time 1.00s (± 0.52%) 1.00s (± 1.05%) ~ 0.98s 1.01s p=0.794 n=6
Check Time 17.22s (± 0.34%) 17.29s (± 0.46%) ~ 17.20s 17.39s p=0.258 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.87s (± 0.30%) 20.94s (± 0.27%) ~ 20.89s 21.02s p=0.148 n=6
mui-docs - node (v18.15.0, x64)
Memory used 1,774,592k (± 0.00%) 1,774,588k (± 0.00%) ~ 1,774,558k 1,774,605k p=0.810 n=6
Parse Time 6.61s (± 0.28%) 6.61s (± 0.55%) ~ 6.59s 6.68s p=0.183 n=6
Bind Time 2.40s (± 0.46%) 2.40s (± 0.31%) ~ 2.39s 2.41s p=1.000 n=6
Check Time 57.50s (± 0.33%) 57.54s (± 0.41%) ~ 57.19s 57.83s p=0.689 n=6
Emit Time 0.16s (± 3.16%) 0.16s (± 3.16%) ~ 0.16s 0.17s p=1.000 n=6
Total Time 66.68s (± 0.31%) 66.70s (± 0.31%) ~ 66.43s 66.97s p=0.810 n=6
self-build-src - node (v18.15.0, x64)
Memory used 2,413,024k (± 0.02%) 2,412,327k (± 0.02%) ~ 2,411,617k 2,412,758k p=0.066 n=6
Parse Time 4.92s (± 0.90%) 4.93s (± 0.72%) ~ 4.88s 4.97s p=1.000 n=6
Bind Time 1.87s (± 0.68%) 1.86s (± 0.34%) ~ 1.85s 1.87s p=0.090 n=6
Check Time 33.46s (± 0.38%) 33.48s (± 0.37%) ~ 33.32s 33.63s p=0.936 n=6
Emit Time 2.71s (± 1.54%) 2.66s (± 0.65%) -0.04s (- 1.60%) 2.64s 2.68s p=0.045 n=6
Total Time 42.99s (± 0.38%) 42.94s (± 0.36%) ~ 42.73s 43.12s p=0.575 n=6
self-compiler - node (v18.15.0, x64)
Memory used 418,696k (± 0.01%) 418,637k (± 0.01%) ~ 418,573k 418,708k p=0.093 n=6
Parse Time 2.84s (± 0.87%) 2.79s (± 3.00%) ~ 2.66s 2.85s p=0.328 n=6
Bind Time 1.08s (± 0.91%) 1.12s (± 6.38%) ~ 1.07s 1.22s p=1.000 n=6
Check Time 15.13s (± 0.22%) 15.09s (± 0.41%) ~ 14.97s 15.15s p=0.225 n=6
Emit Time 1.14s (± 1.06%) 1.13s (± 0.72%) ~ 1.13s 1.15s p=0.126 n=6
Total Time 20.19s (± 0.18%) 20.13s (± 0.24%) -0.06s (- 0.31%) 20.03s 20.15s p=0.005 n=6
vscode - node (v18.15.0, x64)
Memory used 2,815,820k (± 0.00%) 2,815,816k (± 0.00%) ~ 2,815,774k 2,815,889k p=0.689 n=6
Parse Time 10.67s (± 0.26%) 10.66s (± 0.44%) ~ 10.58s 10.70s p=0.571 n=6
Bind Time 3.39s (± 1.20%) 3.39s (± 0.48%) ~ 3.38s 3.42s p=0.357 n=6
Check Time 59.86s (± 0.42%) 59.91s (± 0.36%) ~ 59.59s 60.17s p=1.000 n=6
Emit Time 16.23s (± 0.40%) 16.26s (± 0.33%) ~ 16.21s 16.35s p=0.296 n=6
Total Time 90.16s (± 0.25%) 90.22s (± 0.19%) ~ 90.02s 90.45s p=0.748 n=6
webpack - node (v18.15.0, x64)
Memory used 393,458k (± 0.01%) 393,452k (± 0.01%) ~ 393,401k 393,498k p=1.000 n=6
Parse Time 3.10s (± 0.33%) 3.11s (± 0.51%) ~ 3.08s 3.12s p=0.138 n=6
Bind Time 1.39s (± 1.73%) 1.38s (± 1.45%) ~ 1.36s 1.41s p=0.553 n=6
Check Time 14.06s (± 0.25%) 14.02s (± 0.30%) ~ 13.96s 14.08s p=0.257 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 18.54s (± 0.28%) 18.51s (± 0.22%) ~ 18.47s 18.58s p=0.256 n=6
xstate - node (v18.15.0, x64)
Memory used 513,391k (± 0.01%) 513,374k (± 0.02%) ~ 513,313k 513,510k p=0.688 n=6
Parse Time 3.28s (± 0.36%) 3.28s (± 0.23%) ~ 3.27s 3.29s p=0.396 n=6
Bind Time 1.54s (± 0.41%) 1.54s (± 0.49%) ~ 1.53s 1.55s p=0.718 n=6
Check Time 2.86s (± 0.65%) 2.85s (± 0.99%) ~ 2.82s 2.88s p=0.519 n=6
Emit Time 0.08s (± 4.99%) 0.08s (± 5.21%) ~ 0.07s 0.08s p=0.218 n=6
Total Time 7.75s (± 0.32%) 7.75s (± 0.35%) ~ 7.73s 7.79s p=0.627 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

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 Here are the results of running the top-repos suite comparing main and refs/pull/57281/merge:

Everything looks good!

jakebailey
jakebailey approved these changes Feb 28, 2024
View reviewed changes
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.

I think this is okay, though I think I'd like a second opinion from @rbuckton.

rbuckton
rbuckton requested changes Apr 19, 2024
View reviewed changes
src/compiler/transformers/ts.ts Outdated Show resolved Hide resolved
src/compiler/types.ts Outdated Show resolved Hide resolved
@DanielRosenwasser DanielRosenwasser merged commit 8e8c1b6 into microsoft:main Apr 19, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andarist Andarist Andarist left review comments

@DanielRosenwasser DanielRosenwasser DanielRosenwasser approved these changes

@rbuckton rbuckton rbuckton approved these changes

@jakebailey jakebailey jakebailey approved these changes

Assignees

@jakebailey jakebailey

Labels
For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parenthesized SatisfiesExpressions with comments are not unwrapped consistently in emitted JS
6 participants
@jean-michelet @typescript-bot @jakebailey @DanielRosenwasser @rbuckton @Andarist