Merge pull request #18343 from amcasey/InsertionPosition

amcasey · web-flow · commit 40e459117aeb · 2017-09-12T16:48:45.000-07:00
Improve insertion position of extracted methods
diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -500,13 +500,11 @@ namespace ts {
             case SyntaxKind.ArrowFunction:
                 return true;
             default:
-                staticAssertNever(node);
+                assertTypeIsNever(node);
                 return false;
         }
     }
 
-    function staticAssertNever(_: never): void {}
-
     // Gets the nearest enclosing block scope container that has the provided node
     // as a descendant, that is not the provided node.
     export function getEnclosingBlockScopeContainer(node: Node): Node {
diff --git a/src/harness/unittests/extractMethods.ts b/src/harness/unittests/extractMethods.ts
@@ -224,7 +224,7 @@ namespace ts {
             testExtractRange(`
                 function f() {
                     while (true) {
-                [#| 
+                [#|
                         if (x) {
                             return;
                         } |]
@@ -234,7 +234,7 @@ namespace ts {
             testExtractRange(`
                 function f() {
                     while (true) {
-                [#| 
+                [#|
                         [$|if (x) {
                         }
                         return;|]
@@ -655,6 +655,60 @@ function test(x: number) {
     finally {
         [#|return 1;|]
     }
+}`);
+        // Extraction position - namespace
+        testExtractMethod("extractMethod23",
+            `namespace NS {
+    function M1() { }
+    function M2() {
+        [#|return 1;|]
+    }
+    function M3() { }
+}`);
+        // Extraction position - function
+        testExtractMethod("extractMethod24",
+            `function Outer() {
+    function M1() { }
+    function M2() {
+        [#|return 1;|]
+    }
+    function M3() { }
+}`);
+        // Extraction position - file
+        testExtractMethod("extractMethod25",
+            `function M1() { }
+function M2() {
+    [#|return 1;|]
+}
+function M3() { }`);
+        // Extraction position - class without ctor
+        testExtractMethod("extractMethod26",
+            `class C {
+    M1() { }
+    M2() {
+        [#|return 1;|]
+    }
+    M3() { }
+}`);
+        // Extraction position - class with ctor in middle
+        testExtractMethod("extractMethod27",
+            `class C {
+    M1() { }
+    M2() {
+        [#|return 1;|]
+    }
+    constructor() { }
+    M3() { }
+}`);
+        // Extraction position - class with ctor at end
+        testExtractMethod("extractMethod28",
+            `class C {
+    M1() { }
+    M2() {
+        [#|return 1;|]
+    }
+    M3() { }
+    constructor() { }
 }`);
     });
 
diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts
@@ -698,8 +698,14 @@ namespace ts.refactor.extractMethod {
         }
 
         const changeTracker = textChanges.ChangeTracker.fromContext(context);
-        // insert function at the end of the scope
-        changeTracker.insertNodeBefore(context.file, scope.getLastToken(), newFunction, { prefix: context.newLineCharacter, suffix: context.newLineCharacter });
+        const minInsertionPos = (isReadonlyArray(range.range) ? lastOrUndefined(range.range) : range.range).end;
+        const nodeToInsertBefore = getNodeToInsertBefore(minInsertionPos, scope);
+        if (nodeToInsertBefore) {
+            changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newFunction, { suffix: context.newLineCharacter + context.newLineCharacter });
+        }
+        else {
+            changeTracker.insertNodeBefore(context.file, scope.getLastToken(), newFunction, { prefix: context.newLineCharacter, suffix: context.newLineCharacter });
+        }
 
         const newNodes: Node[] = [];
         // replace range with function call
@@ -832,6 +838,39 @@ namespace ts.refactor.extractMethod {
             return "__return";
         }
 
+        function getStatementsOrClassElements(scope: Scope): ReadonlyArray<Statement> | ReadonlyArray<ClassElement> {
+            if (isFunctionLike(scope)) {
+                const body = scope.body;
+                if (isBlock(body)) {
+                    return body.statements;
+                }
+            }
+            else if (isModuleBlock(scope) || isSourceFile(scope)) {
+                return scope.statements;
+            }
+            else if (isClassLike(scope)) {
+                return scope.members;
+            }
+            else {
+                assertTypeIsNever(scope);
+            }
+
+            return emptyArray;
+        }
+
+        /**
+         * If `scope` contains a function after `minPos`, then return the first such function.
+         * Otherwise, return `undefined`.
+         */
+        function getNodeToInsertBefore(minPos: number, scope: Scope): Node | undefined {
+            const children = getStatementsOrClassElements(scope);
+            for (const child of children) {
+                if (child.pos >= minPos && isFunctionLike(child) && !isConstructorDeclaration(child)) {
+                    return child;
+                }
+            }
+        }
+
         function transformFunctionBody(body: Node) {
             if (isBlock(body) && !writes && substitutions.size === 0) {
                 // already block, no writes to propagate back, no substitutions - can use node as is
diff --git a/tests/baselines/reference/extractMethod/extractMethod23.ts b/tests/baselines/reference/extractMethod/extractMethod23.ts
@@ -0,0 +1,43 @@
+// ==ORIGINAL==
+namespace NS {
+    function M1() { }
+    function M2() {
+        return 1;
+    }
+    function M3() { }
+}
+// ==SCOPE::function 'M2'==
+namespace NS {
+    function M1() { }
+    function M2() {
+        return newFunction();
+
+        function newFunction() {
+            return 1;
+        }
+    }
+    function M3() { }
+}
+// ==SCOPE::namespace 'NS'==
+namespace NS {
+    function M1() { }
+    function M2() {
+        return newFunction();
+    }
+    function newFunction() {
+        return 1;
+    }
+
+    function M3() { }
+}
+// ==SCOPE::global scope==
+namespace NS {
+    function M1() { }
+    function M2() {
+        return newFunction();
+    }
+    function M3() { }
+}
+function newFunction() {
+    return 1;
+}
diff --git a/tests/baselines/reference/extractMethod/extractMethod24.ts b/tests/baselines/reference/extractMethod/extractMethod24.ts
@@ -0,0 +1,43 @@
+// ==ORIGINAL==
+function Outer() {
+    function M1() { }
+    function M2() {
+        return 1;
+    }
+    function M3() { }
+}
+// ==SCOPE::function 'M2'==
+function Outer() {
+    function M1() { }
+    function M2() {
+        return newFunction();
+
+        function newFunction() {
+            return 1;
+        }
+    }
+    function M3() { }
+}
+// ==SCOPE::function 'Outer'==
+function Outer() {
+    function M1() { }
+    function M2() {
+        return newFunction();
+    }
+    function newFunction() {
+        return 1;
+    }
+
+    function M3() { }
+}
+// ==SCOPE::global scope==
+function Outer() {
+    function M1() { }
+    function M2() {
+        return newFunction();
+    }
+    function M3() { }
+}
+function newFunction() {
+    return 1;
+}
diff --git a/tests/baselines/reference/extractMethod/extractMethod25.ts b/tests/baselines/reference/extractMethod/extractMethod25.ts
@@ -0,0 +1,26 @@
+// ==ORIGINAL==
+function M1() { }
+function M2() {
+    return 1;
+}
+function M3() { }
+// ==SCOPE::function 'M2'==
+function M1() { }
+function M2() {
+    return newFunction();
+
+    function newFunction() {
+        return 1;
+    }
+}
+function M3() { }
+// ==SCOPE::global scope==
+function M1() { }
+function M2() {
+    return newFunction();
+}
+function newFunction() {
+    return 1;
+}
+
+function M3() { }
diff --git a/tests/baselines/reference/extractMethod/extractMethod26.ts b/tests/baselines/reference/extractMethod/extractMethod26.ts
@@ -0,0 +1,31 @@
+// ==ORIGINAL==
+class C {
+    M1() { }
+    M2() {
+        return 1;
+    }
+    M3() { }
+}
+// ==SCOPE::class 'C'==
+class C {
+    M1() { }
+    M2() {
+        return this.newFunction();
+    }
+    private newFunction() {
+        return 1;
+    }
+
+    M3() { }
+}
+// ==SCOPE::global scope==
+class C {
+    M1() { }
+    M2() {
+        return newFunction();
+    }
+    M3() { }
+}
+function newFunction() {
+    return 1;
+}
diff --git a/tests/baselines/reference/extractMethod/extractMethod27.ts b/tests/baselines/reference/extractMethod/extractMethod27.ts
@@ -0,0 +1,34 @@
+// ==ORIGINAL==
+class C {
+    M1() { }
+    M2() {
+        return 1;
+    }
+    constructor() { }
+    M3() { }
+}
+// ==SCOPE::class 'C'==
+class C {
+    M1() { }
+    M2() {
+        return this.newFunction();
+    }
+    constructor() { }
+    private newFunction() {
+        return 1;
+    }
+
+    M3() { }
+}
+// ==SCOPE::global scope==
+class C {
+    M1() { }
+    M2() {
+        return newFunction();
+    }
+    constructor() { }
+    M3() { }
+}
+function newFunction() {
+    return 1;
+}
diff --git a/tests/baselines/reference/extractMethod/extractMethod28.ts b/tests/baselines/reference/extractMethod/extractMethod28.ts
@@ -0,0 +1,34 @@
+// ==ORIGINAL==
+class C {
+    M1() { }
+    M2() {
+        return 1;
+    }
+    M3() { }
+    constructor() { }
+}
+// ==SCOPE::class 'C'==
+class C {
+    M1() { }
+    M2() {
+        return this.newFunction();
+    }
+    private newFunction() {
+        return 1;
+    }
+
+    M3() { }
+    constructor() { }
+}
+// ==SCOPE::global scope==
+class C {
+    M1() { }
+    M2() {
+        return newFunction();
+    }
+    M3() { }
+    constructor() { }
+}
+function newFunction() {
+    return 1;
+}
diff --git a/tests/cases/fourslash/extract-method13.ts b/tests/cases/fourslash/extract-method13.ts

Original file line number	Diff line number	Diff line change
`@@ -500,13 +500,11 @@ namespace ts {`
`500`	`500`	`case SyntaxKind.ArrowFunction:`
`501`	`501`	`return true;`
`502`	`502`	`default:`
`503`		`- staticAssertNever(node);`
	`503`	`+ assertTypeIsNever(node);`
`504`	`504`	`return false;`
`505`	`505`	`}`
`506`	`506`	`}`
`507`	`507`
`508`		`- function staticAssertNever(_: never): void {}`
`509`		`-`
`510`	`508`	`// Gets the nearest enclosing block scope container that has the provided node`
`511`	`509`	`// as a descendant, that is not the provided node.`
`512`	`510`	`export function getEnclosingBlockScopeContainer(node: Node): Node {`