Use versionsort for everything that's auto-reordered by name

[tanriol]

This change has been initially discussed in rust-lang/style-team#143 and is motivated by the following example from using nom:

// Formatting before this PR
use nom::number::complete::{le_u128, le_u16, le_u32, le_u64, le_u8};
// Formatting after this PR
use nom::number::complete::{le_u8, le_u16, le_u32, le_u64, le_u128};

I've covered imports with use (including renames), mod and extern crate statements (including renames), and item names. Is there anything else I've missed?

Technically this is a breaking change; however, in most cases it results in more readable code, so I'd consider it a bugfix. The only case I know that this change makes worse is names including fixed-width hex value which are ordered fine today but may become misordered

use some_crate::{CONST_10C0, CONST_1040, CONST_1080};

because the comparison runs between numerical segments of different length. I've thought about making this change configurable or version-gating it, but could not find out whether the config is available in some TLS, while some changed code is called via Ord impls and passing down the config explicitly is going to be a bigger refactoring.

The second commit reverts a change introduced in fa75ef4 (#2535) that was likely unintentional as the comments in the tests still documented the old behavior.

[topecongiro]

Thank you for the PR! I appreciate your work and effort to improve rustfmt.

Technically this is a breaking change;

rustfmt has some serious restriction when it comes to changing its default format style (see rust-lang/rust#54504 for the context if you are interested).
In a nutshell, despite this being a subtle change, we need to add a version-gate against this PR. The easiest way I can think of is to add Version field to UseTree and pass/use it across various comparison methods, though this is still sounds tedious and unclean.
@tanriol Would you mind adding a version-gate to this PR? Or if you are fine not merging this PR right now, we can keep this open or merge this to a different branch and merge to the master branch before we release rustfmt 2.0.

[tanriol]

@topecongiro If this is merged with version-gating, do I understand correctly that it will be impossible to opt in to it until rustfmt reaches 2.0? Is there any expected timeframe for 2.0?

If this is not something that happens soon, I'd prefer this option to be actually usable. Would this imply adding a configuration option with the old sort order as default?

[topecongiro]

We will still be able to opt in fixes that are version gated by adding version = "Two" to the configuration file. And I would rather not have this as something configurable.

Is there any expected timeframe for 2.0?

We do not have a concrete deadline for 2.0.

[topecongiro]

@tanriol Would you mind rebasing this against the rustfmt-2.0 branch?

[tanriol]

Coming back to this: do I understand correctly that master is now 2.0 so I should rebase the PR on top of master?

[calebcartwright]

do I understand correctly that master is now 2.0 so I should rebase the PR on top of master?

Yes, that is correct, though I'd suggest waiting til after #4022 has been merged since there have been some related changes in the AST (ImplItemKind was folded into AssocItemKind, Existential variant was dropped, etc.)

Also, I don't think this will need to be version gated any more since it'll be rolled into the 2.x release

[calebcartwright]

@tanriol just a heads up that #4022 has been merged, so whenever your ready to rebase you'll have the latest version of libsyntax and friends

[tanriol]

The CI errors in rustc-ap-rustc_data_structures-642.0.0 do not look like anything I could cause

   Compiling rustc-ap-rustc_data_structures v642.0.0
error[E0605]: non-primitive cast: `std::num::NonZeroU64` as `u32`
   --> /home/travis/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/rustc-ap-rustc_data_structures-642.0.0/profiling.rs:312:29
    |
312 |             let thread_id = std::thread::current().id().as_u64() as u32;
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: an `as` expression can only be used to convert between primitive types. Consider using the `From` trait
error[E0605]: non-primitive cast: `std::num::NonZeroU64` as `u32`
   --> /home/travis/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/rustc-ap-rustc_data_structures-642.0.0/profiling.rs:471:25
    |
471 |         let thread_id = std::thread::current().id().as_u64() as u32;
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: an `as` expression can only be used to convert between primitive types. Consider using the `From` trait
error: aborting due to 2 previous errors
For more information about this error, try `rustc --explain E0605`.
error: could not compile `rustc-ap-rustc_data_structures`.

Looks like this is the result of rust-lang/rust#70240

[tanriol]

@calebcartwright Does anything else need to be improved?

[calebcartwright]

We're in another round of rustc-ap updates that'll be blockers for this. #4100 will get those issues sorted, and once those make their way into the master branch you should be able to rebase to resolve

[tanriol]

Thank you, subscribed to the linked bug.

[tanriol]

Do I understand correctly that it's #4106 now?

[calebcartwright]

Do I understand correctly that it's #4106 now?

Correct

[tanriol]

@calebcartwright Rebased and green.

[calebcartwright]

I believe this is good to merge now, any objections @topecongiro?

[topecongiro]

My apologies for the late review. LGTM, thank you!