From 91e5aba9c52ac076523846d49332e0abdcc7bbc3 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Fri, 23 Mar 2018 14:13:40 -0700 Subject: [PATCH] Alternate but more general token comment emit fix + organizeImports fix (#22836) @amcasey: Preserve leading trivia when organizing imports When organizing imports, we used to move the leading and trailing trivia of each individual import with that import as it was repositioned or deleted. This had the unfortunate effect of repositioning or deleting the header comment of the file. Our new approach is to leave the leading trivia of the first import ahead of the resulting block of imports (i.e. it no longer follows the first import as it is repositioned or deleted). Trailing trivia on the first import and trivia on subsequent imports are treated as before. --- src/compiler/emitter.ts | 8 ++--- src/harness/unittests/organizeImports.ts | 22 +++++++++++++ src/services/organizeImports.ts | 14 ++++++-- src/services/utilities.ts | 32 ++++++++++++++----- .../organizeImports/CoalesceTrivia.ts | 2 +- .../organizeImports/SortHeaderComment.ts | 11 +++++++ .../reference/organizeImports/SortTrivia.ts | 4 +-- .../organizeImports/UnusedHeaderComment.ts | 8 +++++ .../organizeImports/UnusedTrivia1.ts | 1 + .../organizeImports/UnusedTrivia2.ts | 2 +- 10 files changed, 86 insertions(+), 18 deletions(-) create mode 100644 tests/baselines/reference/organizeImports/SortHeaderComment.ts create mode 100644 tests/baselines/reference/organizeImports/UnusedHeaderComment.ts diff --git a/src/compiler/emitter.ts b/src/compiler/emitter.ts index c18a7f7411366..fbe8e35604f19 100644 --- a/src/compiler/emitter.ts +++ b/src/compiler/emitter.ts @@ -1728,14 +1728,14 @@ namespace ts { writeSemicolon(); } - function emitTokenWithComment(token: SyntaxKind, pos: number, writer: (s: string) => void, contextNode?: Node, indentLeading?: boolean) { - const node = contextNode && getParseTreeNode(contextNode); + function emitTokenWithComment(token: SyntaxKind, pos: number, writer: (s: string) => void, contextNode: Node, indentLeading?: boolean) { + const node = getParseTreeNode(contextNode); const isSimilarNode = node && node.kind === contextNode.kind; const startPos = pos; if (isSimilarNode) { pos = skipTrivia(currentSourceFile.text, pos); } - if (emitLeadingCommentsOfPosition && isSimilarNode) { + if (emitLeadingCommentsOfPosition && isSimilarNode && contextNode.pos !== startPos) { const needsIndent = indentLeading && !positionsAreOnSameLine(startPos, pos, currentSourceFile); if (needsIndent) { increaseIndent(); @@ -1746,7 +1746,7 @@ namespace ts { } } pos = writeTokenText(token, writer, pos); - if (emitTrailingCommentsOfPosition && isSimilarNode) { + if (emitTrailingCommentsOfPosition && isSimilarNode && contextNode.end !== pos) { emitTrailingCommentsOfPosition(pos, /*prefixSpace*/ true); } return pos; diff --git a/src/harness/unittests/organizeImports.ts b/src/harness/unittests/organizeImports.ts index bb5c9cbee1867..eaab3147e44d5 100644 --- a/src/harness/unittests/organizeImports.ts +++ b/src/harness/unittests/organizeImports.ts @@ -329,6 +329,28 @@ F1(); }, libFile); + testOrganizeImports("UnusedHeaderComment", + { + path: "/test.ts", + content: ` +// Header +import { F1 } from "lib"; +`, + }, + libFile); + + testOrganizeImports("SortHeaderComment", + { + path: "/test.ts", + content: ` +// Header +import "lib2"; +import "lib1"; +`, + }, + { path: "/lib1.ts", content: "" }, + { path: "/lib2.ts", content: "" }); + testOrganizeImports("AmbientModule", { path: "/test.ts", diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts index 511fa821fd231..11ccf6c3b9ec6 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -32,6 +32,13 @@ namespace ts.OrganizeImports { return; } + // Special case: normally, we'd expect leading and trailing trivia to follow each import + // around as it's sorted. However, we do not want this to happen for leading trivia + // on the first import because it is probably the header comment for the file. + // Consider: we could do a more careful check that this trivia is actually a header, + // but the consequences of being wrong are very minor. + suppressLeadingTrivia(oldImportDecls[0]); + const oldImportGroups = group(oldImportDecls, importDecl => getExternalModuleName(importDecl.moduleSpecifier)); const sortedImportGroups = stableSort(oldImportGroups, (group1, group2) => compareModuleSpecifiers(group1[0].moduleSpecifier, group2[0].moduleSpecifier)); const newImportDecls = flatMap(sortedImportGroups, importGroup => @@ -41,12 +48,15 @@ namespace ts.OrganizeImports { // Delete or replace the first import. if (newImportDecls.length === 0) { - changeTracker.deleteNode(sourceFile, oldImportDecls[0]); + changeTracker.deleteNode(sourceFile, oldImportDecls[0], { + useNonAdjustedStartPosition: true, // Leave header comment in place + useNonAdjustedEndPosition: false, + }); } else { // Note: Delete the surrounding trivia because it will have been retained in newImportDecls. changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, { - useNonAdjustedStartPosition: false, + useNonAdjustedStartPosition: true, // Leave header comment in place useNonAdjustedEndPosition: false, suffix: getNewLineOrDefaultFromHost(host, formatContext.options), }); diff --git a/src/services/utilities.ts b/src/services/utilities.ts index ef6bb21be4068..e4e49ef2a9fba 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1496,14 +1496,30 @@ namespace ts { */ /* @internal */ export function suppressLeadingAndTrailingTrivia(node: Node) { - Debug.assertDefined(node); - suppress(node, EmitFlags.NoLeadingComments, getFirstChild); - suppress(node, EmitFlags.NoTrailingComments, getLastChild); - function suppress(node: Node, flag: EmitFlags, getChild: (n: Node) => Node) { - addEmitFlags(node, flag); - const child = getChild(node); - if (child) suppress(child, flag, getChild); - } + suppressLeadingTrivia(node); + suppressTrailingTrivia(node); + } + + /** + * Sets EmitFlags to suppress leading trivia on the node. + */ + /* @internal */ + export function suppressLeadingTrivia(node: Node) { + addEmitFlagsRecursively(node, EmitFlags.NoLeadingComments, getFirstChild); + } + + /** + * Sets EmitFlags to suppress trailing trivia on the node. + */ + /* @internal */ + export function suppressTrailingTrivia(node: Node) { + addEmitFlagsRecursively(node, EmitFlags.NoTrailingComments, getLastChild); + } + + function addEmitFlagsRecursively(node: Node, flag: EmitFlags, getChild: (n: Node) => Node) { + addEmitFlags(node, flag); + const child = getChild(node); + if (child) addEmitFlagsRecursively(child, flag, getChild); } function getFirstChild(node: Node): Node | undefined { diff --git a/tests/baselines/reference/organizeImports/CoalesceTrivia.ts b/tests/baselines/reference/organizeImports/CoalesceTrivia.ts index c5bd76cadec78..7495f51dd863a 100644 --- a/tests/baselines/reference/organizeImports/CoalesceTrivia.ts +++ b/tests/baselines/reference/organizeImports/CoalesceTrivia.ts @@ -8,7 +8,7 @@ F2(); // ==ORGANIZED== -/*A*/ import /*B*/ { /*L*/ F1 /*M*/, /*C*/ F2 /*D*/ } /*E*/ from /*F*/ "lib" /*G*/; /*H*/ //I +/*A*/import /*B*/ { /*L*/ F1 /*M*/, /*C*/ F2 /*D*/ } /*E*/ from /*F*/ "lib" /*G*/; /*H*/ //I F1(); F2(); diff --git a/tests/baselines/reference/organizeImports/SortHeaderComment.ts b/tests/baselines/reference/organizeImports/SortHeaderComment.ts new file mode 100644 index 0000000000000..7c33fdaa4ba4f --- /dev/null +++ b/tests/baselines/reference/organizeImports/SortHeaderComment.ts @@ -0,0 +1,11 @@ +// ==ORIGINAL== + +// Header +import "lib2"; +import "lib1"; + +// ==ORGANIZED== + +// Header +import "lib1"; +import "lib2"; diff --git a/tests/baselines/reference/organizeImports/SortTrivia.ts b/tests/baselines/reference/organizeImports/SortTrivia.ts index cd3a0f0bc4b1c..e0badec125ec6 100644 --- a/tests/baselines/reference/organizeImports/SortTrivia.ts +++ b/tests/baselines/reference/organizeImports/SortTrivia.ts @@ -5,5 +5,5 @@ // ==ORGANIZED== -/*F*/ import /*G*/ "lib1" /*H*/; /*I*/ //J -/*A*/ import /*B*/ "lib2" /*C*/; /*D*/ //E +/*A*//*F*/ import /*G*/ "lib1" /*H*/; /*I*/ //J +import /*B*/ "lib2" /*C*/; /*D*/ //E diff --git a/tests/baselines/reference/organizeImports/UnusedHeaderComment.ts b/tests/baselines/reference/organizeImports/UnusedHeaderComment.ts new file mode 100644 index 0000000000000..4569c0af2f049 --- /dev/null +++ b/tests/baselines/reference/organizeImports/UnusedHeaderComment.ts @@ -0,0 +1,8 @@ +// ==ORIGINAL== + +// Header +import { F1 } from "lib"; + +// ==ORGANIZED== + +// Header diff --git a/tests/baselines/reference/organizeImports/UnusedTrivia1.ts b/tests/baselines/reference/organizeImports/UnusedTrivia1.ts index 7c25f40e6c262..5ec3cfda3af95 100644 --- a/tests/baselines/reference/organizeImports/UnusedTrivia1.ts +++ b/tests/baselines/reference/organizeImports/UnusedTrivia1.ts @@ -4,3 +4,4 @@ // ==ORGANIZED== +/*A*/ \ No newline at end of file diff --git a/tests/baselines/reference/organizeImports/UnusedTrivia2.ts b/tests/baselines/reference/organizeImports/UnusedTrivia2.ts index b36fd9733be1b..7da2adeeb2e37 100644 --- a/tests/baselines/reference/organizeImports/UnusedTrivia2.ts +++ b/tests/baselines/reference/organizeImports/UnusedTrivia2.ts @@ -6,6 +6,6 @@ F1(); // ==ORGANIZED== -/*A*/ import /*B*/ { /*C*/ F1 /*D*/ } /*G*/ from /*H*/ "lib" /*I*/; /*J*/ //K +/*A*/import /*B*/ { /*C*/ F1 /*D*/ } /*G*/ from /*H*/ "lib" /*I*/; /*J*/ //K F1();