[unstable option] group_imports

[karyon]

Tracking issue for unstable option: group_imports.

See Processes.md, "Stabilising an Option":

• Is the default value correct?
• The design and implementation of the option are sound and clean.
• The option is well tested, both in unit tests and, optimally, in real usage.
• There is no open bug about the option that prevents its use.

[stjepangolemac]

Would it be possible to add a fourth group for the workspace members? They're technically external crates but still different from 3rd party crates. Something like StdExternalWorkspaceCrate.

(let me know if this isn't the right place to discuss this)

[calebcartwright]

No worries @stjepangolemac fine to ask here (and probably worth having pointers to various issues anyway). In short, we're limited by the information that's actually available. More detailed in the issues linked below

rust-lang/style-team#131 (comment)
#4693
#4709

[ShadowJonathan]

What's preventing the option from stablising as-is? #4693 to me seems to be about the addition of an extra option, but is there any problem with the current options? Any real bugs?

[daboross]

#4709 seems like a fairly big bug to me. Besides that, did we actually decide that this was the only sane ordering? I'm pretty sure it was waiting on having a few different options for grouping imports before stabilizing, if it is ever going to stabilize.

[calebcartwright]

I appreciate the interest and willingness of folks to participate in discussions, but should you choose to do so, a gentle reminder to be sure to thoroughly read the thread and linked material.

The process for stabilizing an option is both linked, and enumerated with checkboxes, in the issue description. None of those enumerated requirements can be cleared yet, including the fact that yes there's indeed real bugs that are either directly linked or transitively linked from issues/discussions that are directly linked. One can also search open issues to find additional related issues that aren't directly linked.

To again echo inline:

• All import options, including this one, have a number of issues with comments that prevent stabilization
  • Clarify recommended ordering of groups, even without rustfmt support? style-team#131 (comment)
• The rustfmt team has no intention of working on any other variants for this option, and it's one that will remain disabled by default. We continue to be skeptical that any additional variants are even practical, and stabilization of the option unequivocally does not hinge on additional variants. The fact that others would like the option to have more powerful variants is understandable but that desire doesn't change the underlying visibility/availability of data problem.
  • Clarify recommended ordering of groups, even without rustfmt support? style-team#131 (comment)
  • group_imports does not differentiate between a local import and an external crate #4709 is not a bug as mentioned and discussed on that thread, but a case where people want more and where more might not be possible.
• Anyone's welcome to attempt to implement a new variant, and should that occur after the option is stabilized, then the new variant would be "unstable" and only available on nightly until such time as it could clear the standard set of criteria

[SimonSapin]

I’d like for this option to have another value named Visibility that makes one group for use example;, one for pub(crate) use example; and another for pub use example;. (I’m less sure of the groups’ order.)

This is to separate private imports (that are only there for convenience of naming things inside a module’s code) from reexports that affect the public API of a module or crate.

[calebcartwright]

Seems I forgot to hit the "comment" button the other day, but yes @SimonSapin that type of grouping and variant should be possible with the information available to rustfmt.

However, I think it would be best to pull this additional-variant request out so it can be tracked as a separate feature requests, both for work tracking/implementation purposes and because it's not necessary for stabilization of the option nor related to it (we have the ability to add new, unstable variants to already stable options)

[calebcartwright]

Minimizing prior comments that were generalized questions about stabilization. Please refer to content in the issue description, subsequent comments above, and the below Discussions with more details about stabilization

#5365
#5367

[jonhoo]

I don't know that this is so much a bug with group_imports as it is perhaps a missing configuration setting: I generally like the one style except for cfg-gated uses, which I like to be grouped by their cfg flags. So, for example:

use foo::{a, b};
use bar::c;

#[cfg(feature = "d")]
use baz::d;

#[cfg(feature = "e")]
use {baz::{e, f}, qux::g};

This gets funkier when combined with imports_granularity since while I generally prefer something like module for that setting generally, I want the imports_granularity=one style specifically for cfgs so that the (potentially quite complicated) #[cfg] line doesn't have to be repeated.

Currently, with group_imports=one, you end up with:

use bar::c;
#[cfg(feature = "d")]
use baz::d;
use foo::{a, b};
#[cfg(feature = "e")]
use {baz::{e, f}, qux::g};

Where it becomes pretty difficult to disentangle which uses belong to which #[cfg]. Note also that group_imports doesn't quite know what to do with the root-level-{} combined use for feature = "e", and decides to sort it (as best I can tell) by the last item in the list (so qux).

[jonhoo]

Unrelated to the above, the other bit I'd love to see from group_imports is differentiating between visibility. At the moment, it produces the following kinds of use lists:

use a;
pub(crate) use b;
pub use c;
use d;
pub use x;
pub(crate) use z;

whereas it feels more natural for the sort order to be:

pub use c;
pub use x;
pub(crate) use b;
pub(crate) use z;
use a;
use d;

(that is, sort by visibility then by item name)

[tgross35]

If the above suggestions are to be configurable, I would rather have it in a dictionary format than adding more variants. That is, instead of adding a StdExternalCrateVisibility option, accept something like:

group_imports = { grouping = "StdExternalCrate", group_by_visibility = true }

@jonhoo it may be worth asking the style team at https://github.com/rust-lang/style-team if they would like to take a position on your above two suggestions in case there is interest in making them the style guide default.

[tgross35]

To what extent is it worth blocking on #4709? The only other related issues I see are feature requests, which could be added as an additional configuration later.

If there isn't anything that can be done for #4709, it seems like stabilization could potentially move forward as-is.

Mini stabilization consideration report:

* [ ]  Is the default value correct?

It seems StdExternalCrate crate would really be best here, but optimal behavior here may be blocked on #4709 and a related style team decision. Not sure if there is any sense in reopening rust-lang/style-team#131.

I would also be curious to hear style team input on #5083 (comment) and #5083 (comment), in case there is a preference to e.g. make the default for all options sort by visibility.

Barring any visibility- or attribute-related input, I would propose going forward with the current Preserve option for now and later change in a rustfmt edition if needed.

* [ ]  The design and implementation of the option are sound and clean.

The design is simple enough.

* [ ]  The option is well tested, both in unit tests and, optimally, in real usage.

The test suite for this option seems reasonably comprehensive. As one data point, I have been using this on various projects for over a year with no noticeable problems.

* [ ]  There is no open bug about the option that prevents its use.

See the above. I propose stabilizing without a fix for #4709 if it seems like there isn't much to possibly be done here.

[RalfJung]

I find #4709 to be a blocker here. As-implemented, I don't think I'd ever want to use this formatting option. (But I should first try this on some real code to see some examples. Maybe imports from local modules are sufficiently rare that it's fine?) It also doesn't behave as documented, as I explained here.

[RalfJung]

My other concern is that in practice one often wants to split the "extern" group into multiple subgroups. For example, in rustc I always want to keep the imports from rustc_* crates in their own group. In a rocket-based application I want to separate rocket imports from other external crates. I don't know a good way to automate that kind of formatting, though... but the rustc_ one is sufficiently standard throughout the compiler (and Miri) sources that I don't think I'd be happy to see StdExternalCrate applied in the rustc repo.

rust-lang/style-team#131 mentioned "at least three" groups, maybe that was referring to this as well.

[RalfJung]

TIL that use *; is a thing. How does rustfmt group that?
I think this is equivalent to use self::*; so it should probably be grouped with the "crate" imports?

(I don't know of a case where this import is actually useful, but would still be good to determine how it gets grouped.)

[petrochenkov]

TIL that use *; is a thing.

It's a thing on 2015 edition only, so it's equivalent to use crate::*; (maybe rustfmt should normalize it to that).

[RalfJung]

Oh right, I had entirely forgotten that in the 2015 edition, use used absolute paths... makes sense.
So, it should definitely be grouped with the "crate" imports then.

[RalfJung]

Currently, * is being grouped with "external".

[RalfJung]

Here's another group_imports issue that is IMO quite severe: #6241.