Improve insertion positions of extracted constants

...mostly by putting them closer to the extraction site.

They now also follow all leading comments in a file (unfortunately,
including the doc comment on the first declaration).

Bonus: Special case declaration lists - declare the new variable as part
of the list, rather than before it, in case it depends on an earlier
entry.

Author: amcasey

src/harness/unittests/extractConstants.ts

@@ -40,7 +40,7 @@ namespace ts {
     }
 }`);
 
-        testExtractConstant("extractConstant_ClassInsertionPosition",
+        testExtractConstant("extractConstant_ClassInsertionPosition1",
             `class C {
     a = 1;
     b = 2;
@@ -51,6 +51,28 @@ namespace ts {
     }
 }`);
 
+        testExtractConstant("extractConstant_ClassInsertionPosition2",
+            `class C {
+    a = 1;
+    M1() { }
+    b = 2;
+    M2() { }
+    M3() {
+        let x = [#|1|];
+    }
+}`);
+
+        testExtractConstant("extractConstant_ClassInsertionPosition3",
+            `class C {
+    M1() { }
+    a = 1;
+    b = 2;
+    M2() { }
+    M3() {
+        let x = [#|1|];
+    }
+}`);
+
         testExtractConstant("extractConstant_Parameters",
             `function F() {
     let w = 1;
@@ -62,7 +84,7 @@ namespace ts {
     let x = [#|t + 1|];
 }`);
 
-// TODO (acasey): handle repeated substitution
+// TODO (18857): handle repeated substitution
 //         testExtractConstant("extractConstant_RepeatedSubstitution",
 //             `namespace X {
 //     export const j = 10;
@@ -75,6 +97,121 @@ namespace ts {
         let x = [#|i + 1|];
     }
 }`);
+
+        testExtractConstant("extractConstant_VariableList_const",
+            `const a = 1, b = [#|a + 1|];`);
+
+        // NOTE: this test isn't normative - it just documents our sub-optimal behavior.
+        testExtractConstant("extractConstant_VariableList_let",
+            `let a = 1, b = [#|a + 1|];`);
+
+        // NOTE: this test isn't normative - it just documents our sub-optimal behavior.
+        testExtractConstant("extractConstant_VariableList_MultipleLines",
+            `const /*About A*/a = 1,
+    /*About B*/b = [#|a + 1|];`);
+
+        // NOTE: this test isn't normative - it just documents our sub-optimal behavior.
+        // `i` doesn't bind in the target scope (file-level), so the extraction is disallowed.
+        // TODO (17098): should probably allow extraction into the same scope
+        testExtractConstantFailed("extractConstant_BlockScopeMismatch", `
+for (let i = 0; i < 10; i++) {
+    for (let j = 0; j < 10; j++) {
+        const x = [#|i + 1|];
+    }
+}
+        `);
+
+        testExtractConstant("extractConstant_StatementInsertionPosition1", `
+const i = 0;
+for (let j = 0; j < 10; j++) {
+    const x = [#|i + 1|];
+}
+        `);
+
+        testExtractConstant("extractConstant_StatementInsertionPosition2", `
+const i = 0;
+function F() {
+    for (let j = 0; j < 10; j++) {
+        const x = [#|i + 1|];
+    }
+}
+        `);
+
+        testExtractConstant("extractConstant_StatementInsertionPosition3", `
+for (let j = 0; j < 10; j++) {
+    const x = [#|2 + 1|];
+}
+        `);
+
+        testExtractConstant("extractConstant_StatementInsertionPosition4", `
+function F() {
+    for (let j = 0; j < 10; j++) {
+        const x = [#|2 + 1|];
+    }
+}
+        `);
+
+        testExtractConstant("extractConstant_StatementInsertionPosition5", `
+function F0() {
+    function F1() {
+        function F2(x = [#|2 + 1|]) {
+        }
+    }
+}
+        `);
+
+        testExtractConstant("extractConstant_StatementInsertionPosition6", `
+class C {
+    x = [#|2 + 1|];
+}
+        `);
+
+        testExtractConstant("extractConstant_StatementInsertionPosition7", `
+const i = 0;
+class C {
+    M() {
+        for (let j = 0; j < 10; j++) {
+            x = [#|i + 1|];
+        }
+    }
+}
+        `);
+
+        testExtractConstant("extractConstant_TripleSlash", `
+/// <reference path="path.js"/>
+
+const x = [#|2 + 1|];
+        `);
+
+        testExtractConstant("extractConstant_PinnedComment", `
+/*! Copyright */
+
+const x = [#|2 + 1|];
+        `);
+
+        testExtractConstant("extractConstant_Directive", `
+"strict";
+
+const x = [#|2 + 1|];
+        `);
+
+        testExtractConstant("extractConstant_MultipleHeaders", `
+/*! Copyright */
+
+/// <reference path="path.js"/>
+
+"strict";
+
+const x = [#|2 + 1|];
+        `);
+
+        // NOTE: this test isn't normative - it just documents our sub-optimal behavior.
+        testExtractConstant("extractConstant_PinnedCommentAndDocComment", `
+/*! Copyright */
+
+/* About x */
+const x = [#|2 + 1|];
+        `);
     });
 
     function testExtractConstant(caption: string, text: string) {

src/harness/unittests/extractFunctions.ts

@@ -365,7 +365,7 @@ function parsePrimaryExpression(): any {
     [#|function G() { }|]
 }`);
 
-// TODO (acasey): handle repeated substitution
+// TODO (18857): handle repeated substitution
 //         testExtractFunction("extractFunction_RepeatedSubstitution",
 //             `namespace X {
 //     export const j = 10;

src/harness/unittests/extractTestHelpers.ts

@@ -113,7 +113,7 @@ namespace ts {
 
             if (hasSyntacticDiagnostics(program)) {
                 // Don't bother generating JS baselines for inputs that aren't valid JS.
-                assert.equal(Extension.Js, extension);
+                assert.equal(Extension.Js, extension, "Syntactic diagnostics found in non-JS file");
                 return;
             }
 

src/services/refactors/extractSymbol.ts

@@ -890,33 +890,56 @@ namespace ts.refactor.extractSymbol {
                 createIdentifier(localNameText));
 
             // Declare
-            const minInsertionPos = node.end;
-            const nodeToInsertBefore = getNodeToInsertConstantBefore(minInsertionPos, scope);
+            const maxInsertionPos = node.pos;
+            const nodeToInsertBefore = getNodeToInsertPropertyBefore(maxInsertionPos, scope);
             changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariable, { suffix: context.newLineCharacter + context.newLineCharacter });
 
             // Consume
             changeTracker.replaceNodeWithNodes(context.file, node, [localReference], { nodeSeparator: context.newLineCharacter });
         }
         else {
-            const newVariable = createVariableStatement(
-                /*modifiers*/ undefined,
-                createVariableDeclarationList(
-                    [createVariableDeclaration(localNameText, variableType, initializer)],
-                    NodeFlags.Const));
+            const newVariableDeclaration = createVariableDeclaration(localNameText, variableType, initializer);
+
+            // If the node is part of an initializer in a list of variable declarations, insert a new
+            // variable declaration into the list (in case it depends on earlier ones).
+            // CONSIDER: If the declaration list isn't const, we might want to split it into multiple
+            // lists so that the newly extracted one can be const.
+            const oldVariableDeclaration = getContainingVariableDeclarationIfInList(node, scope);
+            if (oldVariableDeclaration) {
+                // Declare
+                // CONSIDER: could detect that each is on a separate line
+                changeTracker.insertNodeAt(context.file, oldVariableDeclaration.getStart(), newVariableDeclaration, { suffix: ", " });
 
-            // If the parent is an expression statement, replace the statement with the declaration
-            if (node.parent.kind === SyntaxKind.ExpressionStatement) {
-                changeTracker.replaceNodeWithNodes(context.file, node.parent, [newVariable], { nodeSeparator: context.newLineCharacter });
+                // Consume
+                const localReference = createIdentifier(localNameText);
+                changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference);
+            }
+            else if (node.parent.kind === SyntaxKind.ExpressionStatement) {
+                // If the parent is an expression statement, replace the statement with the declaration.
+                const newVariableStatement = createVariableStatement(
+                    /*modifiers*/ undefined,
+                    createVariableDeclarationList([newVariableDeclaration], NodeFlags.Const));
+                changeTracker.replaceNodeWithNodes(context.file, node.parent, [newVariableStatement], { nodeSeparator: context.newLineCharacter });
             }
             else {
+                const newVariableStatement = createVariableStatement(
+                    /*modifiers*/ undefined,
+                    createVariableDeclarationList([newVariableDeclaration], NodeFlags.Const));
+
                 // Declare
-                const minInsertionPos = node.end;
-                const nodeToInsertBefore = getNodeToInsertConstantBefore(minInsertionPos, scope);
-                changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariable, { suffix: context.newLineCharacter + context.newLineCharacter });
+                const nodeToInsertBefore = getNodeToInsertConstantBefore(node, scope);
+                if (nodeToInsertBefore.pos === 0) {
+                    // If we're at the beginning of the file, we need to take care not to insert before header comments
+                    // (e.g. copyright, triple-slash references).
+                    changeTracker.insertNodeAt(context.file, nodeToInsertBefore.getStart(), newVariableStatement, { suffix: context.newLineCharacter + context.newLineCharacter });
+                }
+                else {
+                    changeTracker.insertNodeBefore(context.file, nodeToInsertBefore, newVariableStatement, { suffix: context.newLineCharacter + context.newLineCharacter });
+                }
 
                 // Consume
                 const localReference = createIdentifier(localNameText);
-                changeTracker.replaceNodeWithNodes(context.file, node, [localReference], { nodeSeparator: context.newLineCharacter });
+                changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference);
             }
         }
 
@@ -927,6 +950,22 @@ namespace ts.refactor.extractSymbol {
         return { renameFilename, renameLocation, edits };
     }
 
+    function getContainingVariableDeclarationIfInList(node: Node, scope: Scope) {
+        let prevNode = undefined;
+        while (node !== undefined && node !== scope) {
+            if (isVariableDeclaration(node) &&
+                node.initializer === prevNode &&
+                isVariableDeclarationList(node.parent) &&
+                node.parent.declarations.length > 1) {
+
+                return node;
+            }
+
+            prevNode = node;
+            node = node.parent;
+        }
+    }
+
     /**
      * @return The index of the (only) reference to the extracted symbol.  We want the cursor
      * to be on the reference, rather than the declaration, because it's closer to where the
@@ -1109,26 +1148,61 @@ namespace ts.refactor.extractSymbol {
             child.pos >= minPos && isFunctionLikeDeclaration(child) && !isConstructorDeclaration(child));
     }
 
-    // TODO (acasey): need to dig into nested statements
-    // TODO (acasey): don't insert before pinned comments, directives, or triple-slash references
-    function getNodeToInsertConstantBefore(maxPos: number, scope: Scope): Node {
-        const children = getStatementsOrClassElements(scope);
-        Debug.assert(children.length > 0); // There must be at least one child, since we extracted from one.
+    function getNodeToInsertPropertyBefore(maxPos: number, scope: ClassLikeDeclaration): Node {
+        const members = scope.members;
+        Debug.assert(members.length > 0); // There must be at least one child, since we extracted from one.
+
+        let prevMember: ClassElement | undefined = undefined;
+        let allProperties = true;
+        for (const member of members) {
+            if (member.pos > maxPos) {
+                return prevMember || members[0];
+            }
+            if (allProperties && !isPropertyDeclaration(member)) {
+                // If it is non-vacuously true that all preceding members are properties,
+                // insert before the current member (i.e. at the end of the list of properties).
+                if (prevMember !== undefined) {
+                    return member;
+                }
 
-        const isClassLikeScope = isClassLike(scope);
-        let prevChild: Statement | ClassElement | undefined = undefined;
-        for (const child of children) {
-            if (child.pos >= maxPos) {
-                break;
+                allProperties = false;
             }
-            prevChild = child;
-            if (isClassLikeScope && !isPropertyDeclaration(child)) {
-                break;
+            prevMember = member;
+        }
+
+        Debug.assert(prevMember !== undefined); // If the loop didn't return, then it did set prevMember.
+        return prevMember;
+    }
+
+    function getNodeToInsertConstantBefore(node: Node, scope: Scope): Node {
+        Debug.assert(!isClassLike(scope));
+
+        let prevScope: Scope | undefined = undefined;
+        for (let curr = node; curr !== scope; curr = curr.parent) {
+            if (isScope(curr)) {
+                prevScope = curr;
             }
         }
 
-        Debug.assert(prevChild !== undefined);
-        return prevChild;
+        for (let curr = (prevScope || node).parent; ; curr = curr.parent) {
+            if (isBlockLike(curr)) {
+                let prevStatement = undefined;
+                for (const statement of curr.statements) {
+                    if (statement.pos > node.pos) {
+                        break;
+                    }
+                    prevStatement = statement;
+                }
+                // There must be at least one statement since we started in one.
+                Debug.assert(prevStatement !== undefined);
+                return prevStatement;
+            }
+
+            if (curr === scope) {
+                Debug.fail("Didn't encounter a block-like before encountering scope");
+                break;
+            }
+        }
     }
 
     function getPropertyAssignmentsForWrites(writes: ReadonlyArray<UsageEntry>): ShorthandPropertyAssignment[] {

tests/baselines/reference/extractConstant/extractConstant_BlockScopes_NoDependencies.js

@@ -5,10 +5,10 @@ for (let i = 0; i < 10; i++) {
     }
 }
 // ==SCOPE::Extract to constant in global scope==
-const newLocal = 1;
-
 for (let i = 0; i < 10; i++) {
     for (let j = 0; j < 10; j++) {
+        const newLocal = 1;
+
         let x = /*RENAME*/newLocal;
     }
 }

tests/baselines/reference/extractConstant/extractConstant_BlockScopes_NoDependencies.ts

@@ -5,10 +5,10 @@ for (let i = 0; i < 10; i++) {
     }
 }
 // ==SCOPE::Extract to constant in global scope==
-const newLocal = 1;
-
 for (let i = 0; i < 10; i++) {
     for (let j = 0; j < 10; j++) {
+        const newLocal = 1;
+
         let x = /*RENAME*/newLocal;
     }
 }