Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[unstable option] imports_granularity #4991

Open
12 of 16 tasks
Tracked by #83 ...
9999years opened this issue Sep 15, 2021 · 44 comments
Open
12 of 16 tasks
Tracked by #83 ...

[unstable option] imports_granularity #4991

9999years opened this issue Sep 15, 2021 · 44 comments
Labels
unstable option tracking issue of an unstable option

Comments

@9999years
Copy link

9999years commented Sep 15, 2021

Tracking issue for unstable option: imports_granularity.

Option documentation.

See Processes.md, "Stabilising an Option":

@calebcartwright calebcartwright added the unstable option tracking issue of an unstable option label Sep 16, 2021
@Tamschi
Copy link

Tamschi commented Oct 10, 2021

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
Copy link
Member

calebcartwright commented Oct 10, 2021

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

lunacookies added a commit to lhvy/pipes-rs that referenced this issue Oct 23, 2021
Unfortunately the `imports_granularity` option of rustfmt
I used to automate this change is unstable:

rust-lang/rustfmt#4991
bors bot pushed a commit to lhvy/pipes-rs that referenced this issue Oct 25, 2021
Unfortunately the `imports_granularity` option of rustfmt I used to automate this change [is unstable](rust-lang/rustfmt#4991).
@Xuanwo
Copy link

Xuanwo commented Feb 18, 2022

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

@calebcartwright
Copy link
Member

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
Copy link

Nutomic commented Feb 21, 2022

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
Copy link
Contributor

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
Copy link

Xuanwo commented Apr 15, 2022

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;

davidlattimore added a commit to davidlattimore/rustfmt that referenced this issue Apr 17, 2022
@repi
Copy link

repi commented Apr 30, 2022

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 a37d3ab) that worked without any problems whatsoever. 🎉

Looking for to latest rustfmt here (with the #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
Copy link
Member

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)

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

@calebcartwright
Copy link
Member

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
Copy link
Contributor

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:

@tommilligan
Copy link
Contributor

  • 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?

#[allow(unreachable_pub)]

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

@tgross35
Copy link
Contributor

tgross35 commented Apr 2, 2023

Would an option like FlatCrate be possible, or would that be a separate feature flag?

For example, the Crate option has this as an example:

use foo::{
    a, b,
    b::{f, g},
    c,
    d::e,
};
use qux::{h, i};

But that's really bulky. Id be happier with something like this, that wraps at the line length:

use foo::{a, b, b::{f, g}, c, d::e,};
use qux::{h, i};

Obviously the difference is pretty dramatic in this example with single-letter modules, but it makes a difference in real-world use too.

(also, just another +1 for stabilization in the near future assuming #5269 gets fixed, I've encountered no issues in my past yearish of using this on everything)

@dcormier
Copy link

dcormier commented Apr 3, 2023

@tgross35, I believe you're looking for the imports_layout setting.

@tgross35
Copy link
Contributor

tgross35 commented Apr 3, 2023

Hm, you're right, but it doesn't seem like the 'Crate" imports_granularity setting is quite coherent. I wrote it up on the imports_layout issue #3361 (comment)

@gilescope
Copy link

I don't think the default value is correct - it takes up a lot of space. I expect something like imports_granularity = "Crate". At the moment cargo fmt by default can have pages of use statements with one import on each line. That seems an odd default... (totally happy with every other cargo defalut fmt setting, but this one I just seem to have to set)

@oxalica
Copy link

oxalica commented May 1, 2023

I don't think the default value is correct - it takes up a lot of space.

Isn't the default value "Preserve" for imports_granularity? It would not change existing uses by default.

@popzxc
Copy link

popzxc commented Aug 23, 2023

Hey! I'm happy to see the rustfmt project active once again.

Can we resume the discussion on whether it's possible to stabilize this feature in some form?
From a quick glance, looks like #5290 should be merged and #5269 fixed before that.
Is there anything else?

@calebcartwright
Copy link
Member

Hey! I'm happy to see the rustfmt project active once again.

I assume this was said with the best of intent and while I appreciate the positive sentiment, I'd dispute the characterization. However, that's not a topic I'm interested in delving into, not to mention that doing so here would be off topic anyway

From a quick glance, looks like #5290 should be merged and #5269 fixed before that.
Is there anything else?

I won't say definitively that's the ultimate, exhaustive list of blocking issues, but yes, it's accurate that those remain some of the known bodies of work pending (and blocking) for stabilization of this option.

That's been the case for a while, and at least in the case of #5269, is unlikely to change any time soon unless someone else works on them; neither Yacin nor I are likely to be able to allocate any of our rustfmt-bandwidth on those for the foreseeable future.

@popzxc
Copy link

popzxc commented Aug 24, 2023

Sorry that my remark turned out to be inaccurate, I was just happy to see that PRs are being actively merged & discussed. I hope it didn't appear insensitive.

Then, I guess, the main question I have is: do you see any blockers here besides development effort (that can be done by external contributors)? E.g. is there enough capacity to discuss topics, accept/reject proposed solutions, etc (basically everything but submitting PRs)?

I'm interested in working on it, but certainly don't want to overburden the rustfmt maintainers.

@9999years
Copy link
Author

I've added a list of open issues regarding imports_granularity to the original ticket. Please let me know if there's others to add to the list.

@yanchith
Copy link

yanchith commented Jun 4, 2024

Hi all. I hope I am not too late with the feedback. I would like to use imports_granularity = "Module", as in theory it is exactly what I'd like to see.

However, I have a couple of issues with how it works as of nightly-2024-05-13.

  1. I think the following diff shows bugs, or at least really unexpected features.
use core::mem::{self, MaybeUninit};

... becomes ...

use core::mem::{  // HorizontalVertical is not respected
    MaybeUninit,
   {self},        // self became wrapped in additional {} 
};
  1. This is technically not a bug, but I'd like to have module imports as separate use statements and only merge non-module items of the last depth level. Maybe this is another imports_granularity? "ModulesWithModuleUsesAsOne".
use core::fmt;
use core::slice;

... becomes ...

use core::{fmt, slice}; // <- Meh?
  1. This one is once again more of a bug in my eyes:
use library1 as lib1;
use library2 as lib2;

... becomes ...

use {library1 as lib1, library2 as lib2}; // <- These don't even share a common prefix.

@mnkhouri
Copy link

@yanchith I filed #6191 so that your point 3. can be discussed, since I also expected the same result you described

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unstable option tracking issue of an unstable option
Projects
None yet
Development

No branches or pull requests