-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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.
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
Simplify relationship check for conditional type on target side #46429
Conversation
I should mention that with this PR, the check time for this repro drops from ~10s to about 0.4s. |
@typescript-bot test this |
Heya @andrewbranch, I've started to run the inline community code test suite on this PR at 75ab347. You can monitor the build here. Update: The results are in! |
Heya @andrewbranch, I've started to run the extended test suite on this PR at 75ab347. You can monitor the build here. |
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 75ab347. You can monitor the build here. |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 75ab347. You can monitor the build here. |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 75ab347. You can monitor the build here. Update: The results are in! |
Hurray, I helped find something maybe useful! |
@andrewbranch |
@@ -48,7 +55,13 @@ tests/cases/compiler/conditionalTypeAssignabilityWhenDeferred.ts(116,3): error T | |||
function f<T>(t: T) { | |||
var x: T | null = Math.random() > 0.5 ? null : t; | |||
onlyNullablePlease(x); // should work | |||
~ | |||
!!! error TS2345: Argument of type 'T | null' is not assignable to parameter of type 'null extends T | null ? any : never'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is interesting - is this actually a distributive conditional type? Why is it deferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always defer if either of the check and extends types contains generics. This is somewhat of a blunt instrument, and we could possibly do better in cases like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And by "contains generics" I mean we defer when isGenericType
returns true for either of the check and extends types.
This is impossible in the general case - any condition with an |
@andrewbranch Here they are:Comparison Report - main..46429
System
Hosts
Scenarios
Developer Information: |
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 75ab347. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 75ab347. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the tarball bundle task on this PR at 75ab347. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 75ab347. You can monitor the build here. |
Agreed, and so be it. But it does seem like there are classes of cases where we ought to do better in conditional type construction. Both of the test cases that revert back to failing with this PR seem quite trivial--for example, checking if the check and extends types are identical, or if the extends type is a union that contains the check type, would cause us to resolve the conditional type to the true case. |
Hey @ahejlsberg, 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 |
@andrewbranch I've checked against my reduced repro from this comment as well as https://github.com/jonohill/tsc-bug-44851 and https://github.com/aarbmx6s/vscode-ajv-issue. Performance issues are gone in all three. Not sure what's up with @amcasey's repro. |
I'll reiterate that I'm with ignoring my repro since I probably made nonsense out of the logic while tinkering with it. Obviously, never being slow is the ideal, but it's more important to focus on code people might actually write. Alternatively, we could call this good and file a separate bug to go investigate that repro. |
Please, no more plot twists, my heart can't handle it anymore. |
Yeah, if the real world code is fast then I’m happy. Just wanted to make sure it was checked against the latest changes. |
is there an example of a type that is no longer allowed as of this PR? the typescript 4.5 blog post mentions this PR
but i can't seem to find a minimal example of what this means exactly |
@DetachHead good news (??), we found one: #47127 (comment) |
This is a BAD news for me. I deeply dependency this feature. Because of this breaking change, I cant update TypeScript, it's so bad for me, could you add a compiler option to support it as a migrate plan? |
…osoft#46429) * Simplify relationship check for conditional type on target side * Accept new baselines * Better support for non-distribution-dependent types * Accept new API baselines * Accept new baselines
This PR simplifies our relationship checking logic for conditional types on the target side. With this PR, a source type
S
is assignable to a target typeA extends B ? C : D
whenA
is never referenced inC
orD
, andB
has noinfer
positions, andS
is assignable toC
(except ifA
never assignable toB
for any instantiation), andS
is assignable toD
(except ifA
is always assignable toB
for any instantiation).The PR revises some of the logic that was added in #30639, specifically the logic that determines if
A
is referenced inC
orD
in a distributive conditional type. There are no baseline changes as a result of this PR, but it fixes the performance degradation reported in #44851.Fixes #44851.