reorder-imports removes and adds newlines where it shouldn't

[crumblingstatue]

use d; use c; use b; use a; gets formatted to

// <<An empty line is inserted here>>
use a;
use b;
use c;use d; // <- newline is removed between `use c;` and `use d;`

[nrc]

cc @studoot

[studoot]

Mhm - I can repro this. The problem is (superficially) within missing_spans::format_missing_inner (which doesn't insert a newline before use d; because it presumes that it lies at the start of the file, which it did in the originating file) and missing_spans::format_missing_with_indent (which does insert a newline beforeuse a; because it's presumed the declaration is not at the start of the file, which was true in the originating file).

The underlying problem is that the functions in missing_spans expect that the spans passed to them (which are derived from the original source file) will correspond to the position of the text in the final file. This is true for most of the transformations in rustfmt, but not necessarily so when you're reordering things, as reorder-imports does.

So, some changes to the functions in missing_spans are needed, to remove that assumption and to bake in the idea that the destination location might be different than the source location, and to base formatting decisions on where the text is actually being inserted in the final file.

I've got some ideas for how to do this (and it'll enable some work I've been doing around optimising (as opposed to just reordering) imports), so give me a bit of time & I'll raise a PR for this bug. And plenty of test cases...

[studoot]

Right - I've created PR #1133 to fix this issue. It fixes this issue and passes all tests (as well as a new one added for this case). Sorry it's taken so long - I had some holiday (where I thought I'd rust, but I rested instead) and some family things get in the way.

I think the output side of rustfmtneeds a bit of an overhaul, though - I was thinking that an approach might be to replace the raw StringBuffer with something that had a few smarts and knew how to strip trailing space on a line, allowed you to ask for Rust items to be formatted as 'definitely starting on a new line' or 'definitely the last thing on a line'... Does that sound like something worth me taking a look at, @nrc?

[nrc]

I don't think StringBuffer is the place to add the smarts, but we could definitely add a layer of abstraction over that to do this sort of thing if you think it would be beneficial (it feels like it could be, but I'm not totally sure without experimenting).

[studoot]

No, I agree with you about StringBuffer - I was thinking more of something that had the push_str method (to minimise changes to existing source) and then other methods that contain the smartness, which I can use (initially) for formatting imports (to prove its worth).

[ishitatsuyuki]

There's still some issues with the weird sorting:

use a::a;
use b::a;
use a::b;

gets to

use a::a;
use b::a;
use a::b;

Note the weird blank line there.

Additionally, the mechanism should be way simpler, by treating use statements as a block and split the block at the newline. This allows internal lib use statements to be placed separately.

[conradkleinespel]

@ishitatsuyuki Can't seem to reproduce this now. I had a quick look at the commit log but wasn't able to figure out at what point this may have been fixed.

[crumblingstatue]

The original issue that I have reported seems to be fixed, so I'm closing this.