Skip to content

Do not narrow using JSDoc type assertion nodes #56048

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

Merged

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Oct 9, 2023

spotted by @jakebailey here

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 9, 2023
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@jakebailey
Copy link
Member

Just to further describe things, switch (value as "a" | "b") or switch (value satisfies "a" | "b") don't allow narrowing to work today, but switch (/** @type {"a" | "b"} */ (value)) does, because the existing code thinks it can skip parens safely. But, that type assertion in JS should behave just like as/satisfies.

@jakebailey
Copy link
Member

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 9, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 9, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 9, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 5b057c5. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 9, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 9, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 9, 2023

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/158160/artifacts?artifactName=tgz&fileId=FB1C5B46B9F616011551E5F144FFFC008199B2D8886F8501653DA59FB616AAA302&fileName=/typescript-5.3.0-insiders.20231009.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.3.0-pr-56048-8".;

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

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

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,127k (± 0.01%) 295,102k (± 0.02%) ~ 295,003k 295,150k p=0.336 n=6
Parse Time 2.64s (± 0.32%) 2.64s (± 0.44%) ~ 2.63s 2.66s p=1.000 n=6
Bind Time 0.83s (± 0.66%) 0.84s (± 1.06%) ~ 0.83s 0.85s p=0.341 n=6
Check Time 8.08s (± 0.25%) 8.07s (± 0.44%) ~ 8.03s 8.12s p=0.808 n=6
Emit Time 7.06s (± 0.28%) 7.07s (± 0.21%) ~ 7.05s 7.09s p=0.330 n=6
Total Time 18.61s (± 0.12%) 18.62s (± 0.27%) ~ 18.57s 18.70s p=1.000 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 192,062k (± 1.27%) 190,610k (± 0.01%) ~ 190,597k 190,638k p=0.199 n=6
Parse Time 1.34s (± 0.87%) 1.34s (± 0.78%) ~ 1.33s 1.36s p=0.800 n=6
Bind Time 0.73s (± 0.00%) 0.73s (± 0.00%) ~ 0.73s 0.73s p=1.000 n=6
Check Time 9.11s (± 0.14%) 9.10s (± 0.35%) ~ 9.05s 9.14s p=0.624 n=6
Emit Time 2.63s (± 0.56%) 2.63s (± 0.62%) ~ 2.61s 2.65s p=0.624 n=6
Total Time 13.81s (± 0.19%) 13.80s (± 0.34%) ~ 13.73s 13.85s p=0.934 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,302k (± 0.00%) 347,304k (± 0.00%) ~ 347,288k 347,318k p=0.936 n=6
Parse Time 2.46s (± 0.21%) 2.46s (± 0.33%) ~ 2.45s 2.47s p=0.140 n=6
Bind Time 0.94s (± 0.00%) 0.95s (± 0.54%) +0.01s (+ 0.71%) 0.94s 0.95s p=0.025 n=6
Check Time 6.89s (± 0.44%) 6.89s (± 0.41%) ~ 6.86s 6.94s p=1.000 n=6
Emit Time 4.01s (± 0.34%) 4.02s (± 0.59%) ~ 3.99s 4.06s p=0.681 n=6
Total Time 14.30s (± 0.27%) 14.32s (± 0.35%) ~ 14.27s 14.41s p=0.520 n=6
TFS - node (v18.15.0, x64)
Memory used 302,590k (± 0.01%) 302,591k (± 0.01%) ~ 302,539k 302,618k p=1.000 n=6
Parse Time 1.99s (± 0.63%) 1.99s (± 1.07%) ~ 1.96s 2.02s p=0.805 n=6
Bind Time 1.01s (± 1.58%) 1.00s (± 0.41%) ~ 1.00s 1.01s p=0.227 n=6
Check Time 6.27s (± 0.37%) 6.27s (± 0.33%) ~ 6.24s 6.29s p=1.000 n=6
Emit Time 3.57s (± 0.51%) 3.56s (± 0.52%) ~ 3.54s 3.59s p=0.568 n=6
Total Time 12.84s (± 0.26%) 12.82s (± 0.15%) ~ 12.80s 12.85s p=0.421 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,495k (± 0.00%) 470,481k (± 0.00%) ~ 470,454k 470,502k p=0.298 n=6
Parse Time 2.57s (± 0.53%) 2.58s (± 0.29%) ~ 2.57s 2.59s p=0.611 n=6
Bind Time 1.00s (± 0.41%) 0.99s (± 0.76%) -0.01s (- 1.33%) 0.98s 1.00s p=0.010 n=6
Check Time 16.70s (± 0.37%) 16.59s (± 0.22%) -0.11s (- 0.68%) 16.54s 16.62s p=0.005 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.27s (± 0.37%) 20.16s (± 0.18%) -0.12s (- 0.57%) 20.11s 20.19s p=0.005 n=6
xstate - node (v18.15.0, x64)
Memory used 512,660k (± 0.02%) 512,621k (± 0.01%) ~ 512,528k 512,718k p=0.574 n=6
Parse Time 3.26s (± 0.42%) 3.27s (± 0.36%) ~ 3.25s 3.28s p=0.244 n=6
Bind Time 1.55s (± 0.33%) 1.56s (± 0.33%) ~ 1.55s 1.56s p=0.311 n=6
Check Time 2.87s (± 1.52%) 2.89s (± 1.04%) ~ 2.85s 2.93s p=0.421 n=6
Emit Time 0.08s (± 4.99%) 0.09s (± 6.44%) ~ 0.08s 0.09s p=0.282 n=6
Total Time 7.75s (± 0.70%) 7.79s (± 0.45%) ~ 7.75s 7.85s p=0.261 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)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,381ms (± 1.72%) 2,383ms (± 1.35%) ~ 2,326ms 2,414ms p=0.936 n=6
Req 2 - geterr 5,355ms (± 1.60%) 5,332ms (± 1.57%) ~ 5,284ms 5,502ms p=0.873 n=6
Req 3 - references 327ms (± 1.26%) 330ms (± 1.41%) ~ 324ms 334ms p=0.373 n=6
Req 4 - navto 278ms (± 1.11%) 280ms (± 1.14%) ~ 274ms 283ms p=0.277 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 82ms (± 8.11%) 78ms (± 7.94%) ~ 74ms 90ms p=0.117 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,494ms (± 0.54%) 2,477ms (± 1.17%) ~ 2,438ms 2,528ms p=0.229 n=6
Req 2 - geterr 4,064ms (± 0.27%) 4,134ms (± 1.91%) ~ 4,051ms 4,210ms p=0.230 n=6
Req 3 - references 344ms (± 0.40%) 338ms (± 1.62%) ~ 332ms 345ms p=0.061 n=6
Req 4 - navto 285ms (± 0.29%) 284ms (± 0.62%) ~ 280ms 285ms p=0.177 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 87ms (± 0.86%) 82ms (± 7.61%) ~ 76ms 88ms p=0.206 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,587ms (± 0.53%) 2,602ms (± 0.50%) ~ 2,585ms 2,624ms p=0.149 n=6
Req 2 - geterr 1,717ms (± 2.14%) 1,675ms (± 1.69%) ~ 1,649ms 1,727ms p=0.066 n=6
Req 3 - references 114ms (± 8.64%) 113ms (± 9.29%) ~ 106ms 128ms p=0.868 n=6
Req 4 - navto 360ms (± 0.58%) 358ms (± 0.43%) ~ 355ms 359ms p=0.078 n=6
Req 5 - completionInfo count 2,073 (± 0.00%) 2,073 (± 0.00%) ~ 2,073 2,073 p=1.000 n=6
Req 5 - completionInfo 310ms (± 1.59%) 301ms (± 2.02%) 🟩-9ms (- 3.01%) 295ms 309ms p=0.044 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v18.15.0, x64)
Execution time 152.42ms (± 0.21%) 152.21ms (± 0.16%) -0.22ms (- 0.14%) 151.18ms 154.21ms p=0.000 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 227.84ms (± 0.15%) 227.85ms (± 0.19%) ~ 226.42ms 235.86ms p=0.145 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 229.03ms (± 0.16%) 229.10ms (± 0.18%) ~ 227.51ms 235.78ms p=0.241 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 228.47ms (± 0.15%) 228.61ms (± 0.16%) +0.14ms (+ 0.06%) 227.09ms 231.91ms p=0.000 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - 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.

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.

This seems correct to me. Technically a breaking change but not one I expect anyone has ever encountered.

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@sandersn sandersn merged commit 6a9bc38 into microsoft:main Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants