Cutoff reverse mapped type recursion #53595
Draft
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
typescript-bot
added
Author: Team
For Milestone Bug
Member
Author
|
@typescript-bot perf test this |
Collaborator
|
Heya @weswigham, I've started to run the perf test suite on this PR at c472b67. You can monitor the build here. Update: The results are in! |
weswigham
commented
Mar 30, 2023
| const newTypeParam = (constraint.type as IndexedAccessType).objectType; | ||
| const newMappedType = replaceIndexedAccess(target, constraint.type as ReplaceableIndexedAccessType, newTypeParam); | ||
| target = newMappedType as MappedType; | ||
| constraint = getIndexType(newTypeParam) as IndexType; |
Member
Author
There was a problem hiding this comment.
By moving this logic from reverse mapped type property construction forward into reverse mapped type construction, we can cut down on the number of reverse mapped type identities (and property symbols) we make - this is critical to actually being able to block the recursion, since it makes a generative recursion into a self recursion.
weswigham
commented
Mar 30, 2023
| // Since `inferReverseMappedType` avoids pulling on structure in its inference result, we *shouldn't* have a recursive | ||
| // type resolution request, but if we somehow do, we should error. | ||
| if (!pushTypeResolution(symbol, TypeSystemPropertyName.Type)) { | ||
| reportCircularityError(symbol); |
Member
Author
There was a problem hiding this comment.
In the current version of this PR, this error is never reported, since I've avoided pulling on the structure elsewhere, but this is what's needed to prevent another stack depth error from happening with any other incidental recursion in reverse mapped type resolution. Plus, basically every other branch of getTypeOfSymbol has a guard like this. Cue "how did we even manage to get by without circularity guards here?" response.
weswigham
commented
Mar 30, 2023
| inferTypes([inference], sourceType, templateType); | ||
| return getTypeFromInference(inference) || unknownType; | ||
| // Skip subtype reducing the inference result (this can eagerly resolve structure, which we'd rather avoid while doing this inference) | ||
| return getTypeFromInference(inference, UnionReduction.Literal) || unknownType; |
Member
Author
There was a problem hiding this comment.
By only doing Literal reductions on this inference result, we save some time, and prevent pulling on the structure of a recursive reverse mapped type while it's still being constructed. What we lose is, conceivably, reducing a reverse mapped type property that resolves to {x} | {x,y} to {x,y} (Or, perhaps more relevant, Function | RecursiveReverseMappingThatResolvesToASpecificFunction becoming Function, which is closer to what mongoose has); but I'd argue that most reverse mapped types only involve a single type, maybe union'd with undefined anyway.
The alternative is just issuing an error on the recursion, and that code is in-place, but that error feels bad because it only appears on instantiation and usage of the reverse mapped type, and not when it's made, even though the error appears at its' declaration site.
(note: as an aside, the command-line compiler does not like this and won't display the error, since it's generated for a file that already had direct diagnostics cached, resulting in a very silent, sneaky any type popping out of inference without an error reported. I suspect any deferred structure resolution that has a circularity can trigger this poor error reporting behavior. I can dodge this by using currentNode as the error node for the circularity error instead of the symbol's declaration site, but then you loose where the circularity occurs, so it probably at least needs a related span... better would be fixing up the program to actually pick up diagnostics added in files which it already collected diagnostics in once.)
Collaborator
|
@weswigham Here they are:
CompilerComparison Report - main..53595
System
Hosts
Scenarios
TSServerComparison Report - main..53595
System
Hosts
Scenarios
StartupComparison Report - main..53595
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
Author
|
Eh, a 0.75% improvement in node 16, but not 14 or 18? Weird. At least it's definitely not perf negative. |
Member
Author
|
@typescript-bot run dt |
Collaborator
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at c472b67. You can monitor the build here. Update: The results are in! |
Collaborator
|
Hey @weswigham, the results of running the DT tests are ready. Branch only errors:Package: convict |
Andarist
reviewed
Mar 31, 2023
| @@ -24005,6 +23995,17 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (inInferTypeForHomomorphicMappedType) { | |||
Contributor
There was a problem hiding this comment.
tangent q: is there any reason why this is a "global" flag and not something based on source/target identities? 🤔
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 #53179
I'm still working on minimizing the repro, since it pulls in all of mongoose and mongodb, but I know this fixes the problem, and want to queue up a perf run to see if this change ends up being perf positive (I think it might be).