Tweak co/contra inference logic from #52123 #52180
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 22cfe3a. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 22cfe3a. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 22cfe3a. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 22cfe3a. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 22cfe3a. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 22cfe3a. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
This PR turns out to only fix some of the examples in the linked issue.
|
|
Interesting situation... Co-variant inferences have no common supertype, so we pick the first in anticipation of an error being issued as a result. However, we could have succeeded by picking the contra-variant inference. Yup, we could do better here. I like your proposed fix. |
ahejlsberg
approved these changes
Jan 10, 2023
|
All user/top100/etc tests are clean; waiting on perf tests though since I have to merge a fix to main first to unblock it. |
|
@typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 60e15f3. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 60e15f3. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
11 tasks
|
Just tested this on #49929 and it works really well; even more cases I didn't notice are fixed, including inference I worked around with this: function castToNodeArray<T extends Node>(nodes: readonly T[]): NodeArray<T> {
return cast<NodeArray<T>, readonly T[]>(nodes, isNodeArray);
}I'll add these newly found examples as test cases here. |
|
Nah, nevermind; forgot that it was to do with the fact that I had to change cast's signature until #52123, so the above code was already not needed even before this PR, which is more about the union thing when there's ambiguity. |
|
@jakebailey Here they are:
CompilerComparison Report - main..52180
System
Hosts
Scenarios
TSServerComparison Report - main..52180
System
Hosts
Scenarios
StartupComparison Report - main..52180
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perf looks the same (normal noise). Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #52179
In the case of
coAndContraVariantInferences4, we end up withModifieras the covariant type, but that isn't a subtype of bothModifierandDecorator, so it shouldn't be chosen; we instead wantNode, the contravariant inference. But, if you flip things and test forDecoratorinstead,Modifieris still the covariant inference, but now it doesn't matchDecorator, so we get the desired result,Node.This tweaks the logic from #52123 by ensuring that even the current inference's candidates are checked against the inferred covariant type; this effectively removes the problematic choice for the covariant type produced via union ordering.
I'm not an expert; maybe the better win is to change
getCovariantInferencesomehow. I just tried this and was surprised that it fixed all of my examples and broke no other test.