Recursive conditional types #40002
Merged
Recursive conditional types #40002
+1,392
−116
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
the
For Milestone Bug
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 7c4d923. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 7c4d923. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 7c4d923. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 7c4d923. You can monitor the build here. Update: The results are in! |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@ahejlsberg Here they are:Comparison Report - master..40002
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham
approved these changes
Aug 11, 2020
src/compiler/checker.ts
Outdated
| @@ -19533,7 +19494,7 @@ namespace ts { | |||
| priority = savePriority; | |||
| } | |||
|
|
|||
| function invokeOnce(source: Type, target: Type, action: (source: Type, target: Type) => void) { | |||
| function invokeWithDepthLimit(source: Type, target: Type, action: (source: Type, target: Type) => void) { | |||
There was a problem hiding this comment.
This is pretty much the inference equivalent of recursiveTypeRelatedTo at this point.
| } | ||
| if (type.flags & TypeFlags.Conditional) { | ||
| // The root object represents the origin of the conditional type | ||
| return (type as ConditionalType).root; |
There was a problem hiding this comment.
Huh, and this doesn't affect the user baselines or DT?
There was a problem hiding this comment.
No. Previously conditional types couldn't be recursive, so they weren't included here. But for that same reason we have no tests that could be affected by this.
|
Awesome to finally see this! |
|
@ahejlsberg maybe something worthwhile is to build another PR on top of this to see if changing the definition of |
|
The tests revealed OOMs in a few projects due to the switch to use Intuitively, in inference we want to terminate when we encounter a duplicate attempt to infer from source and target types with the same origin, so |
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at fed0e8c. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at fed0e8c. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..40002
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DanielRosenwasser
reviewed
Aug 14, 2020
| if (sourceIdentity) (sourceStack || (sourceStack = [])).push(sourceIdentity); | ||
| if (targetIdentity) (targetStack || (targetStack = [])).push(targetIdentity); |
There was a problem hiding this comment.
Suggested change
| if (sourceIdentity) (sourceStack || (sourceStack = [])).push(sourceIdentity); | |
| if (targetIdentity) (targetStack || (targetStack = [])).push(targetIdentity); | |
| if (sourceIdentity) (sourceStack ||= []).push(sourceIdentity); | |
| if (targetIdentity) (targetStack ||= []).push(targetIdentity); |
There was a problem hiding this comment.
I'm going to leave it for now. We have lots of occurrences of that pattern, so maybe another PR to clean them all up.
|
@ahejlsberg According to your example in the OP, this also fixes #26223 |
|
Test runs all look clean. Slight regression in check time for material-ui, but it's worth it for the added precision in type inference. |
4 tasks
|
Does this change mean we no longer need hacks like |
10 tasks
|
Can we get a playground for this PR? I'd like to play around with the new options this gives us. |
|
@tjjfvi you can choose "Nightly" version |
why the max number is 43 ? not 48 ? not 98 ? |
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.
With this PR we officially support recursive conditional types. For example:
A bit of context... When conditional types were first introduced, we explicitly restricted them to be non-recursive, i.e. we made it an error for a conditional type to directly or indirectly reference itself. This restriction was put in place primarily as a safeguard against runaway infinite recursion which the compiler didn't handle well at the time. However, it turns out it was still possible to construct recursive conditionally resolved types by combining object types (which have deferred resolution of property types) and conditional types using indexed access types (see #14833 for the core idea). In spite of being cumbersome and non-intuitive, this trick has become commonplace in several libraries. Consequently, over time we have "hardened" the compiler against infinite recursion with depth limiters in relationships, type inference, type instantiation, constraint computation, and so on. We're now at a point where it seems reasonable support an intuitive way of writing recursive conditional types.
Some more examples:
Note that this PR doesn't change the recursion depth limits that are already in place. For example, an error is reported on
T4above because its resolution exceeds the limit of 50 nested type instantiations.The type inference streamlining contained in the PR fixes several issues with inference to recursive types. For example:
Previously, only the
unbox(b1)call produced the expected type inference.Fixes #26223.
Fixes #26980.
Fixes #37801.