Merge pull request #18343 from amcasey/InsertionPosition

Improve insertion position of extracted methods

(cherry picked from commit 40e4591)

src/compiler/core.ts

@@ -2596,4 +2596,6 @@ namespace ts {
     export function isCheckJsEnabledForFile(sourceFile: SourceFile, compilerOptions: CompilerOptions) {
         return sourceFile.checkJsDirective ? sourceFile.checkJsDirective.enabled : compilerOptions.checkJs;
     }
+
+    export function assertTypeIsNever(_: never): void {}
 }

src/harness/unittests/extractMethods.ts

@@ -594,6 +594,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() { }
 }`);
     });
 

src/services/refactors/extractMethod.ts

@@ -669,8 +669,14 @@ namespace ts.refactor.extractMethod {
         }
 
         const changeTracker = textChanges.ChangeTracker.fromCodeFixContext(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
@@ -752,6 +758,39 @@ namespace ts.refactor.extractMethod {
         const renameFilename = renameRange.getSourceFile().fileName;
         const renameLocation = getRenameLocation(edits, renameFilename, functionNameText);
         return { renameFilename, renameLocation, edits };
+
+        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 getRenameLocation(edits: ReadonlyArray<FileTextChanges>, renameFilename: string, functionNameText: string): number {
@@ -784,6 +823,7 @@ namespace ts.refactor.extractMethod {
         }
     }
 
+
     function transformFunctionBody(body: Node, writes: ReadonlyArray<UsageEntry>, substitutions: ReadonlyMap<Node>, hasReturn: boolean): { body: Block, returnValueProperty: string } {
         if (isBlock(body) && !writes && substitutions.size === 0) {
             // already block, no writes to propagate back, no substitutions - can use node as is

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 /*RENAME*/newFunction();
+
+        function newFunction() {
+            return 1;
+        }
+    }
+    function M3() { }
+}
+// ==SCOPE::namespace 'NS'==
+namespace NS {
+    function M1() { }
+    function M2() {
+        return /*RENAME*/newFunction();
+    }
+    function newFunction() {
+        return 1;
+    }
+
+    function M3() { }
+}
+// ==SCOPE::global scope==
+namespace NS {
+    function M1() { }
+    function M2() {
+        return /*RENAME*/newFunction();
+    }
+    function M3() { }
+}
+function newFunction() {
+    return 1;
+}

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 /*RENAME*/newFunction();
+
+        function newFunction() {
+            return 1;
+        }
+    }
+    function M3() { }
+}
+// ==SCOPE::function 'Outer'==
+function Outer() {
+    function M1() { }
+    function M2() {
+        return /*RENAME*/newFunction();
+    }
+    function newFunction() {
+        return 1;
+    }
+
+    function M3() { }
+}
+// ==SCOPE::global scope==
+function Outer() {
+    function M1() { }
+    function M2() {
+        return /*RENAME*/newFunction();
+    }
+    function M3() { }
+}
+function newFunction() {
+    return 1;
+}

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 /*RENAME*/newFunction();
+
+    function newFunction() {
+        return 1;
+    }
+}
+function M3() { }
+// ==SCOPE::global scope==
+function M1() { }
+function M2() {
+    return /*RENAME*/newFunction();
+}
+function newFunction() {
+    return 1;
+}
+
+function M3() { }

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./*RENAME*/newFunction();
+    }
+    private newFunction() {
+        return 1;
+    }
+
+    M3() { }
+}
+// ==SCOPE::global scope==
+class C {
+    M1() { }
+    M2() {
+        return /*RENAME*/newFunction();
+    }
+    M3() { }
+}
+function newFunction() {
+    return 1;
+}

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./*RENAME*/newFunction();
+    }
+    constructor() { }
+    private newFunction() {
+        return 1;
+    }
+
+    M3() { }
+}
+// ==SCOPE::global scope==
+class C {
+    M1() { }
+    M2() {
+        return /*RENAME*/newFunction();
+    }
+    constructor() { }
+    M3() { }
+}
+function newFunction() {
+    return 1;
+}

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./*RENAME*/newFunction();
+    }
+    private newFunction() {
+        return 1;
+    }
+
+    M3() { }
+    constructor() { }
+}
+// ==SCOPE::global scope==
+class C {
+    M1() { }
+    M2() {
+        return /*RENAME*/newFunction();
+    }
+    M3() { }
+    constructor() { }
+}
+function newFunction() {
+    return 1;
+}

tests/cases/fourslash/extract-method13.ts

@@ -37,12 +37,12 @@ edit.applyRefactor({
     constructor(q: string = C.newFunction()) {
     }
 
-    private static newFunction(): string {
-        return "a" + "b";
-    }
-
     private static newFunction_1() {
         return 1 + 1;
     }
+
+    private static newFunction(): string {
+        return "a" + "b";
+    }
 }`
 });