Option to create groups for std, external crates, and other imports #4445

MattX · 2020-09-30T22:02:56Z

This adds a configuration flag, reorder_imports_opinionated, that causes existing import groups to be discarded and replaced by three groups for standard imports, other imports, and self/super/crate imports.

The option name isn't great, we can change it if there are better ideas.

I know @calebcartwright raised a possible issue in #4107 (comment), but I'm not sure if it is still a blocker.

Resolves #4107.

calebcartwright · 2020-10-01T02:42:50Z

Thanks for the PR @MattX! Excited to see this one. I don't think I'll have a chance to give this a review tonight, but a few initial thoughts for you:

I think we'll want to use a more rich type for the config option instead of a simple boolean, for example an enum where each variant can represent a set of the groupings/order (even if there's only two variants in the beginning). I could see different camps wanting this feature albeit with different ordering and only having a boolean option would make that tricky
We probably need to bikeshed on the name a bit. To me it's more of a imports_groupings or group_imports or import_grouping_strategy or something else, but I don't think we want to have the word opinionated in there
Let's add some additional tests that incorporate this new option in conjunction with the various other import-related options to make sure there's no unexpected/undesired behavior when using multiple options together

calebcartwright · 2020-10-01T02:43:58Z

I know @calebcartwright raised a possible issue in #4107 (comment), but I'm not sure if it is still a blocker.

The bug with comment association will be a blocker for being able to stabilize this option, but not a blocker for being able to make it available on nightly as an unstable option

MattX · 2020-10-01T22:35:18Z

Thanks for the feedback!

I've renamed the option to group_imports for now. I'm happy to change it to anything else.
I've made the parameter into an enum. Not sure how to describe the current regrouping behavior, so I've called it StdExternalCrate for now. Also happy to change that.
I've added two tests to check the interaction with merge_imports and reorder_imports.

The way I've implemented it, this new option has no effect when reorder_imports is false. This is more because it's more straightforward to implement than because it makes sense. After all, it seems possible to group imports without sorting them within each group (but I don't know if this is really something people want).

In any case, it might make sense to add a warning or error when reorder_imports = false and group_imports != None. I didn't see existing machinery to validate the config this way, so I haven't added this for now.

src/formatting/reorder.rs

calebcartwright · 2020-10-04T00:39:12Z

Thanks for the updates, I like the new name better.

The way I've implemented it, this new option has no effect when reorder_imports is false. This is more because it's more straightforward to implement than because it makes sense. After all, it seems possible to group imports without sorting them within each group (but I don't know if this is really something people want).

I was wondering about this one. I don't think we'll be able to couple the two options together and have this one only be operational one another has a certain value. Logically it makes sense that the majority of folks that would want an enforced grouping (and a sorting of those groups) would want to have the elements sorted within that group, but it's almost inevitable that there will be some folks that want to use their own/different sorting within the groups.

I imagine it'll add a bit of complexity but it's something we'll need to do

(Sorry for the PR close/reopen, accidentally hit the wrong button)

calebcartwright · 2020-10-04T00:45:34Z

Configurations.md

+## `group_imports`
+
+Discard existing import groups, and create three groups for:
+1. `std`, `core` and `alloc`,
+2. external crates,
+3. `self`, `super` and `crate` imports.
+
+Within each group, imports are sorted as with `reorder_imports`.
+
+This has no effect is `reorder_imports` is `false`.
+
+- **Default value**: `None`
+- **Possible values**: `None`, `StdExternalCrate`
+- **Stable**: No
+
+#### `None` (default):


Let's keep the description of the option generic about what the option does/what it controls as opposed to describing the behavior of a particular supported variant (e.g. Controls the strategy for how imports are grouped together...)

And given the denotations of None within Rust, I'd suggest we use Preserve instead, which will also be consistent with a few other rustfmt config option variants

MattX · 2020-10-04T15:10:55Z

Thanks for the feedback! I was actually able to decouple ordering and grouping without much difficulty. I've also renamed None to Preserve.

calebcartwright · 2020-10-06T01:32:35Z

src/config.rs

@@ -78,6 +78,8 @@ create_config! {
    imports_indent: IndentStyle, IndentStyle::Block, false, "Indent of imports";
    imports_layout: ListTactic, ListTactic::Mixed, false, "Item layout inside a import block";
    merge_imports: bool, false, false, "Merge imports";
+    group_imports: GroupImportsTactic, GroupImportsTactic::Preserve, false,
+        "Reorganize import groups";


Minor nit but could we update this with something similar to what was changed in the Configurations.md file, like Controls the strategy for how imports are grouped together?

calebcartwright · 2020-10-06T01:34:26Z

src/formatting/reorder.rs

@@ -227,19 +229,34 @@ fn rewrite_reorderable_items(
            if context.config.merge_imports() {
                normalized_items = merge_use_trees(normalized_items);
            }
-            normalized_items.sort();


I'll confess this gave me pause at first glance, but see why we had to wrap the sorting behind the reorder_imports check here now since we can get here even if reorder_imports false with non-Preservation group import strategies.

calebcartwright

Excellent work @MattX, thank you! One super minor item left inline, but otherwise this is good to go from my perspective

tests/source/group_imports-merge_imports.rs

calebcartwright

👍

calebcartwright · 2020-10-06T02:32:13Z

Everything LGTM to me @MattX. Going to hold off a bit on merging though so @topecongiro can weigh in since this is a new config option

MattX · 2020-10-06T02:37:15Z

Sounds good!

MattX · 2020-10-06T14:08:44Z

Once it's reviewed, any chance this can be tagged as hacktoberfest-accepted? I want my t-shirt :p

topecongiro

Thank you for the PR! I think that the current implementation is good enough for the first round of implementing this feature, except for the inline comment. Would you mind fixing it and adding a test?

topecongiro · 2020-10-14T11:15:54Z

src/formatting/reorder.rs


-            wrap_reorderable_items(context, &item_vec, nested_shape)
+            Some(item_vec.join("\n\n"))


We need to respect the current indentation here to indent inside inner modules correctly. E.g.,

mod test { use crate::foo::bar; use std::path; }

Good catch, I've added code to handle this case but I don't know if it's the best way to do things. Let me know if it's not.

MattX · 2020-10-27T00:16:52Z

Looks like the rustc_ap_rustc_data_structures crate is failing to compile in integ tests. The tests work for me locally (I'm probably using a different minor version?). I'll retry the checks when this is fixed.

calebcartwright · 2020-10-27T02:35:40Z

There's a broken toolstate issue on nightly at the moment that requires us to bump the rustc-ap crates here. Those crates (snapshots of rustc internals) are published once a week, and I'm waiting to grab the next one which should be done in ~16 hours or so.

calebcartwright · 2020-10-27T22:58:59Z

crates bumped, you should be able to rebase now

It might actually be better to have a three-way option.

Tests for interaction with `no_reorder` and `merge_imports`.

Also reword configuration to be less specific.

MattX · 2020-11-16T16:26:39Z

Hey, just pinging on this to make sure it gets merged before it's too outdated. Let me know if there's anything that still needs to be done on my end.

calebcartwright · 2020-11-16T23:23:53Z

@topecongiro going to go ahead and merge as the change you requested appears to have been resolved

calebcartwright · 2020-11-16T23:26:29Z

@MattX - note that while this has been merged, and thus immediately available for anyone building rustfmt 2.0 from source, it's not available in the version of rustfmt distributed via rustup as that's on a different branch.

If you'd like to see this new feature supported in a released version of rustfmt, that will need to be backported to a target 1.x branch (1.4.27 at the moment). If you're interested in such a PR please let us know, otherwise I'll try to take a look at cherry-picking/backporting at some point down the road

MattX · 2020-11-17T00:05:38Z

Ok! I'm opening a new PR for 1.4.27.

MattX changed the title ~~Option to create groups for std, local create, and other imports~~ Option to create groups for std, local crate, and other imports Oct 1, 2020

MattX force-pushed the master branch from b097fb4 to 364981f Compare October 1, 2020 22:29

MattX commented Oct 1, 2020

View reviewed changes

src/formatting/reorder.rs Show resolved Hide resolved

calebcartwright closed this Oct 4, 2020

calebcartwright reopened this Oct 4, 2020

calebcartwright reviewed Oct 4, 2020

View reviewed changes

MattX force-pushed the master branch from c1d5bad to ee577df Compare October 4, 2020 15:04

MattX requested a review from calebcartwright October 4, 2020 16:20

calebcartwright reviewed Oct 6, 2020

View reviewed changes

tests/source/group_imports-merge_imports.rs Outdated Show resolved Hide resolved

MattX force-pushed the master branch 2 times, most recently from 0da8517 to ee1acb5 Compare October 6, 2020 02:23

MattX changed the title ~~Option to create groups for std, local crate, and other imports~~ Option to create groups for std, external crates, and other imports Oct 6, 2020

calebcartwright approved these changes Oct 6, 2020

View reviewed changes

calebcartwright requested a review from topecongiro October 6, 2020 02:31

calebcartwright added the hacktoberfest-accepted label Oct 6, 2020

topecongiro suggested changes Oct 14, 2020

View reviewed changes

MattX requested a review from topecongiro October 28, 2020 00:21

MattX added 12 commits October 27, 2020 20:25

Add config option.

38056e4

It might actually be better to have a three-way option.

Add test case for opinionated reordering

5ac3b72

Opinionated reordering

d0fe771

Update Documentation.md examples

ce79ded

Rename tests

a469976

Removed temp test

eba49fe

Rename reorder_imports_opinionated -> group_imports

16fd87b

Add extra tests

4f39fc4

Tests for interaction with `no_reorder` and `merge_imports`.

Decouple reordering and grouping

93b7cc3

Change None -> Preserve for group_imports config

ec219b5

Also reword configuration to be less specific.

Move test files; change description wording.

a0b4a3b

Handle indented import groups

f8f55c7

MattX force-pushed the master branch from f648f23 to f8f55c7 Compare October 28, 2020 00:25

kaiwk mentioned this pull request Nov 5, 2020

Format 'use declaration' in a more readable way #4513

Closed

calebcartwright added the pr-ready-to-merge label Nov 10, 2020

calebcartwright merged commit 17d90ca into rust-lang:master Nov 16, 2020

MattX mentioned this pull request Nov 17, 2020

[Backport] Option to create groups for imports #4531

Merged

thomaseizinger mentioned this pull request Dec 30, 2020

[unstable option] merge_imports #3362

Closed

karyon added the 1x-backport:completed label Oct 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to create groups for std, external crates, and other imports #4445

Option to create groups for std, external crates, and other imports #4445

MattX commented Sep 30, 2020

calebcartwright commented Oct 1, 2020

calebcartwright commented Oct 1, 2020

MattX commented Oct 1, 2020

calebcartwright commented Oct 4, 2020 •

edited

Loading

calebcartwright Oct 4, 2020

MattX commented Oct 4, 2020

calebcartwright Oct 6, 2020

calebcartwright Oct 6, 2020

calebcartwright left a comment

calebcartwright left a comment

calebcartwright commented Oct 6, 2020

MattX commented Oct 6, 2020

MattX commented Oct 6, 2020

topecongiro left a comment

topecongiro Oct 14, 2020

MattX Oct 27, 2020

MattX commented Oct 27, 2020

calebcartwright commented Oct 27, 2020

calebcartwright commented Oct 27, 2020

MattX commented Nov 16, 2020

calebcartwright commented Nov 16, 2020

calebcartwright commented Nov 16, 2020

MattX commented Nov 17, 2020


		wrap_reorderable_items(context, &item_vec, nested_shape)
		Some(item_vec.join("\n\n"))

Option to create groups for std, external crates, and other imports #4445

Option to create groups for std, external crates, and other imports #4445

Conversation

MattX commented Sep 30, 2020

calebcartwright commented Oct 1, 2020

calebcartwright commented Oct 1, 2020

MattX commented Oct 1, 2020

calebcartwright commented Oct 4, 2020 • edited Loading

calebcartwright Oct 4, 2020

Choose a reason for hiding this comment

MattX commented Oct 4, 2020

calebcartwright Oct 6, 2020

Choose a reason for hiding this comment

calebcartwright Oct 6, 2020

Choose a reason for hiding this comment

calebcartwright left a comment

Choose a reason for hiding this comment

calebcartwright left a comment

Choose a reason for hiding this comment

calebcartwright commented Oct 6, 2020

MattX commented Oct 6, 2020

MattX commented Oct 6, 2020

topecongiro left a comment

Choose a reason for hiding this comment

topecongiro Oct 14, 2020

Choose a reason for hiding this comment

MattX Oct 27, 2020

Choose a reason for hiding this comment

MattX commented Oct 27, 2020

calebcartwright commented Oct 27, 2020

calebcartwright commented Oct 27, 2020

MattX commented Nov 16, 2020

calebcartwright commented Nov 16, 2020

calebcartwright commented Nov 16, 2020

MattX commented Nov 17, 2020

calebcartwright commented Oct 4, 2020 •

edited

Loading