-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Avoid infinite recursion with inferReverseMappedType
#57837
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
Conversation
@typescript-bot run dt |
Hey @gabritto, the results of running the DT tests are ready. |
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
@typescript-bot perf test this faster |
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
inferReverseMappedType
inferReverseMappedType
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.
The less-broadly used inferTypeForHomomorphicMappedType
has caching/circularity prevention mechanisms already, but I guess since we use this lazily to generate the types of properties of reverse mapped types (directly, in more places than just the aforementioned function), it's gotta get dedicated circularity guards. I'm just wondering if the fail case should be unknown
, or if it should be the constraint of the target
?
What's the difference between calling
I also have this same question. I'm returning |
|
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 seems fine - definitely an improvement. Just wondering if we should also be doing caching within this function, rather than relying on callers of it to cache the results somehow.
src/compiler/checker.ts
Outdated
const typeParameter = getIndexedAccessType(constraint.type, getTypeParameterFromMappedType(target)) as TypeParameter; | ||
const templateType = getTemplateTypeFromMappedType(target); | ||
const inference = createInferenceInfo(typeParameter); | ||
inferTypes([inference], sourceType, templateType); | ||
return getTypeFromInference(inference) || unknownType; | ||
} | ||
|
||
function inferReverseMappedType(source: Type, target: MappedType, constraint: IndexType): Type { |
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.
Should we cache the results inside this, so we guarantee deeply nested results get cached? Of the callers of this function, only getTypeOfReverseMappedSymbol
currently caches - the other uses don't (directly) cache the result, so could, in theory, witness different results for the same set of input types at different points. You can probably only easily trigger such a behavior with a reverse mapped type over an array...? Maybe?
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 was adding the cache to inferReverseMappedType
and looking at inferTypeForHomomorphicMappedType
. I'm now wondering: could I do away with the recursion detection in inferTypeForHomomorphicMappedType
(that uses homomorphicMappedTypeInferenceStack
)?
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'm pretty sure that we can, however this once again begs the question of what to return when we find a circularity. inferTypeForHomomorphicMappedType
returns undefined
in this case. I'll have to think some more about this, but I'm planning on removing that circularity detection code in inferTypeForHomomorphicMappedType
.
I can't tell from the discussion if this is ready to merge. Do we want to investigate better caching before taking this? Do we need to figure out what happens in case of circularity? |
I want to do two things still: 1. think about what to return in case of circularity 2. get rid of the circularity detection in |
I think that PR might already exist: #56300 ;p I need to take another look at it to see what is left to do there though |
@typescript-bot test it |
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
@gabritto Here are the results of running the user tests comparing Everything looks good! |
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@gabritto Here are the results of running the top 400 repos comparing Everything looks good! |
Latest changes: I got rid of the circularity detection that existed in |
Fixes #57786.
For reference, the test case reported in the issue loops until stack overflow with the following function calls repeating: