Merge pull request microsoft#18864 from amcasey/GH18857

Handle repeated substitution of the same symbol during extraction

Author: amcasey

src/harness/unittests/extractConstants.ts

@@ -84,12 +84,11 @@ namespace ts {
     let x = [#|t + 1|];
 }`);
 
-// TODO (18857): handle repeated substitution
-//         testExtractConstant("extractConstant_RepeatedSubstitution",
-//             `namespace X {
-//     export const j = 10;
-//     export const y = [#|j * j|];
-// }`);
+        testExtractConstant("extractConstant_RepeatedSubstitution",
+            `namespace X {
+    export const j = 10;
+    export const y = [#|j * j|];
+}`);
 
         testExtractConstant("extractConstant_VariableList_const",
             `const a = 1, b = [#|a + 1|];`);

src/harness/unittests/extractFunctions.ts

@@ -355,12 +355,11 @@ function parsePrimaryExpression(): any {
     [#|function G() { }|]
 }`);
 
-// TODO (18857): handle repeated substitution
-//         testExtractFunction("extractFunction_RepeatedSubstitution",
-//             `namespace X {
-//     export const j = 10;
-//     export const y = [#|j * j|];
-// }`);
+        testExtractFunction("extractFunction_RepeatedSubstitution",
+            `namespace X {
+    export const j = 10;
+    export const y = [#|j * j|];
+}`);
     });
 
     function testExtractFunction(caption: string, text: string) {

src/services/refactors/extractSymbol.ts

@@ -1048,7 +1048,7 @@ namespace ts.refactor.extractSymbol {
         }
     }
 
-    function transformFunctionBody(body: Node, writes: ReadonlyArray<UsageEntry>, substitutions: ReadonlyMap<Node>, hasReturn: boolean): { body: Block, returnValueProperty: string } {
+    function transformFunctionBody(body: Node, writes: ReadonlyArray<UsageEntry>, substitutions: ReadonlyMap<() => Node>, hasReturn: boolean): { body: Block, returnValueProperty: string } {
         if (isBlock(body) && !writes && substitutions.size === 0) {
             // already block, no writes to propagate back, no substitutions - can use node as is
             return { body: createBlock(body.statements, /*multLine*/ true), returnValueProperty: undefined };
@@ -1096,21 +1096,21 @@ namespace ts.refactor.extractSymbol {
                 const oldIgnoreReturns = ignoreReturns;
                 ignoreReturns = ignoreReturns || isFunctionLikeDeclaration(node) || isClassLike(node);
                 const substitution = substitutions.get(getNodeId(node).toString());
-                const result = substitution || visitEachChild(node, visitor, nullTransformationContext);
+                const result = substitution ? substitution() : visitEachChild(node, visitor, nullTransformationContext);
                 ignoreReturns = oldIgnoreReturns;
                 return result;
             }
         }
     }
 
-    function transformConstantInitializer(initializer: Expression, substitutions: ReadonlyMap<Node>): Expression {
+    function transformConstantInitializer(initializer: Expression, substitutions: ReadonlyMap<() => Node>): Expression {
         return substitutions.size
             ? visitor(initializer) as Expression
             : initializer;
 
         function visitor(node: Node): VisitResult<Node> {
             const substitution = substitutions.get(getNodeId(node).toString());
-            return substitution || visitEachChild(node, visitor, nullTransformationContext);
+            return substitution ? substitution() : visitEachChild(node, visitor, nullTransformationContext);
         }
     }
 
@@ -1239,7 +1239,7 @@ namespace ts.refactor.extractSymbol {
     interface ScopeUsages {
         readonly usages: Map<UsageEntry>;
         readonly typeParameterUsages: Map<TypeParameter>; // Key is type ID
-        readonly substitutions: Map<Node>;
+        readonly substitutions: Map<() => Node>;
     }
 
     interface ReadsAndWrites {
@@ -1258,7 +1258,7 @@ namespace ts.refactor.extractSymbol {
 
         const allTypeParameterUsages = createMap<TypeParameter>(); // Key is type ID
         const usagesPerScope: ScopeUsages[] = [];
-        const substitutionsPerScope: Map<Node>[] = [];
+        const substitutionsPerScope: Map<() => Node>[] = [];
         const functionErrorsPerScope: Diagnostic[][] = [];
         const constantErrorsPerScope: Diagnostic[][] = [];
         const visibleDeclarationsInExtractedRange: Symbol[] = [];
@@ -1270,8 +1270,8 @@ namespace ts.refactor.extractSymbol {
 
         // initialize results
         for (const scope of scopes) {
-            usagesPerScope.push({ usages: createMap<UsageEntry>(), typeParameterUsages: createMap<TypeParameter>(), substitutions: createMap<Expression>() });
-            substitutionsPerScope.push(createMap<Expression>());
+            usagesPerScope.push({ usages: createMap<UsageEntry>(), typeParameterUsages: createMap<TypeParameter>(), substitutions: createMap<() => Expression>() });
+            substitutionsPerScope.push(createMap<() => Expression>());
 
             functionErrorsPerScope.push(
                 isFunctionLikeDeclaration(scope) && scope.kind !== SyntaxKind.FunctionDeclaration
@@ -1567,18 +1567,20 @@ namespace ts.refactor.extractSymbol {
             }
         }
 
-        function tryReplaceWithQualifiedNameOrPropertyAccess(symbol: Symbol, scopeDecl: Node, isTypeNode: boolean): PropertyAccessExpression | EntityName {
+        function tryReplaceWithQualifiedNameOrPropertyAccess(symbol: Symbol, scopeDecl: Node, isTypeNode: boolean): () => (PropertyAccessExpression | EntityName) {
             if (!symbol) {
                 return undefined;
             }
             if (symbol.getDeclarations().some(d => d.parent === scopeDecl)) {
-                return createIdentifier(symbol.name);
+                return () => createIdentifier(symbol.name);
             }
             const prefix = tryReplaceWithQualifiedNameOrPropertyAccess(symbol.parent, scopeDecl, isTypeNode);
             if (prefix === undefined) {
                 return undefined;
             }
-            return isTypeNode ? createQualifiedName(<EntityName>prefix, createIdentifier(symbol.name)) : createPropertyAccess(<Expression>prefix, symbol.name);
+            return isTypeNode
+                ? () => createQualifiedName(<EntityName>prefix(), createIdentifier(symbol.name))
+                : () => createPropertyAccess(<Expression>prefix(), symbol.name);
         }
     }
 

tests/baselines/reference/extractConstant/extractConstant_RepeatedSubstitution.ts

@@ -0,0 +1,19 @@
+// ==ORIGINAL==
+namespace X {
+    export const j = 10;
+    export const y = j * j;
+}
+// ==SCOPE::Extract to constant in enclosing scope==
+namespace X {
+    export const j = 10;
+    const newLocal = j * j;
+
+    export const y = /*RENAME*/newLocal;
+}
+// ==SCOPE::Extract to constant in global scope==
+const newLocal = X.j * X.j;
+
+namespace X {
+    export const j = 10;
+    export const y = /*RENAME*/newLocal;
+}

tests/baselines/reference/extractFunction/extractFunction_RepeatedSubstitution.ts

@@ -0,0 +1,22 @@
+// ==ORIGINAL==
+namespace X {
+    export const j = 10;
+    export const y = j * j;
+}
+// ==SCOPE::Extract to function in namespace 'X'==
+namespace X {
+    export const j = 10;
+    export const y = /*RENAME*/newFunction();
+
+    function newFunction() {
+        return j * j;
+    }
+}
+// ==SCOPE::Extract to function in global scope==
+namespace X {
+    export const j = 10;
+    export const y = /*RENAME*/newFunction();
+}
+function newFunction() {
+    return X.j * X.j;
+}