From 0736554db762e93a9dc5b2d87eb525c562e783a7 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 a695b55f5ca..f2f6980f488 100644 --- a/src/compiler/emitter.ts +++ b/src/compiler/emitter.ts @@ -1768,14 +1768,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(); @@ -1786,7 +1786,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 044fee1367c..3de5588af8e 100644 --- a/src/harness/unittests/organizeImports.ts +++ b/src/harness/unittests/organizeImports.ts @@ -339,6 +339,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 b4999e968fb..52f9a50366f 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -34,6 +34,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 => @@ -43,12 +50,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 2772b8920e7..bd32b1a9a4f 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1486,14 +1486,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 c5bd76cadec..7495f51dd86 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 00000000000..7c33fdaa4ba --- /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 cd3a0f0bc4b..e0badec125e 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 00000000000..4569c0af2f0 --- /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 7c25f40e6c2..5ec3cfda3af 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 b36fd9733be..7da2adeeb2e 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();