Only introduce return properties at the top level

amcasey · amcasey · commit e2d94a2922a6 · 2017-09-13T13:36:25.000-07:00
...not in nested functions.
diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts
@@ -733,6 +733,33 @@ function parsePrimaryExpression(): any {
         testExtractMethod("extractMethod30",
             `function F<T>() {
     [#|let t: T;|]
+}`);
+        // Return in nested function
+        testExtractMethod("extractMethod31",
+            `namespace N {
+
+    export const value = 1;
+
+    () => {
+        var f: () => number;
+        [#|f = function (): number {
+            return value;
+        }|]
+    }
+}`);
+        // Return in nested class
+        testExtractMethod("extractMethod32",
+            `namespace N {
+
+    export const value = 1;
+
+    () => {
+        [#|var c = class {
+            M() {
+                return value;
+            }
+        }|]
+    }
 }`);
     });
 
diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts
@@ -855,6 +855,7 @@ namespace ts.refactor.extractMethod {
             return { body: createBlock(body.statements, /*multLine*/ true), returnValueProperty: undefined };
         }
         let returnValueProperty: string;
+        let ignoreReturns = false;
         const statements = createNodeArray(isBlock(body) ? body.statements.slice(0) : [isStatement(body) ? body : createReturn(<Expression>body)]);
         // rewrite body if either there are writes that should be propagated back via return statements or there are substitutions
         if (writes || substitutions.size) {
@@ -877,7 +878,7 @@ namespace ts.refactor.extractMethod {
         }
 
         function visitor(node: Node): VisitResult<Node> {
-            if (node.kind === SyntaxKind.ReturnStatement && writes) {
+            if (!ignoreReturns && node.kind === SyntaxKind.ReturnStatement && writes) {
                 const assignments: ObjectLiteralElementLike[] = getPropertyAssignmentsForWrites(writes);
                 if ((<ReturnStatement>node).expression) {
                     if (!returnValueProperty) {
@@ -893,8 +894,12 @@ namespace ts.refactor.extractMethod {
                 }
             }
             else {
+                const oldIgnoreReturns = ignoreReturns;
+                ignoreReturns = ignoreReturns || isFunctionLike(node) || isClassLike(node);
                 const substitution = substitutions.get(getNodeId(node).toString());
-                return substitution || visitEachChild(node, visitor, nullTransformationContext);
+                const result = substitution || visitEachChild(node, visitor, nullTransformationContext);
+                ignoreReturns = oldIgnoreReturns;
+                return result;
             }
         }
     }
diff --git a/tests/baselines/reference/extractMethod/extractMethod31.ts b/tests/baselines/reference/extractMethod/extractMethod31.ts
@@ -0,0 +1,45 @@
+// ==ORIGINAL==
+namespace N {
+
+    export const value = 1;
+
+    () => {
+        var f: () => number;
+        f = function (): number {
+            return value;
+        }
+    }
+}
+// ==SCOPE::function in namespace 'N'==
+namespace N {
+
+    export const value = 1;
+
+    () => {
+        var f: () => number;
+        f = /*RENAME*/newFunction(f);
+    }
+
+    function newFunction(f: () => number) {
+        f = function(): number {
+            return value;
+        };
+        return f;
+    }
+}
+// ==SCOPE::function in global scope==
+namespace N {
+
+    export const value = 1;
+
+    () => {
+        var f: () => number;
+        f = /*RENAME*/newFunction(f);
+    }
+}
+function newFunction(f: () => number) {
+    f = function(): number {
+        return N.value;
+    };
+    return f;
+}
diff --git a/tests/baselines/reference/extractMethod/extractMethod32.ts b/tests/baselines/reference/extractMethod/extractMethod32.ts
@@ -0,0 +1,46 @@
+// ==ORIGINAL==
+namespace N {
+
+    export const value = 1;
+
+    () => {
+        var c = class {
+            M() {
+                return value;
+            }
+        }
+    }
+}
+// ==SCOPE::function in namespace 'N'==
+namespace N {
+
+    export const value = 1;
+
+    () => {
+        /*RENAME*/newFunction();
+    }
+
+    function newFunction() {
+        var c = class {
+            M() {
+                return value;
+            }
+        };
+    }
+}
+// ==SCOPE::function in global scope==
+namespace N {
+
+    export const value = 1;
+
+    () => {
+        /*RENAME*/newFunction();
+    }
+}
+function newFunction() {
+    var c = class {
+        M() {
+            return N.value;
+        }
+    };
+}

Original file line number	Diff line number	Diff line change
`@@ -855,6 +855,7 @@ namespace ts.refactor.extractMethod {`
`855`	`855`	`return { body: createBlock(body.statements, /multLine/ true), returnValueProperty: undefined };`
`856`	`856`	`}`
`857`	`857`	`let returnValueProperty: string;`
	`858`	`+ let ignoreReturns = false;`
`858`	`859`	`const statements = createNodeArray(isBlock(body) ? body.statements.slice(0) : [isStatement(body) ? body : createReturn(<Expression>body)]);`
`859`	`860`	`// rewrite body if either there are writes that should be propagated back via return statements or there are substitutions`
`860`	`861`	`if (writes \|\| substitutions.size) {`
`@@ -877,7 +878,7 @@ namespace ts.refactor.extractMethod {`
`877`	`878`	`}`
`878`	`879`
`879`	`880`	`function visitor(node: Node): VisitResult<Node> {`
`880`		`- if (node.kind === SyntaxKind.ReturnStatement && writes) {`
	`881`	`+ if (!ignoreReturns && node.kind === SyntaxKind.ReturnStatement && writes) {`
`881`	`882`	`const assignments: ObjectLiteralElementLike[] = getPropertyAssignmentsForWrites(writes);`
`882`	`883`	`if ((<ReturnStatement>node).expression) {`
`883`	`884`	`if (!returnValueProperty) {`
`@@ -893,8 +894,12 @@ namespace ts.refactor.extractMethod {`
`893`	`894`	`}`
`894`	`895`	`}`
`895`	`896`	`else {`
	`897`	`+ const oldIgnoreReturns = ignoreReturns;`
	`898`	`+ ignoreReturns = ignoreReturns \|\| isFunctionLike(node) \|\| isClassLike(node);`
`896`	`899`	`const substitution = substitutions.get(getNodeId(node).toString());`
`897`		`- return substitution \|\| visitEachChild(node, visitor, nullTransformationContext);`
	`900`	`+ const result = substitution \|\| visitEachChild(node, visitor, nullTransformationContext);`
	`901`	`+ ignoreReturns = oldIgnoreReturns;`
	`902`	`+ return result;`
`898`	`903`	`}`
`899`	`904`	`}`
`900`	`905`	`}`