-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Cache instantiation expression types early enough to prevent reentrancy during printback #59931
base: main
Are you sure you want to change the base?
Cache instantiation expression types early enough to prevent reentrancy during printback #59931
Conversation
…cy during error printback
let hasSomeApplicableSignature = false; | ||
let nonApplicableType: Type | undefined; | ||
const result = getInstantiatedType(exprType); | ||
links.instantiationExpressionTypes.set(exprType.id, result); | ||
const errorType = hasSomeApplicableSignature ? nonApplicableType : exprType; | ||
if (errorType) { | ||
diagnostics.add(createDiagnosticForNodeArray(getSourceFileOfNode(node), typeArguments, Diagnostics.Type_0_has_no_signatures_for_which_the_type_argument_list_is_applicable, typeToString(errorType))); |
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 root cause of the stack overflow was basically that the typeToString
on this line eventually recursively calls into getInstantiationExpressionType
to make the version of the type that's a member of the mapped type being printed, which is identical, but re-triggers the error emit and then spins.
@typescript-bot test it |
if (!links.instantiationExpressionTypes) { | ||
links.instantiationExpressionTypes = new Map(); | ||
} | ||
if (links.instantiationExpressionTypes.has(exprType.id)) { | ||
return links.instantiationExpressionTypes.get(exprType.id)!; |
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.
if (!links.instantiationExpressionTypes) { | |
links.instantiationExpressionTypes = new Map(); | |
} | |
if (links.instantiationExpressionTypes.has(exprType.id)) { | |
return links.instantiationExpressionTypes.get(exprType.id)!; | |
links.instantiationExpressionTypes ??= new Map(); | |
const cachedType = links.instantiationExpressionTypes.get(exprType.id); | |
if (cachedType) { | |
return cachedType; |
@@ -6238,6 +6238,7 @@ export interface NodeLinks { | |||
potentialReflectCollisions?: Node[]; | |||
potentialUnusedRenamedBindingElementsInTypes?: BindingElement[]; | |||
externalHelpersModule?: Symbol; // Resolved symbol for the external helpers module | |||
instantiationExpressionTypes?: Map<number, Type>; // Cache of instantiation expression types for the node |
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.
Is there any reason you cache by IDs instead of by 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.
Historic convention and habit, mostly. I can swap it if you'd like it.
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
@weswigham Here are the results of running the user tests with tsc comparing Everything looks good! |
@weswigham Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Fixes #59821