Fixed declaration emit issue related to a qualifier being reused cross-file by Andarist · Pull Request #58810

Fixed declaration emit issue related to a qualifier being reused cross-file by Andarist · Pull Request #58810 · microsoft/TypeScript

		factory.updateLiteralTypeNode(node.argument, rewriteModuleSpecifier(node, node.argument.literal)),
		node.attributes,
		node.qualifier,
		visitNode(node.qualifier, visitExistingNodeTreeSymbols, isEntityName),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely suspect a lot of this needs this same fix.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% - any child node we copy should have its' children visited - any missing visitor calls on manually remapped nodes are an oversight, presumably.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

		getEffectiveDotDotDotForParameter(p),
		setTextRange(context, factory.createIdentifier(getNameForJSDocFunctionParameter(p, i)), p),
		p.questionToken,
		p.questionToken ? factory.createToken(SyntaxKind.QuestionToken) : undefined,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, likely need to be cherry picking this to 5.5.

Starting jobs; this comment will be updated as builds start and complete.

Command	Status	Results
`cherry-pick this to release-5.5`	✅ Started	✅ Results

DanielRosenwasser pushed a commit that referenced this pull request

Jun 15, 2024

…e-5.5 (#58842)

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

microsoft locked as resolved and limited conversation to collaborators

Oct 16, 2025