-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Conditional type simplifications & Globally cached conditional type instances #29437
Conditional type simplifications & Globally cached conditional type instances #29437
Conversation
…nd fix restrictive instantiations
Does this mean |
src/compiler/checker.ts
Outdated
@@ -9982,6 +9982,26 @@ namespace ts { | |||
if (checkType === wildcardType || extendsType === wildcardType) { | |||
return wildcardType; | |||
} | |||
// Simplifications for types of the form `T extends U ? U : never` and `T extends U ? never : T`. |
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.
First form should be T extends U ? T : never
.
src/compiler/checker.ts
Outdated
// Simplifications for types of the form `T extends U ? U : never` and `T extends U ? never : T`. | ||
const originalCheckType = getActualTypeVariable(root.checkType); | ||
const trueType = instantiateType(root.trueType, mapper); | ||
const falseType = instantiateType(root.falseType, mapper); |
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.
These instantiations were always deferred before. Now they're eager. I'm somewhat concerned this creates extra work that we don't need to do.
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.
I could defer them to only when simplified again, but then only types with a declared never
would be simplifiable, whereas like this one branch can be instantiated to never, allowing a broader set of conditional types to be reduced.
src/compiler/checker.ts
Outdated
if (isTypeAssignableTo(getRestrictiveInstantiation(checkType), getRestrictiveInstantiation(extendsType))) { // Always true | ||
return getIntersectionType([trueType, extendsType]); | ||
} | ||
else if (getUnionType([getIntersectionType([checkType, extendsType]), neverType]).flags & TypeFlags.Never) { // Always false |
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.
Shouldn't this check the permissive form of the check and extends types? I'm not really following the logic here.
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.
So, the permissive form check returns true
for X extends never
, while this version of the check does not. We want to ignore the never extends never case here, since that will become a never
result in both branches anyway (meaning we'd like x extends never or x extends something instantiable to never to simplify even when the permissive instantiation says there's technically one instantiatiom which'd give another result).
With negated types in place, I'd probably write the check as isTypeAssignableTo(checkType, getNegatedType(extendsType))
instead, but without that in place, this accomplishes the same thing when only normal types exist.
Yeah - |
Done~ |
@ahejlsberg I've run this on user, RWC, and DT suites - there's no visible adverse effects. I think this is solid. |
Split the bugfix part into a separate PR for 3.3 👍 |
And rename a thing |
...wait, why? Intuitively I expect that to simplify to |
@fatcerberus It is best not to think of |
@typescript-bot test this & @typescript-bot run dt |
Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at 47fbc47. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the extended test suite on this PR at 47fbc47. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot run dt - I just made some changes after running into a manifestation of #30152 with the new simplifications on DT and included a small workaround. |
Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at 8268eeb. You can monitor the build here. It should now contribute to this PR's status checks. |
Sweet, the only change on DT is now a @ahejlsberg can we look at merging this again? #28748 is a pretty popular issue to fix and the global caching of conditionals should be a nice perf win. |
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.
Add a getConditionalTypeWorker
that doesn't have to worry about caching.
@ahejlsberg done. |
It's too bad that intersections of known concrete object types don't reduce to their mapped identity type automatically (e.g., type OldExtract<T, U> = T extends U ? { [K in keyof T]: T[K] } : never maybe? I'm also not sure how |
(this is not a full answer, just a counter-example) |
Yeah, I'll buy that... optional properties are weird. (The value |
Fixes #28748
Conditional types of the form
T extends U ? T : never
simplify toT & U
ifT
always extendsU
, andnever
ifT
can never extendU
.Conditional types of the form
T extends U ? never : T
simplify toT
(T & not U
to be more precise, once those are available) ifT
never extendsU
, andnever
ifT
always extendsU
.