Handle recursive type references up to a certain level of expansion in inference #38011
Conversation
This file contains hidden or 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
weswigham
commented
Apr 16, 2020
src/compiler/checker.ts
Outdated
| @@ -18822,15 +18839,27 @@ namespace ts { | |||
| // its symbol with the instance side which would lead to false positives. | |||
| const isNonConstructorObject = target.flags & TypeFlags.Object && | |||
| !(getObjectFlags(target) & ObjectFlags.Anonymous && target.symbol && target.symbol.flags & SymbolFlags.Class); | |||
| const symbolOrType = isNonConstructorObject ? isTupleType(target) ? target.target : target.symbol : undefined; | |||
| const symbolOrType = getObjectFlags(target) & ObjectFlags.Reference && (target as TypeReference).node ? getNormalizedType(target, /*writing*/ false) : isNonConstructorObject ? isTupleType(target) ? target.target : target.symbol : undefined; | |||
There was a problem hiding this comment.
This line (and the bail without setting Circularity below) essentially "undoes" #33678 insofar as if I only made this change, #37982 would be fixed (as now we'd explore all recursive type references so long as their normalized forms aren't identical), however doing just that reintroduces the issue #33678 fixed, since in that case, we have something like type T = [A<NonNullable<T>>] which expands to [A<NonNullable<NonNullable<T>>>] and so on. As it expands, at no point are the types "identical" (notable future work: collapsing noop conditional types like repeated NonNullable!), in fact, all that's shared is that the type reference is sourced from the same .target and .node. So when it was eliminating the recursion based on .target alone. it was essentially saying "you can't infer to a 1-tuple while inferring to a 1-tuple, if that 1-tuple is marked as recursive" (which, while in line with how limited inference is for other symbol-based object types without #31633, is a regression in behavior for recursive type references), which, in turn, meant you couldn't infer to, say, the second Q reference of [Q<string>, [Q<boolean | number>]] if Q caused the containing tuples to be marked as potentially recursive!
All this to say: the change in behavior was a direct consequence of two things: #33678 limited recursive type reference inference, and recursive type references are made syntactically, so they way they are declared changes if they're marked as potentially recursive or not, resulting in the differences noted in #37982. So the bulk of this change is about improving #33678 to allow exploring multiple instances of the "same" recursive type reference, so that syntactic determination if the reference is possibly recursive or not cannot be observed (up to a limit).
|
@typescript-bot user test this |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 73317dc. You can monitor the build here. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 73317dc. You can monitor the build here. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 73317dc. You can monitor the build here. |
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 73317dc. 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. |
|
RWC and user baselines look fine, just waiting for DT and perf to come back~ |
|
@weswigham Here they are:Comparison Report - master..38011
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perf and DT both look good, too~ |
|
ping @ahejlsberg |
ahejlsberg
approved these changes
Jun 24, 2020
cangSDARM
added a commit
to cangSDARM/TypeScript
that referenced
this pull request
Jun 28, 2020
* upstream/master: LEGO: check in for master to temporary branch. Preserve newlines between try/catch/finally, if/else, do/while (microsoft#39280) not narrow static property without type annotation in constructor. (microsoft#39252) Switch to ES Map/Set internally (microsoft#33771) fix(38840): omit completions for a spread like argument in a function definition (microsoft#38897) fix(38785): include in NavigationBar child items from default exported functions (microsoft#38915) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Avoid effect of element access expression (microsoft#39174) Update typescript-eslint to 3.4.1-alpha.1 (microsoft#39260) Handle 'keyof' for generic tuple types (microsoft#39218) Disable unsound T[K] rule in subtype relations (microsoft#39249) LEGO: check in for master to temporary branch. Upgrade typescript-eslint version (microsoft#39242) Handle recursive type references up to a certain level of expansion in inference (microsoft#38011) Do not consider binding patterns in contextual types for return type inference where all the signature type parameters have defaults (microsoft#39081) LEGO: check in for master to temporary branch. # Conflicts: # src/compiler/program.ts # src/compiler/types.ts
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 #37982, which regressed with #33678 which this modifies.
When #33678 moved inference of recursive type references into
inferFromObjectTypesWorker, it implicitly forbade all nested inferences from nested recursive types of the same target. Eg, if you have aRecursive<string | Recursive<boolean>>, it prevented any inference from occurring on the inner reference. With this change to the circularity checking logic ininferFromObjectTypes(and expandedisDeeplyNestedTypeto handle expanding recursive type references), we can now make inferences multiple times as the reference expands (if it does), up to a limit. (Namely 5 expansions - the same limit we have in place in relationship checking).