Add `ImportGranularity::ModuleCondensed` #5781

smoelius · 2023-06-15T09:33:41Z

Given that imports_granularity is unstable, is this something you would consider merging?

From the docs I added:

ModuleCondensed:

Like Module, but singleton imports are included in parent modules' use statements where it doesn't introduce nested braces.
use foo::b::{f, g};
use foo::{a, b, c, d::e};
use qux::{h, i};

I have long perceived this to be Clippy's convention. I ran this on Clippy's source code and about 2/3 of the files were left unchanged. Since this supposed convention has been enforced by hand, I consider this confirmation.

smoelius · 2023-06-15T10:01:05Z

@flip1995 Sorry to ping you, but would you be able to comment on whether this is Clippy's preferred convention?

flip1995 · 2023-06-15T10:34:56Z

We don't really have a import convention in Clippy, other than that we don't allow wildcard imports, enforced by one of our pedantic lints.

That being said, I like this convention and would adapt it for Clippy. Especially when 2/3 of Clippy follow it already.

calebcartwright · 2023-06-19T14:39:04Z

is this something you would consider merging?

In principle yes. I don't envision us adding any variants ourselves but we're open to supporting others so long as someone submits the PR like this and it doesn't add any undue maintenance burden for us. We're unlikely to be able to spend any review cycles on this for the foreseeable future though.

Haven't looked at the specifics of the proposed change yet, but would suggest including a #[unstable_variant] attr on the new config variant if you haven't already done so. Stabilization of imports granularity is still a few releases out, but it's definitely on the near term horizon and probably better to go ahead and get that attr on the new variant so that we don't accidentally forget

smoelius1 · 2023-06-20T13:48:27Z

@calebcartwright Thank you very much for your reply.

I am currently travelling, but will add the unstable_variant attribute no later than next week.

smoelius · 2023-06-23T01:03:48Z

Haven't looked at the specifics of the proposed change yet, but would suggest including a #[unstable_variant] attr on the new config variant if you haven't already done so.

Done. Please let me know what else is needed. Thanks.

calebcartwright · 2023-07-03T17:24:32Z

@fee1-dead @CosmicHorrorDev - I know you're both busy (with both Rust and life things 😄), but just tagging this as one for you to potentially review in case you're interested and get around to it before Yacin and/or myself.

I'm in favor of supporting the style associated with the variant, but haven't had a chance to look at the associated implementation

fee1-dead

In addition, could you add unit test cases to test this behavior? thanks.

fee1-dead · 2023-07-04T00:49:33Z

src/imports.rs

+        let mut n = 0;
+        while n < self.path.len() && n < other.path.len() && self.path[n] == other.path[n] {
+            n += 1;
+        }
+        n


Using an iterator e.g. something like below should work better?

Suggested change

let mut n = 0;

while n < self.path.len() && n < other.path.len() && self.path[n] == other.path[n] {

n += 1;

}

n

self.path.iter().zip(other.iter()).take_while(|(a, b)| a == b).count()

fee1-dead · 2023-07-04T01:00:31Z

src/imports.rs

+        // Search front-to-back for a tree to merge the singleton into. If
+        // multiple trees are equally good candidates, prefer the earliest one.


Could you add this as a unit test/comment the part in the ui test that is exercising the "prefer the earliest one" logic?

I added a unit test (with an explanation).

fee1-dead · 2023-07-04T01:06:36Z

src/imports.rs

+    for n in (0..use_trees.len()).rev() {
+        let singleton = &use_trees[n];


I would prefer if you use iterators over using indices, perhaps using iter_mut and into_slice. If that seems too complicated, then it's okay for this to remain using indices.

I'm not opposed to the idea, but I'm having a hard time seeing how it could work (probably my failure of imagination).

My difficulties include:

The inner loop must only consider the trees at indices 0..n. So we would need to somehow recover the bound from the outer loop. (We could use enumerate, but I'm not sure that would be more clear.)

If the inner loop selects a candidate for absorption, the tree at index n gets swap_removed, and swap_remove requires that the index be passed.

I'm assuming you mean that the inner loop should also use an iterator. In that case, the inner loop iterator would need to be mutable. But then wouldn't we run into borrow checker problems, with immutable and mutable iterators trying to refer to the same vector?

fee1-dead · 2023-07-04T01:10:11Z

src/imports.rs

+                continue;
+            }
+            if curr_len < 1
+                || !(curr_tree.is_singleton() || curr_len + 1 == curr_tree.path.len())


This logic is a bit hard to read. Does the below preserve the same behavior?

Suggested change

|| !(curr_tree.is_singleton() || curr_len + 1 == curr_tree.path.len())

|| !curr_tree.is_singleton() && curr_len + 1 != curr_tree.path.len()

Yes, I think so.

smoelius · 2023-07-04T13:18:54Z

In addition, could you add unit test cases to test this behavior? thanks.

Done. (Apologies, as I hadn't noticed the existing unit tests.)

smoelius · 2023-07-04T13:41:51Z

Sorry for the force push (minor grammatical adjustment). It should be good now.

fee1-dead

LGTM

smoelius · 2023-10-05T10:23:46Z

We're unlikely to be able to spend any review cycles on this for the foreseeable future though.

Point very well taken, but I thought I would check in. Any chance of merging this?

smoelius · 2024-01-06T14:05:06Z

I thought I would check in again. Any chance of merging this?

ytmimi

@smoelius I don't think we're ready to move forward with this one just yet. I haven't spent too much time reviewing the details of condense_use_trees, but by looking at the test cases, the way the relative order of imports changes strikes me as odd. Do you think there's a way to better preserve the order?

Also, reorder_imports=true is on by default. can we add test cases where we set reorder_imports=false. I'd want to see what the output looks like in that case.

ytmimi · 2024-01-07T18:48:12Z

src/imports.rs

+    }
+
+    fn is_singleton(&self) -> bool {
+        !matches!(self.path.last().unwrap().kind, UseSegmentKind::List(_))


can we find a way to write this without unwrap()

Sure. (FWIW, that pattern appears elsewhere in the file, though.)

ytmimi · 2024-01-07T18:59:23Z

tests/target/imports/imports_granularity_module_condensed.rs

+use a::g::{h, i};
+use a::j::k::{self, l};
+use a::j::{self, m};
+use a::n::{o::p, q};
+use a::{b::c, d::e, f};
+pub use a::{r::s, t};
+use b::{self, c::d};


This is what we started with:

use a::{b::c, d::e}; use a::{f, g::{h, i}}; use a::{j::{self, k::{self, l}, m}, n::{o::p, q}}; pub use a::{r::s, t}; use b::{c::d, self};

I'm finding it a odd that the relative order of the import got moved around like they did. For example use a::{b::c, d::e}; was the first import, but then it got moved down to the 5th import after merging. I think I would have expected the output to look more like this:

use a::{b::c, d::e, f}; use a::g::{h, i}; use a::j::{self, m}; use a::j::k::{self, l}; use a::n::{o::p, q}; pub use a::{r::s, t}; use b::{self, c::d};

I forget if visibility impacts import sorting. If it doesn't, I also would have expected pub use a::{r::s, t}; to be the second import. Something like this:

use a::{b::c, d::e, f}; pub use a::{r::s, t}; use a::g::{h, i}; use a::j::{self, m}; use a::j::k::{self, l}; use a::n::{o::p, q}; use b::{self, c::d};

For both of the examples given after "I think I would have expected...", vanilla rustfmt reorders them as in the referenced code (playground).

I think this case may be the culprit:

rustfmt/src/imports.rs

Line 955 in 6356fca

(Ident(..), _) => Ordering::Less,

That is, a single identifier (like g) is considered "less" than a list (like {b::c, d::e, f}).

If you agree with my analysis, then maybe the sort order is outside the scope of this PR?

EDIT: Ignoring reorder_imports still needs to be addressed, of course.

smoelius · 2024-01-08T14:42:53Z

Also, reorder_imports=true is on by default. can we add test cases where we set reorder_imports=false. I'd want to see what the output looks like in that case.

I added such a test case. I also took the liberty of adding such a test case for imports_granularity: Module. At least for the given input, the results are the same for both imports_granularity: Module and imports_granularity: ModuleCondensed.

Apologies, as I did not realize there was a reorder_imports option and so did not consider how it would interact with imports_granularity. I will give it some thought.

smoelius · 2024-01-16T11:17:23Z

@ytmimi I think imports_granularity: ModuleCondensed now works sensibly when reorder_imports is off.

Also, it appears that imports_granularity has an effect only when either reorder_imports is on, or group_imports is not Preserve (the default). So I updated the reorder_imports: false tests to use group_imports: One.

smoelius · 2024-03-17T11:14:35Z

@ytmimi Would it be possible to give this PR another look?

ytmimi added the pr-not-reviewed label Jun 21, 2023

Add ImportGranularity::ModuleCondensed

6a1142d

smoelius force-pushed the module-condensed branch from 1646197 to 6a1142d Compare June 23, 2023 01:00

Add unstable_variant attribute

adc8828

flip1995 mentioned this pull request Jul 3, 2023

Add imports_granularity = "Module" to rustfmt.toml rust-lang/rust-clippy#11095

Merged

fee1-dead suggested changes Jul 4, 2023

View reviewed changes

Address review comments

bfa696f

smoelius force-pushed the module-condensed branch from 41fc046 to bfa696f Compare July 4, 2023 13:41

fee1-dead approved these changes Jul 5, 2023

View reviewed changes

ytmimi reviewed Jan 7, 2024

View reviewed changes

smoelius added 2 commits January 8, 2024 04:19

Rewrite is_singleton to not use unwrap

5f96cf1

Add reorder_imports: false tests

660aa46

smoelius added 3 commits January 10, 2024 05:44

Remove unnecessary use_trees.sort() call

5a662f8

Make ModuleCondensed work sensibly when reorder_imports is off

838e998

Add group_imports: One setting and make filenames more consistent

1fde348

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ImportGranularity::ModuleCondensed` #5781

Add `ImportGranularity::ModuleCondensed` #5781

smoelius commented Jun 15, 2023

smoelius commented Jun 15, 2023

flip1995 commented Jun 15, 2023

calebcartwright commented Jun 19, 2023 •

edited

Loading

smoelius1 commented Jun 20, 2023 •

edited

Loading

smoelius commented Jun 23, 2023

calebcartwright commented Jul 3, 2023

fee1-dead left a comment

fee1-dead Jul 4, 2023

smoelius Jul 4, 2023

fee1-dead Jul 4, 2023

smoelius Jul 4, 2023

fee1-dead Jul 4, 2023

smoelius Jul 4, 2023

fee1-dead Jul 4, 2023

smoelius Jul 4, 2023

smoelius commented Jul 4, 2023

smoelius commented Jul 4, 2023 •

edited

Loading

fee1-dead left a comment

smoelius commented Oct 5, 2023

smoelius commented Jan 6, 2024

ytmimi left a comment •

edited

Loading

ytmimi Jan 7, 2024

smoelius Jan 8, 2024

ytmimi Jan 7, 2024

smoelius Jan 10, 2024 •

edited

Loading

smoelius commented Jan 8, 2024

smoelius commented Jan 16, 2024

smoelius commented Mar 17, 2024

		// Search front-to-back for a tree to merge the singleton into. If
		// multiple trees are equally good candidates, prefer the earliest one.

		for n in (0..use_trees.len()).rev() {
		let singleton = &use_trees[n];

	\|\| !(curr_tree.is_singleton() \|\| curr_len + 1 == curr_tree.path.len())
	\|\| !curr_tree.is_singleton() && curr_len + 1 != curr_tree.path.len()

Add ImportGranularity::ModuleCondensed #5781

Are you sure you want to change the base?

Add ImportGranularity::ModuleCondensed #5781

Conversation

smoelius commented Jun 15, 2023

ModuleCondensed:

smoelius commented Jun 15, 2023

flip1995 commented Jun 15, 2023

calebcartwright commented Jun 19, 2023 • edited Loading

smoelius1 commented Jun 20, 2023 • edited Loading

smoelius commented Jun 23, 2023

calebcartwright commented Jul 3, 2023

fee1-dead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoelius commented Jul 4, 2023

smoelius commented Jul 4, 2023 • edited Loading

fee1-dead left a comment

Choose a reason for hiding this comment

smoelius commented Oct 5, 2023

smoelius commented Jan 6, 2024

ytmimi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoelius Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

smoelius commented Jan 8, 2024

smoelius commented Jan 16, 2024

smoelius commented Mar 17, 2024

Add `ImportGranularity::ModuleCondensed` #5781

Add `ImportGranularity::ModuleCondensed` #5781

`ModuleCondensed`:

calebcartwright commented Jun 19, 2023 •

edited

Loading

smoelius1 commented Jun 20, 2023 •

edited

Loading

smoelius commented Jul 4, 2023 •

edited

Loading

ytmimi left a comment •

edited

Loading

smoelius Jan 10, 2024 •

edited

Loading