Consult cached contextual types only when no contextFlags #52611
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 7c22dcb. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 7c22dcb. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 7c22dcb. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 7c22dcb. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 7c22dcb. You can monitor the build here. |
|
@ahejlsberg Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@ahejlsberg Here they are:Comparison Report - main..52611
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
| contextualTypes[contextualTypeCount] = type; | ||
| contextualIsCache[contextualTypeCount] = isCache; |
There was a problem hiding this comment.
I'm trying to grok what happens here - with my limited knowledge of the compiler internals - and I'm getting a little bit lost. Would you mind giving some more context behind the change?
I wonder when we want to consult the cache and when we don't + why do we even "push" things here with isCache === false 🤔 especially the second part is interesting
There was a problem hiding this comment.
The process of obtaining a contextual type can sometimes be expensive. For example, given a deeply nested array literal on the source side and a correspondingly nested type on the target side, getContextualType recurses all the way to the top to obtain the target type and then breaks down the type as it returns out of the recursion. This all adds up, particularly when an array literal has thousands of entries. By pushing a cache entry on the contextual type stack during the processing of an array or object literal we can improve performance by short-circuiting a lot of this recursion.
However... During the type inference phase of processing a generic function call, we sometimes need to instantiate the contextual type using the inferences collected so far (that's how we compute the contextual type to assign to the r parameters in the example). We trigger this eager instantiation by including ContextFlags.Signature in a call to getContextualType. In those cases we can't trust the cached contextual types because they may have resulted from breaking down the constraint of a generic type instead of breaking down an instantiation of that generic type with the current inferences.
Hope this clarifies it a bit.
There was a problem hiding this comment.
I'm trying to still get this.
It sounds like there are places where if there is no special type-checking context (i.e. no context flags), then you can trust these "cache" contextual types - that seems counter-intuitive.
There was a problem hiding this comment.
On entry to checkObjectLiteral and checkArrayLiteral we obtain the contextual type for the object/array literal with no context flags and push that on the contextual type stack as a cache entry. Then, any requests for the contextual type of a child node (as occurs when we check the elements of the literal) will hit that cache entry and shorten the recursive walk up the parent chain. As long as the child requests are for contextual types with no context flags, we can trust the cached type. But for other requests we have to continue walking up to the first non-cache entry and then break that down according to the specified context flags.
There was a problem hiding this comment.
Let me try to rephrase - it seems like when there are no context flags, those contextual types are effectively a "source of truth" - there's no pass where we're trying to infer type parameters, skip context-sensitive expressions, etc. So when you push on a contextual type with no context flags (which in this PR, is now called a "cache" contextual type), why is that not "better" than any other contextual type that's available?
It might be the case that my assumption around "source of truth" is incorrect. Maybe understanding what went wrong in the test case would be a good explainer.
There was a problem hiding this comment.
No, that's not how it works. When isCache is false, contextual type stack entries are a "source of truth" for all requests regardless of context flags. But when isCache is true, contextual type stack entries can be used only for requests with no context flags. The effect of not using a cache entry is that the search for a contextual type will continue further up the parent chain until possibly an entry that is the "source of truth" is found. Then, as recursion returns out of the nested getContextualType calls, the type is picked apart using the specified context flags which, for example, might specify that generic contextual types should be instantiated with the inferences made so far in order to contextually type parameters of an arrow function or function expression.
There was a problem hiding this comment.
When it comes to "what went wrong" you have a breakdown here. It doesn't exactly mention the interaction with this cache though as I didn't quite understand how this part works when I was debugging it. Maybe you will find it a little bit helpful.
There was a problem hiding this comment.
I've been asking others on the team to understand this - our understanding is that the presence of ContextFlags indicates a non-standard checking mode, so any types calculated under those can't be trusted. So I'm really struggling with the intuition here.
I can understand the rule that contextual types calculated with no context flags can only be used by other computations with no context flags. But I cannot understand why a contextual type calculated with ContextFlags.Completions would ever be reusable during type-checking with any other ContextFlags.
There was a problem hiding this comment.
Okay, I spoke with @ahejlsberg yesterday evening, and I see where the confusion came from.
First of all, there's something I mentioned which is that when we are pushing a contextual type, we determine if it's a "cache" based on if there are context flags. That's not true, that came from me misreading the code. If I understand correctly, there will be instances of contextual types requested with no context flags that are still not caches - but it is definitely supposed to be the case that any cache should be calculated with no type flags.
Second, there was another thing I mentioned about ContextFlags meaning types are not "trustworthy". I'm not going to say that's entirely untrue, but it comes more from a confusion between ContextFlags and CheckFlags. Anders explained that the intent of ContextFlags is more of a strategy around how we "crack into" contextual types, and what we do with them after receiving them.
And that explained exactly what happened in the test case. Something more minimal than the original is this:
export interface Event<T> {
nested: {
nestedCallback: (response: T) => void;
}
}
export type CustomEvents = {
a: Event<string>
b: Event<number>
};
declare function emit<T extends keyof CustomEvents>(type: T, data: CustomEvents[T]): void
emit('a', /*1*/ {
nested: /*2*/ {
nestedCallback: /*3*/ (r) => { },
},
});So in this example, as of #51865, the way type-checking will work is that as we walk the argument list and we hit the object literal (/*1*/), we now proactively push a contextual type, which is the uninferred/uninstantiated CustomEvents[T].
That's fine, but when we hit the inner object literal (/2/), we need to do the same. And because we need to "dig in" to get the type for the property nested. This ends up requesting the apparent type of the constraint - because again, CustomEvents[T] is uninferred and uninstantiated. So we use the apparent members of CustomEvents[keyof CustomEvents], which is just Event<string> | Event<number> - but notice that we've lost information about T at this point. Regardless, we proactively push that type.
Now when we get to the function expression, we have this contextual type readily available - but it's not the right one! When contextually typing a signature, we use ContextFlags.Signature which does a little extra - it performs some instantiation. But the contextual type it retrieved was already instantiated incorrectly with its constraint. I'm going to hand wave the details here, but the point is that that inner contextual type wasn't really "trustworthy" because it lost information. So we end up with a union of signatures, but we don't assign contextual types to parameters from unions of signatures.
There was no issue with the "outer" property in the example at #52575 because the outer contextual type was still CustomEvents[T] which was instantiable.
So this PR makes it so that any custom strategy does not reuse these "proactive/naive" contextual types.
I realize a lot of this is a rehash of what @Andarist said in #52575 (comment) - but I think explicitly calling out that the object literals got these proactive contextual types is part of what made it clearer to me.
There was a problem hiding this comment.
I realize a lot of this is a rehash of what @Andarist said in #52575 (comment) - but I think explicitly calling out that the object literals got these proactive contextual types is part of what made it clearer to me.
It might be "just" a rehash of what I said - but it's a very good rehash :P and it helped me to understand the true origin of the issue much better (I merely described what happened, without exactly understanding why)
| nested: { | ||
| nestedCallback: (r) => {}, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
This is a weird one. It currently works OK on the main branch - so it isn't fixed by this PR per se. However, one of my PRs regressed this case - it was caught by the bot here. I've re-checked that your PR here fixes it (see here) and that this "regression" was introduced with the const type parameters PR. Somehow my PR impacted things for this case to only manifest with it 🤷♂️
I figured out that it might be worthwhile to include this test case here even if it isn't exactly fixed by this PR right now.
| }); | |
| }); | |
| // repro from 52589#issuecomment-1416180638 | |
| declare class MyCompiler { | |
| compile(): void; | |
| } | |
| interface WebpackPluginInstance { | |
| apply: (compiler: MyCompiler) => void; | |
| } | |
| type WebpackPluginFunction = (this: MyCompiler, compiler: MyCompiler) => void; | |
| interface Optimization { | |
| minimizer?: (WebpackPluginInstance | WebpackPluginFunction)[]; | |
| } | |
| declare const A: <T, P extends keyof T>( | |
| obj: T, | |
| prop: P, | |
| factory: () => T[P] | |
| ) => void; | |
| const applyOptimizationDefaults = (optimization: Optimization) => { | |
| A(optimization, "minimizer", () => [ | |
| { | |
| apply: (compiler) => {}, | |
| }, | |
| ]); | |
| }; | |
There was a problem hiding this comment.
Would be nice to get more code coverage in these scenarios.
| @@ -2110,6 +2110,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
|
|
|||
| const contextualTypeNodes: Node[] = []; | |||
| const contextualTypes: (Type | undefined)[] = []; | |||
| const contextualIsCache: boolean[] = []; | |||
There was a problem hiding this comment.
Should this be contextualIsCached (or preferably contextualTypeIsCached)?
There was a problem hiding this comment.
Or is it more like "cacheable"?
There was a problem hiding this comment.
The flag is intended to indicate whether this is entry is a cache (to be consulted only for requests with no context flags) or a base entry (to be consulted always). It refers to the combined node/type entry, not just the type. Not loving the name I gave it, but not sure I like your suggestions any more,
Fixes #52575.