Add missing relationship allowing a type to be assignable to a conditional when assignable to both branches #30639
Conversation
…e to both branches of a conditional
weswigham
force-pushed
the
conditional-both-branch-relation
branch
from
7344f40
to
346d1f9
Compare
|
Edit: I don’t think there is anything serious going on here; I’m just thinking that this is a new manifestation of the behaviour in #30020. Can you check this example on your branch? function foo<T>(x: T, y: ([T] extends [string] ? string : boolean) extends boolean ? 1 : 2, z: 2) {
y = z; // should error, might not.
}
foo(true, 1, 2) |
|
It in fact does not error like it should at present - I started looking into it this afternoon, and fixing #30708 (and its extension in this PR to generic conditionals) looks like it's going to need some revisions to how we measure "permissive" instantiations (a specially-handled |
Awesome stuff! This work here also breaks stuff down into positive and negative occurrences of ?/ |
There was a problem hiding this comment.
I noticed a while back that you can’t make a React element with a generic TagName due to a conditional type in LibraryManagedAttributes (a pattern which, by the way, I do not particularly condone, but was looking into as, say, “opposition research” for an alternative approach), so I tested this to see if it helps with that problem. It does get one step closer, but on second look, clearly components are not going to be assignable to both sides of that large nested conditional.
Nonetheless, this seems like a win—any reason we shouldn’t merge it? Is there still interest in #27932 (which IIUC would supersede the changes here)?
By the way, there aren’t merge conflicts but it doesn’t build as-is as trueType and falseType have been renamed.
|
👏 👏 |
|
Cross-posting from #31277. Might be worth running user tests and DT on this. |
|
@weswigham
Does that sound right to you? |
|
@weswigham now that the perf test suite has material-ui, can you merge from master and summarise the results of the perf test? |
weswigham
force-pushed
the
conditional-both-branch-relation
branch
from
80226c8
to
1024ad1
Compare
weswigham
force-pushed
the
conditional-both-branch-relation
branch
from
1024ad1
to
786e5a4
Compare
|
@weswigham would you be able to get this up-to-date with master? Let's take this for 4.3 -- early so that we have a chance to shake out any subtle problems. |
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 8642a5f. You can monitor the build here. Update: The results are in! |
|
@sandersn Here they are:Comparison Report - master..30639
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Performance still looks fine. I'm going to merge this since it's ready. |
|
Anyone coming back here wondering why this doesn't work for distributive conditional types: it was reverted in #46429 (which is of course linked above) |
Fixes #29505
Fixes #29662
Fixes #26933
Fixes #25413 (with the caveat that the conditional in the example needs to be non-distributive)
This is super simple compared to #27932. To account for distributivity, we simply instantiate the distributed type with a replacement mapper so we're always comparing with "some arbitrary subtype" of it. Combined with our newer conditional type simplification rules, this seems to handle all the common cases people have submitted. ❤️
The place where this differs from #27932 is the
never-case handling for distributive conditionals. Unlike that PR, this PR chooses to say that even for distributive conditionals, so long as you're assignable to all possible types for both branches, you're assignable to the conditional. Nothing is assignable toneverother thannever, so that case of the distributive type really stands out. It almost feels likeT extends R ? A : BwhenT=nevershould actually reduce toA&Brather thannever- for most conditionals this does end up essentially beingneveranyway (since most conditionals "filter" types), but for a certain class of "dumb" conditionals which map rather than filer, that type is still useful, and actually seems to be the real bound of the type (unlikenever, which is too far down the matrix, as it were).