Fix organize imports overlap (microsoft#43228)

* Fix organize imports overlap

* Refactored multiline end position

* Added tests for single line trailing trivia

* Fix rearranging imports

* Fix lint error

* PR coments

Author: armanio123

src/services/organizeImports.ts

@@ -63,22 +63,26 @@ namespace ts.OrganizeImports {
                     ? coalesce(importGroup)
                     : importGroup);
 
-            // Delete or replace the first import.
+            // Delete all nodes if there are no imports.
             if (newImportDecls.length === 0) {
-                changeTracker.delete(sourceFile, oldImportDecls[0]);
+                // Consider the first node to have trailingTrivia as we want to exclude the
+                // "header" comment.
+                changeTracker.deleteNodes(sourceFile, oldImportDecls, {
+                    trailingTriviaOption: textChanges.TrailingTriviaOption.Include,
+                }, /*hasTrailingComment*/ true);
             }
             else {
                 // Note: Delete the surrounding trivia because it will have been retained in newImportDecls.
-                changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, {
+                const replaceOptions = {
                     leadingTriviaOption: textChanges.LeadingTriviaOption.Exclude, // Leave header comment in place
                     trailingTriviaOption: textChanges.TrailingTriviaOption.Include,
                     suffix: getNewLineOrDefaultFromHost(host, formatContext.options),
-                });
-            }
-
-            // Delete any subsequent imports.
-            for (let i = 1; i < oldImportDecls.length; i++) {
-                changeTracker.deleteNode(sourceFile, oldImportDecls[i]);
+                };
+                changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, replaceOptions);
+                const hasTrailingComment = changeTracker.nodeHasTrailingComment(sourceFile, oldImportDecls[0], replaceOptions);
+                changeTracker.deleteNodes(sourceFile, oldImportDecls.slice(1), {
+                    trailingTriviaOption: textChanges.TrailingTriviaOption.Include,
+                }, hasTrailingComment);
             }
         }
     }

src/services/textChanges.ts

@@ -168,7 +168,7 @@ namespace ts.textChanges {
         return { pos: getAdjustedStartPosition(sourceFile, startNode, options), end: getAdjustedEndPosition(sourceFile, endNode, options) };
     }
 
-    function getAdjustedStartPosition(sourceFile: SourceFile, node: Node, options: ConfigurableStart) {
+    function getAdjustedStartPosition(sourceFile: SourceFile, node: Node, options: ConfigurableStartEnd, hasTrailingComment = false) {
         const { leadingTriviaOption } = options;
         if (leadingTriviaOption === LeadingTriviaOption.Exclude) {
             return node.getStart(sourceFile);
@@ -199,6 +199,17 @@ namespace ts.textChanges {
             // when b is deleted - we delete it
             return leadingTriviaOption === LeadingTriviaOption.IncludeAll ? fullStart : start;
         }
+
+        // if node has a trailing comments, use comment end position as the text has already been included.
+        if (hasTrailingComment) {
+            // Check first for leading comments as if the node is the first import, we want to exclude the trivia;
+            // otherwise we get the trailing comments.
+            const comment = getLeadingCommentRanges(sourceFile.text, fullStart)?.[0] || getTrailingCommentRanges(sourceFile.text, fullStart)?.[0];
+            if (comment) {
+                return skipTrivia(sourceFile.text, comment.end, /*stopAfterLineBreak*/ true, /*stopAtComments*/ true);
+            }
+        }
+
         // get start position of the line following the line that contains fullstart position
         // (but only if the fullstart isn't the very beginning of the file)
         const nextLineStart = fullStart > 0 ? 1 : 0;
@@ -208,7 +219,38 @@ namespace ts.textChanges {
         return getStartPositionOfLine(getLineOfLocalPosition(sourceFile, adjustedStartPosition), sourceFile);
     }
 
-    function getAdjustedEndPosition(sourceFile: SourceFile, node: Node, options: ConfigurableEnd) {
+    /** Return the end position of a multiline comment of it is on another line; otherwise returns `undefined`; */
+    function getEndPositionOfMultilineTrailingComment(sourceFile: SourceFile, node: Node, options: ConfigurableEnd): number | undefined {
+        const { end } = node;
+        const { trailingTriviaOption } = options;
+        if (trailingTriviaOption === TrailingTriviaOption.Include) {
+            // If the trailing comment is a multiline comment that extends to the next lines,
+            // return the end of the comment and track it for the next nodes to adjust.
+            const comments = getTrailingCommentRanges(sourceFile.text, end);
+            if (comments) {
+                const nodeEndLine = getLineOfLocalPosition(sourceFile, node.end);
+                for (const comment of comments) {
+                    // Single line can break the loop as trivia will only be this line.
+                    // Comments on subsequest lines are also ignored.
+                    if (comment.kind === SyntaxKind.SingleLineCommentTrivia || getLineOfLocalPosition(sourceFile, comment.pos) > nodeEndLine) {
+                        break;
+                    }
+
+                    // Get the end line of the comment and compare against the end line of the node.
+                    // If the comment end line position and the multiline comment extends to multiple lines,
+                    // then is safe to return the end position.
+                    const commentEndLine = getLineOfLocalPosition(sourceFile, comment.end);
+                    if (commentEndLine > nodeEndLine) {
+                        return skipTrivia(sourceFile.text, comment.end, /*stopAfterLineBreak*/ true, /*stopAtComments*/ true);
+                    }
+                }
+            }
+        }
+
+        return undefined;
+    }
+
+    function getAdjustedEndPosition(sourceFile: SourceFile, node: Node, options: ConfigurableEnd): number {
         const { end } = node;
         const { trailingTriviaOption } = options;
         if (trailingTriviaOption === TrailingTriviaOption.Exclude) {
@@ -222,6 +264,12 @@ namespace ts.textChanges {
             }
             return end;
         }
+
+        const multilineEndPosition = getEndPositionOfMultilineTrailingComment(sourceFile, node, options);
+        if (multilineEndPosition) {
+            return multilineEndPosition;
+        }
+
         const newEnd = skipTrivia(sourceFile.text, end, /*stopAfterLineBreak*/ true);
 
         return newEnd !== end && (trailingTriviaOption === TrailingTriviaOption.Include || isLineBreak(sourceFile.text.charCodeAt(newEnd - 1)))
@@ -293,6 +341,18 @@ namespace ts.textChanges {
             this.deleteRange(sourceFile, getAdjustedRange(sourceFile, node, node, options));
         }
 
+        public deleteNodes(sourceFile: SourceFile, nodes: readonly Node[], options: ConfigurableStartEnd = { leadingTriviaOption: LeadingTriviaOption.IncludeAll }, hasTrailingComment: boolean): void {
+            // When deleting multiple nodes we need to track if the end position is including multiline trailing comments.
+            for (const node of nodes) {
+                const pos = getAdjustedStartPosition(sourceFile, node, options, hasTrailingComment);
+                const end = getAdjustedEndPosition(sourceFile, node, options);
+
+                this.deleteRange(sourceFile, { pos, end });
+
+                hasTrailingComment = !!getEndPositionOfMultilineTrailingComment(sourceFile, node, options);
+            }
+        }
+
         public deleteModifier(sourceFile: SourceFile, modifier: Modifier): void {
             this.deleteRange(sourceFile, { pos: modifier.getStart(sourceFile), end: skipTrivia(sourceFile.text, modifier.end, /*stopAfterLineBreak*/ true) });
         }
@@ -337,6 +397,10 @@ namespace ts.textChanges {
             this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNodes, options);
         }
 
+        public nodeHasTrailingComment(sourceFile: SourceFile, oldNode: Node, configurableEnd: ConfigurableEnd = useNonAdjustedPositions): boolean {
+            return !!getEndPositionOfMultilineTrailingComment(sourceFile, oldNode, configurableEnd);
+        }
+
         private nextCommaToken(sourceFile: SourceFile, node: Node): Node | undefined {
             const next = findNextToken(node, node.parent, sourceFile);
             return next && next.kind === SyntaxKind.CommaToken ? next : undefined;
@@ -1289,8 +1353,9 @@ namespace ts.textChanges {
                 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,
-                        { leadingTriviaOption: isFirstImport ? LeadingTriviaOption.Exclude : hasJSDocNodes(node) ? LeadingTriviaOption.JSDoc : LeadingTriviaOption.StartLine });
+                    deleteNode(changes, sourceFile, node, {
+                        leadingTriviaOption: isFirstImport ? LeadingTriviaOption.Exclude : hasJSDocNodes(node) ? LeadingTriviaOption.JSDoc : LeadingTriviaOption.StartLine,
+                    });
                     break;
 
                 case SyntaxKind.BindingElement:

tests/cases/fourslash/organizeImports4.ts

@@ -0,0 +1,16 @@
+/// <reference path="fourslash.ts" />
+
+// Regression test for GH#43107
+
+//// import * as something from "path";/** 
+////  * some comment here
+////  * and there
+////  */
+//// import * as somethingElse from "anotherpath";
+//// import * as AnotherThing from "somepath";/** 
+////  * some comment here
+////  * and there
+////  */
+//// import * as AnotherThingElse from "someotherpath";
+
+verify.organizeImports(''); 

tests/cases/fourslash/organizeImports5.ts

@@ -0,0 +1,16 @@
+/// <reference path="fourslash.ts" />
+
+// Regression test for GH#43107
+
+//// import * as something from "path";/** 
+////  * some comment here
+////  * and there
+////  */
+//// import * as somethingElse from "anotherpath";
+//// import * as AnotherThing from "somepath";/** 
+////  * some comment here
+////  * and there
+////  */
+//// import * as AnotherThingElse from "someotherpath";
+
+verify.organizeImports(''); 

tests/cases/fourslash/organizeImports6.ts

@@ -0,0 +1,21 @@
+/// <reference path="fourslash.ts" />
+
+// Regression test for GH#43107
+
+//// import * as something from "path"; /* small comment */ // single line one.
+//// /* some comment here
+//// * and there
+//// */
+//// import * as somethingElse from "anotherpath";
+//// import * as anotherThing from "someopath"; /* small comment */ // single line one.
+//// /* some comment here
+//// * and there
+//// */
+//// import * as anotherThingElse from "someotherpath";
+//// 
+//// anotherThing;
+
+verify.organizeImports(
+`import * as anotherThing from "someopath"; /* small comment */ // single line one.
+
+anotherThing;`);

tests/cases/fourslash/organizeImports7.ts

@@ -0,0 +1,23 @@
+/// <reference path="fourslash.ts" />
+
+// Regression test for GH#43107
+
+//// import * as something from "path"; /**
+////  * some comment here
+////  * and there
+////  */
+//// import * as somethingElse from "anotherpath";
+//// 
+//// something;
+//// somethingElse;
+
+verify.organizeImports(
+`import * as somethingElse from "anotherpath";
+import * as something from "path"; /**
+ * some comment here
+ * and there
+ */
+
+something;
+somethingElse;`
+);