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

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);

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:

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

tests/cases/fourslash/server/convertFunctionToEs6Class-server2.ts

@@ -0,0 +1,31 @@
+// @allowNonTsExtensions: true
+// @Filename: test123.js
+
+/// <reference path="../fourslash.ts" />
+
+//// /**
+////  * 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
+`,
+});

tests/cases/fourslash/unusedClassInNamespaceWithTrivia1.ts

@@ -8,4 +8,5 @@
 //// } |]
 
 verify.rangeAfterCodeFix(`namespace greeter {
+   /* comment1 */
 }`);

tests/cases/fourslash/unusedClassInNamespaceWithTrivia2.ts

@@ -0,0 +1,16 @@
+/// <reference path='fourslash.ts' />
+
+// @noUnusedLocals: true
+//// [| namespace greeter {
+////   // Do not remove
+////   /**
+////    * JSDoc Comment
+////    */
+////   class /* comment2 */ class1 {
+////   }
+//// } |]
+
+verify.rangeAfterCodeFix(`namespace greeter {
+   // Do not remove
+}`);
+ 

tests/cases/fourslash/unusedFunctionInNamespace1.ts

@@ -8,4 +8,5 @@
 //// } |]
 
 verify.rangeAfterCodeFix(`namespace greeter {
+   // some legit comments
 }`);

tests/cases/fourslash/unusedFunctionInNamespaceWithTrivia.ts

@@ -0,0 +1,16 @@
+/// <reference path='fourslash.ts' />
+
+// @noUnusedLocals: true
+//// [| namespace greeter {
+////   // Do not remove
+////   /**
+////    * JSDoc Comment
+////    */
+////   function function1() {
+////   }/*1*/
+//// } |]
+
+verify.rangeAfterCodeFix(`namespace greeter {
+    // Do not remove
+ }`);
+ 

tests/cases/fourslash/unusedVariableWithTrivia1.ts

@@ -0,0 +1,20 @@
+/// <reference path='fourslash.ts' />
+
+// @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;
+}`
+});

tests/cases/fourslash/unusedVariableWithTrivia2.ts

@@ -0,0 +1,17 @@
+/// <reference path='fourslash.ts' />
+
+// @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
+}`
+});