Skip to content

Clean up inference of type parameters, contravariant types #49915

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
Merged
merged 2 commits into from
Jul 19, 2022
Merged

Clean up inference of type parameters, contravariant types #49915

merged 2 commits into from
Jul 19, 2022

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jul 15, 2022 •
edited
Loading

These are some oddities and cleanups that I noticed in writing #49887 (see also #43962 (comment)).

Namely, inferFromContravariantTypes only did a contravariant inference if strictFunctionTypes was set, but that function call was used in many places that strictFunctionTypes should not be applied, like in the the alias type argument path that was the target of #49887.

I can't think of any particular test case that would be affected by this (probably, people always have this flag on, though notably not our own repo, oops), but it feels wrong.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 15, 2022
@jakebailey
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 15, 2022 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 15, 2022 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 15, 2022 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 15, 2022 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

@jakebailey
Great news! no new errors were found between main..refs/pull/49915/merge

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..49915

Metric main 49915 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 335,936k (± 0.01%) 335,885k (± 0.01%) -50k (- 0.01%) 335,847k 335,960k
Parse Time 2.06s (± 0.54%) 2.07s (± 0.61%) +0.01s (+ 0.39%) 2.05s 2.11s
Bind Time 0.90s (± 0.53%) 0.90s (± 0.41%) -0.00s (- 0.22%) 0.89s 0.90s
Check Time 5.79s (± 0.57%) 5.79s (± 0.43%) -0.01s (- 0.14%) 5.74s 5.85s
Emit Time 6.38s (± 0.78%) 6.39s (± 0.41%) +0.01s (+ 0.13%) 6.32s 6.44s
Total Time 15.13s (± 0.44%) 15.14s (± 0.31%) +0.01s (+ 0.07%) 15.07s 15.28s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,880k (± 0.01%) 192,878k (± 0.01%) -1k (- 0.00%) 192,819k 192,928k
Parse Time 0.85s (± 0.99%) 0.84s (± 0.43%) -0.01s (- 0.82%) 0.84s 0.85s
Bind Time 0.57s (± 0.78%) 0.57s (± 1.32%) +0.00s (+ 0.35%) 0.56s 0.59s
Check Time 6.71s (± 0.70%) 6.70s (± 0.54%) -0.01s (- 0.16%) 6.62s 6.79s
Emit Time 2.50s (± 1.01%) 2.48s (± 0.97%) -0.02s (- 0.76%) 2.44s 2.56s
Total Time 10.63s (± 0.63%) 10.60s (± 0.38%) -0.03s (- 0.27%) 10.51s 10.68s
Monaco - node (v14.15.1, x64)
Memory used 325,735k (± 0.01%) 325,720k (± 0.01%) -15k (- 0.00%) 325,669k 325,767k
Parse Time 1.57s (± 0.82%) 1.58s (± 0.62%) +0.00s (+ 0.25%) 1.56s 1.60s
Bind Time 0.78s (± 0.79%) 0.78s (± 0.61%) +0.00s (+ 0.51%) 0.78s 0.80s
Check Time 5.69s (± 0.49%) 5.69s (± 0.34%) -0.00s (- 0.02%) 5.66s 5.74s
Emit Time 3.35s (± 0.61%) 3.37s (± 0.80%) +0.02s (+ 0.69%) 3.32s 3.45s
Total Time 11.39s (± 0.27%) 11.42s (± 0.29%) +0.03s (+ 0.25%) 11.37s 11.54s
TFS - node (v14.15.1, x64)
Memory used 288,808k (± 0.01%) 288,817k (± 0.01%) +8k (+ 0.00%) 288,776k 288,845k
Parse Time 1.34s (± 1.84%) 1.33s (± 1.56%) -0.01s (- 0.82%) 1.28s 1.37s
Bind Time 0.75s (± 3.45%) 0.76s (± 3.57%) +0.01s (+ 1.07%) 0.73s 0.86s
Check Time 5.32s (± 0.36%) 5.33s (± 0.27%) +0.02s (+ 0.34%) 5.31s 5.38s
Emit Time 3.57s (± 2.23%) 3.61s (± 2.28%) +0.04s (+ 1.23%) 3.41s 3.73s
Total Time 10.97s (± 0.63%) 11.04s (± 0.81%) +0.06s (+ 0.56%) 10.79s 11.17s
material-ui - node (v14.15.1, x64)
Memory used 446,567k (± 0.04%) 446,661k (± 0.01%) +94k (+ 0.02%) 446,596k 446,774k
Parse Time 1.87s (± 0.43%) 1.87s (± 0.40%) -0.00s (- 0.11%) 1.85s 1.89s
Bind Time 0.72s (± 1.67%) 0.72s (± 1.01%) -0.01s (- 0.83%) 0.70s 0.73s
Check Time 13.19s (± 0.87%) 13.13s (± 0.59%) -0.06s (- 0.46%) 12.98s 13.31s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.78s (± 0.71%) 15.72s (± 0.47%) -0.07s (- 0.43%) 15.56s 15.91s
xstate - node (v14.15.1, x64)
Memory used 541,319k (± 0.01%) 541,297k (± 0.00%) -22k (- 0.00%) 541,226k 541,344k
Parse Time 2.60s (± 0.36%) 2.59s (± 0.50%) -0.01s (- 0.35%) 2.57s 2.63s
Bind Time 1.15s (± 1.04%) 1.15s (± 1.31%) -0.01s (- 0.69%) 1.10s 1.18s
Check Time 1.56s (± 0.49%) 1.54s (± 0.61%) -0.01s (- 0.84%) 1.53s 1.57s
Emit Time 0.07s (± 6.66%) 0.07s (± 3.14%) 🟩-0.00s (- 4.05%) 0.07s 0.08s
Total Time 5.39s (± 0.34%) 5.36s (± 0.39%) -0.03s (- 0.54%) 5.32s 5.42s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 49915 10
Baseline main 10

Developer Information:

Download Benchmark

@jakebailey jakebailey requested review from sandersn and weswigham July 19, 2022 21:37
weswigham
weswigham approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Assuming the extended test suites are fine, these changes look good to me.

@jakebailey
Copy link
Member Author

It was clean, yeah.

jakebailey
jakebailey commented Jul 19, 2022
View reviewed changes
src/compiler/checker.ts Outdated
@@ -30072,7 +30080,7 @@ namespace ts {

// Instantiate a generic signature in the context of a non-generic signature (section 3.8.5 in TypeScript spec)
function instantiateSignatureInContextOf(signature: Signature, contextualSignature: Signature, inferenceContext?: InferenceContext, compareTypes?: TypeComparer): Signature {
const context = createInferenceContext(signature.typeParameters!, signature, InferenceFlags.None, compareTypes);
const context = createInferenceContext(signature.typeParameters!, signature, inferenceContext?.flags ?? InferenceFlags.None, compareTypes);
Copy link
Member Author

Choose a reason for hiding this comment

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

Uh, I think this is cruft from my other PR. Will remove and retest.

@jakebailey
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this inline

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 19, 2022 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 19, 2022 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

@jakebailey
Great news! no new errors were found between main..refs/pull/49915/merge

@jakebailey jakebailey merged commit aba867d into microsoft:main Jul 19, 2022
@jakebailey jakebailey deleted the fix-43962-followup branch July 19, 2022 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weswigham weswigham weswigham approved these changes

@sandersn sandersn Awaiting requested review from sandersn

Assignees

@jakebailey jakebailey

Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakebailey @typescript-bot @weswigham