Review changes and adjustment of fourslash tests

Author: rFlorian

src/compiler/diagnosticMessages.json

@@ -4800,11 +4800,11 @@
         "category": "Message",
         "code": 95073
     },
-    "Add const modifier to unresolved variable": {
+    "Add 'const' to unresolved variable": {
         "category": "Message",
         "code": 95074
     },
-    "Add const modifiers to all unresolved variables": {
+    "Add 'const' to all unresolved variables": {
         "category": "Message",
         "code": 95075
     }

src/services/codefixes/addMissingConstInForLoop.ts

@@ -6,18 +6,48 @@ namespace ts.codefix {
         errorCodes,
         getCodeActions: (context) => {
             const changes = textChanges.ChangeTracker.with(context, t => makeChange(t, context.sourceFile, context.span.start));
-            return [createCodeFixAction(fixId, changes, Diagnostics.Add_const_modifier_to_unresolved_variable, fixId, Diagnostics.Add_const_modifiers_to_all_unresolved_variables)];
+            if (changes) {
+                return [createCodeFixAction(fixId, changes, Diagnostics.Add_const_to_unresolved_variable, fixId, Diagnostics.Add_const_to_all_unresolved_variables)];
+            }
         },
         fixIds: [fixId],
         getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => makeChange(changes, diag.file, diag.start)),
     });
 
     function makeChange(changeTracker: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number) {
-        const token = getTokenAtPosition(sourceFile, pos);
-        // fails on 'for ([x, y] of [[1,2]]) {}' when called for y
-        // since findPrecedingMatchingToken does not return the open paren here after iterating over another identifier (x)
-        const openParenToken = <Node>findPrecedingMatchingToken(token, SyntaxKind.OpenParenToken, sourceFile);
-        Debug.assert(!!openParenToken, "openParenToken must be defined");
-        changeTracker.insertNodeAt(sourceFile, openParenToken.getEnd(), createToken(SyntaxKind.ConstKeyword), { suffix: " " });
+        const forInitializer = findAncestor(getTokenAtPosition(sourceFile, pos), node =>
+            isForInOrOfStatement(node.parent) ? node.parent.initializer === node
+                : isPossiblyPartOfDestructuring(node) ? false : "quit");
+        if (!forInitializer) return;
+        if (alreadyContainsConstCodeFixForInitializer(changeTracker, forInitializer, sourceFile)) return;
+        changeTracker.insertNodeBefore(sourceFile, forInitializer, createToken(SyntaxKind.ConstKeyword));
+    }
+
+    function isPossiblyPartOfDestructuring(node: Node): boolean {
+        switch (node.kind) {
+            case SyntaxKind.Identifier:
+            case SyntaxKind.ArrayLiteralExpression:
+            case SyntaxKind.ObjectLiteralExpression:
+            case SyntaxKind.PropertyAssignment:
+            case SyntaxKind.ShorthandPropertyAssignment:
+                return true;
+            default:
+                return false;
+        }
+    }
+
+    function alreadyContainsConstCodeFixForInitializer(changeTracker: textChanges.ChangeTracker, forInitializer: Node, sourceFile: SourceFile): boolean {
+        return changeTracker.getChanges().some(change => {
+            const textChanges = change.textChanges;
+            if (!textChanges) return false;
+            return textChanges.some(textChange => {
+                if (textChange.newText !== "const ") return false;
+                const changeStart = textChange.span.start;
+                const changeEnd = changeStart + textChange.span.length;
+                const initStart = forInitializer.getStart(sourceFile);
+                const initEnd = forInitializer.getEnd();
+                return initStart <= changeEnd && changeStart <= initEnd;
+            });
+        });
     }
 }

src/services/textChanges.ts

@@ -403,6 +403,9 @@ namespace ts.textChanges {
             else if (isStringLiteral(before) && isImportDeclaration(before.parent) || isNamedImports(before)) {
                 return { suffix: ", " };
             }
+            else if (isForInitializer(before)) {
+                return { suffix: " " };
+            }
             return Debug.failBadSyntaxKind(before); // We haven't handled this kind of node yet -- add it
         }
 

tests/cases/fourslash/codeFixAddMissingConstInForInLoop1.ts

@@ -0,0 +1,8 @@
+/// <reference path='fourslash.ts' />
+
+////for (x in []) {}
+
+verify.codeFix({
+    description: "Add 'const' to unresolved variable",
+    newFileContent: "for (const x in []) {}"
+});

tests/cases/fourslash/codeFixAddMissingConstInForInLoop2.ts

@@ -0,0 +1,12 @@
+/// <reference path='fourslash.ts' />
+
+////for (x in []) {}
+////for (y in []) {}
+
+verify.codeFixAll({
+    fixId: "addMissingConstInForLoop",
+    fixAllDescription: "Add 'const' to all unresolved variables",
+    newFileContent: 
+`for (const x in []) {}
+for (const y in []) {}`
+});

tests/cases/fourslash/codeFixAddMissingConstInForLoop1.ts

@@ -0,0 +1,8 @@
+/// <reference path='fourslash.ts' />
+
+////for ([x] of [[1,2]]) {}
+
+verify.codeFix({
+    description: "Add 'const' to unresolved variable",
+    newFileContent: "for (const [x] of [[1,2]]) {}"
+});

tests/cases/fourslash/codeFixAddMissingConstInForLoop3.ts

@@ -0,0 +1,12 @@
+/// <reference path='fourslash.ts' />
+
+////for ([x, y] of [[1,2]]) {}
+////for ([x] of [[1,2]]) {}
+
+verify.codeFixAll({
+    fixId: "addMissingConstInForLoop",
+    fixAllDescription: "Add 'const' to all unresolved variables",
+    newFileContent: 
+`for (const [x, y] of [[1,2]]) {}
+for (const [x] of [[1,2]]) {}`
+});