Avoid incorrectly reusing assertion nodes from property assignments#60576
Conversation
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
…nodes-with-different-types
|
If this fixes #60573, we should definitely add a dts test that shows that things are fixed. |
| if (assertionNode && !isConstTypeReference(assertionNode) && resolver.canReuseTypeNodeAnnotation(context, node, assertionNode, symbol)) { | ||
| result = serializeExistingTypeNode(assertionNode, context); |
There was a problem hiding this comment.
I was largely unsure about this given you ask a question about node then use that to work with assertionNode, but this is effectively the same check as:
function typeFromPropertyAssignment(node: PropertyAssignment | ShorthandPropertyAssignment, symbol: Symbol, context: SyntacticTypeNodeBuilderContext) {
const typeAnnotation = getEffectiveTypeAnnotationNode(node);
let result;
if (typeAnnotation && resolver.canReuseTypeNodeAnnotation(context, node, typeAnnotation, symbol)) {
result = serializeExistingTypeNode(typeAnnotation, context);
}So I definitely like this change more than the initial version.
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
…nodes-with-different-types
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot pack this |
|
Starting jobs; this comment will be updated as builds start and complete.
|
| //// [declarationAssertionNodeNotReusedWhenTypeNotEquivalent1.d.ts] | ||
| export declare const unwrapped: { | ||
| prop1: "hello"; | ||
| }; |
There was a problem hiding this comment.
@jakebailey here is the requested dts test
weswigham
left a comment
There was a problem hiding this comment.
Yeah, this looks solid now. It'd probably be relevant to ask what output this limits under isolatedDeclarations, but... 🤷♂️ it kinda is what it is.
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
@typescript-bot pack this |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@typescript-bot cherry-pick this to release-5.7 |
|
Hey, @jakebailey! I've created #60679 for you. |
…e-5.7 (#60679) Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
fixes #60573 (a regression from #59282 )