Merge pull request microsoft#18861 from amcasey/ConstantInsertionPosition

Improve insertion positions of extracted constants

Author: amcasey

src/compiler/diagnosticMessages.json

@@ -3724,7 +3724,7 @@
         "code": 95003
     },
 
-    "Extract to {0}": {
+    "Extract to {0} in {1}": {
         "category": "Message",
         "code": 95004
     },

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,26 +84,126 @@ 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;
 //     export const y = [#|j * j|];
 // }`);
 
-        testExtractConstantFailed("extractConstant_BlockScopes_Dependencies",
-            `for (let i = 0; i < 10; i++) {
+        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|];`);
+
+        testExtractConstant("extractConstant_BlockScopeMismatch", `
+for (let i = 0; i < 10; i++) {
     for (let j = 0; j < 10; j++) {
-        let x = [#|i + 1|];
+        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|];
+        `);
+
+        testExtractConstant("extractConstant_PinnedCommentAndDocComment", `
+/*! Copyright */
+
+/* About x */
+const x = [#|2 + 1|];
+        `);
     });
 
     function testExtractConstant(caption: string, text: string) {
         testExtractSymbol(caption, text, "extractConstant", Diagnostics.Extract_constant);
     }
-
-    function testExtractConstantFailed(caption: string, text: string) {
-        testExtractSymbolFailed(caption, text, Diagnostics.Extract_constant);
-    }
 }

src/harness/unittests/extractFunctions.ts

@@ -355,7 +355,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/codefixes/importFixes.ts

@@ -407,28 +407,6 @@ namespace ts.codefix {
                     moduleSpecifierWithoutQuotes
                 );
 
-                function getSourceFileImportLocation(node: SourceFile) {
-                    // For a source file, it is possible there are detached comments we should not skip
-                    const text = node.text;
-                    let ranges = getLeadingCommentRanges(text, 0);
-                    if (!ranges) return 0;
-                    let position = 0;
-                    // However we should still skip a pinned comment at the top
-                    if (ranges.length && ranges[0].kind === SyntaxKind.MultiLineCommentTrivia && isPinnedComment(text, ranges[0])) {
-                        position = ranges[0].end + 1;
-                        ranges = ranges.slice(1);
-                    }
-                    // As well as any triple slash references
-                    for (const range of ranges) {
-                        if (range.kind === SyntaxKind.SingleLineCommentTrivia && isRecognizedTripleSlashComment(node.text, range.pos, range.end)) {
-                            position = range.end + 1;
-                            continue;
-                        }
-                        break;
-                    }
-                    return position;
-                }
-
                 function getSingleQuoteStyleFromExistingImports() {
                     const firstModuleSpecifier = forEach(sourceFile.statements, node => {
                         if (isImportDeclaration(node) || isExportDeclaration(node)) {