Fix organize imports inserting extra blank lines

[armanio123]

Fixes #41417

[andrewbranch]

I should have thought of this yesterday, but can you see if this commit is enough to fix the problem? There are a couple fixes in #37814 that are probably worth adopting.

---

Edit 1: I think that will probably fix the problem for specifiers where the sorting results in a different ordering of previousNode and nextNode, but not for ones that move around without changing the original order. E.g., when

import {
  a,
  c,
  b
} from "."

becomes

import {
  a,
  b,
  c
} from "."

we would still mess up the space between a and b because their relative order stayed the same, but we would use default spacing between b and c, since their order swapped. So I actually predict that commit will not be sufficient. But I think the general idea is that we only want to look at source newlines between two siblings if their original nodes were right next to each other. I feel like I tried to do something like that and it didn’t work out, but I don’t remember why 😕

---

Edit 2: I think I remembered why: list separators (like commas) are not represented in either previousNode or nextNode, so a direct position comparison wasn’t possible. You’d have to actually look at tokens from the source text to see if the siblings nodes are separated by anything other than punctuation and trivia. That might be feasible since this is only running during refactors/codefixes.

[andrewbranch]

I believe this would be broken by spaces between the node end and the list separator, or by duplicate list separators:

import {
  , Foo
  , Bar
} from ""

Foo’s end is 16, but Bar’s pos is 20. 17–19 don’t appear in the span of either. This is why I speculated that you might have to look at tokens between nodes to see if they’re just list separators. Or maybe you can do something similar to what’s in getNodeAtPosition with forEachChild to handle this... that is, it should be easy to tell that the position you were given falls between two nodes, and you could choose to return the next one instead of the common parent. If you implement this correctly, I believe you should be able to drop the + 1, which indicates the assumption that the list separator occurs immediately after previousNode and is exactly one character long.

[andrewbranch]

Also, whatever function this ends up using, it’s likely to be more expensive than the next couple conditions in this function, so this calculation should be moved to be the last.

[armanio123]

Makes sense. I have changed the algorithm for instead navigating the parent until the next specifier is found. I have limited to only look in the case the previousNode is an import specifier. Only for not affecting other scenarios.

Let me know what you think.

[andrewbranch]

I’m pretty confident in this logic being universal, so it would be nice to make it work for other scenarios and to keep such specific logic out of the emitter. To make sure I wasn’t giving you bad advice, I prototyped what I had in mind really quick and it seems to work, though the function isn’t beautiful. Here’s my attempt: andrewbranch@c40b579

[andrewbranch]

One alternative more along the lines of what you have now would be to write a utility function that returns the index of a node in its parent’s list-like container. It would have to be a pretty big switch/case to determine what kind of node the parent is to access the right property. It would be more lines of code, but maybe a bit cleaner and more efficient than what I have here. A downside would be that it would need to be kept up to date when new node kinds are added in the future, but that doesn’t happen too often.

[sandersn]

#42630 includes the commits from this PR, so I'm going to close this one.