From ef377d5a66266a230ea5a06bc73fac43cfc06374 Mon Sep 17 00:00:00 2001 From: Jesse Trinity <42591254+jessetrinity@users.noreply.github.com> Date: Tue, 31 Mar 2020 10:18:06 -0700 Subject: [PATCH] Don't delete comments when deleting unused declarations (#37467) * don't delete comment on variable declaration * add more declaration kinds * don't copy comment in convertes6 class * don't copy comments in convertToES6Class * add tests * use isAnyImportSyntax * handle mixed comment types * update tests --- .../codefixes/convertFunctionToEs6Class.ts | 5 ++- src/services/textChanges.ts | 32 ++++++++++++++++--- ...s => convertFunctionToEs6Class-server1.ts} | 2 +- .../convertFunctionToEs6Class-server2.ts | 31 ++++++++++++++++++ ...s => unusedClassInNamespaceWithTrivia1.ts} | 1 + .../unusedClassInNamespaceWithTrivia2.ts | 16 ++++++++++ .../fourslash/unusedFunctionInNamespace1.ts | 1 + .../unusedFunctionInNamespaceWithTrivia.ts | 16 ++++++++++ .../fourslash/unusedVariableWithTrivia1.ts | 20 ++++++++++++ .../fourslash/unusedVariableWithTrivia2.ts | 17 ++++++++++ 10 files changed, 135 insertions(+), 6 deletions(-) rename tests/cases/fourslash/server/{convertFunctionToEs6Class-server.ts => convertFunctionToEs6Class-server1.ts} (92%) create mode 100644 tests/cases/fourslash/server/convertFunctionToEs6Class-server2.ts rename tests/cases/fourslash/{unusedClassInNamespaceWithTrivia.ts => unusedClassInNamespaceWithTrivia1.ts} (88%) create mode 100644 tests/cases/fourslash/unusedClassInNamespaceWithTrivia2.ts create mode 100644 tests/cases/fourslash/unusedFunctionInNamespaceWithTrivia.ts create mode 100644 tests/cases/fourslash/unusedVariableWithTrivia1.ts create mode 100644 tests/cases/fourslash/unusedVariableWithTrivia2.ts diff --git a/src/services/codefixes/convertFunctionToEs6Class.ts b/src/services/codefixes/convertFunctionToEs6Class.ts index 0769cb26162..d9917596ff1 100644 --- a/src/services/codefixes/convertFunctionToEs6Class.ts +++ b/src/services/codefixes/convertFunctionToEs6Class.ts @@ -48,7 +48,10 @@ namespace ts.codefix { return undefined; } - copyLeadingComments(ctorDeclaration, newClassDeclaration, sourceFile); + // Deleting a declaration only deletes JSDoc style comments, so only copy those to the new node. + if (hasJSDocNodes(ctorDeclaration)) { + copyLeadingComments(ctorDeclaration, newClassDeclaration, sourceFile); + } // Because the preceding node could be touched, we need to insert nodes before delete nodes. changes.insertNodeAfter(sourceFile, precedingNode!, newClassDeclaration); diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index eccca7121ae..bf1dae65602 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -42,6 +42,15 @@ namespace ts.textChanges { * include all trivia between the node and the previous token */ IncludeAll, + /** + * Include attached JSDoc comments + */ + JSDoc, + /** + * Only delete trivia on the same line as getStart(). + * Used to avoid deleting leading comments + */ + StartLine, } export enum TrailingTriviaOption { @@ -162,6 +171,15 @@ namespace ts.textChanges { if (leadingTriviaOption === LeadingTriviaOption.Exclude) { return node.getStart(sourceFile); } + if (leadingTriviaOption === LeadingTriviaOption.StartLine) { + return getLineStartPositionForPosition(node.getStart(sourceFile), sourceFile); + } + if (leadingTriviaOption === LeadingTriviaOption.JSDoc) { + const JSDocComments = getJSDocCommentRanges(node, sourceFile.text); + if (JSDocComments?.length) { + return getLineStartPositionForPosition(JSDocComments[0].pos, sourceFile); + } + } const fullStart = node.getFullStart(); const start = node.getStart(sourceFile); if (fullStart === start) { @@ -1249,10 +1267,11 @@ namespace ts.textChanges { } case SyntaxKind.ImportDeclaration: - const isFirstImport = sourceFile.imports.length && node === first(sourceFile.imports).parent || node === find(sourceFile.statements, isImportDeclaration); + case SyntaxKind.ImportEqualsDeclaration: + const isFirstImport = sourceFile.imports.length && node === first(sourceFile.imports).parent || node === find(sourceFile.statements, isAnyImportSyntax); + // For first import, leave header comment in place, otherwise only delete JSDoc comments deleteNode(changes, sourceFile, node, - // For first import, leave header comment in place - isFirstImport ? { leadingTriviaOption: LeadingTriviaOption.Exclude } : undefined); + { leadingTriviaOption: isFirstImport ? LeadingTriviaOption.Exclude : hasJSDocNodes(node) ? LeadingTriviaOption.JSDoc : LeadingTriviaOption.StartLine }); break; case SyntaxKind.BindingElement: @@ -1296,6 +1315,11 @@ namespace ts.textChanges { deleteNode(changes, sourceFile, node, { leadingTriviaOption: LeadingTriviaOption.Exclude }); break; + case SyntaxKind.ClassDeclaration: + case SyntaxKind.FunctionDeclaration: + deleteNode(changes, sourceFile, node, { leadingTriviaOption: hasJSDocNodes(node) ? LeadingTriviaOption.JSDoc : LeadingTriviaOption.StartLine }); + break; + default: if (isImportClause(node.parent) && node.parent.name === node) { deleteDefaultImport(changes, sourceFile, node.parent); @@ -1372,7 +1396,7 @@ namespace ts.textChanges { break; case SyntaxKind.VariableStatement: - deleteNode(changes, sourceFile, gp); + deleteNode(changes, sourceFile, gp, { leadingTriviaOption: hasJSDocNodes(gp) ? LeadingTriviaOption.JSDoc : LeadingTriviaOption.StartLine }); break; default: diff --git a/tests/cases/fourslash/server/convertFunctionToEs6Class-server.ts b/tests/cases/fourslash/server/convertFunctionToEs6Class-server1.ts similarity index 92% rename from tests/cases/fourslash/server/convertFunctionToEs6Class-server.ts rename to tests/cases/fourslash/server/convertFunctionToEs6Class-server1.ts index 24a7acbf8ed..316572063a5 100644 --- a/tests/cases/fourslash/server/convertFunctionToEs6Class-server.ts +++ b/tests/cases/fourslash/server/convertFunctionToEs6Class-server1.ts @@ -14,7 +14,7 @@ verify.codeFix({ description: "Convert function to an ES2015 class", newFileContent: -`// Comment\r +`// Comment class fn {\r constructor() {\r this.baz = 10;\r diff --git a/tests/cases/fourslash/server/convertFunctionToEs6Class-server2.ts b/tests/cases/fourslash/server/convertFunctionToEs6Class-server2.ts new file mode 100644 index 00000000000..2f9c1a88bdf --- /dev/null +++ b/tests/cases/fourslash/server/convertFunctionToEs6Class-server2.ts @@ -0,0 +1,31 @@ +// @allowNonTsExtensions: true +// @Filename: test123.js + +/// + +//// /** +//// * JSDoc Comment +//// */ +//// function fn() { +//// this.baz = 10; +//// } +//// /*1*/fn.prototype.bar = function () { +//// console.log('hello world'); +//// } + +verify.codeFix({ + description: "Convert function to an ES2015 class", + newFileContent: +`/**\r + * JSDoc Comment\r + */\r +class fn {\r + constructor() {\r + this.baz = 10;\r + }\r + bar() {\r + console.log('hello world');\r + }\r +}\r +`, +}); diff --git a/tests/cases/fourslash/unusedClassInNamespaceWithTrivia.ts b/tests/cases/fourslash/unusedClassInNamespaceWithTrivia1.ts similarity index 88% rename from tests/cases/fourslash/unusedClassInNamespaceWithTrivia.ts rename to tests/cases/fourslash/unusedClassInNamespaceWithTrivia1.ts index cae0c52ac79..3298a3e0ab1 100644 --- a/tests/cases/fourslash/unusedClassInNamespaceWithTrivia.ts +++ b/tests/cases/fourslash/unusedClassInNamespaceWithTrivia1.ts @@ -8,4 +8,5 @@ //// } |] verify.rangeAfterCodeFix(`namespace greeter { + /* comment1 */ }`); diff --git a/tests/cases/fourslash/unusedClassInNamespaceWithTrivia2.ts b/tests/cases/fourslash/unusedClassInNamespaceWithTrivia2.ts new file mode 100644 index 00000000000..8578239756c --- /dev/null +++ b/tests/cases/fourslash/unusedClassInNamespaceWithTrivia2.ts @@ -0,0 +1,16 @@ +/// + +// @noUnusedLocals: true +//// [| namespace greeter { +//// // Do not remove +//// /** +//// * JSDoc Comment +//// */ +//// class /* comment2 */ class1 { +//// } +//// } |] + +verify.rangeAfterCodeFix(`namespace greeter { + // Do not remove +}`); + \ No newline at end of file diff --git a/tests/cases/fourslash/unusedFunctionInNamespace1.ts b/tests/cases/fourslash/unusedFunctionInNamespace1.ts index 7a63ff13caa..9c3a21c04dc 100644 --- a/tests/cases/fourslash/unusedFunctionInNamespace1.ts +++ b/tests/cases/fourslash/unusedFunctionInNamespace1.ts @@ -8,4 +8,5 @@ //// } |] verify.rangeAfterCodeFix(`namespace greeter { + // some legit comments }`); diff --git a/tests/cases/fourslash/unusedFunctionInNamespaceWithTrivia.ts b/tests/cases/fourslash/unusedFunctionInNamespaceWithTrivia.ts new file mode 100644 index 00000000000..bfcaa5852b0 --- /dev/null +++ b/tests/cases/fourslash/unusedFunctionInNamespaceWithTrivia.ts @@ -0,0 +1,16 @@ +/// + +// @noUnusedLocals: true +//// [| namespace greeter { +//// // Do not remove +//// /** +//// * JSDoc Comment +//// */ +//// function function1() { +//// }/*1*/ +//// } |] + +verify.rangeAfterCodeFix(`namespace greeter { + // Do not remove + }`); + \ No newline at end of file diff --git a/tests/cases/fourslash/unusedVariableWithTrivia1.ts b/tests/cases/fourslash/unusedVariableWithTrivia1.ts new file mode 100644 index 00000000000..fb6d48e1265 --- /dev/null +++ b/tests/cases/fourslash/unusedVariableWithTrivia1.ts @@ -0,0 +1,20 @@ +/// + +// @noUnusedLocals: true +////[|namespace greeter { +//// // do not remove comment +//// let a = 0; +//// // comment +//// let b = 0; +//// b; +////}|] + +verify.codeFix({ + description: "Remove unused declaration for: 'a'", + newRangeContent: `namespace greeter { + // do not remove comment + // comment + let b = 0; + b; +}` +}); diff --git a/tests/cases/fourslash/unusedVariableWithTrivia2.ts b/tests/cases/fourslash/unusedVariableWithTrivia2.ts new file mode 100644 index 00000000000..c2ea84f3acb --- /dev/null +++ b/tests/cases/fourslash/unusedVariableWithTrivia2.ts @@ -0,0 +1,17 @@ +/// + +// @noUnusedLocals: true +////[|namespace greeter { +//// // Do not remove +//// /** +//// * JSDoc Comment +//// */ +//// let a = 0; +////}|] + +verify.codeFix({ + description: "Remove unused declaration for: 'a'", + newRangeContent: `namespace greeter { + // Do not remove +}` +});