Skip to content

Reuse cached resolved signatures early #60208

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
Nov 6, 2024
Merged

Reuse cached resolved signatures early #60208

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
c40b6ca
Reuse cached resolved signatures early
Andarist Oct 12, 2024
cfb820c
add comment
Andarist Oct 23, 2024
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
32 changes: 21 additions & 11 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35777,10 +35777,28 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (!result) {
result = chooseOverload(candidates, assignableRelation, isSingleNonGenericCandidate, signatureHelpTrailingComma);
}
const links = getNodeLinks(node);
if (links.resolvedSignature !== resolvingSignature && !candidatesOutArray) {
// There are 2 situations in which it's good to preemptively return the cached result here:
//
// 1. if the signature resolution originated on a node that itself depends on the contextual type
// then it's possible that the resolved signature might not be the same as the one that would be computed in source order
// since resolving such signature leads to resolving the potential outer signature, its arguments and thus the very same signature
// it's possible that this inner resolution sets the resolvedSignature first.
// In such a case we ignore the local result and reuse the correct one that was cached.
//
// 2. In certain circular-like situations it's possible that the compiler reentries this function for the same node.
// It's possible to resolve the inner call against preemptively set empty members (for example in `resolveAnonymousTypeMembers`) of some type.
// When that happens the compiler might report an error for that inner call but at the same time it might end up resolving the actual members of the other type.
// This in turn creates a situation in which the outer call fails in `getSignatureApplicabilityError` due to a cached `RelationComparisonResult.Failed`
// but when the compiler tries to report that error (in the code below) it also tries to elaborate it and that can succeed as types would be related against the *resolved* members of the other type.
// This can hit `No error for last overload signature` assert but since that error was already reported when the inner call failed we can skip this step altogether here by returning the cached signature early.
Debug.assert(links.resolvedSignature);
return links.resolvedSignature;
}
if (result) {
return result;
}

result = getCandidateForOverloadFailure(node, candidates, args, !!candidatesOutArray, checkMode);
// Preemptively cache the result; getResolvedSignature will do this after we return, but
// we need to ensure that the result is present for the error checks below so that if
Expand All @@ -35789,7 +35807,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// don't hit this issue because they only observe this result after it's had a chance to
// be cached, but the error reporting code below executes before getResolvedSignature sets
// resolvedSignature.
getNodeLinks(node).resolvedSignature = result;
links.resolvedSignature = result;

// No signatures were applicable. Now report errors based on the last applicable signature with
// no arguments excluded from assignability checks.
Expand Down Expand Up @@ -36802,19 +36820,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
resolutionStart = resolutionTargets.length;
}
links.resolvedSignature = resolvingSignature;
let result = resolveSignature(node, candidatesOutArray, checkMode || CheckMode.Normal);
const result = resolveSignature(node, candidatesOutArray, checkMode || CheckMode.Normal);
resolutionStart = saveResolutionStart;
// When CheckMode.SkipGenericFunctions is set we use resolvingSignature to indicate that call
// resolution should be deferred.
if (result !== resolvingSignature) {
// if the signature resolution originated on a node that itself depends on the contextual type
// then it's possible that the resolved signature might not be the same as the one that would be computed in source order
// since resolving such signature leads to resolving the potential outer signature, its arguments and thus the very same signature
// it's possible that this inner resolution sets the resolvedSignature first.
// In such a case we ignore the local result and reuse the correct one that was cached.
if (links.resolvedSignature !== resolvingSignature) {
result = links.resolvedSignature;
}
// If signature resolution originated in control flow type analysis (for example to compute the
// assigned type in a flow assignment) we don't cache the result as it may be based on temporary
// types from the control flow analysis.
Expand Down
22 changes: 22 additions & 0 deletions tests/baselines/reference/mixinWithBaseDependingOnSelfNoCrash1.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
mixinWithBaseDependingOnSelfNoCrash1.ts(11,48): error TS2345: Argument of type 'typeof BaseItem' is not assignable to parameter of type 'new (...args: any[]) => any'.
Type 'typeof BaseItem' provides no match for the signature 'new (...args: any[]): any'.


==== mixinWithBaseDependingOnSelfNoCrash1.ts (1 errors) ====
// https://github.com/microsoft/TypeScript/issues/60202

declare class Document<Parent> {}

declare class BaseItem extends Document<typeof Item> {}

declare function ClientDocumentMixin<
BaseClass extends new (...args: any[]) => any,
>(Base: BaseClass): any;

declare class Item extends ClientDocumentMixin(BaseItem) {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

due to the circular nature of this example, the compiler goes into resolveCall for the same node twice in a nested manner

  1. the first getResolvedSignature ends up calling resolveAnonymousTypeMembers for typeof BaseItem
  2. in the process it preemptively sets empty signatures, members, index infos on that type here
  3. but then it also computes real construct signatures here
  4. this in turn leads to calling getResolvedSignature again for the same node and all
  5. in that inner call we see no construct signatures on typeof BaseItem (those were set to an empty array) so the error is raised because signaturesRelatedTo fails to determine that this source (typeof BaseItem) has the required target signatures
  6. this inner call errors and exits, caching the getCandidateForOverloadFailure's result as the links.resolvedSignature. That signature is (Base: new (...args: any[]) => any): any
  7. this leads to resolving (): BaseItem as getDefaultConstructSignatures and that replaces the empty construct signatures preemptively set by resolveAnonymousTypeMembers
  8. now we climb up the stack, going back to the first/outer getResolvedSignature call for this. When calling getSignatureApplicabilityError the compiler returns an error because it reuses cached RelationComparisonResult.Failed that was cached when relating the argument and parameter types in that inner call
  9. so now the compiler computes getCandidateForOverloadFailure again and tries to elaborate this error (again!).
  10. this time it succeeds when relating those 2 types because the construct signatures of typeof BaseItem changed "in-between". So the debug assert kicks finally kicks in and the compiler crashes

Copy link

Choose a reason for hiding this comment

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

Thanks a ton for this PR and the explanation! It makes the crash make much more sense to me now.

I will say some of these steps still seem to have a bit of concerning logic and seem to help explain something else weird I noted on my issue, namely that there doesn't have to be a "real" loop in the base type for you to get an error like Argument of type 'typeof BaseItem' is not assignable to parameter of type 'new (...args: any[]) => any'.

Playground (make sure to make any edit to overcome the crash and view an error!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh your example is somewhat convoluted to the point that I'm not sure if it should error or not. It's definitely outside of the "regular TS" territory 😅 so all I was after here was to fix the crash. Once this gets fixed you might want to raise a new issue about the circularity issue if you thing it shouldn't be reported

~~~~~~~~
!!! error TS2345: Argument of type 'typeof BaseItem' is not assignable to parameter of type 'new (...args: any[]) => any'.
!!! error TS2345: Type 'typeof BaseItem' provides no match for the signature 'new (...args: any[]): any'.

export {};

32 changes: 32 additions & 0 deletions tests/baselines/reference/mixinWithBaseDependingOnSelfNoCrash1.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//// [tests/cases/conformance/classes/mixinWithBaseDependingOnSelfNoCrash1.ts] ////

=== mixinWithBaseDependingOnSelfNoCrash1.ts ===
// https://github.com/microsoft/TypeScript/issues/60202

declare class Document<Parent> {}
>Document : Symbol(Document, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 0, 0))
>Parent : Symbol(Parent, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 2, 23))

declare class BaseItem extends Document<typeof Item> {}
>BaseItem : Symbol(BaseItem, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 2, 33))
>Document : Symbol(Document, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 0, 0))
>Item : Symbol(Item, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 8, 24))

declare function ClientDocumentMixin<
>ClientDocumentMixin : Symbol(ClientDocumentMixin, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 4, 55))

BaseClass extends new (...args: any[]) => any,
>BaseClass : Symbol(BaseClass, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 6, 37))
>args : Symbol(args, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 7, 25))

>(Base: BaseClass): any;
>Base : Symbol(Base, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 8, 2))
>BaseClass : Symbol(BaseClass, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 6, 37))

declare class Item extends ClientDocumentMixin(BaseItem) {}
>Item : Symbol(Item, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 8, 24))
>ClientDocumentMixin : Symbol(ClientDocumentMixin, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 4, 55))
>BaseItem : Symbol(BaseItem, Decl(mixinWithBaseDependingOnSelfNoCrash1.ts, 2, 33))

export {};

41 changes: 41 additions & 0 deletions tests/baselines/reference/mixinWithBaseDependingOnSelfNoCrash1.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
//// [tests/cases/conformance/classes/mixinWithBaseDependingOnSelfNoCrash1.ts] ////

=== mixinWithBaseDependingOnSelfNoCrash1.ts ===
// https://github.com/microsoft/TypeScript/issues/60202

declare class Document<Parent> {}
>Document : Document<Parent>
> : ^^^^^^^^^^^^^^^^

declare class BaseItem extends Document<typeof Item> {}
>BaseItem : BaseItem
> : ^^^^^^^^
>Document : Document<typeof Item>
> : ^^^^^^^^^^^^^^^^^^^^^
>Item : typeof Item
> : ^^^^^^^^^^^

declare function ClientDocumentMixin<
>ClientDocumentMixin : <BaseClass extends new (...args: any[]) => any>(Base: BaseClass) => any
> : ^ ^^^^^^^^^ ^^ ^^ ^^^^^

BaseClass extends new (...args: any[]) => any,
>args : any[]
> : ^^^^^

>(Base: BaseClass): any;
>Base : BaseClass
> : ^^^^^^^^^

declare class Item extends ClientDocumentMixin(BaseItem) {}
>Item : Item
> : ^^^^
>ClientDocumentMixin(BaseItem) : any
> : ^^^
>ClientDocumentMixin : <BaseClass extends new (...args: any[]) => any>(Base: BaseClass) => any
> : ^ ^^^^^^^^^ ^^ ^^ ^^^^^
>BaseItem : typeof BaseItem
> : ^^^^^^^^^^^^^^^

export {};

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

// https://github.com/microsoft/TypeScript/issues/60202

declare class Document<Parent> {}

declare class BaseItem extends Document<typeof Item> {}

declare function ClientDocumentMixin<
BaseClass extends new (...args: any[]) => any,
>(Base: BaseClass): any;

declare class Item extends ClientDocumentMixin(BaseItem) {}

export {};