Fix organize imports overlap #43228

armanio123 · 2021-03-13T09:00:12Z

Text changes will now include multiline comments if they are considered trailing comments from the previous node.

jessetrinity

I think you're on the right track - I'm pretty sure the overlap issue is just coming from newlines at this point.

jessetrinity · 2021-03-16T21:39:21Z

src/services/textChanges.ts

+ if (hasTrailingComment) {
+ const comment = getTrailingCommentRanges(sourceFile.text, fullStart)?.[0];
+ if (comment) {
+ return comment.end;


Suggested change

return comment.end;

return skipTrivia(sourceFile.text, comment.end, /*stopAfterLineBreak*/ true, /*stopAtComments*/ true);

Since you call skipTrivia when you do getAdjustedEndPosition, it looks like you would need to do the same for the comment range for getAdjustedStartPosition, otherwise the carriage return / new lines will overlap.

This seems to fix the issue except when the trailing comment is on the first import, so there might be another place where you have to do this.

I'm not sure I understand in which condition this is an issue. Do you have a sample code I can test?

jessetrinity · 2021-03-16T21:47:03Z

src/services/textChanges.ts

+ const commentEndLine = getLineOfLocalPosition(sourceFile, comment.end);
+ if (commentEndLine > nodeEndLine) {
+ const newEnd = skipTrivia(sourceFile.text, comment.end, /*stopAfterLineBreak*/ true, /*stopAtComments*/ true);
+ return { end: newEnd, hasTrailingComment: true };


Can hasTrailingComment be determined by the trivia option outside of this function, such as in the deleteNodes loop? It seems awkward to return this new object if we're almost always interested in only the end position.

If refactoring is too difficult I would at minimum change the name of the function since getAdjustedEndPosition implies we're still just getting the position.

Done. The new method name is a little confusing but I wasn't sure how to name it. Suggestions are welcome.

jessetrinity · 2021-03-16T21:53:33Z

src/services/textChanges.ts

+ 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) {


This warrants a test example.

jessetrinity · 2021-03-16T21:58:50Z

src/services/textChanges.ts

+ // When deleting multiple nodes we need to track if the end position is including multiline trailing comments.
+ let hasTrailingComment = false;
+
+ for (const node of nodes) {


This seems pretty specific to import declaration nodes. What happens when a node with a trailing multiline comment is between two import statements? for example:

import * as b from "b"; console.log();/** * what happens to me? */ import * as a from "a"

Unfortunately, the result would be console.log();/** as we have no way of recognizing what the previous node is about.

I think this is a design flaw and suggest addressing it on another ticket. So far this PR will not break that scenario.

jessetrinity · 2021-03-16T22:03:02Z

tests/cases/fourslash/organizeImports4.ts

+//// import * as AnotherThingElse from "someotherpath";
+
+
+verify.organizeImports(''); 


All of these imports are unused so they will just be deleted. We probably do want to test that but you will want to also add tests in which the imports are used, and with non-imports between the imports, also with trailing trivia.

Good catch I was so focused on the issue that I didn't even considered. I did and the crash still happens. Working on it.

jessetrinity

Just a couple more small comments from me.

jessetrinity · 2021-04-19T18:32:11Z

src/services/textChanges.ts

@@ -345,6 405,13 @@ namespace ts.textChanges {
 this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNodes, options);
 }

+ public replaceAndDeleteNodeWithNodes(sourceFile: SourceFile, oldNode: Node, newNodes: readonly Node[], deleteNodes: Node[], replaceOptions: ChangeNodeOptions = useNonAdjustedPositions, deleteOptions: ConfigurableStartEnd = { leadingTriviaOption: LeadingTriviaOption.IncludeAll }): void {


This name is a bit confusing - I probably wouldn't even extract this to a function. It's pretty simple and it looks like you only use it once anyway.

Agree, done.

jessetrinity · 2021-04-19T18:35:57Z

tests/cases/fourslash/organizeImports6.ts

+
+// Regression test for GH#43107
+
+//// import * as something from "path"; /* small comment */ // single line one.


I think you want this single line comment test to also not delete the import so you can see that the comment is preserved (by adding usages of something and somethingElse below).

It was actually meant to test that the single line is deleted. I found I used to have that problem. It also makes sense to test the contrary so I have updated to do it for both.

Fix organize imports overlap

478cd6b

typescript-bot assigned armanio123 Mar 13, 2021

typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Mar 13, 2021

armanio123 requested a review from jessetrinity March 13, 2021 09:01

jessetrinity suggested changes Mar 16, 2021

View reviewed changes

armanio123 added 4 commits March 22, 2021 20:25

Refactored multiline end position

758c4f1

Added tests for single line trailing trivia

26e6586

Fix rearranging imports

1206c8f

Fix lint error

344aedc

andrewbranch requested a review from jessetrinity April 13, 2021 18:01

jessetrinity approved these changes Apr 19, 2021

View reviewed changes

PR coments

b99c009

armanio123 merged commit 1a04b17 into microsoft:master Apr 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix organize imports overlap #43228

Fix organize imports overlap #43228

armanio123 commented Mar 13, 2021

jessetrinity left a comment

jessetrinity Mar 16, 2021

jessetrinity Mar 16, 2021

armanio123 Mar 23, 2021

jessetrinity Mar 16, 2021

armanio123 Mar 23, 2021

jessetrinity Mar 16, 2021

armanio123 Mar 23, 2021

jessetrinity Mar 16, 2021

armanio123 Mar 24, 2021

jessetrinity Mar 16, 2021

armanio123 Mar 23, 2021

armanio123 Mar 24, 2021

jessetrinity left a comment

jessetrinity Apr 19, 2021

armanio123 Apr 19, 2021

jessetrinity Apr 19, 2021

armanio123 Apr 19, 2021

	return comment.end;
	return skipTrivia(sourceFile.text, comment.end, /stopAfterLineBreak/ true, /stopAtComments/ true);

		//// import * as AnotherThingElse from "someotherpath";


		verify.organizeImports('');


		// Regression test for GH#43107

		//// import * as something from "path"; /* small comment */ // single line one.

Fix organize imports overlap #43228

Fix organize imports overlap #43228

Conversation

armanio123 commented Mar 13, 2021

jessetrinity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessetrinity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment