Skip to content

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

Merged
Anders Hejlsberg (ahejlsberg) merged 3 commits into
mainfrom
fix54005
May 2, 2023
Merged

Add fallback when both co- and contra-variant inference candidates exist#54072
Anders Hejlsberg (ahejlsberg) merged 3 commits into
mainfrom
fix54005

Conversation

@ahejlsberg

@ahejlsbergAnders Hejlsberg (ahejlsberg) commented Apr 30, 2023

Copy link
Copy Markdown
Member

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.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 30, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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

TypeScript Bot (typescript-bot) commented Apr 30, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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

TypeScript Bot (typescript-bot) commented Apr 30, 2023

Copy link
Copy Markdown
Contributor

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

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 30, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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

TypeScript Bot (typescript-bot) commented Apr 30, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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
Copy Markdown
Contributor

Anders Hejlsberg (@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
Copy Markdown
Contributor

Anders Hejlsberg (@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
Copy Markdown
Contributor

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

@jakebailey

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented May 1, 2023

Copy link
Copy Markdown
Contributor

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

@typescript-bot

TypeScript Bot (typescript-bot) commented May 1, 2023

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/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".;

Comment threadsrc/compiler/checker.ts Outdated
Comment on lines +25170 to +25171
every(context.inferences, other => other === inference ||
getConstraintOfTypeParameter(other.typeParameter) !== inference.typeParameter ||

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.

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

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

@typescript-bot

TypeScript Bot (typescript-bot) commented May 2, 2023

Copy link
Copy Markdown
Contributor

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

@typescript-bot

TypeScript Bot (typescript-bot) commented May 2, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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

TypeScript Bot (typescript-bot) commented May 2, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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

TypeScript Bot (typescript-bot) commented May 2, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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

TypeScript Bot (typescript-bot) commented May 2, 2023

Copy link
Copy Markdown
Contributor

Heya Anders Hejlsberg (@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
Copy Markdown
Contributor

Anders Hejlsberg (@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
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..54072

Metricmain54072DeltaBestWorstp-value
Angular - node (v16.17.1, x64)
Memory used365,246k (± 0.01%)365,264k (± 0.00%)~365,246k365,293kp=0.229 n=6
Parse Time3.55s (± 0.75%)3.54s (± 0.39%)~3.52s3.55sp=0.563 n=6
Bind Time1.17s (± 0.00%)1.17s (± 0.35%)~1.17s1.18sp=0.405 n=6
Check Time9.56s (± 0.29%)9.56s (± 0.40%)~9.50s9.60sp=0.936 n=6
Emit Time7.90s (± 0.50%)7.93s (± 0.60%)~7.87s7.99sp=0.376 n=6
Total Time22.19s (± 0.12%)22.20s (± 0.24%)~22.14s22.29sp=0.744 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used192,765k (± 0.04%)192,717k (± 0.04%)~192,654k192,851kp=0.298 n=6
Parse Time1.59s (± 0.34%)1.59s (± 0.40%)~1.58s1.60sp=0.201 n=6
Bind Time0.82s (± 0.50%)0.83s (± 1.01%)~0.82s0.84sp=0.527 n=6
Check Time10.30s (± 0.58%)10.32s (± 0.82%)~10.20s10.43sp=0.936 n=6
Emit Time3.02s (± 0.44%)3.02s (± 1.37%)~2.98s3.08sp=0.935 n=6
Total Time15.74s (± 0.45%)15.76s (± 0.76%)~15.58s15.92sp=0.873 n=6
Monaco - node (v16.17.1, x64)
Memory used345,849k (± 0.01%)345,854k (± 0.00%)~345,825k345,871kp=0.872 n=6
Parse Time2.72s (± 0.45%)2.71s (± 0.28%)~2.70s2.72sp=0.240 n=6
Bind Time1.08s (± 0.70%)1.08s (± 0.48%)~1.08s1.09sp=0.784 n=6
Check Time7.90s (± 0.19%)7.88s (± 0.22%)~7.85s7.90sp=0.321 n=6
Emit Time4.46s (± 0.98%)4.48s (± 0.76%)~4.44s4.54sp=0.332 n=6
Total Time16.16s (± 0.38%)16.15s (± 0.26%)~16.09s16.21sp=0.872 n=6
TFS - node (v16.17.1, x64)
Memory used300,107k (± 0.01%)300,097k (± 0.01%)~300,055k300,136kp=0.748 n=6
Parse Time2.15s (± 0.54%)2.15s (± 0.87%)~2.13s2.18sp=0.461 n=6
Bind Time1.24s (± 0.97%)1.24s (± 0.88%)~1.23s1.25sp=0.666 n=6
Check Time7.29s (± 0.36%)7.26s (± 0.27%)~7.23s7.28sp=0.125 n=6
Emit Time4.38s (± 0.80%)4.37s (± 0.57%)~4.34s4.40sp=0.808 n=6
Total Time15.06s (± 0.42%)15.01s (± 0.14%)~14.97s15.03sp=0.570 n=6
material-ui - node (v16.17.1, x64)
Memory used481,593k (± 0.00%)481,624k (± 0.00%)+31k (+ 0.01%)481,607k481,644kp=0.025 n=6
Parse Time3.24s (± 0.34%)3.24s (± 0.48%)~3.21s3.25sp=0.863 n=6
Bind Time0.94s (± 1.42%)0.93s (± 0.00%)~0.93s0.93sp=0.176 n=6
Check Time17.88s (± 0.73%)17.89s (± 0.36%)~17.78s17.95sp=1.000 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time22.07s (± 0.64%)22.06s (± 0.31%)~21.96s22.12sp=0.936 n=6
xstate - node (v16.17.1, x64)
Memory used560,355k (± 0.01%)560,380k (± 0.01%)~560,296k560,450kp=0.575 n=6
Parse Time3.98s (± 0.31%)3.98s (± 0.19%)~3.97s3.99sp=0.505 n=6
Bind Time1.76s (± 0.46%)1.76s (± 0.29%)~1.75s1.76sp=0.929 n=6
Check Time3.06s (± 0.52%)3.07s (± 0.74%)~3.05s3.11sp=0.304 n=6
Emit Time0.09s (± 0.00%)0.09s (± 0.00%)~0.09s0.09sp=1.000 n=6
Total Time8.88s (± 0.20%)8.89s (± 0.26%)~8.87s8.93sp=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)
BenchmarkNameIterations
Current540726
Baselinemain6

Developer Information:

Download Benchmark

@typescript-bot

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@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
Copy Markdown
Contributor

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

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.

Inference regression (5.0.4 vs 4.9.5) with nullable enum function parameter

3 participants

@ahejlsberg@typescript-bot@jakebailey