Fixed single signature type instantiations leaking between calls with different mappers #59972
Conversation
… different mappers
| // the first one here has a chance to pollute the cache | ||
| type Result1 = ComponentProps<typeof ComponentWithForwardRef>; | ||
| // that could be incorrectly reused by this one | ||
| type Result2 = Test<{ component: typeof ComponentWithForwardRef }>; // no `T` leak |
There was a problem hiding this comment.
in the process of resolving the first conditional type here a result with a conditional type's restrictive mapper was cached, then when resolving the second conditional type that cached result was reused incorrectly, failing to produce a type with its correct mapper applied to it
src/compiler/checker.ts
Outdated
| if (type.objectFlags & ObjectFlags.SingleSignatureType) { | ||
| result = instantiateAnonymousType(type, mapper); | ||
| target.instantiations.set(id, result); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
It might feel like a downgrade that those are not cached right now. getObjectTypeInstantiation (the containing function) is only called from a single place and all the functions called from the branch leading to getObjectTypeInstantiation are implementing instantiation caching (createNormalizedTypeReference, instantiateReverseMappedType and getObjectTypeInstantiation).
But now not the branch I added - the one that calls instantiateAnonymousType for single signature types. I have no idea what I would use to create the cache key there. The only unique bits there are the results of the mapper but mapper isn't exactly called eagerly there so it doesn't have access to those.
However, I have a feeling that perhaps caching is already - sort of - implemented for those single signature types. It's just at a different level, like in the getReturnTypeOfSignature.
There was a problem hiding this comment.
My gut is making me worried that we're dropping some perf here due to not caching quickly, but maybe it doesn't matter at all?
There was a problem hiding this comment.
I was definitely worried about this when proposing this change but it feels that the getReturnTypeOfSignature caching should already cover most of this. It would still be best if @weswigham could take a look at this though.
I have just pushed out a new test case to this PR because I narrowed down another issue caused by those cache leaks here.
|
@typescript-bot test it |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
fixes #59937 , cc @weswigham