Imports reordering

[davidBar-On]

Fix for issue #3943. The root cause of the issue is that ::Foo from use ::Foo is represented by UseSegment::List as one item vector [::Foo]. However, in case of use ::{Foo}, after removing the {} the representation was two items vector [, Foo]. That caused the wrong sorting, as, for example cmp() considers [, Foo] as greater then [::foo]

The fix assumes that the one item vector [::Foo] is the right representation.

[calebcartwright]

after removing the {} the representation was two items vector [, Foo]

This sounds like the real underlying bug. Is that first element an Ident UseSegment with an empty string?

[davidBar-On]

after removing the {} the representation was two items vector [, Foo]

This sounds like the real underlying bug. Is that first element an Ident UseSegment with an empty string?

Yes, it is an Ident UseSegment with an empty string.

[calebcartwright]

Yes, it is an Ident UseSegment with an empty string.

That definitely feels like the bug then. I'd prefer to avoid having such an element in the tree's path in the first place rather than leaving the insertion of that item and then retroactively popping it later on.

[davidBar-On]

I'd prefer to avoid having such an element in the tree's path in the first place rather than leaving the insertion of that item and then retroactively popping it later on.

I have submitted a re-implemented the change - now in from_ast() function. This change is more complex than the first one, as more checks should be done to make sure this is the right case, including checks for whether there are comments or "as".

(The failure in the automatic tests seem to be because of network problem - maybe I should not submit changes during the USA night.)

[calebcartwright]

Thanks for the update. I took a quick look at this and now I understand the thinking behind your original proposed fix.

The part I am still struggling with is the representation of the global prefix of a tree as an Ident usesegment variant with an empty string. Something about that feels wrong/like a work around to me, especially since it's already caused a bug of it being handled inconsistently (mashed into a prefix in some cases but not others) what do you think?

I'd think we'd be able to represent that boolean global indicator more explicitly, such as a field on our UseTree which is incorporated into the comparisons for sorting, or maybe even as a new use segment variant, or as a boolean field on some of the existing variants

[davidBar-On]

The part I am still struggling with is the representation of the global prefix of a tree as an Ident usesegment variant with an empty string. Something about that feels wrong/like a work around to me, especially since it's already caused a bug of it being handled inconsistently (mashed into a prefix in some cases but not others) what do you think?

After evaluating the issue you raised, I think that I took the wrong direction, and that the change should be to add the empty UseSegment to imports such as ::foo and not removing them from imports such as ::{Fooo}. This will make the representation equivalent to representation of imports such as crate::foo. That is, there should always be a leading UseSegment, which may be empty.

Do you agree? If yes, I will re-implement the change according to this approach.

[calebcartwright]

That is, there should always be a leading UseSegment, which may be empty.
Do you agree? If yes, I will re-implement the change according to this approach

Could you elaborate on what you mean here? Not sure I follow what exactly you are proposing.

If there's anything unclear in my last comment please lmk, but basically I want to move to a more explicit representation of whether or not there's global prefix. I don't feel strongly about whether that's implemented via a new UseSegement variant (e.g. UseSegment::GlobalPrefix) that's either included in the path (or not), or whether that new variant is always present but has a boolean indicating yes/no, or something else entirely, but I just think we can do better than inserting :: into an Ident variant

[davidBar-On]

... basically I want to move to a more explicit representation of whether or not there's global prefix. I don't feel strongly about whether that's implemented via a new UseSegement variant (e.g. UseSegment::GlobalPrefix) that's either included in the path (or not), or whether that new variant is always present but has a boolean indicating yes/no, or something else entirely, but I just think we can do better than inserting :: into an Ident variant

If I understand correctly what you mean, what I suggested was supposed to be similar, but with using empty Ident instead of new UseSegment variant - which is a better idea.

I have implemented and submitted this approach by using UseSegment::Empty as the new variant name.

The main issue may be that there are at least two cases now where the ordering is different because of the removal of the :: from the indent. It may be that the current ordering is what was meant originally, but still it is not backward compatible. The two cases I identified are:

1. ::foo now comes before ::Foo: the reason is that Ord for UseSegment cmp() puts lowercase letters before uppercase. However, because the :: was added to the Indent, the Indents did not start with a letter, so compare_as_versions() was used for the comparison, and it probably orders by the ASCII representation order - uppercase before lowercase.
   Note that {foo, Foo} is the current and previous order. The issue is only with the first segment that starts with ::.

2. ::* is not put at the end of the list: e.g. ::* now precedes Supper::x. This is because now Global is not the first entity in the UseTree, as it is preceded be Empty, and Empty is ordered before any Ident.

Please let me know if this is o.k. or if any change to the ordering should be done.

[calebcartwright]

I have implemented and submitted this approach by using UseSegment::Empty as the new variant name.

I think we should iterate on that name. It's really just a global prefix on the tree, not an "empty" segment of the path.

The main issue may be that there are at least two cases now where the ordering is different because of the removal of the :: from the indent. It may be that the current ordering is what was meant originally, but still it is not backward compatible. The two cases I identified are

As far as I'm aware, there's no known bugs with the current ordering. Any internal refactoring we do should never result in different formatting being emitted. By changing aspects of how these structs/enums reflect certain attributes of use trees, we're going to have to update the comparison functions accordingly as well in order to preserve the ordering

[davidBar-On]

I think we should iterate on that name. It's really just a global prefix on the tree, not an "empty" segment of the path.

O.k. will change Emptyto GlobalPrefix as you suggested before.

As far as I'm aware, there's no known bugs with the current ordering. Any internal refactoring we do should never result in different formatting being emitted. By changing aspects of how these structs/enums reflect certain attributes of use trees, we're going to have to update the comparison functions accordingly as well in order to preserve the ordering

It is not straight forward to keep the current sorting order, as with the new representation the GlobalPrefix with the following item should be considered when comparing, while before it was one item (::<name>). Therefore, I would like to make sure that the current sorting is expected order, since currently the sort rules of the first item is different than the sort rules for other items.

Following is the current order of an an example imports list:

use self::*;
use ::Bar;
use ::bar;
use ::foo::bar;
use ::foo::bar::baz;
use ::foo::bar::Baz;
use ::foo::Bar;
use ::foo::*;
use bar;
use bar::*;
use Bar;
use Bar::*;
use ::*;

Examples for the inconsistencies I see here are:

1. ::Bar precedes ::bar but use ::foo::bar precedes ::foo::Bar and bar precedes Bar
2. bar::* precedes Bar::* but ::* is always ordered last.

As I wrote above, the reason for the difference between sort rules of the first item and the sort rules of the other items is that compare_as_versions() is used when Ident does not start with a letter. This is probably done to make sure that "2" < "10", but it happened also to be applied to the :: prefix.

If the the inconsistencies in the sorting order of the first and other items are not intentional, then the sorting order may need to be (with comments to the changes):

use self::*;               // No change - "self" is part special prefixes that are ordered first
use ::bar;                // Now first item "bar" precedes "Bar"
use ::foo::bar;
use ::foo::bar::baz;
use ::foo::bar::Baz;
use ::foo::Bar;
use ::foo::*;
use ::Bar;
use ::*;                 // Now "::*" precedes "bar" as with all other items does not start start with "::"
use bar;
use bar::*;
use Bar;
use Bar::*;

Please let me know if the original ordering should be kept or it should be modified.

[calebcartwright]

It is not straight forward to keep the current sorting order, as with the new representation the GlobalPrefix with the following item should be considered when comparing, while before it was one item (::).

By "current sorting order" are you referring to the end result that's finally idempotent after two passes, or the first interim result?

The target ordering is explicitly governed by this section of the Style Guide. If the final, idempotent result of rustfmt does not comply with the Style Guide prescription then that would be a bug, and one that would be a bit concerning.

I've not gone through it in detail, but my understanding is that the ordering emitted by rustfmt is correct, there's just a bug in these cases which require two runs to get there

[davidBar-On]

By "current sorting order" are you referring to the end result that's finally idempotent after two passes, or the first interim result?

I was referring to the end result that's finally idempotent after two passes.

The target ordering is explicitly governed by this section of the Style Guide. If the final, idempotent result of rustfmt does not comply with the Style Guide prescription then that would be a bug, and one that would be a bit concerning.

The guide does not refer to the case of GlobalPrefix. Except for ::* (i.e. GlobalPrefix::*), the existing sorting order is that GlobalPrefix is sorted before any other prefix (except for self, super and crate prefixes that always come first). Therefore always puting ::* at the end of the list seems to be a bug (which is caused by the use of compare_as_versions() as I explained above).

Second issue is that the guide specifies that "imports must be sorted ascii-betically". However, for some reason, lowercase letters are sorted before uppercase letters by Ord for UseSegment. Assuming that this is the right ordering and that the guide should be updated, sort order is in general o.k., e.g. std::x is sorted before std::X. The only exception is with GlobalPrefix, where ::x is sorted after ::X. Again this seem to be a bug.

(Note that in this PR I didn't have to change any existing rustfmt file or test file, so probably these issues are relatively rare.)

[calebcartwright]

Sorry for the delay, have been caught up on some other things

The guide does not refer to the case of GlobalPrefix. Except for ::*

That's because that portion of the guide was finalized when the only edition was 2015

https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html#the-crate-keyword-refers-to-the-current-crate
https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html#use-paths

Let me think on this one for a bit. #3943 is not a critical bug, and this is starting to turn into a can of worms, so if we can revert back to simpler/direct fix for #3943 that doesn't impact sort order I think that'll be fine for now and can open a new issue to question sort order more broadly

[davidBar-On]

if we can revert back to simpler/direct fix for #3943 that doesn't impact sort order I think that'll be fine for now

Reverted back the changes to the change in from_ast() that didn't change the sort order (mainly removed the GlobalPrefix segment type and its handling).

can open a new issue to question sort order more broadly

New issue should be open or just keeping this issue open is sufficient?

[calebcartwright]

New issue should be open or just keeping this issue open is sufficient?

New issue. Trying to split the resolution of a relatively minor idempotency issue from a potential change to the sort ordering that'd have significant formatting impacts. This PR can be used to resolve the former, a new issue to discuss and track the latter.

[davidBar-On]

New issue. Trying to split the resolution of a relatively minor idempotency issue from a potential change to the sort ordering that'd have significant formatting impacts. This PR can be used to resolve the former, a new issue to discuss and track the latter.

Opened issue #4648 about the two inconsistencies in the current imports sorting order.

[calebcartwright]

This will need to be rebased with the conflicts resolved. Sorry again about this one, while we were going down the proverbial rabbit hole there were other changes in flight to reform merge_imports as imports_granularity which changed some of the same code.

Give the back and forth we had on potentially doing some refactoring, as well as some of the debug commits, would be preferable to squash down to the desired commit(s) for merging too, though I can always squash on merge instead.

[davidBar-On]

This will need to be rebased with the conflicts resolved .... other changes in flight to reform merge_imports as imports_granularity which changed some of the same code

Merged the latest changes.

[calebcartwright]

This will need to be rebased with the conflicts resolved .... other changes in flight to reform merge_imports as imports_granularity which changed some of the same code

Merged the latest changes.

GitHub still seems to be a bit concerned, though admittedly that could be coming from older commits from your branch:

[davidBar-On]

GitHub still seems to be a bit concerned, though admittedly that could be coming from older commits from your branch:

I believe that the last commit (9a09e5b) is o.k.

What happened is that before pushing the changes I wanted to merge the latest master changes. When pushing this commit (a13b236), for some reason the master changes (mainly the import related) were shown as changes and "All checks have failed". As the master changes were not related to this PR, I git reset my local computer repository to the commit before the master merge, made the changes, and pushed this commit (9a09e5b). Therefore, this commit should not include the issues from 9a09e5b and should be o.k.

[calebcartwright]

and should be o.k.

Fair enough. For future reference, it'd help me streamline things a bit if we can cleanup the commits on the PR beforehand. Always feel free to open an early draft, especially if doing so makes it easier to have a dialog on certain changes, but it would be beneficial for the commits to be well structured when publishing the PR/requesting a review and merge (you can always squash/restructure them on your topic branch first).

Besides making the changes easier to review, having well structured commits would make it possible to rebase the commits back on to master directly, and having that option makes things a bit easier for me as well for certain backporting scenarios.

Bit bogged down at the moment trying to get the next release out, but should have some time for reviews shortly after.

[davidBar-On]

it'd help me streamline things a bit if we can cleanup the commits on the PR beforehand

O.k., will make sure to do it in the future.
Will not do it to this PR as the last commit should be a catch-up of all changes, but done it for other two "to be reviewed" PRs that I opened before (by squashing several commits into one).

[calebcartwright]

Thanks again, but similar to some other PRs I'm going to close given the 2.0 target/focus. Think this is still worth revisiting at some point down the road, but would prefer to do so with a clean PR targeting the rustfmt mainline