Skip to content

Propagate intersectionState in typeRelatedToSomeType #56207

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
merged 2 commits into from
Nov 21, 2023
Merged

Conversation

ahejlsberg
Copy link
Member

Fixes #53412.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 24, 2023
@ahejlsberg
Copy link
Member Author

@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

Heya @ahejlsberg, I've started to run the tsc-only perf test suite on this PR at e7018d2. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 24, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@ahejlsberg
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,090k (± 0.01%) 295,104k (± 0.01%) ~ 295,074k 295,153k p=0.872 n=6
Parse Time 2.63s (± 0.20%) 2.63s (± 0.21%) ~ 2.63s 2.64s p=0.640 n=6
Bind Time 0.84s (± 0.90%) 0.84s (± 0.97%) ~ 0.83s 0.85s p=0.729 n=6
Check Time 8.03s (± 0.24%) 8.04s (± 0.29%) ~ 8.01s 8.07s p=0.625 n=6
Emit Time 7.07s (± 0.27%) 7.08s (± 0.15%) ~ 7.06s 7.09s p=1.000 n=6
Total Time 18.58s (± 0.09%) 18.59s (± 0.18%) ~ 18.55s 18.62s p=0.681 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,582k (± 1.63%) 191,645k (± 1.25%) ~ 190,631k 196,543k p=0.093 n=6
Parse Time 1.35s (± 1.46%) 1.34s (± 0.91%) ~ 1.34s 1.37s p=0.655 n=6
Bind Time 0.73s (± 0.00%) 0.73s (± 0.00%) ~ 0.73s 0.73s p=1.000 n=6
Check Time 9.16s (± 0.42%) 9.13s (± 0.49%) ~ 9.08s 9.21s p=0.295 n=6
Emit Time 2.64s (± 0.50%) 2.63s (± 0.71%) ~ 2.61s 2.65s p=0.934 n=6
Total Time 13.87s (± 0.31%) 13.84s (± 0.34%) ~ 13.79s 13.93s p=0.418 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,381k (± 0.00%) 347,370k (± 0.00%) ~ 347,347k 347,389k p=0.261 n=6
Parse Time 2.45s (± 1.09%) 2.47s (± 0.21%) ~ 2.46s 2.47s p=0.084 n=6
Bind Time 0.95s (± 2.10%) 0.94s (± 0.00%) ~ 0.94s 0.94s p=0.176 n=6
Check Time 6.93s (± 0.37%) 6.93s (± 0.38%) ~ 6.90s 6.98s p=0.871 n=6
Emit Time 4.04s (± 0.46%) 4.04s (± 0.22%) ~ 4.03s 4.05s p=0.934 n=6
Total Time 14.37s (± 0.23%) 14.38s (± 0.17%) ~ 14.35s 14.42s p=0.466 n=6
TFS - node (v18.15.0, x64)
Memory used 302,561k (± 0.00%) 302,579k (± 0.01%) ~ 302,556k 302,636k p=0.109 n=6
Parse Time 2.00s (± 0.60%) 2.00s (± 1.21%) ~ 1.96s 2.03s p=0.607 n=6
Bind Time 1.00s (± 0.52%) 1.00s (± 0.41%) ~ 1.00s 1.01s p=0.114 n=6
Check Time 6.26s (± 0.42%) 6.26s (± 0.20%) ~ 6.24s 6.27s p=0.935 n=6
Emit Time 3.58s (± 0.45%) 3.58s (± 0.71%) ~ 3.55s 3.62s p=1.000 n=6
Total Time 12.83s (± 0.13%) 12.85s (± 0.29%) ~ 12.81s 12.90s p=0.627 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,530k (± 0.01%) 470,480k (± 0.01%) -50k (- 0.01%) 470,445k 470,523k p=0.031 n=6
Parse Time 2.57s (± 0.53%) 2.57s (± 0.35%) ~ 2.56s 2.58s p=0.560 n=6
Bind Time 1.00s (± 0.75%) 1.00s (± 0.75%) ~ 0.99s 1.01s p=0.487 n=6
Check Time 16.68s (± 0.28%) 16.67s (± 0.55%) ~ 16.57s 16.79s p=0.810 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.25s (± 0.16%) 20.24s (± 0.48%) ~ 20.13s 20.36s p=0.377 n=6
xstate - node (v18.15.0, x64)
Memory used 512,597k (± 0.01%) 512,623k (± 0.02%) ~ 512,528k 512,738k p=0.689 n=6
Parse Time 3.27s (± 0.19%) 3.27s (± 0.30%) ~ 3.26s 3.29s p=1.000 n=6
Bind Time 1.55s (± 0.26%) 1.55s (± 0.41%) ~ 1.54s 1.56s p=0.673 n=6
Check Time 2.85s (± 0.73%) 2.84s (± 0.44%) ~ 2.83s 2.86s p=0.870 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 6.19%) ~ 0.08s 0.09s p=0.174 n=6
Total Time 7.74s (± 0.28%) 7.74s (± 0.22%) ~ 7.72s 7.76s p=0.935 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

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@ahejlsberg Here are the results of running the user test suite comparing main and refs/pull/56207/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!

@SimpleCreations
Copy link

I wonder if this PR fixes #55666 which was also bisected to #52392 👀

@typescript-bot
Copy link
Collaborator

Hey @ahejlsberg, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@ahejlsberg
Copy link
Member Author

I wonder if this PR fixes #55666 which was also bisected to #52392 👀

Unfortunately, it does not.

@jakebailey
Copy link
Member

At this point, I wonder if we should just make intersectionState a required parameter just to be sure that it's propagated everywhere. I actually just tried that out locally and it didn't break any tests (which, makes sense as this PR only added tests).

@ahejlsberg
Copy link
Member Author

ahejlsberg commented Oct 24, 2023

At this point, I wonder if we should just make intersectionState a required parameter just to be sure that it's propagated everywhere

Not sure that would help. There are plenty of cases where we don't want to propagate it, so we'd just have to add a bunch of IntersectionState.None arguments. Plus there are three optional parameters preceding it for which arguments would also have to be added.

@ahejlsberg
Copy link
Member Author

Tests and performance look good. I think this one can be merged.

@jakebailey
Copy link
Member

At this point, I wonder if we should just make intersectionState a required parameter just to be sure that it's propagated everywhere

Not sure that would help. There are plenty of cases where we don't want to propagate it, so we'd just have to add a bunch of IntersectionState.None arguments. Plus there are three optional parameters preceding it for which arguments would also have to be added.

main...jakebailey:TypeScript:required-intersection-state has the change (which did require moving it up, of course); there are quite a few places that it's not propagated and I didn't hit any places where I needed to add .None besides the very top of the stack.

@ahejlsberg ahejlsberg merged commit 9302332 into main Nov 21, 2023
@ahejlsberg ahejlsberg deleted the fix53412 branch November 21, 2023 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team 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.

[5.0] Intersected array of intersection types with union field not compatible
4 participants