Introduce getSynthesizedDeepClone

[amcasey]

During refactoring, it seems quite common to require that a node appear in more than one place in the rewritten tree.

[amcasey]

Apparently, this is a public API change. What's the correct way to handle that?

[weswigham]

If it's desirable, just accept the new API baselines.

[ghost]

Or mark it @internal like getSynthesizedClone.

[amcasey]

@Andy-MS's right - it was an oversight on my part.

[rbuckton]

No need to write child => getSynthesizedDeepClone(child) when just getSynthesizedDeepClone would do.

[rbuckton]

This whole operation seems somewhat expensive. The call to visitEachChild will likely resolve in a call to one of the updateX functions in factory.ts, which in turn calls updateNode, which in turn sets the original pointer on the new node, clones the original node's emitNode, and sets the pos and end of the new node. Then getSynthesizedClone discards the new object for a fresh one.

If we only need this for entity-name-like expressions, could we instead write a more specific version that doesn't do all of this extra work that is just thrown out? See serializeQualifiedNameAsExpression in ts.ts for a function that already does this (albeit with some extra options you don't need).

[amcasey]

I agree that's it's overkill for this application. It was actually added for #18997 and this was a proof-of-concept application. If there's a way to make this less expensive, I'd be happy to rework it.

[rbuckton]

Since visitEachChild guarantees it returns a new node if any of the children change, you can use that to ensure the result is a clone by doing this instead:

function getSynthesizedDeepClone<T extends Node>(node: T | undefined): T | undefined {
    const visited = visitEachChild(node, getSynthesizedDeepClone, nullTransformationContext);
    if (visited === node) return getSynthesizedClone(node);
    visited.pos = -1;
    visited.end = -1;
    visited.parent = undefined;
    return visited;
}

[rbuckton]

Also, consider moving this to services so that it isn't used unintentionally in compiler.

[amcasey]

@rbuckton I believe I've addressed your feedback.

[amcasey]

@rbuckton - I believe most/all callers will want to copy trivia. Does that mean deep-clone should actually be built on getMutableClone, rather than getSynthesizedClone?

[rbuckton]

I'd rather not use getMutableClone as I'd like to eventually get rid of that function. You could always call setTextRange in your getDeepSynthesizedClone to copy positions.

[amcasey]

@rbuckton Could I just refrain from clearing pos and end? (I assume I still have to clear parent.)

[amcasey]

As of this PR, the positions don't make a difference. I'll add a test once #18931 is merged.

[amcasey]

@rbuckton signed off offline.