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

Add fallback when both co- and contra-variant inference candidates exist #54072

Merged
merged 3 commits into from
May 2, 2023
Merged

Add fallback when both co- and contra-variant inference candidates exist #54072

merged 3 commits into from
May 2, 2023

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Apr 30, 2023 •
edited
Loading

In cases where both co- and contra-variant inference candidates exist, this PR adds the ability to fall back to the secondary inference when the constraint check fails for the primary inference.

Fixes #54005.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Apr 30, 2023
@ahejlsberg
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 30, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 30, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 30, 2023 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 30, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 30, 2023 •
edited
Loading

Heya @ahejlsberg, I've started to run the extended test suite on this PR at f0f477e. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Package install failed"

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

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

@jakebailey
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 1, 2023 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 1, 2023 •
edited
Loading

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/153602/artifacts?artifactName=tgz&fileId=F183196F0EFF7B9788739321E9D293638E4A21829C7352033326EE52B01E173002&fileName=/typescript-5.1.0-insiders.20230501.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.1.0-pr-54072-11".;

jakebailey
jakebailey reviewed May 1, 2023
View reviewed changes
src/compiler/checker.ts Outdated
Comment on lines 25170 to 25171
every(context.inferences, other => other === inference ||
getConstraintOfTypeParameter(other.typeParameter) !== inference.typeParameter ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of #52180 was motivated by noticing that when we treat the "current" inference as special, we incur some order-dependent behavior. Where this came up (if my memory was correct) was when I was editing the strictFunctionTypes PR, where I could edit the file and see errors change around non-deterministically.

I tested restoring the change from #52180 and this PR still passes (i.e. putting back other !== inference && getConstraintOfTypeParameter(other.typeParameter) !== inference.typeParameter), which makes me feel like reverting it back to the state in #52123 that skips over other === inference will bring back that oddity (and we just don't have a test for it).

I'll try and see if I can make it break again like I did the first time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out we deleted the code where I was able to reproduce this, so, I guess I don't really know anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking carefully at the code you had in #52180 and the original code plus the line I added here, I've convinced myself they're just two ways of saying the same thing. So I'll restore the change from #52180.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! You're right, I missed that new line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'm happy with whichever is more performant or clearer. They're definitely equivalent now that I think closely.

@ahejlsberg
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 2, 2023 •
edited
Loading

Heya @ahejlsberg, I've started to run the extended test suite on this PR at c377c44. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 2, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 2, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 2, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 2, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Package install failed"

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..54072

Metric main 54072 Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 365,246k (± 0.01%) 365,264k (± 0.00%) ~ 365,246k 365,293k p=0.229 n=6
Parse Time 3.55s (± 0.75%) 3.54s (± 0.39%) ~ 3.52s 3.55s p=0.563 n=6
Bind Time 1.17s (± 0.00%) 1.17s (± 0.35%) ~ 1.17s 1.18s p=0.405 n=6
Check Time 9.56s (± 0.29%) 9.56s (± 0.40%) ~ 9.50s 9.60s p=0.936 n=6
Emit Time 7.90s (± 0.50%) 7.93s (± 0.60%) ~ 7.87s 7.99s p=0.376 n=6
Total Time 22.19s (± 0.12%) 22.20s (± 0.24%) ~ 22.14s 22.29s p=0.744 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 192,765k (± 0.04%) 192,717k (± 0.04%) ~ 192,654k 192,851k p=0.298 n=6
Parse Time 1.59s (± 0.34%) 1.59s (± 0.40%) ~ 1.58s 1.60s p=0.201 n=6
Bind Time 0.82s (± 0.50%) 0.83s (± 1.01%) ~ 0.82s 0.84s p=0.527 n=6
Check Time 10.30s (± 0.58%) 10.32s (± 0.82%) ~ 10.20s 10.43s p=0.936 n=6
Emit Time 3.02s (± 0.44%) 3.02s (± 1.37%) ~ 2.98s 3.08s p=0.935 n=6
Total Time 15.74s (± 0.45%) 15.76s (± 0.76%) ~ 15.58s 15.92s p=0.873 n=6
Monaco - node (v16.17.1, x64)
Memory used 345,849k (± 0.01%) 345,854k (± 0.00%) ~ 345,825k 345,871k p=0.872 n=6
Parse Time 2.72s (± 0.45%) 2.71s (± 0.28%) ~ 2.70s 2.72s p=0.240 n=6
Bind Time 1.08s (± 0.70%) 1.08s (± 0.48%) ~ 1.08s 1.09s p=0.784 n=6
Check Time 7.90s (± 0.19%) 7.88s (± 0.22%) ~ 7.85s 7.90s p=0.321 n=6
Emit Time 4.46s (± 0.98%) 4.48s (± 0.76%) ~ 4.44s 4.54s p=0.332 n=6
Total Time 16.16s (± 0.38%) 16.15s (± 0.26%) ~ 16.09s 16.21s p=0.872 n=6
TFS - node (v16.17.1, x64)
Memory used 300,107k (± 0.01%) 300,097k (± 0.01%) ~ 300,055k 300,136k p=0.748 n=6
Parse Time 2.15s (± 0.54%) 2.15s (± 0.87%) ~ 2.13s 2.18s p=0.461 n=6
Bind Time 1.24s (± 0.97%) 1.24s (± 0.88%) ~ 1.23s 1.25s p=0.666 n=6
Check Time 7.29s (± 0.36%) 7.26s (± 0.27%) ~ 7.23s 7.28s p=0.125 n=6
Emit Time 4.38s (± 0.80%) 4.37s (± 0.57%) ~ 4.34s 4.40s p=0.808 n=6
Total Time 15.06s (± 0.42%) 15.01s (± 0.14%) ~ 14.97s 15.03s p=0.570 n=6
material-ui - node (v16.17.1, x64)
Memory used 481,593k (± 0.00%) 481,624k (± 0.00%) +31k (+ 0.01%) 481,607k 481,644k p=0.025 n=6
Parse Time 3.24s (± 0.34%) 3.24s (± 0.48%) ~ 3.21s 3.25s p=0.863 n=6
Bind Time 0.94s (± 1.42%) 0.93s (± 0.00%) ~ 0.93s 0.93s p=0.176 n=6
Check Time 17.88s (± 0.73%) 17.89s (± 0.36%) ~ 17.78s 17.95s p=1.000 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 22.07s (± 0.64%) 22.06s (± 0.31%) ~ 21.96s 22.12s p=0.936 n=6
xstate - node (v16.17.1, x64)
Memory used 560,355k (± 0.01%) 560,380k (± 0.01%) ~ 560,296k 560,450k p=0.575 n=6
Parse Time 3.98s (± 0.31%) 3.98s (± 0.19%) ~ 3.97s 3.99s p=0.505 n=6
Bind Time 1.76s (± 0.46%) 1.76s (± 0.29%) ~ 1.75s 1.76s p=0.929 n=6
Check Time 3.06s (± 0.52%) 3.07s (± 0.74%) ~ 3.05s 3.11s p=0.304 n=6
Emit Time 0.09s (± 0.00%) 0.09s (± 0.00%) ~ 0.09s 0.09s p=1.000 n=6
Total Time 8.88s (± 0.20%) 8.89s (± 0.26%) ~ 8.87s 8.93s p=0.460 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-148-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current 54072 6
Baseline main 6

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

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

@ahejlsberg ahejlsberg merged commit ae6393e into main May 2, 2023
@ahejlsberg ahejlsberg deleted the fix54005 branch May 2, 2023 13:39
@Andarist Andarist mentioned this pull request Mar 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakebailey jakebailey jakebailey approved these changes

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

Assignees

@ahejlsberg ahejlsberg

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inference regression (5.0.4 vs 4.9.5) with nullable enum function parameter
3 participants
@ahejlsberg @typescript-bot @jakebailey