Uh oh!
There was an error while loading. Please reload this page.
Fixed declaration emit issue related to a qualifier being reused cross-file - #58810
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 Wesley Wigham (@weswigham) or Titian Cernicova-Dragomir (@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.
Wesley Wigham (weswigham)
left a comment
There was a problem hiding this comment.
Jake Bailey (@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?
Jake Bailey (jakebailey)
commented
Jun 11, 2024
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. |
Mateusz Burzyński (Andarist)
commented
Jun 11, 2024
I can make this audit as part of this PR (or separately), if you are OK with waiting till tomorrow. |
Jake Bailey (jakebailey)
commented
Jun 11, 2024
I think we could wait a day on this one; we still have time before the release. |
Mateusz Burzyński (Andarist)
commented
Jun 11, 2024
Uh oh!
There was an error while loading. Please reload this page.
| 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.
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
LGTM, likely need to be cherry picking this to 5.5.
Jake Bailey (jakebailey)
commented
Jun 12, 2024
TypeScript Bot (@typescript-bot) cherry-pick this to release-5.5 |
TypeScript Bot (typescript-bot)
commented
Jun 12, 2024
Hey, Jake Bailey (@jakebailey)! I've created #58842 for you. |
…e-5.5 (#58842) Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
fixes#58807