fix(51202): Circular instantiation expression crashing IDEs #51214
Conversation
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| @@ -5481,6 +5481,12 @@ namespace ts { | |||
| } | |||
| } | |||
| else { | |||
| if (getObjectFlags(type) & ObjectFlags.InstantiationExpressionType) { | |||
There was a problem hiding this comment.
I think having a fallback like this is good; still, I wonder if there's any way you can also special-case the logic here for typeof.
if (getObjectFlags(type) & ObjectFlags.InstantiationExpressionType)) {
if (context.visitedTypes?.has(typeId)) {
return createElidedInformationPlaceholder(context);
}
const instantiationExpressionType = type as InstantiationExpressionType;
if (isTypeQueryNode(instantiationExpressionType.node)) {
// deep clone the type query node
}
return visitAndTransformType(type, createTypeNodeFromObjectType);
}There was a problem hiding this comment.
Is ok to emit
foo("");
>foo("") : typeof foo<T>
>foo : <T>(t: T) => typeof foo<T>
instead of
foo("");
>foo("") : (t: string) => any
>foo : <T>(t: T) => (t: T) => any
>"" : ""
?
There was a problem hiding this comment.
Personally, I think so - other reviewers might have other thoughts.
src/compiler/checker.ts
Outdated
| if (context.visitedTypes?.has(typeId)) { | ||
| return createElidedInformationPlaceholder(context); | ||
| } |
There was a problem hiding this comment.
I wonder if there's a way to test this case. The current tests don't, right?
src/compiler/checker.ts
Outdated
| if (isInstantiationExpressionType) { | ||
| const instantiationExpressionType = type as InstantiationExpressionType; | ||
| if (isTypeQueryNode(instantiationExpressionType.node)) { | ||
| return factory.cloneNode(instantiationExpressionType.node); |
There was a problem hiding this comment.
Use serializeExistingTypeNode here. Specifically, we may need to do things like remap jsdoc syntax into TS syntax in the subtree (type arguments), fixup module specifiers in import types for the new context, or track if the symbol referenced by a query is still visible within the new context.
There was a problem hiding this comment.
(Specifically, you should probably do checks similar to what occur in serializeTypeForDeclaration to see if this type node is safe to reuse)
There was a problem hiding this comment.
Do you mean the following checks?
if (!isErrorType(type) && enclosingDeclaration) {
const declWithExistingAnnotation = getDeclarationWithTypeAnnotation(symbol, enclosingDeclaration);
if (declWithExistingAnnotation && !isFunctionLikeDeclaration(declWithExistingAnnotation) && !isGetAccessorDeclaration(declWithExistingAnnotation)) {
// try to reuse the existing annotation
const existing = getEffectiveTypeAnnotationNode(declWithExistingAnnotation) !;
if (typeNodeIsEquivalentToType(existing, declWithExistingAnnotation, type) && existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount(existing, type)) {
const result = serializeExistingTypeNode(context, existing, includePrivateSymbol, bundled);
if (result) {
return result;
}
}
}
}
There was a problem hiding this comment.
What's the status here? I think the above is effectively what you have now in the current PR?
a-tarasyuk
requested review from
weswigham
and removed request for
rbuckton and
sheetalkamat
|
Would you mind stealing the tests from #52789 as well? There are more cases than just the return type. |
typescript-bot
added
For Milestone Bug
|
@weswigham Do you have any other feedback for this one? (I had a different fix for this via a duplicate bug, but this one seems better and it looks okay to me.) |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 389e1e2. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 389e1e2. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at 389e1e2. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at 389e1e2. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 389e1e2. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 389e1e2. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here they are:Comparison Report - main..51214
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
1 similar comment
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Everything is clean; not surprising of course given it was previously a crash, though I was somewhat expecting to see the result of this PR allowing us to write more typeofs in the output. |
Fixes #51202
Fixes #52757