diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 95cd84ee620bf..7104b8d835910 100644 --- a/src/services/codefixes/importFixes.ts +++ b/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, 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; diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 0b29cdc438b4b..9a8ed7791a5cc 100644 --- a/src/services/textChanges.ts +++ b/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# - // ###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, 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, 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# + // ###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, 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, 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 { diff --git a/tests/cases/fourslash/completionsImport_named_addToNamedImports.ts b/tests/cases/fourslash/completionsImport_named_addToNamedImports.ts index fa1be5ca7d2a5..1883a43922001 100644 --- a/tests/cases/fourslash/completionsImport_named_addToNamedImports.ts +++ b/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;`, }); diff --git a/tests/cases/fourslash/completionsImport_named_didNotExistBefore.ts b/tests/cases/fourslash/completionsImport_named_didNotExistBefore.ts index a69c0ab19ca86..e44a42c3d51b5 100644 --- a/tests/cases/fourslash/completionsImport_named_didNotExistBefore.ts +++ b/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`, }); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport0.ts b/tests/cases/fourslash/importNameCodeFixExistingImport0.ts index e5b634998783c..922285e9afca3 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport0.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport0.ts @@ -7,4 +7,4 @@ //// export function f1() {} //// export var v1 = 5; -verify.importFixAtPosition([`{ v1, f1 }`]); +verify.importFixAtPosition([`{ f1, v1 }`]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport1.ts b/tests/cases/fourslash/importNameCodeFixExistingImport1.ts index 49680692b6c58..f06ce49f0ea6b 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport1.ts +++ b/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 }`]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport10.ts b/tests/cases/fourslash/importNameCodeFixExistingImport10.ts index a9525d272da00..d29fd9e5682de 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport10.ts +++ b/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 }` ]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport11.ts b/tests/cases/fourslash/importNameCodeFixExistingImport11.ts index 786e0e397756e..878e253680911 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport11.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport11.ts @@ -14,8 +14,7 @@ verify.importFixAtPosition([ `{ - v1, v2, - v3, - f1 + f1, v1, v2, + v3 }` ]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport12.ts b/tests/cases/fourslash/importNameCodeFixExistingImport12.ts index 87ba29ab0aa21..39a2ca0d59a11 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport12.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport12.ts @@ -1,6 +1,6 @@ /// -//// 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,}`]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport13.ts b/tests/cases/fourslash/importNameCodeFixExistingImport13.ts new file mode 100644 index 0000000000000..c9d242ffdc7e0 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport13.ts @@ -0,0 +1,12 @@ +/// + +//// 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,}`]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport14.ts b/tests/cases/fourslash/importNameCodeFixExistingImport14.ts new file mode 100644 index 0000000000000..3e6b557830430 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport14.ts @@ -0,0 +1,20 @@ +/// + +//// 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 +}` +]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport15.ts b/tests/cases/fourslash/importNameCodeFixExistingImport15.ts new file mode 100644 index 0000000000000..1788831aebd2d --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport15.ts @@ -0,0 +1,21 @@ +/// + +////import [|{ +//// v1, v2, +//// v4 +////}|] from "./module"; +////v3/*0*/(); + +// @Filename: module.ts +//// export function v3() {} +//// export var v1 = 5; +//// export var v2 = 5; +//// export var v4 = 5; + +verify.importFixAtPosition([ +`{ + v1, v2, + v3, + v4 +}` +]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport16.ts b/tests/cases/fourslash/importNameCodeFixExistingImport16.ts new file mode 100644 index 0000000000000..a365136607d55 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport16.ts @@ -0,0 +1,12 @@ +/// + +//// import [|{v1, v2, v3,}|] from "./module"; +//// v4/*0*/(); + +// @Filename: module.ts +//// export function v4() {} +//// export var v1 = 5; +//// export var v2 = 5; +//// export var v3 = 5; + +verify.importFixAtPosition([`{v1, v2, v3, v4,}`]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport17.ts b/tests/cases/fourslash/importNameCodeFixExistingImport17.ts new file mode 100644 index 0000000000000..d0bfcfe106467 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport17.ts @@ -0,0 +1,20 @@ +/// + +//// import [|{ +//// v1, +//// v2 +//// }|] from "./module"; +//// v3/*0*/(); + +// @Filename: module.ts +//// export function v3() {} +//// export var v1 = 5; +//// export var v2 = 5; + +verify.importFixAtPosition([ +`{ + v1, + v2, + v3 +}` +]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport18.ts b/tests/cases/fourslash/importNameCodeFixExistingImport18.ts new file mode 100644 index 0000000000000..60ba577ecde90 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport18.ts @@ -0,0 +1,21 @@ +/// + +////import [|{ +//// v1, v2, +//// v3 +////}|] from "./module"; +////v4/*0*/(); + +// @Filename: module.ts +//// export function v4() {} +//// export var v1 = 5; +//// export var v2 = 5; +//// export var v3 = 5; + +verify.importFixAtPosition([ +`{ + v1, v2, + v3, + v4 +}` +]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport6.ts b/tests/cases/fourslash/importNameCodeFixExistingImport6.ts index 0d3636e443955..7b2bd7d5383f5 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport6.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport6.ts @@ -10,4 +10,4 @@ //// export var v1 = 5; //// export function f1(); -verify.importFixAtPosition([`{ v1, f1 }`]); +verify.importFixAtPosition([`{ f1, v1 }`]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport7.ts b/tests/cases/fourslash/importNameCodeFixExistingImport7.ts index f0b1a77f69012..8a8ceb926c230 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport7.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport7.ts @@ -7,4 +7,4 @@ //// export var v1 = 5; //// export function f1(); -verify.importFixAtPosition([`{ v1, f1 }`]); +verify.importFixAtPosition([`{ f1, v1 }`]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport8.ts b/tests/cases/fourslash/importNameCodeFixExistingImport8.ts index a1b676a8b8309..39a2ca0d59a11 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport8.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport8.ts @@ -9,4 +9,4 @@ //// export var v2 = 5; //// export var v3 = 5; -verify.importFixAtPosition([`{v1, v2, v3, f1,}`]); +verify.importFixAtPosition([`{f1, v1, v2, v3,}`]); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport9.ts b/tests/cases/fourslash/importNameCodeFixExistingImport9.ts index 67960de32d4c2..bf7709e12d954 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport9.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport9.ts @@ -11,6 +11,6 @@ verify.importFixAtPosition([ `{ - v1, f1 + f1, v1 }` ]);