Merge pull request #18448 from amcasey/NestedReturn

Only introduce return properties at the top level

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;
+            }
+        }|]
+    }
 }`);
     });
 

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;
             }
         }
     }

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;
+}

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;
+        }
+    };
+}