Merge pull request #18919 from amcasey/ExtractLocalRefinements

Improve Extract Constant's handling of expression statements

Author: amcasey

src/harness/unittests/extractConstants.ts

@@ -33,6 +33,20 @@ namespace ts {
         testExtractConstant("extractConstant_ExpressionStatementExpression",
             `[#|"hello"|];`);
 
+        testExtractConstant("extractConstant_ExpressionStatementInNestedScope", `
+let i = 0;
+function F() {
+    [#|i++|];
+}
+        `);
+
+        testExtractConstant("extractConstant_ExpressionStatementConsumesLocal", `
+function F() {
+    let i = 0;
+    [#|i++|];
+}
+        `);
+
         testExtractConstant("extractConstant_BlockScopes_NoDependencies",
             `for (let i = 0; i < 10; i++) {
     for (let j = 0; j < 10; j++) {

src/services/refactors/extractSymbol.ts

@@ -911,12 +911,13 @@ namespace ts.refactor.extractSymbol {
                 const localReference = createIdentifier(localNameText);
                 changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference);
             }
-            else if (node.parent.kind === SyntaxKind.ExpressionStatement) {
-                // If the parent is an expression statement, replace the statement with the declaration.
+            else if (node.parent.kind === SyntaxKind.ExpressionStatement && scope === findAncestor(node, isScope)) {
+                // If the parent is an expression statement and the target scope is the immediately enclosing one,
+                // replace the statement with the declaration.
                 const newVariableStatement = createVariableStatement(
                     /*modifiers*/ undefined,
                     createVariableDeclarationList([newVariableDeclaration], NodeFlags.Const));
-                changeTracker.replaceNodeWithNodes(context.file, node.parent, [newVariableStatement], { nodeSeparator: context.newLineCharacter });
+                changeTracker.replaceRange(context.file, { pos: node.parent.getStart(), end: node.parent.end }, newVariableStatement);
             }
             else {
                 const newVariableStatement = createVariableStatement(
@@ -940,8 +941,14 @@ namespace ts.refactor.extractSymbol {
                 }
 
                 // Consume
-                const localReference = createIdentifier(localNameText);
-                changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference);
+                if (node.parent.kind === SyntaxKind.ExpressionStatement) {
+                    // If the parent is an expression statement, delete it.
+                    changeTracker.deleteRange(context.file, { pos: node.parent.getStart(), end: node.parent.end });
+                }
+                else {
+                    const localReference = createIdentifier(localNameText);
+                    changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference);
+                }
             }
         }
 
@@ -1344,14 +1351,13 @@ namespace ts.refactor.extractSymbol {
         }
 
         for (let i = 0; i < scopes.length; i++) {
-            if (!isReadonlyArray(targetRange.range)) {
-                const scopeUsages = usagesPerScope[i];
-                // Special case: in the innermost scope, all usages are available.
-                // (The computed value reflects the value at the top-level of the scope, but the
-                // local will actually be declared at the same level as the extracted expression).
-                if (i > 0 && (scopeUsages.usages.size > 0 || scopeUsages.typeParameterUsages.size > 0)) {
-                    constantErrorsPerScope[i].push(createDiagnosticForNode(targetRange.range, Messages.CannotAccessVariablesFromNestedScopes));
-                }
+            const scopeUsages = usagesPerScope[i];
+            // Special case: in the innermost scope, all usages are available.
+            // (The computed value reflects the value at the top-level of the scope, but the
+            // local will actually be declared at the same level as the extracted expression).
+            if (i > 0 && (scopeUsages.usages.size > 0 || scopeUsages.typeParameterUsages.size > 0)) {
+                const errorNode = isReadonlyArray(targetRange.range) ? targetRange.range[0] : targetRange.range;
+                constantErrorsPerScope[i].push(createDiagnosticForNode(errorNode, Messages.CannotAccessVariablesFromNestedScopes));
             }
 
             let hasWrite = false;

tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.js

@@ -0,0 +1,14 @@
+// ==ORIGINAL==
+
+function F() {
+    let i = 0;
+    i++;
+}
+        
+// ==SCOPE::Extract to constant in enclosing scope==
+
+function F() {
+    let i = 0;
+    const /*RENAME*/newLocal = i++;
+}
+        

tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.ts

@@ -0,0 +1,14 @@
+// ==ORIGINAL==
+
+function F() {
+    let i = 0;
+    i++;
+}
+        
+// ==SCOPE::Extract to constant in enclosing scope==
+
+function F() {
+    let i = 0;
+    const /*RENAME*/newLocal = i++;
+}
+        

tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.js

@@ -0,0 +1,23 @@
+// ==ORIGINAL==
+
+let i = 0;
+function F() {
+    i++;
+}
+        
+// ==SCOPE::Extract to constant in enclosing scope==
+
+let i = 0;
+function F() {
+    const /*RENAME*/newLocal = i++;
+}
+        
+// ==SCOPE::Extract to constant in global scope==
+
+let i = 0;
+const /*RENAME*/newLocal = i++;
+
+function F() {
+    
+}
+        

tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.ts

@@ -0,0 +1,23 @@
+// ==ORIGINAL==
+
+let i = 0;
+function F() {
+    i++;
+}
+        
+// ==SCOPE::Extract to constant in enclosing scope==
+
+let i = 0;
+function F() {
+    const /*RENAME*/newLocal = i++;
+}
+        
+// ==SCOPE::Extract to constant in global scope==
+
+let i = 0;
+const /*RENAME*/newLocal = i++;
+
+function F() {
+    
+}
+