Added alphabetization logic to missing import quick fixes

src/services/codefixes/importFixes.ts

@@ -597,10 +597,7 @@ namespace ts.codefix {
                 const newImportSpecifier = createImportSpecifier(/*propertyName*/ undefined, createIdentifier(symbolName));
                 if (namedBindings && namedBindings.kind === SyntaxKind.NamedImports && namedBindings.elements.length !== 0) {
                     // There are already named imports; add another.
-                    return ChangeTracker.with(context, t => t.insertNodeInListAfter(
-                        sourceFile,
-                        namedBindings.elements[namedBindings.elements.length - 1],
-                        newImportSpecifier));
+                    return insertNodeInsideListNodesAlphabetically(context, sourceFile, namedBindings.elements, newImportSpecifier);
                 }
                 if (!namedBindings || namedBindings.kind === SyntaxKind.NamedImports && namedBindings.elements.length === 0) {
                     return ChangeTracker.with(context, t =>
@@ -621,6 +618,27 @@ namespace ts.codefix {
         }
     }
 
+    /**
+     * This function should be used to insert nodes alphabetically in lists when nodes don't carry separators as the part of the node range,
+     * e.g. export lists, import lists, etc.
+     * Linear search is used instead of binary as the (generally few) nodes are not guaranteed to be in order.
+     */
+    function insertNodeInsideListNodesAlphabetically(context: SymbolContext, sourceFile: SourceFile, containingList: NodeArray<ImportSpecifier>, newNode: ImportSpecifier) {
+        return ChangeTracker.with(context, t => {
+            if (compareStringsCaseInsensitive(newNode.name.text, containingList[0].name.text) === Comparison.LessThan) {
+                return t.insertNodeInListBeforeFirst(sourceFile, containingList, containingList[0], newNode);
+            }
+
+            for (let i = 1; i < containingList.length; i += 1) {
+                if (compareStringsCaseInsensitive(newNode.name.text, containingList[i].name.text) === Comparison.LessThan) {
+                    return t.insertNodeInListAfter(sourceFile, containingList[i - 1], newNode);
+                }
+            }
+
+            return t.insertNodeInListAfter(sourceFile, containingList[containingList.length - 1], newNode);
+        });
+    }
+
     function getCodeActionForUseExistingNamespaceImport(namespacePrefix: string, context: SymbolContext, symbolToken: Identifier): CodeFixAction {
         const { symbolName, sourceFile } = context;
 

src/services/textChanges.ts

@@ -483,107 +483,150 @@ namespace ts.textChanges {
             if (index < 0) {
                 return this;
             }
-            const end = after.getEnd();
             if (index !== containingList.length - 1) {
                 // any element except the last one
                 // use next sibling as an anchor
                 const nextToken = getTokenAtPosition(sourceFile, after.end, /*includeJsDocComment*/ false);
                 if (nextToken && isSeparator(after, nextToken)) {
-                    // for list
-                    // a, b, c
-                    // create change for adding 'e' after 'a' as
-                    // - find start of next element after a (it is b)
-                    // - use this start as start and end position in final change
-                    // - build text of change by formatting the text of node + separator + whitespace trivia of b
-
-                    // in multiline case it will work as
-                    //   a,
-                    //   b,
-                    //   c,
-                    // result - '*' denotes leading trivia that will be inserted after new text (displayed as '#')
-                    //   a,*
-                    // ***insertedtext<separator>#
-                    // ###b,
-                    //   c,
-                    // find line and character of the next element
-                    const lineAndCharOfNextElement = getLineAndCharacterOfPosition(sourceFile, skipWhitespacesAndLineBreaks(sourceFile.text, containingList[index + 1].getFullStart()));
-                    // find line and character of the token that precedes next element (usually it is separator)
-                    const lineAndCharOfNextToken = getLineAndCharacterOfPosition(sourceFile, nextToken.end);
-                    let prefix: string;
-                    let startPos: number;
-                    if (lineAndCharOfNextToken.line === lineAndCharOfNextElement.line) {
-                        // next element is located on the same line with separator:
-                        // a,$$$$b
-                        //  ^    ^
-                        //  |    |-next element
-                        //  |-separator
-                        // where $$$ is some leading trivia
-                        // for a newly inserted node we'll maintain the same relative position comparing to separator and replace leading trivia with spaces
-                        // a,    x,$$$$b
-                        //  ^    ^     ^
-                        //  |    |     |-next element
-                        //  |    |-new inserted node padded with spaces
-                        //  |-separator
-                        startPos = nextToken.end;
-                        prefix = spaces(lineAndCharOfNextElement.character - lineAndCharOfNextToken.character);
-                    }
-                    else {
-                        // next element is located on different line that separator
-                        // let insert position be the beginning of the line that contains next element
-                        startPos = getStartPositionOfLine(lineAndCharOfNextElement.line, sourceFile);
-                    }
-
-                    // write separator and leading trivia of the next element as suffix
-                    const suffix = `${tokenToString(nextToken.kind)}${sourceFile.text.substring(nextToken.end, containingList[index + 1].getStart(sourceFile))}`;
-                    this.replaceRange(sourceFile, createTextRange(startPos, containingList[index + 1].getStart(sourceFile)), newNode, { prefix, suffix });
+                    this.insertNodeInListAfterNotLast(sourceFile, containingList, nextToken, index, newNode);
                 }
             }
             else {
-                const afterStart = after.getStart(sourceFile);
-                const afterStartLinePosition = getLineStartPositionForPosition(afterStart, sourceFile);
-
-                let separator: SyntaxKind.CommaToken | SyntaxKind.SemicolonToken;
-                let multilineList = false;
-
-                // insert element after the last element in the list that has more than one item
-                // pick the element preceding the after element to:
-                // - pick the separator
-                // - determine if list is a multiline
-                if (containingList.length === 1) {
-                    // if list has only one element then we'll format is as multiline if node has comment in trailing trivia, or as singleline otherwise
-                    // i.e. var x = 1 // this is x
-                    //     | new element will be inserted at this position
-                    separator = SyntaxKind.CommaToken;
-                }
-                else {
-                    // element has more than one element, pick separator from the list
-                    const tokenBeforeInsertPosition = findPrecedingToken(after.pos, sourceFile);
-                    separator = isSeparator(after, tokenBeforeInsertPosition) ? tokenBeforeInsertPosition.kind : SyntaxKind.CommaToken;
-                    // determine if list is multiline by checking lines of after element and element that precedes it.
-                    const afterMinusOneStartLinePosition = getLineStartPositionForPosition(containingList[index - 1].getStart(sourceFile), sourceFile);
-                    multilineList = afterMinusOneStartLinePosition !== afterStartLinePosition;
-                }
-                if (hasCommentsBeforeLineBreak(sourceFile.text, after.end)) {
-                    // in this case we'll always treat containing list as multiline
-                    multilineList = true;
-                }
-                if (multilineList) {
-                    // insert separator immediately following the 'after' node to preserve comments in trailing trivia
-                    this.replaceRange(sourceFile, createTextRange(end), createToken(separator));
-                    // use the same indentation as 'after' item
-                    const indentation = formatting.SmartIndenter.findFirstNonWhitespaceColumn(afterStartLinePosition, afterStart, sourceFile, this.formatContext.options);
-                    // insert element before the line break on the line that contains 'after' element
-                    let insertPos = skipTrivia(sourceFile.text, end, /*stopAfterLineBreak*/ true, /*stopAtComments*/ false);
-                    if (insertPos !== end && isLineBreak(sourceFile.text.charCodeAt(insertPos - 1))) {
-                        insertPos--;
-                    }
-                    this.replaceRange(sourceFile, createTextRange(insertPos), newNode, { indentation, prefix: this.newLineCharacter });
-                }
-                else {
-                    this.replaceRange(sourceFile, createTextRange(end), newNode, { prefix: `${tokenToString(separator)} ` });
+                this.insertNodeInListAfterLast(sourceFile, containingList, after, index, newNode);
+            }
+        }
+
+        public insertNodeInListBeforeFirst(sourceFile: SourceFile, containingList: NodeArray<Node>, first: Node, newNode: Node): void {
+            const startPosition = first.getStart(sourceFile);
+            const afterStartLinePosition = getLineStartPositionForPosition(startPosition, sourceFile);
+            const { multilineList, separator } = this.getMultilineAndSeparatorOfList(sourceFile, containingList, afterStartLinePosition, 1, first);
+
+            if (!multilineList) {
+                this.changes.push({
+                    kind: ChangeKind.ReplaceWithSingleNode,
+                    sourceFile,
+                    range: { pos: startPosition, end: startPosition },
+                    node: newNode,
+                    options: { suffix: `${tokenToString(separator)} ` }
+                });
+                return;
+            }
+
+            const last = containingList[containingList.length - 1];
+            const indentation = formatting.SmartIndenter.findFirstNonWhitespaceColumn(afterStartLinePosition, last.end, sourceFile, this.formatContext.options);
+            const insertPos = skipTrivia(sourceFile.text, startPosition, /*stopAfterLineBreak*/ true, /*stopAtComments*/ false) - indentation;
+
+            this.changes.push({
+                kind: ChangeKind.ReplaceWithSingleNode,
+                sourceFile,
+                range: { pos: insertPos, end: insertPos },
+                node: newNode,
+                options: { indentation, suffix: `${tokenToString(separator)}${this.newLineCharacter}` }
+            });
+        }
+
+        private insertNodeInListAfterNotLast(sourceFile: SourceFile, containingList: NodeArray<Node>, nextToken: Node, index: number, newNode: Node): void {
+            // for list
+            // a, b, c
+            // create change for adding 'e' after 'a' as
+            // - find start of next element after a (it is b)
+            // - use this start as start and end position in final change
+            // - build text of change by formatting the text of node + separator + whitespace trivia of b
+
+            // in multiline case it will work as
+            //   a,
+            //   b,
+            //   c,
+            // result - '*' denotes leading trivia that will be inserted after new text (displayed as '#')
+            //   a,*
+            // ***insertedtext<separator>#
+            // ###b,
+            //   c,
+            // find line and character of the next element
+            const lineAndCharOfNextElement = getLineAndCharacterOfPosition(sourceFile, skipWhitespacesAndLineBreaks(sourceFile.text, containingList[index + 1].getFullStart()));
+            // find line and character of the token that precedes next element (usually it is separator)
+            const lineAndCharOfNextToken = getLineAndCharacterOfPosition(sourceFile, nextToken.end);
+            let prefix: string;
+            let startPos: number;
+            if (lineAndCharOfNextToken.line === lineAndCharOfNextElement.line) {
+                // next element is located on the same line with separator:
+                // a,$$$$b
+                //  ^    ^
+                //  |    |-next element
+                //  |-separator
+                // where $$$ is some leading trivia
+                // for a newly inserted node we'll maintain the same relative position comparing to separator and replace leading trivia with spaces
+                // a,    x,$$$$b
+                //  ^    ^     ^
+                //  |    |     |-next element
+                //  |    |-new inserted node padded with spaces
+                //  |-separator
+                startPos = nextToken.end;
+                prefix = spaces(lineAndCharOfNextElement.character - lineAndCharOfNextToken.character);
+            }
+            else {
+                // next element is located on different line that separator
+                // let insert position be the beginning of the line that contains next element
+                startPos = getStartPositionOfLine(lineAndCharOfNextElement.line, sourceFile);
+            }
+
+            // write separator and leading trivia of the next element as suffix
+            const suffix = `${tokenToString(nextToken.kind)}${sourceFile.text.substring(nextToken.end, containingList[index + 1].getStart(sourceFile))}`;
+            this.replaceRange(sourceFile, createTextRange(startPos, containingList[index + 1].getStart(sourceFile)), newNode, { prefix, suffix });
+        }
+
+        private insertNodeInListAfterLast(sourceFile: SourceFile, containingList: NodeArray<Node>, after: Node, index: number, newNode: Node): void {
+            const end = after.getEnd();
+            const afterStart = after.getStart(sourceFile);
+            const afterStartLinePosition = getLineStartPositionForPosition(afterStart, sourceFile);
+
+            const { multilineList, separator } = this.getMultilineAndSeparatorOfList(sourceFile, containingList, afterStartLinePosition, index - 1, after);
+
+            if (multilineList) {
+                // insert separator immediately following the 'after' node to preserve comments in trailing trivia
+                this.replaceRange(sourceFile, createTextRange(end), createToken(separator));
+                // use the same indentation as 'after' item
+                const indentation = formatting.SmartIndenter.findFirstNonWhitespaceColumn(afterStartLinePosition, afterStart, sourceFile, this.formatContext.options);
+                // insert element before the line break on the line that contains 'after' element
+                let insertPos = skipTrivia(sourceFile.text, end, /*stopAfterLineBreak*/ true, /*stopAtComments*/ false);
+                if (insertPos !== end && isLineBreak(sourceFile.text.charCodeAt(insertPos - 1))) {
+                    insertPos--;
                 }
+                this.replaceRange(sourceFile, createTextRange(insertPos), newNode, { indentation, prefix: this.newLineCharacter });
             }
-            return this;
+            else {
+                this.replaceRange(sourceFile, createTextRange(end), newNode, { prefix: `${tokenToString(separator)} ` });
+            }
+        }
+
+        private getMultilineAndSeparatorOfList(sourceFile: SourceFile, containingList: NodeArray<Node>, afterStartLinePosition: number, sampleIndex: number, sampleNode: Node) {
+            let separator: SyntaxKind.CommaToken | SyntaxKind.SemicolonToken;
+            let multilineList = false;
+
+            // insert element after the last element in the list that has more than one item
+            // pick the element preceding the after element to:
+            // - pick the separator
+            // - determine if list is a multiline
+            if (containingList.length === 1) {
+                // if list has only one element then we'll format is as multiline if node has comment in trailing trivia, or as singleline otherwise
+                // i.e. var x = 1 // this is x
+                //     | new element will be inserted at this position
+                separator = SyntaxKind.CommaToken;
+            }
+            else {
+                // element has more than one element, pick separator from the list
+                const tokenBeforeInsertPosition = findPrecedingToken(sampleNode.pos, sourceFile);
+                separator = isSeparator(sampleNode, tokenBeforeInsertPosition) ? tokenBeforeInsertPosition.kind : SyntaxKind.CommaToken;
+                // determine if list is multiline by checking lines of after element and element that precedes it.
+                const afterMinusOneStartLinePosition = getLineStartPositionForPosition(containingList[sampleIndex].getStart(sourceFile), sourceFile);
+                multilineList = afterMinusOneStartLinePosition !== afterStartLinePosition;
+            }
+            if (hasCommentsBeforeLineBreak(sourceFile.text, sampleNode.end)) {
+                // in this case we'll always treat containing list as multiline
+                multilineList = true;
+            }
+
+            return { multilineList, separator };
         }
 
         private finishInsertNodeAtClassStart(): void {

tests/cases/fourslash/completionsImport_named_addToNamedImports.ts

@@ -18,6 +18,6 @@ verify.applyCodeActionFromCompletion("", {
     name: "foo",
     source: "/a",
     description: `Add 'foo' to existing import declaration from "./a"`,
-    newFileContent: `import { x, foo } from "./a";
+    newFileContent: `import { foo, x } from "./a";
 f;`,
 });

tests/cases/fourslash/completionsImport_named_didNotExistBefore.ts

@@ -18,6 +18,6 @@ verify.applyCodeActionFromCompletion("", {
     name: "Test1",
     source: "/a",
     description: `Add 'Test1' to existing import declaration from "./a"`,
-    newFileContent: `import { Test2, Test1 } from "./a";
+    newFileContent: `import { Test1, Test2 } from "./a";
 t`,
 });

tests/cases/fourslash/importNameCodeFixExistingImport0.ts

@@ -7,4 +7,4 @@
 //// export function f1() {}
 //// export var v1 = 5;
 
-verify.importFixAtPosition([`{ v1, f1 }`]);
+verify.importFixAtPosition([`{ f1, v1 }`]);

tests/cases/fourslash/importNameCodeFixExistingImport1.ts

@@ -8,4 +8,4 @@
 //// export var v1 = 5;
 //// export default var d1 = 6;
 
-verify.importFixAtPosition([`{ v1, f1 }`]);
+verify.importFixAtPosition([`{ f1, v1 }`]);

tests/cases/fourslash/importNameCodeFixExistingImport10.ts

@@ -10,12 +10,11 @@
 //// export function f1() {}
 //// export var v1 = 5;
 //// export var v2 = 5;
-//// export var v3 = 5;
 
 verify.importFixAtPosition([
 `{
+    f1,
     v1,
-    v2,
-    f1
+    v2
 }`
 ]);

tests/cases/fourslash/importNameCodeFixExistingImport11.ts

@@ -14,8 +14,7 @@
 
 verify.importFixAtPosition([
 `{
-    v1, v2,
-    v3,
-    f1
+    f1, v1, v2,
+    v3
 }`
 ]);

tests/cases/fourslash/importNameCodeFixExistingImport12.ts

@@ -1,6 +1,6 @@
 /// <reference path="fourslash.ts" />
 
-//// import [|{}|] from "./module";
+//// import [|{v1, v2, v3,}|] from "./module";
 //// f1/*0*/();
 
 // @Filename: module.ts
@@ -9,4 +9,4 @@
 //// export var v2 = 5;
 //// export var v3 = 5;
 
-verify.importFixAtPosition([`{ f1 }`]);
+verify.importFixAtPosition([`{f1, v1, v2, v3,}`]);

tests/cases/fourslash/importNameCodeFixExistingImport13.ts

@@ -0,0 +1,12 @@
+/// <reference path="fourslash.ts" />
+
+//// import [|{v1, v3, v4,}|] from "./module";
+//// v2/*0*/();
+
+// @Filename: module.ts
+//// export var v1 = 5;
+//// export function v2() {}
+//// export var v3 = 5;
+//// export var v4 = 5;
+
+verify.importFixAtPosition([`{v1, v2, v3, v4,}`]);

tests/cases/fourslash/importNameCodeFixExistingImport14.ts

@@ -0,0 +1,20 @@
+/// <reference path="fourslash.ts" />
+
+//// import [|{
+////     v1,
+////     v3
+//// }|] from "./module";
+//// v2/*0*/();
+
+// @Filename: module.ts
+//// export function v2() {}
+//// export var v1 = 5;
+//// export var v3 = 5;
+
+verify.importFixAtPosition([
+`{
+    v1,
+    v2,
+    v3
+}`
+]);