From 5a981550b50e185788bad74ac7e57eea8dff49e0 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 23 Mar 2018 10:09:55 -0700 Subject: [PATCH] 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. Caveat: We had to hack around emitter limitation #22831. Fixes #22723 Fixes #22724 Fixes #22731 --- src/compiler/emitter.ts | 17 +++++++++- src/harness/unittests/organizeImports.ts | 24 ++++++++++++++ src/services/organizeImports.ts | 14 ++++++-- src/services/utilities.ts | 32 +++++++++++++++---- .../organizeImports/CoalesceTrivia.ts | 2 +- .../organizeImports/SortHeaderComment.ts | 13 ++++++++ .../reference/organizeImports/SortTrivia.ts | 4 +-- .../organizeImports/UnusedHeaderComment.ts | 10 ++++++ .../organizeImports/UnusedTrivia1.ts | 1 + .../organizeImports/UnusedTrivia2.ts | 2 +- 10 files changed, 105 insertions(+), 14 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 a695b55f5ca8a..c05d3c12a13c3 100644 --- a/src/compiler/emitter.ts +++ b/src/compiler/emitter.ts @@ -2130,7 +2130,22 @@ namespace ts { function emitImportDeclaration(node: ImportDeclaration) { emitModifiers(node, node.modifiers); - emitTokenWithComment(SyntaxKind.ImportKeyword, node.modifiers ? node.modifiers.end : node.pos, writeKeyword, node); + const pos = node.modifiers ? node.modifiers.end : node.pos; + if (node.emitNode && node.emitNode.flags && (node.emitNode.flags & EmitFlags.NoLeadingComments)) { + // HACK: If we called emitTokenWithComment, it would emit the leading trivia, regardless + // of the EmitFlags on node. However, in order to be able to organize imports without + // disrupting header comments, we need to be able to control this. For now, inline the + // parts of emitTokenWithComment that do what we want. + // TODO (https://github.com/Microsoft/TypeScript/issues/22831): Remove this hack. + const keywordPos = skipTrivia(currentSourceFile.text, pos); + const trailingCommentPos = writeTokenText(SyntaxKind.ImportKeyword, writeKeyword, keywordPos); + if (emitTrailingCommentsOfPosition) { + emitTrailingCommentsOfPosition(trailingCommentPos, /*prefixSpace*/ true); + } + } + else { + emitTokenWithComment(SyntaxKind.ImportKeyword, pos, writeKeyword, node); + } writeSpace(); if (node.importClause) { emit(node.importClause); diff --git a/src/harness/unittests/organizeImports.ts b/src/harness/unittests/organizeImports.ts index 044fee1367cfd..86baccc4803af 100644 --- a/src/harness/unittests/organizeImports.ts +++ b/src/harness/unittests/organizeImports.ts @@ -339,6 +339,30 @@ 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 b4999e968fbda..52f9a50366f84 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 2772b8920e79a..ad7951b50b915 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1486,14 +1486,32 @@ namespace ts { */ /* @internal */ export function suppressLeadingAndTrailingTrivia(node: Node) { + suppressLeadingTrivia(node); + suppressTrailingTrivia(node); + } + + /** + * Sets EmitFlags to suppress leading trivia on the node. + */ + /* @internal */ + export function suppressLeadingTrivia(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); - } + addEmitFlagsRecursively(node, EmitFlags.NoLeadingComments, getFirstChild); + } + + /** + * Sets EmitFlags to suppress trailing trivia on the node. + */ + /* @internal */ + export function suppressTrailingTrivia(node: Node) { + Debug.assertDefined(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..eb0329e077514 --- /dev/null +++ b/tests/baselines/reference/organizeImports/SortHeaderComment.ts @@ -0,0 +1,13 @@ +// ==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..a1b8ede0ffdc6 --- /dev/null +++ b/tests/baselines/reference/organizeImports/UnusedHeaderComment.ts @@ -0,0 +1,10 @@ +// ==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();