Merge pull request #19147 from amcasey/ReturnDeclExpr

Demote some extraction ranges to produce better results
microsoft · Oct 24, 2017 · fd89808 · fd89808
2 parents 228a885 + e4d5d1c
commit fd89808
Show file tree

Hide file tree

Showing 16 changed files with 116 additions and 29 deletions.
diff --git a/src/harness/unittests/extractFunctions.ts b/src/harness/unittests/extractFunctions.ts
@@ -362,27 +362,32 @@ function parsePrimaryExpression(): any {
 }`);
 
         testExtractFunction("extractFunction_VariableDeclaration_Var", `
-[#|var x = 1;|]
+[#|var x = 1;
+"hello"|]
 x;
 `);
 
         testExtractFunction("extractFunction_VariableDeclaration_Let_Type", `
-[#|let x: number = 1;|]
+[#|let x: number = 1;
+"hello";|]
 x;
 `);
 
         testExtractFunction("extractFunction_VariableDeclaration_Let_NoType", `
-[#|let x = 1;|]
+[#|let x = 1;
+"hello";|]
 x;
 `);
 
         testExtractFunction("extractFunction_VariableDeclaration_Const_Type", `
-[#|const x: number = 1;|]
+[#|const x: number = 1;
+"hello";|]
 x;
 `);
 
         testExtractFunction("extractFunction_VariableDeclaration_Const_NoType", `
-[#|const x = 1;|]
+[#|const x = 1;
+"hello";|]
 x;
 `);
 
@@ -404,7 +409,8 @@ x; y; z;
 `);
 
         testExtractFunction("extractFunction_VariableDeclaration_ConsumedTwice", `
-[#|const x: number = 1;|]
+[#|const x: number = 1;
+"hello";|]
 x; x;
 `);
 

diff --git a/src/harness/unittests/extractRanges.ts b/src/harness/unittests/extractRanges.ts
@@ -162,6 +162,19 @@ namespace ts {
                     }|]|]
                 }
             `);
+
+            // Variable statements
+            testExtractRange(`[#|let x = [$|1|];|]`);
+            testExtractRange(`[#|let x = [$|1|], y;|]`);
+            testExtractRange(`[#|[$|let x = 1, y = 1;|]|]`);
+
+            // Variable declarations
+            testExtractRange(`let [#|x = [$|1|]|];`);
+            testExtractRange(`let [#|x = [$|1|]|], y = 2;`);
+            testExtractRange(`let x = 1, [#|y = [$|2|]|];`);
+
+            // Return statements
+            testExtractRange(`[#|return [$|1|];|]`);
         });
 
         testExtractRangeFailed("extractRangeFailed1",
@@ -340,6 +353,18 @@ switch (x) {
             refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements.message
         ]);
 
+        testExtractRangeFailed("extractRangeFailed12",
+        `let [#|x|];`,
+        [
+            refactor.extractSymbol.Messages.StatementOrExpressionExpected.message
+        ]);
+
+        testExtractRangeFailed("extractRangeFailed13",
+        `[#|return;|]`,
+        [
+            refactor.extractSymbol.Messages.CannotExtractRange.message
+        ]);
+
         testExtractRangeFailed("extract-method-not-for-token-expression-statement", `[#|a|]`, [refactor.extractSymbol.Messages.CannotExtractIdentifier.message]);
     });
 }
diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts
@@ -244,12 +244,52 @@ namespace ts.refactor.extractSymbol {
             return { targetRange: { range: statements, facts: rangeFacts, declarations } };
         }
 
+        if (isReturnStatement(start) && !start.expression) {
+            // Makes no sense to extract an expression-less return statement.
+            return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] };
+        }
+
         // We have a single node (start)
-        const errors = checkRootNode(start) || checkNode(start);
+        const node = refineNode(start);
+
+        const errors = checkRootNode(node) || checkNode(node);
         if (errors) {
             return { errors };
         }
-        return { targetRange: { range: getStatementOrExpressionRange(start), facts: rangeFacts, declarations } };
+        return { targetRange: { range: getStatementOrExpressionRange(node), facts: rangeFacts, declarations } };
+
+        /**
+         * Attempt to refine the extraction node (generally, by shrinking it) to produce better results.
+         * @param node The unrefined extraction node.
+         */
+        function refineNode(node: Node) {
+            if (isReturnStatement(node)) {
+                if (node.expression) {
+                    return node.expression;
+                }
+            }
+            else if (isVariableStatement(node)) {
+                let numInitializers = 0;
+                let lastInitializer: Expression | undefined = undefined;
+                for (const declaration of node.declarationList.declarations) {
+                    if (declaration.initializer) {
+                        numInitializers++;
+                        lastInitializer = declaration.initializer;
+                    }
+                }
+                if (numInitializers === 1) {
+                    return lastInitializer;
+                }
+                // No special handling if there are multiple initializers.
+            }
+            else if (isVariableDeclaration(node)) {
+                if (node.initializer) {
+                    return node.initializer;
+                }
+            }
+
+            return node;
+        }
 
         function checkRootNode(node: Node): Diagnostic[] | undefined {
             if (isIdentifier(isExpressionStatement(node) ? node.expression : node)) {

diff --git a/tests/baselines/reference/extractFunction/extractFunction29.ts b/tests/baselines/reference/extractFunction/extractFunction29.ts
@@ -26,7 +26,7 @@ interface UnaryExpression {
 function parseUnaryExpression(operator: string): UnaryExpression {
     return /*RENAME*/newFunction();
 
-    function newFunction() {
+    function newFunction(): UnaryExpression {
         return {
             kind: "Unary",
             operator,
@@ -49,7 +49,7 @@ function parseUnaryExpression(operator: string): UnaryExpression {
     return /*RENAME*/newFunction(operator);
 }
 
-function newFunction(operator: string) {
+function newFunction(operator: string): UnaryExpression {
     return {
         kind: "Unary",
         operator,

diff --git a/tests/baselines/reference/extractFunction/extractFunction32.ts b/tests/baselines/reference/extractFunction/extractFunction32.ts
@@ -17,11 +17,11 @@ namespace N {
     export const value = 1;
 
     () => {
-        /*RENAME*/newFunction();
+        var c = /*RENAME*/newFunction()
     }
 
     function newFunction() {
-        var c = class {
+        return class {
             M() {
                 return value;
             }
@@ -34,12 +34,12 @@ namespace N {
     export const value = 1;
 
     () => {
-        /*RENAME*/newFunction();
+        var c = /*RENAME*/newFunction()
     }
 }
 
 function newFunction() {
-    var c = class {
+    return class {
         M() {
             return N.value;
         }

diff --git a/...s/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.js b/...s/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.js
@@ -1,6 +1,7 @@
 // ==ORIGINAL==
 
-/*[#|*/const x = 1;/*|]*/
+/*[#|*/const x = 1;
+"hello";/*|]*/
 x;
 
 // ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
 
 function newFunction() {
     const x = 1;
+    "hello";
     return x;
 }
diff --git a/...s/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.ts b/...s/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.ts
@@ -1,6 +1,7 @@
 // ==ORIGINAL==
 
-/*[#|*/const x = 1;/*|]*/
+/*[#|*/const x = 1;
+"hello";/*|]*/
 x;
 
 // ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
 
 function newFunction() {
     const x = 1;
+    "hello";
     return x;
 }
diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_Type.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_Type.ts
@@ -1,6 +1,7 @@
 // ==ORIGINAL==
 
-/*[#|*/const x: number = 1;/*|]*/
+/*[#|*/const x: number = 1;
+"hello";/*|]*/
 x;
 
 // ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
 
 function newFunction() {
     const x: number = 1;
+    "hello";
     return x;
 }
diff --git a/.../baselines/reference/extractFunction/extractFunction_VariableDeclaration_ConsumedTwice.ts b/.../baselines/reference/extractFunction/extractFunction_VariableDeclaration_ConsumedTwice.ts
@@ -1,6 +1,7 @@
 // ==ORIGINAL==
 
-/*[#|*/const x: number = 1;/*|]*/
+/*[#|*/const x: number = 1;
+"hello";/*|]*/
 x; x;
 
 // ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x; x;
 
 function newFunction() {
     const x: number = 1;
+    "hello";
     return x;
 }
diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.js b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.js
@@ -1,6 +1,7 @@
 // ==ORIGINAL==
 
-/*[#|*/let x = 1;/*|]*/
+/*[#|*/let x = 1;
+"hello";/*|]*/
 x;
 
 // ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
 
 function newFunction() {
     let x = 1;
+    "hello";
     return x;
 }
diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.ts
@@ -1,6 +1,7 @@
 // ==ORIGINAL==
 
-/*[#|*/let x = 1;/*|]*/
+/*[#|*/let x = 1;
+"hello";/*|]*/
 x;
 
 // ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
 
 function newFunction() {
     let x = 1;
+    "hello";
     return x;
 }
diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_Type.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_Type.ts
@@ -1,6 +1,7 @@
 // ==ORIGINAL==
 
-/*[#|*/let x: number = 1;/*|]*/
+/*[#|*/let x: number = 1;
+"hello";/*|]*/
 x;
 
 // ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
 
 function newFunction() {
     let x: number = 1;
+    "hello";
     return x;
 }
diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.js b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.js
@@ -1,6 +1,7 @@
 // ==ORIGINAL==
 
-/*[#|*/var x = 1;/*|]*/
+/*[#|*/var x = 1;
+"hello"/*|]*/
 x;
 
 // ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
 
 function newFunction() {
     var x = 1;
+    "hello";
     return x;
 }
diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.ts
@@ -1,6 +1,7 @@
 // ==ORIGINAL==
 
-/*[#|*/var x = 1;/*|]*/
+/*[#|*/var x = 1;
+"hello"/*|]*/
 x;
 
 // ==SCOPE::Extract to function in global scope==
@@ -10,5 +11,6 @@ x;
 
 function newFunction() {
     var x = 1;
+    "hello";
     return x;
 }
diff --git a/tests/cases/fourslash/extract-method14.ts b/tests/cases/fourslash/extract-method14.ts
@@ -7,7 +7,7 @@
 // @Filename: foo.js
 //// function foo() {
 ////     var i = 10;
-////     /*a*/return i++;/*b*/
+////     /*a*/return i + 1;/*b*/
 //// }
 
 goTo.select('a', 'b');
@@ -18,13 +18,11 @@ edit.applyRefactor({
     newContent:
 `function foo() {
     var i = 10;
-    let __return;
-    ({ __return, i } = /*RENAME*/newFunction(i));
-    return __return;
+    return /*RENAME*/newFunction(i);
 }
 
 function newFunction(i) {
-    return { __return: i++, i };
+    return i + 1;
 }
 `
 });
diff --git a/tests/cases/fourslash/extract-method22.ts b/tests/cases/fourslash/extract-method22.ts
@@ -3,7 +3,7 @@
 // You may not extract variable declarations with the export modifier
 
 //// namespace NS {
-////     /*start*/export var x = 10;/*end*/
+////     /*start*/export var x = 10, y = 15;/*end*/
 //// }
 
 goTo.select('start', 'end')