Fixed declaration emit issue related to a qualifier being reused cross-file#58810
Fixed declaration emit issue related to a qualifier being reused cross-file#58810jakebailey merged 3 commits intomicrosoft:mainfrom
Conversation
| factory.updateLiteralTypeNode(node.argument, rewriteModuleSpecifier(node, node.argument.literal)), | ||
| node.attributes, | ||
| node.qualifier, | ||
| visitNode(node.qualifier, visitExistingNodeTreeSymbols, isEntityName), |
There was a problem hiding this comment.
I definitely don't have enough context on the workings of the checker to give any informed feedback, but I can at least ask at a high level do we have some confidence whether anything more could be done to the structure of the code to avoid this general class of problem?
There was a problem hiding this comment.
Tbh, I don't know this part of TS too well either so we need to wait for @weswigham or @dragomirtitian to give some feedback on this. I'd expect that other things in this function (like node.attributes here) should be wrapped like this too (if only this is the right fix).
There was a problem hiding this comment.
I would definitely suspect a lot of this needs this same fix.
There was a problem hiding this comment.
100% - any child node we copy should have its' children visited - any missing visitor calls on manually remapped nodes are an oversight, presumably.
weswigham
left a comment
There was a problem hiding this comment.
@jakebailey do we just want to take this as-is, or should we use this opportunity to audit all the factory calls in the visitor?
|
I think it'd be fine to merge this since it clearly helps code in the wild, but I do think we should audit effectively everything and backport what we find. |
|
I can make this audit as part of this PR (or separately), if you are OK with waiting till tomorrow. |
|
I think we could wait a day on this one; we still have time before the release. |
|
|
src/compiler/checker.ts
Outdated
| getEffectiveDotDotDotForParameter(p), | ||
| setTextRange(context, factory.createIdentifier(getNameForJSDocFunctionParameter(p, i)), p), | ||
| p.questionToken, | ||
| p.questionToken ? factory.createToken(SyntaxKind.QuestionToken) : undefined, |
There was a problem hiding this comment.
Some other code paths in this function do a similar "cloning" of the .questionToken so I assume that it's the right thing to do here and in other places that I touched
There was a problem hiding this comment.
Those 2 call sites related to .questionToken are also related to JSDoc. I wonder if this cloning even matters in this context.
| return factory.updateTypeParameterDeclaration( | ||
| node, | ||
| node.modifiers, | ||
| visitNodes(node.modifiers, visitExistingNodeTreeSymbols, isModifier), |
There was a problem hiding this comment.
those modifiers are just tokens so i guess i could just map over them and clone them?
| factory.updateLiteralTypeNode(node.argument, rewriteModuleSpecifier(node, node.argument.literal)), | ||
| node.attributes, | ||
| node.qualifier, | ||
| visitNode(node.attributes, visitExistingNodeTreeSymbols, isImportAttributes), |
There was a problem hiding this comment.
those are not jus flat tokens so I wasn't quite sure what to do about those, it seemed like maybe i should deep clone them?
| return factory.updateTypePredicateNode(node, node.assertsModifier ? factory.createToken(SyntaxKind.AssertsKeyword) : undefined, parameterName, visitNode(node.type, visitExistingNodeTreeSymbols, isTypeNode)); | ||
| } | ||
|
|
||
| if (isTupleTypeNode(node) || isTypeLiteralNode(node) || isMappedTypeNode(node)) { |
There was a problem hiding this comment.
there are also places in this function that do smth like:
if (visited === node) {
visited = factory.cloneNode(node)
}this pattern is not applied consistently across the whole function, should it be? 🤔
There was a problem hiding this comment.
I suspect yes.... Sure seems like though that we should make some sort of wrapper for the visitor func that would check that. Though I feel like I've seen that exact code in some visitor transformer code already (maybe we're not going through that)
There was a problem hiding this comment.
I think this might be a bigger change so I'd leave it out of this PR. I just want to flag this as a potential problem.
jakebailey
left a comment
There was a problem hiding this comment.
LGTM, likely need to be cherry picking this to 5.5.
|
@typescript-bot cherry-pick this to release-5.5 |
|
Hey, @jakebailey! I've created #58842 for you. |
…e-5.5 (#58842) Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
fixes #58807