Fix bug due to sharing of .type.members

Function extraction failed when using two identical `TypeLiteral`s ---
the problem is that the parameters' `.type.members` are shared in their
two occurences in the resulting code, and when the formatter bangs
position properties on the second, it's actually changing the first too.

* `typeToAutoImportableTypeNode`: wrap in `getSynthesizedDeepClone` to
  avoid the offeding sharing.

* `getSynthesizedDeepCloneWorker`: add optional nodes/token visitors so
  it doesn't skip cloning the above.

* A few very minor tweaks which I saw when tracing this (comment update,
  two `!`s).

Fixes #44301

Author: elibarzilay

src/services/codefixes/helpers.ts

@@ -127,7 +127,7 @@ namespace ts.codefix {
                 }
 
                 for (const signature of signatures) {
-                    // Need to ensure nodes are fresh each time so they can have different positions.
+                    // Ensure nodes are fresh so they can have different positions when going through formatting.
                     outputMethod(quotePreference, signature, getSynthesizedDeepClones(modifiers, /*includeTrivia*/ false), getSynthesizedDeepClone(name, /*includeTrivia*/ false));
                 }
 
@@ -310,15 +310,16 @@ namespace ts.codefix {
     }
 
     export function typeToAutoImportableTypeNode(checker: TypeChecker, importAdder: ImportAdder, type: Type, contextNode: Node | undefined, scriptTarget: ScriptTarget, flags?: NodeBuilderFlags, tracker?: SymbolTracker): TypeNode | undefined {
-        const typeNode = checker.typeToTypeNode(type, contextNode, flags, tracker);
+        let typeNode = checker.typeToTypeNode(type, contextNode, flags, tracker);
         if (typeNode && isImportTypeNode(typeNode)) {
             const importableReference = tryGetAutoImportableReferenceFromTypeNode(typeNode, scriptTarget);
             if (importableReference) {
                 importSymbols(importAdder, importableReference.symbols);
-                return importableReference.typeNode;
+                typeNode = importableReference.typeNode;
             }
         }
-        return typeNode;
+        // Ensure nodes are fresh so they can have different positions when going through formatting.
+        return getSynthesizedDeepClone(typeNode);
     }
 
     function createDummyParameters(argCount: number, names: (string | undefined)[] | undefined, types: (TypeNode | undefined)[] | undefined, minArgumentCount: number | undefined, inJs: boolean): ParameterDeclaration[] {

src/services/textChanges.ts

@@ -991,7 +991,7 @@ namespace ts.textChanges {
             const { options = {}, range: { pos } } = change;
             const format = (n: Node) => getFormattedTextOfNode(n, sourceFile, pos, options, newLineCharacter, formatContext, validate);
             const text = change.kind === ChangeKind.ReplaceWithMultipleNodes
-                ? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(change.options!.joiner || newLineCharacter) // TODO: GH#18217
+                ? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(change.options?.joiner || newLineCharacter)
                 : format(change.node);
             // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line
             const noIndent = (options.preserveLeadingWhitespace || options.indentation !== undefined || getLineStartPositionForPosition(pos, sourceFile) === pos) ? text : text.replace(/^\s+/, "");
@@ -1054,7 +1054,7 @@ namespace ts.textChanges {
     }
 
     function assignPositionsToNode(node: Node): Node {
-        const visited = visitEachChild(node, assignPositionsToNode, nullTransformationContext, assignPositionsToNodeArray, assignPositionsToNode)!; // TODO: GH#18217
+        const visited = visitEachChild(node, assignPositionsToNode, nullTransformationContext, assignPositionsToNodeArray, assignPositionsToNode);
         // create proxy node for non synthesized nodes
         const newNode = nodeIsSynthesized(visited) ? visited : Object.create(visited) as Node;
         setTextRangePosEnd(newNode, getPos(node), getEnd(node));

src/services/utilities.ts

@@ -2377,9 +2377,14 @@ namespace ts {
     }
 
     function getSynthesizedDeepCloneWorker<T extends Node>(node: T, replaceNode?: (node: Node) => Node | undefined): T {
-        const visited = replaceNode ?
-            visitEachChild(node, n => getSynthesizedDeepCloneWithReplacements(n, /*includeTrivia*/ true, replaceNode), nullTransformationContext) :
-            visitEachChild(node, getSynthesizedDeepClone, nullTransformationContext);
+        const nodeClone: (n: T) => T = replaceNode
+            ? n => getSynthesizedDeepCloneWithReplacements(n, /*includeTrivia*/ true, replaceNode)
+            : getSynthesizedDeepClone;
+        const nodesClone: (ns: NodeArray<T>) => NodeArray<T> = replaceNode
+            ? ns => ns && getSynthesizedDeepClonesWithReplacements(ns, /*includeTrivia*/ true, replaceNode)
+            : ns => ns && getSynthesizedDeepClones(ns);
+        const visited =
+            visitEachChild(node, nodeClone, nullTransformationContext, nodesClone, nodeClone);
 
         if (visited === node) {
             // This only happens for leaf nodes - internal nodes always see their children change.
@@ -2390,8 +2395,8 @@ namespace ts {
             return setTextRange(clone, node);
         }
 
-        // PERF: As an optimization, rather than calling getSynthesizedClone, we'll update
-        // the new node created by visitEachChild with the extra changes getSynthesizedClone
+        // PERF: As an optimization, rather than calling factory.cloneNode, we'll update
+        // the new node created by visitEachChild with the extra changes factory.cloneNode
         // would have made.
         (visited as Mutable<T>).parent = undefined!;
         return visited;

tests/cases/fourslash/extract-method-two-type-literals.ts

@@ -0,0 +1,17 @@
+/// <reference path='fourslash.ts' />
+
+////(x: {}, y: {}) => (/*1*/x + y/*2*/);
+
+goTo.select("1", "2");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "function_scope_1",
+    actionDescription: "Extract to function in global scope",
+    newContent:
+`(x: {}, y: {}) => (/*RENAME*/newFunction(x, y));
+
+function newFunction(x: {}, y: {}) {
+    return x + y;
+}
+`
+});