Skip to content

Consult cached contextual types only when no contextFlags #52611

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 10, 2023
Merged

Consult cached contextual types only when no contextFlags #52611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a261ad
Consult cached contextual types only when no contextFlags
ahejlsberg Feb 4, 2023
7c22dcb
Add regression test
ahejlsberg Feb 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 22 additions & 15 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2110,6 +2110,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

const contextualTypeNodes: Node[] = [];
const contextualTypes: (Type | undefined)[] = [];
const contextualIsCache: boolean[] = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be contextualIsCached (or preferably contextualTypeIsCached)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is it more like "cacheable"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

let contextualTypeCount = 0;

const inferenceContextNodes: Node[] = [];
Expand Down Expand Up @@ -19199,7 +19200,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function checkExpressionForMutableLocationWithContextualType(next: Expression, sourcePropType: Type) {
pushContextualType(next, sourcePropType);
pushContextualType(next, sourcePropType, /*isCache*/ false);
const result = checkExpressionForMutableLocation(next, CheckMode.Contextual);
popContextualType();
return result;
Expand Down Expand Up @@ -19516,7 +19517,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
// recreate a tuple from the elements, if possible
// Since we're re-doing the expression type, we need to reapply the contextual type
pushContextualType(node, target);
pushContextualType(node, target, /*isCache*/ false);
const tupleizedType = checkArrayLiteral(node, CheckMode.Contextual, /*forceTuple*/ true);
popContextualType();
if (isTupleLikeType(tupleizedType)) {
Expand Down Expand Up @@ -29035,10 +29036,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// We cannot answer semantic questions within a with block, do not proceed any further
return undefined;
}
const index = findContextualNode(node);
// Cached contextual types are obtained with no ContextFlags, so we can only consult them for
// requests with no ContextFlags.
const index = findContextualNode(node, /*includeCaches*/ !contextFlags);
if (index >= 0) {
const cached = contextualTypes[index];
if (cached || !contextFlags) return cached;
return contextualTypes[index];
}
const { parent } = node;
switch (parent.kind) {
Expand Down Expand Up @@ -29111,19 +29113,24 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return undefined;
}

function pushContextualType(node: Node, type: Type | undefined) {
function pushCachedContextualType(node: Expression) {
pushContextualType(node, getContextualType(node, /*contextFlags*/ undefined), /*isCache*/ true);
}

function pushContextualType(node: Expression, type: Type | undefined, isCache: boolean) {
contextualTypeNodes[contextualTypeCount] = node;
contextualTypes[contextualTypeCount] = type;
contextualIsCache[contextualTypeCount] = isCache;
Comment on lines 29122 to +29123
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@DanielRosenwasser DanielRosenwasser Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

contextualTypeCount++;
}

function popContextualType() {
contextualTypeCount--;
}

function findContextualNode(node: Node) {
function findContextualNode(node: Node, includeCaches: boolean) {
for (let i = contextualTypeCount - 1; i >= 0; i--) {
if (node === contextualTypeNodes[i]) {
if (node === contextualTypeNodes[i] && (includeCaches || !contextualIsCache[i])) {
return i;
}
}
Expand All @@ -29150,7 +29157,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

function getContextualJsxElementAttributesType(node: JsxOpeningLikeElement, contextFlags: ContextFlags | undefined) {
if (isJsxOpeningElement(node) && contextFlags !== ContextFlags.Completions) {
const index = findContextualNode(node.parent);
const index = findContextualNode(node.parent, /*includeCaches*/ !contextFlags);
if (index >= 0) {
// Contextually applied type is moved from attributes up to the outer jsx attributes so when walking up from the children they get hit
// _However_ to hit them from the _attributes_ we must look for them here; otherwise we'll used the declared type
Expand Down Expand Up @@ -29486,7 +29493,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const elementCount = elements.length;
const elementTypes: Type[] = [];
const elementFlags: ElementFlags[] = [];
pushContextualType(node, getContextualType(node, /*contextFlags*/ undefined));
pushCachedContextualType(node);
const inDestructuringPattern = isAssignmentTarget(node);
const inConstContext = isConstContext(node);
const contextualType = getApparentTypeOfContextualType(node, /*contextFlags*/ undefined);
Expand Down Expand Up @@ -29669,7 +29676,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
let propertiesArray: Symbol[] = [];
let spread: Type = emptyObjectType;

pushContextualType(node, getContextualType(node, /*contextFlags*/ undefined));
pushCachedContextualType(node);
const contextualType = getApparentTypeOfContextualType(node, /*contextFlags*/ undefined);
const contextualTypeHasPattern = contextualType && contextualType.pattern &&
(contextualType.pattern.kind === SyntaxKind.ObjectBindingPattern || contextualType.pattern.kind === SyntaxKind.ObjectLiteralExpression);
Expand Down Expand Up @@ -36828,16 +36835,16 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
type.flags & TypeFlags.InstantiableNonPrimitive && maybeTypeOfKind(getBaseConstraintOfType(type) || unknownType, TypeFlags.StringLike));
}

function getContextNode(node: Expression): Node {
if (node.kind === SyntaxKind.JsxAttributes && !isJsxSelfClosingElement(node.parent)) {
function getContextNode(node: Expression): Expression {
if (isJsxAttributes(node) && !isJsxSelfClosingElement(node.parent)) {
return node.parent.parent; // Needs to be the root JsxElement, so it encompasses the attributes _and_ the children (which are essentially part of the attributes)
}
return node;
}

function checkExpressionWithContextualType(node: Expression, contextualType: Type, inferenceContext: InferenceContext | undefined, checkMode: CheckMode): Type {
const contextNode = getContextNode(node);
pushContextualType(contextNode, contextualType);
pushContextualType(contextNode, contextualType, /*isCache*/ false);
pushInferenceContext(contextNode, inferenceContext);
const type = checkExpression(node, checkMode | CheckMode.Contextual | (inferenceContext ? CheckMode.Inferential : 0));
// In CheckMode.Inferential we collect intra-expression inference sites to process before fixing any type
Expand Down Expand Up @@ -37228,7 +37235,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (links.contextFreeType) {
return links.contextFreeType;
}
pushContextualType(node, anyType);
pushContextualType(node, anyType, /*isCache*/ false);
const type = links.contextFreeType = checkExpression(node, CheckMode.SkipContextSensitive);
popContextualType();
return type;
Expand Down
62 changes: 62 additions & 0 deletions tests/baselines/reference/contextualTypeCaching.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
=== tests/cases/compiler/contextualTypeCaching.ts ===
// Repro from #52575

export interface Event<T> {
>Event : Symbol(Event, Decl(contextualTypeCaching.ts, 0, 0))
>T : Symbol(T, Decl(contextualTypeCaching.ts, 2, 23))

callback: (response: T) => void;
>callback : Symbol(Event.callback, Decl(contextualTypeCaching.ts, 2, 27))
>response : Symbol(response, Decl(contextualTypeCaching.ts, 3, 15))
>T : Symbol(T, Decl(contextualTypeCaching.ts, 2, 23))

nested: {
>nested : Symbol(Event.nested, Decl(contextualTypeCaching.ts, 3, 36))

nestedCallback: (response: T) => void;
>nestedCallback : Symbol(nestedCallback, Decl(contextualTypeCaching.ts, 4, 13))
>response : Symbol(response, Decl(contextualTypeCaching.ts, 5, 25))
>T : Symbol(T, Decl(contextualTypeCaching.ts, 2, 23))
}
}

export type CustomEvents = {
>CustomEvents : Symbol(CustomEvents, Decl(contextualTypeCaching.ts, 7, 1))

a: Event<string>
>a : Symbol(a, Decl(contextualTypeCaching.ts, 9, 28))
>Event : Symbol(Event, Decl(contextualTypeCaching.ts, 0, 0))

b: Event<number>
>b : Symbol(b, Decl(contextualTypeCaching.ts, 10, 20))
>Event : Symbol(Event, Decl(contextualTypeCaching.ts, 0, 0))

};

declare function emit<T extends keyof CustomEvents>(type: T, data: CustomEvents[T]): void
>emit : Symbol(emit, Decl(contextualTypeCaching.ts, 12, 2))
>T : Symbol(T, Decl(contextualTypeCaching.ts, 14, 22))
>CustomEvents : Symbol(CustomEvents, Decl(contextualTypeCaching.ts, 7, 1))
>type : Symbol(type, Decl(contextualTypeCaching.ts, 14, 52))
>T : Symbol(T, Decl(contextualTypeCaching.ts, 14, 22))
>data : Symbol(data, Decl(contextualTypeCaching.ts, 14, 60))
>CustomEvents : Symbol(CustomEvents, Decl(contextualTypeCaching.ts, 7, 1))
>T : Symbol(T, Decl(contextualTypeCaching.ts, 14, 22))

emit('a', {
>emit : Symbol(emit, Decl(contextualTypeCaching.ts, 12, 2))

callback: (r) => {},
>callback : Symbol(callback, Decl(contextualTypeCaching.ts, 16, 11))
>r : Symbol(r, Decl(contextualTypeCaching.ts, 17, 15))

nested: {
>nested : Symbol(nested, Decl(contextualTypeCaching.ts, 17, 24))

nestedCallback: (r) => {},
>nestedCallback : Symbol(nestedCallback, Decl(contextualTypeCaching.ts, 18, 13))
>r : Symbol(r, Decl(contextualTypeCaching.ts, 19, 25))

},
});

56 changes: 56 additions & 0 deletions tests/baselines/reference/contextualTypeCaching.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
=== tests/cases/compiler/contextualTypeCaching.ts ===
// Repro from #52575

export interface Event<T> {
callback: (response: T) => void;
>callback : (response: T) => void
>response : T

nested: {
>nested : { nestedCallback: (response: T) => void; }

nestedCallback: (response: T) => void;
>nestedCallback : (response: T) => void
>response : T
}
}

export type CustomEvents = {
>CustomEvents : { a: Event<string>; b: Event<number>; }

a: Event<string>
>a : Event<string>

b: Event<number>
>b : Event<number>

};

declare function emit<T extends keyof CustomEvents>(type: T, data: CustomEvents[T]): void
>emit : <T extends keyof CustomEvents>(type: T, data: CustomEvents[T]) => void
>type : T
>data : CustomEvents[T]

emit('a', {
>emit('a', { callback: (r) => {}, nested: { nestedCallback: (r) => {}, },}) : void
>emit : <T extends keyof CustomEvents>(type: T, data: CustomEvents[T]) => void
>'a' : "a"
>{ callback: (r) => {}, nested: { nestedCallback: (r) => {}, },} : { callback: (r: string) => void; nested: { nestedCallback: (r: string) => void; }; }

callback: (r) => {},
>callback : (r: string) => void
>(r) => {} : (r: string) => void
>r : string

nested: {
>nested : { nestedCallback: (r: string) => void; }
>{ nestedCallback: (r) => {}, } : { nestedCallback: (r: string) => void; }

nestedCallback: (r) => {},
>nestedCallback : (r: string) => void
>(r) => {} : (r: string) => void
>r : string

},
});

25 changes: 25 additions & 0 deletions tests/cases/compiler/contextualTypeCaching.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// @strict: true
// @noEmit: true

// Repro from #52575

export interface Event<T> {
callback: (response: T) => void;
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', {
callback: (r) => {},
nested: {
nestedCallback: (r) => {},
},
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
});
});
// 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) => {},
},
]);
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to get more code coverage in these scenarios.