Support custom "project local" crate group in `group_imports`

[daniel5151]

📋 Description

For the sake of example, lets say a project is composed of the following crates: coolapp-utils, coolapp-backend, and coolapp-frontend

It would be great if group_imports supported a custom grouping between "external" crates (from crates.io) and "project local" crates:

// somewhere in `coolapp_frontend`

use alloc::alloc::Layout;
use core::f32;
use std::sync::Arc;

use chrono::Utc;
use uuid::Uuid;
use log::Level;

// At the moment, `group_imports = StdExternalCrate` fuses this group with the group above.
use coolapp_utils::frobnicate;
use coolapp_backend::api::ApiVersion;

use super::schema::{Context, Payload};
use super::update::convert_publish_payload;
use crate::models::Event;

Note that different projects have different notions of what constitutes a "project local" crate. Some examples I've come across in the past have included:

• workspace local crates
• crates that share a prefix, but are pulled from crates.io
• crates imported via a local relative path
• etc...

Since there wouldn't be any way to automatically determine which crates fall into which category, one reasonable implementation might be to modify the current "External" crate grouping to support being arbitrarily divided into two (or more?) groups.

While this wouldn't be a *perfect* solution (after all, users could still inadvertently import "project local" crates above "external" crates), it would be a step in the right direction, and offer a much needed bit of flexibility to group_imports.

[calebcartwright]

Thanks for reaching out with your request! group_imports is indeed a new option, but we knew even when adding initial support that there would likely eventually be various grouping strategies that folks would be interested in. The current variant is just what was including in the initial addition of the option, and no one has implemented any other variants since

Generally speaking, we're open to supporting any additional variants and associated groupings, but would have to rely on someone from the community to submit an implementation. The Style Guide, and thus rustfmt's default behavior, will continue to be a do-nothing/preserve on this front, so we can't really allocate any maintainer bandwidth into implementation.

The other thing I want to emphasize (as alluded to in the issue description) is that rustfmt's only source of information is the AST provided by rustc which would indeed limit the ability to actually implement the requested grouping.

I suppose an additional variant that attempted to take the original groupings into account could work in theory, but I imagine it could be more tricky than it may seem at first glance.

For example, consider this as input:

use alloc::alloc::Layout;
use core::f32;
use std::sync::Arc;

use chrono::Utc;
use uuid::Uuid;

use log::Level;
use super::schema::{Context, Payload};
use super::update::convert_publish_payload;
use crate::models::Event;

or this

use alloc::alloc::Layout;
use core::f32;
use std::sync::Arc;

use log::Level;
use super::schema::{Context, Payload};
use super::update::convert_publish_payload;
use crate::models::Event;

use chrono::Utc;
use uuid::Uuid;

Should there be two ext groups since there was a separation between them, or should log::Level be moved with the others in a single ext group since it was the only ext in a different group, and developer probably did that accidentally vs. trying to maintain two separate ext groups? Would the position in the original group make a difference? Or the quantity (e.g. two ext in the crate group)? Just some rhetorical examples of how there could potentially be a fair amount of edge cases here.

The current variant isn't infinitely flexible, but it's quite predictable and deterministic which is essentially a requirement given the AST/pretty-printing nature of rustfmt. If anyone's interested in seeing this requested grouping and can come up with such an implementation though we'll be happy to review!

[daniel5151]

Thanks for the well thought out and detailed response @calebcartwright!

My *gut* reaction to those ambiguous cases would be for rustfmt to preserve groupings as best as possible, without worrying too much about the developer's intentions. e.g: for the second sample input, the output would be:

use alloc::alloc::Layout;
use core::f32;
use std::sync::Arc;

use log::Level;

use chrono::Utc;
use uuid::Uuid;

use super::schema::{Context, Payload};
use super::update::convert_publish_payload;
use crate::models::Event;

While this output would most likely not be what the developer indented, rustfmt's output wouldn't be very "surprising". The rule would be simple, predictable, and deterministic, and could fill a role somewhere between the extremely rigid StdExternalCrate option and not using group_imports at all.

we can't really allocate any maintainer bandwidth into implementation.

Yep, totally fair! I opened this issue since I just wanted to get some more visibility on the group_imports feature, and to check if other folks would be interested in seeing it extended.

While I'd love to contribute this feature myself, Rust AST munging isn't something I've done in the past, so I might not be the right man for the job 😅. That said, who knows? Maybe I'll find some free time at some point to take a crack at it. After all, group_imports is definitely something I'd love to enable in my own projects :)

[calebcartwright]

While this output would most likely not be what the developer indented, rustfmt's output wouldn't be very "surprising". The rule would be simple, predictable, and deterministic, and could fill a role somewhere between the extremely rigid StdExternalCrate option and not using group_imports at all.

Understood, it's just that there would be more of a "depends" factor with this variant vs. StdExternalCrate in terms of being able to describe what the result would because presence/absence of blank lines in the input will have an impact. That's not a problem nor a blocker, and we do this in other places as well, but it does bear emphasis.

While I'd love to contribute this feature myself, Rust AST munging isn't something I've done in the past, so I might not be the right man for the job sweat_smile. That said, who knows? Maybe I'll find some free time at some point to take a crack at it

No worries! For you, or anyone interested in working on this, most of the relevant code can be found in reorder.rs and imports.rs

rustfmt/src/formatting/reorder.rs

Lines 201 to 206 in 98f2347

	fn rewrite_reorderable_or_regroupable_items(
	context: &RewriteContext<'_>,
	reorderable_items: &[&ast::Item],
	shape: Shape,
	span: Span,
	) -> Option<String> {

https://github.com/rust-lang/rustfmt/blob/master/src/formatting/imports.rs

[jhpratt]

I'll throw out another possibility — why not make the option accept an array? Then users could put local imports first if they so desired, for example. There could be the "Std", "External", and "Crate" options that currently exist, with a "Project" (name TBD) being considered "External" unless explicitly specified. The first three could be mandatory.

[calebcartwright]

Not sure I understand what you're suggesting @jhpratt, could you elaborate?

[jhpratt]

group_imports = ["Std", "External", "Crate"] # currently "StdExternalCrate"

In the above, "Crate" would include a proposed "Project", but it could be specified separately.

group_imports = ["Std", "External", "Project", "Crate"]

This would also let people reorder them however they'd like.

group_imports = ["Crate", "External", "Std", "Project"] # not sure why, but to each their own

[calebcartwright]

Thank you for the additional detail, though I confess that I'm still unclear on the relevance to this particular issue so please let me know if there's simply some dots i'm not connecting!

The type of the option as a string based array vs. enum variants is certainly a discussion we can have, but that feels orthogonal to (a) the fact that rustfmt can't actually make the "project" determination for the reasons discussed above and in other issues, and (b) the request for more granular/multi-groups for external.

There's a need for an implementation to be developed within rustfmt to support the requested style, regardless of whether the type of the option were to be changed; switching the option type from an enum to an array doesn't change that, and if anything would add complexity that would have be resolved first.

[jhpratt]

Just another possibility that would provide more flexibility, I guess. Not 100% relevant to this request admittedly. I'll step aside, noting that I'd use this option if it were supported 🙂

[calebcartwright]

No worries! If you'd like to continue the discussion then I think a new issue would be best, to support respective focused conversations