[unstable option] `imports_granularity`

[9999years]

Tracking issue for unstable option: imports_granularity.

Option documentation.

See Processes.md, "Stabilising an Option":

• [x] Is the default value correct?
• [x] The design and implementation of the option are sound and clean.
• [x] 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.
  • [x] #5269
  • [ ] #6028
  • [ ] #6027
  • [x] #5030
  • [x] #4681
  • [x] #5253
  • [x] #5311
  • [ ] #5296
  • [x] #4406
  • [x] #4536
  • [x] #3984
  • [x] #3999

[Tamschi]

When using

unstable_features = true
imports_granularity = "Crate"

, cargo +nightly fmt will currently remove comments like this:

use triomphe::{
	Arc,
	// This crate walks along the graph a whole lot and normally doesn't do much refcounting,
	// so the offset version is likely a bit better here.
	//
	// This is unbenched though. Someone might want to check.
	OffsetArc,
};

❯ cargo +nightly fmt --version
rustfmt 1.4.37-nightly (a8f2463c 2021-10-09)

(I wasn't sure whether this deserves its own issue.)

[calebcartwright]

The dropping of comments with the various options that manipulate imports is a known issue, and one of the hurdles that will have to be overcome before the options, including this one, can be stabilized.

It is good to have an inline example here too though, so thank you @Tamschi

[Xuanwo]

Hi, is there any progress to make this option stable?

[calebcartwright]

Hi, is there any progress to make this option stable?

The absence of updates should be explicitly interpreted as meaning that there are no updates. The process and requirements for stabilizing an option are both linked, and enumerated with checkboxes, in the issue description and will be updated as-and-when there are updates to share

I appreciate the interest folks have in being able to use an option on stable. However, this remains a relatively new option with many well known bugs, and as stated above, is not going to be eligible for stabilization any time soon.

[Nutomic]

For what its worth, we have been using imports_granularity="Crate" in Lemmy for over a year without any problems. Though we dont put any comments around imports.

[davidlattimore]

I sent a PR to make rustfmt preserve comments during flattening when they're on the line before a leaf node, or on the same line as a leaf node. e.g.

use a::{
   // Before a::b
   b,
   c, // After a::c
};

If import_granularity=Item, then we'll get:

// Before a::b
use a::b;
use a::c; // After a::c

What I'm less sure about, is what to do when flattening and there's a comment before a non-leaf node. e.g.

use a::{
   // Before a::b
   b::{b1, b2},
   c,
};

One easy option is to not flatten non-leaf nodes that have comments, then we'd get:

// Before a::b
use a::b::{b1, b2};
use a::c;

That would mean we couldn't fully convert granularity to Item in such cases.

Another option would be to attach the comment to the first leaf contained within. Then we'd get:

// Before a::b
use a::b::b1;
use a::b::b2;
use a::c;

Although if there was already a comment attached to b1, then I guess we'd need to merge the comments, which could get tricky. Thoughts?

[Xuanwo]

Another option would be to attach the comment to the first leaf contained within.

Seems reasonable to me.

Although if there was already a comment attached to b1,

Not bad to me, though.

Converting from

use a::{
   // Before a::b
   b::{
        // Before a::b::b1
        b1, 
        b2
   },
   c,
};

to

// Before a::b
// Before a::b::b1
use a::b::b1;
use a::b::b2;
use a::c;

[repi]

Did a reformatting of a fairly large Rust code base (~200k LoC, 767 source files, 60 crates) with imports_granularity=item to reduce git merge conflicts on import changes, and happy to report that with latest rustfmt (used https://github.com/rust-lang/rustfmt/commit/a37d3ab0e1c7c05f1a6410fb7ddf5539f0d030f8) that worked without any problems whatsoever. 🎉

Looking for to latest rustfmt here (with the https://github.com/rust-lang/rustfmt/issues/5030 fix) getting deployed out to Rust Nightly in rustup so it can be used more easily, and (hopefully) soon stabilization of this great option! Thanks to everyone that has contributed to this feature/option!

[calebcartwright]

I'll also add that I do view folks sharing their experience to be helpful feedback for us, e.g. https://github.com/rust-lang/rustfmt/issues/4991#issuecomment-1046885320 and https://github.com/rust-lang/rustfmt/issues/4991#issuecomment-1113996096

That level of direct feedback absolutely helps feed into that 3rd gate of "well tested, optimally in real world usage", and is appreciated

[calebcartwright]

Big thanks again to @davidlattimore for addressing the comment issue, and I believe this has us on a good track towards stabilization. I believe there's really only two things left we need to consider before proceeding:

• do we have the ability to stabilize the option while leaving certain variants unstable (also so we can add new unstable variants in the future)
• do any of the current variants have any reported bugs

The latter is something anyone could help with and which would be most appreciated. The former is something we need to look at anyway as that's a more general need than any one config option (though anyone interested in helping with that should certainly feel free)

[tommilligan]

I'll also add that I do view folks sharing their experience to be helpful feedback for us, e.g. #4991 (comment) and #4991 (comment)

I've been using both imports_granularity and group_imports at work for the last few months, and have had no issues apart from the aforementioned comments issue (now fixed). For reference our Rust codebase is ~79k LoC, ~300 files, ~50 crates.

• do any of the current variants have any reported bugs

I've searched the current open issues referencing imports_granularity, and I believe we can consider this box checked. I found:

• #5360 (new feature request, orthogonal to the current feature/current implementation is forwards compatible)
• #5321 (unrelated)
• #4746 (a bug that is general with rustfmt, not imports_granularity specifically)

[tommilligan]

• do we have the ability to stabilize the option while leaving certain variants unstable (also so we can add new unstable variants in the future)

@calebcartwright If I understand this correctly, what you're requesting is that after stabilisation, it would be possible to:

• Pass an option such as imports_granularity = "Crate", which is considered stable and will run on rustfmt stable
• Pass an option such as imports_granularity = "NewVariant", which is considered unstable, and will not run on rustfmt stable

This behaviour should extend outwards to the publically exported API of the crate, such that adding a new variant is not a breaking change. However, I see that the current config option enums are unreachable from the crate root, so maybe this isn't a concern?

https://github.com/rust-lang/rustfmt/blob/5fa2727ddeef534a7cd437f9e288c221a2cf0b6a/src/config/mod.rs#L15

I would be happy to take a first pass at this, I have some bandwidth today

[calebcartwright]

@tommilligan - thank you!

I've searched the current open issues referencing imports_granularity, and I believe we can consider this box checked. I found:

#5360 would potentially be impactful, depending on if, and how, we decide to handle it. It wouldn't necessarily need to be implemented in order to be able to stabilize this option (fully or partially, for some variants), but would at least need to rule out parts of that proposal in order to proceed with stabilization.

what you're requesting is that after stabilisation, it would be possible to:

Correct. I want to make sure that we have the ability to make certain variants available on stable even if other variants aren't quite ready, and I also want us to be able to add new variants in the future which would inherently be unstable when first introduced.

This behaviour should extend outwards to the publically exported API of the crate, such that adding a new variant is not a breaking change. However, I see that the current config option enums are unreachable from the crate root, so maybe this isn't a concern?

I don't think so, or at least I believe it's something we can handle internally, even if it requires something new like a post-processing/pre-formatting validity check of the config.

I would be happy to take a first pass at this, I have some bandwidth today

:heart: :rocket:

[tommilligan]

do we have the ability to stabilize the option while leaving certain variants unstable (also so we can add new unstable variants in the future)

I have opened a tracking issue at #5378 and a first implementation pass at #5379

[calebcartwright]

Also note that #4725 would be a blocker for stabilizing the Item variant (it's closed, but note that it has the backport pending label which means the fix hasn't actually landed on the trunk branch yet)

[davidlattimore]

Thanks @calebcartwright - I created a PR to backport it.

[utkarshgupta137]

Not sure if this is the right forum for requests, but there should be an option or some mechanism to skip this for core/std/alloc, at least in the crate mode. The idea is that a single crate would have a lot of related functionality but core/std/alloc have a wide range of functionality & can have a lot of unrelated imports in a single file.

[calebcartwright]

@utkarshgupta137 - it's not clear to me precisely what you are proposing, so perhaps some concrete examples would help visualize things.

However, it seems as if you're saying that the single, global imports_granularity option should actually support divergent granularity levels for different import groups (with the specific group example given as core/std/alloc)?

[utkarshgupta137]

@utkarshgupta137 - it's not clear to me precisely what you are proposing, so perhaps some concrete examples would help visualize things.

However, it seems as if you're saying that the single, global imports_granularity option should actually support divergent granularity levels for different import groups (with the specific group example given as core/std/alloc)?

Exactly.

Current behaviour with imports_granularity = Crate:

use std::{
    fmt,
    io::{self, Write},
    net::{self, TcpStream, ToSocketAddrs},
    ops::DerefMut,
    path::PathBuf,
    str::{from_utf8, FromStr},
    time::Duration,
};

use futures_util::{
    future::{Future, FutureExt},
    ready,
    sink::Sink,
    stream::{self, Stream, StreamExt, TryStreamExt as _},
};

Desired behaviour:

use std::fmt;
use std::io::{self, Write};
use std::net::{self, TcpStream, ToSocketAddrs};
use std::ops::DerefMut;
use std::path::PathBuf;
use std::str::{from_utf8, FromStr};
use std::time::Duration;

use futures_util::{
    future::{Future, FutureExt},
    ready,
    sink::Sink,
    stream::{self, Stream, StreamExt, TryStreamExt as _},
};

[calebcartwright]

Thank you for clarifying.

I'll wait to let the masses weigh in, but speaking purely personally, will note that sounds a bit too bespoke and has too much friction with the consistency goals imo.

Though technically possible, I'm not sure how practical it would be (especially since it would open a fairly large door), and it would certainly derail the current stabilization progress.

[faern]

I agree with @calebcartwright. Sounds too bespoke and edge casey for me also. What about other crates with a huge API surface in very many unrelated areas? Such as async-std for example. Should rustfmt then allow specifying a list of crates to except from the import merge rule?

[utkarshgupta137]

I too would prefer that this option gets merged first & then we can consider this later on.

[calebcartwright]

this option gets merged first & then we can consider this later on.

The challenge is that I'm not sure I see a great a way in which we could stabilize this option and then introduce the behavior you're referring to. The only way I can think of that wouldn't necessitate breaking/changing a stabilized option (not permitted) would be to have a rather intense new variant for imports_granularity with a very long name that provided what you're asking for.

If that's a solution/approach you could live with in a hypothetical future where the multiple granularities model would be supported then we can continue proceeding with stabilization. However, if not and you/others will still want divergent granularities for different groups then a number of things would need to be reconsidered, including this option.

(edits: typos)

[tommilligan]

If that's a solution/approach you could live with in a hypothetical future where the multiple granularities model would be supported then we can continue proceeding with stabilization.

I can see a path forward where the existing option is considered the default formatting style for all imports. A new option can then be added and stabilised which takes a mapping of crate name -> granularity level, overriding the default value.

However, I agree that this is a very complex case and I'm not sure what value it brings to the user. I'm not personally interested it, though I am very interested in having imports_granularity stabilised - and I think many users would say the same. I would be keen to press forward with stabilisation, rather than scope creeping the original feature.

[calebcartwright]

I can see a path forward where the existing option is considered the default formatting style for all imports. A new option can then be added and stabilised which takes a mapping of crate name -> granularity level, overriding the default value.

Agree with the thrust of your comment, but this is precisely the type of thing we'd want to avoid as it just leads to a more convoluted config surface that's increasingly difficult to consume and explain. This is even more true for imports where there's already a disproportionately large number of configuration options that have to be considered against each other

[utkarshgupta137]

Would it be acceptable to add an attribute, similar to rustfmt_skip later on?

Now that I think about it, setting this attribute in each file instead of globally for each crate would make it much more useful.

[calebcartwright]

Would it be acceptable to add an attribute, similar to rustfmt_skip later on?

I understand and appreciate you're just trying to brainstorm @utkarshgupta137, but have to stress this is an absolutely massive creep of scope and is completely unrelated to the stabilization of a configuration option.

I don't want this stabilization tracking issue to go off the rails on an orthogonal topic, but will say that while I understand why some users may want to have attribute-based, divergent formatting within their codebase, that's highly unlikely to ever come to fruition both because of running starkly counter to the consistency goals as well as technical reasons.

n.b. this has been previously requested and discussed elsewhere, so we're not going to rehash any aspect of that discussion/debate here.

---

there should be an option or some mechanism to skip this for core/std/alloc, at least in the crate mode.

Back to your original ask :point_up:, Thanks for sharing your suggestion, but the overall consensus appears to be against rethinking this option to support your ask, so I'm going to go ahead and say that he answer to your ask is a "no". We're not going to pivot and we'll continue with imports_granularity as-is.

Perhaps your request can be revisited down the road, but will have to be considered against the config surface in it's future form.

[tommilligan]

So, to again summarize progress towards stabilization:

---

Also note that https://github.com/rust-lang/rustfmt/issues/4725 would be a blocker for stabilizing the Item variant

As above, backported in #5380, so I consider this unblocked.

I want to make sure that we have the ability to make certain variants available on stable even if other variants aren't quite ready, and I also want us to be able to add new variants in the future which would inherently be unstable when first introduced.

This is now implemented in #5379.

Are there any variants of imports_granularity that need to be annotated as unstable, to allow us to proceed with stabilisation of the option as a whole? IIRC in the past there has been some discussion about the Item variant being unstable, but as above I think these are now fixed and backported. I would consider all of the variants stable at present.

#5360 would potentially be impactful, depending on if, and how, we decide to handle it. It wouldn't necessarily need to be implemented in order to be able to stabilize this option (fully or partially, for some variants), but would at least need to rule out parts of that proposal in order to proceed with stabilization.

There has now been suitable discussion on the PR, which resulted in (ref):

What I might be willing to consider is a new variant for imports_granularity that's something
akin to OneModule (or anything better named) that would provide your requested output.

This sounds future-compatible with the unstable variants work that is now completed, so I don't consider this blocking. If more work is required it can be behind an unstable variant.

https://github.com/rust-lang/rustfmt/issues/5410

There was some discussion about whether this impacts imports_granularity; in summary, no it does not. The solution could be "to introduce yet another imports related config option", if desired (so not relevant to stabilisation here).

---

From the tracking list above:

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

There have been several real-world codebases formatted with this option, with no current issues noted:

• https://github.com/rust-lang/rustfmt/issues/4991#issuecomment-1046885320
• https://github.com/rust-lang/rustfmt/issues/4991#issuecomment-1113996096
• https://github.com/rust-lang/rustfmt/issues/4991#issuecomment-1152157410

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

I noted #5269 which potentially impacts all import options (sort order). However, there does not seem to be pressure or activity towards resolving it at present.

I think we could either:

• Continue, considering import sort order orthogonal to this feature.
• Block on fixing the issue as a whole.
• Block on fixing #5296 for specific imports_granularity functionality. This would ensure the behaviour of imports_granularity doesn't change in the future; however, it may make sort order inconsistent with other current rustfmt features.

---

Apologies for the wall of text, but I think that's everything to consider in one place. IMO the only potential blocker remaining is to consider the impact of #5269, which is a far-reaching issue (not specific to this feature)

[calebcartwright]

Thank you for the summary @tommilligan and generally agreed.

I want to do a final pass through things, but just so folks know where my head is at, I'd anticipate all variants except One being ready for stabilization.

To be clear, it's not that I necessarily think that variant is explicitly not ready, but simply that I'm not yet convinced that it is ready, especially compared to the others, and we err on the side of caution when it comes to stabilization.

[laralove143]

this suddenly broke on me idk why, i ran cargo fmt and it complained it cant set the option on stable so its seeing the config

/Users/lara/Library/Preferences/rustfmt/rustfmt.toml

unstable_features = true
imports_granularity = "Crate"

right now it's just not doing anything, if the code is

use crate::Foo;
use crate::Bar;

and i run cargo +nightly fmt the code stays the same

i've done rustup update and even reinstalled the components, also the group_imports doesnt seem to be working either, if there's no empty lines it just treats them all the same (sorts std somewhere in the middle)

idk what broke it and it might be something i overlooked but help please