Comments on use statements incorrectly rearranged when use statements are rearranged

[bddap]

Input:

use c; // use c;
use b; // use b;
use a; // use a;

Expected:

use a; // use a;
use b; // use b;
use c; // use c;

Got:

use a;
use b; // use b;
use c; // use c; // use a;

rustc 1.36.0 (a53f9df32 2019-07-03)
binary: rustc
commit-hash: a53f9df32fbb0b5f4382caaad8f1a46f36ea887c
commit-date: 2019-07-03
host: x86_64-apple-darwin
release: 1.36.0
LLVM version: 8.0

[mbarbar]

I will add that this also has some issues with leading comments. The problem can be reproduced with two uses, but I think the issue becomes clearer with 3 (like your example):

/*cpre*/ use c; // cpost
/*bpre*/ use b; // bpost
/*apre*/ use a; // apost

produces

/*cpre*/
/*apre*/ use a;
/*bpre*/ use b; // bpost
use c; // cpost // apost

The problem seems to be in src/reorder.rs's walk_reorderable_items (or in rewrite_reorderable_items, depending on how you look at it). The span variable created does not include the first leading comment or the final trailing comment. It's kind of like a fence-post error. When we're reordering, cpre doesn't "belong" to use c and apost doesn't "belong" to use a because searching for comments is limited by the span.

Simply extending the span to the end of the previous item and start of the next item I believe would fail because then the first and last item would "steal" comments belonging to other items. I'll take a better look when I have more time. I think it can be solved by very explicitly including the first pre- and last post-comments to span by searching for the comments before we even get to rewrite_reorderable_items.

[dhardy]

I think this is the same bug:

// original
use crate::text::TextApi;
// for doc use
#[allow(unused)]
use crate::text::TextApiExt;
use crate::macros::autoimpl;

// ——— rustfmt output ———
use crate::text::TextApi;
// for doc use
use crate::macros::autoimpl;
#[allow(unused)]
use crate::text::TextApiExt;

[ytmimi]

related to #3127

[RalfJung]

#5485 is an example of this that I almost missed, where this meant a pretty fundamental change in the semantics of the comment.

Is there a tag for "rustfmt does something to the source code that changes its meaning for human readers" (aka 'pretty critical')?

[calebcartwright]

Is there a tag for "rustfmt does something to the source code that changes its meaning for human readers" (aka 'pretty critical')?

To be honest I can't imagine anything comment related that would be considered critical, and even if it were, cases where comments are outright dropped would be viewed as more severe.

I understand this is a source of frustration for those that encounter it, and I wish the option hadn't been stabilized given the gap with comments. However, there's no undoing the past, and this isn't something likely to be addressed in the near term.

If you find yourself regularly running into this (presumably due to regular additions/removals of imports triggering order changes) then you may want to consider setting reorder_imports to false in the interim.

[RalfJung]

I can't imagine anything comment related that would be considered critical

That is rather surprising to me. I view this as equally critical as if rustfmt had transformed compiling code into incorrect syntax -- actually probably more critical since these comment changes will not be auto-detected by the next build, they need careful human review. So IMO it's on the same level as, e.g., changing foo(a, b) into foo(b, a) (where both have the same type so this will keep compiling), except no test suite can possibly catch the problem.

[calebcartwright]

Folks are entitled to their own preferred priority and perceptions of criticality, but my prior comment stands relative to our position.

This isn't widespread, has a perfectly viable workaround that's available on stable, and as always version control can be leveraged to check/detect diffs.

[RalfJung]

I doubt the work-around of disabling use reordering would be accepted in the rustc repo where this hit me, but -- fair.

Either way thanks for all your hard work on rustfmt!

[calebcartwright]

#4504 would likely tackle most (though perhaps not all) of these items. If someone wanted pick up those proposed changes and apply them against the current master branch (that PR was based on the now defunct 2.0 branch) that'd be great and a good step in the right direction. It's just not something we have the capacity to get to ourselves for the foreseeable future