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

Add ImportGranularity::ModuleCondensed #5781

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion Configurations.md
Original file line number Diff line number Diff line change
Expand Up @@ -1864,7 +1864,7 @@ Similar to other `import` related configuration options, this option operates wi
Note that rustfmt will not modify the granularity of imports containing comments if doing so could potentially lose or misplace said comments.

- **Default value**: `Preserve`
- **Possible values**: `Preserve`, `Crate`, `Module`, `Item`, `One`
- **Possible values**: `Preserve`, `Crate`, `Module`, `ModuleCondensed`, `Item`, `One`
- **Stable**: No (tracking issue: [#4991](https://github.com/rust-lang/rustfmt/issues/4991))


Expand Down Expand Up @@ -1904,6 +1904,16 @@ use foo::{a, b, c};
use qux::{h, i};
```

#### `ModuleCondensed`:

Like `Module`, but singleton imports are included in parent modules' `use` statements where it doesn't introduce nested braces.

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

#### `Item`:

Flatten imports so that each has its own `use` statement.
Expand Down
5 changes: 5 additions & 0 deletions src/config/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ pub enum ImportGranularity {
Crate,
/// Use one `use` statement per module.
Module,
/// Use one `use` statement per module, but merge singleton imports into
/// parent modules' `use` statements where it doesn't introduce nested
/// braces.
#[unstable_variant]
ModuleCondensed,
/// Use one `use` statement per imported item.
Item,
/// Use one `use` statement including all items.
Expand Down
70 changes: 69 additions & 1 deletion src/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ pub(crate) fn normalize_use_trees_with_granularity(
ImportGranularity::Item => return flatten_use_trees(use_trees, ImportGranularity::Item),
ImportGranularity::Preserve => return use_trees,
ImportGranularity::Crate => SharedPrefix::Crate,
ImportGranularity::Module => SharedPrefix::Module,
ImportGranularity::Module | ImportGranularity::ModuleCondensed => SharedPrefix::Module,
ImportGranularity::One => SharedPrefix::One,
};

Expand All @@ -244,9 +244,65 @@ pub(crate) fn normalize_use_trees_with_granularity(
}
}
}

if import_granularity == ImportGranularity::ModuleCondensed {
condense_use_trees(&mut result);
}

result
}

/// Merge singleton imports into parent modules' use-trees where it does't
/// introduce nested braces.
fn condense_use_trees(use_trees: &mut Vec<UseTree>) {
// Sort by path length to put singletons after the trees into which they
// may be absorbed.
use_trees.sort_unstable_by_key(|tree| tree.path.len());

// Search for singletons back-to-front to preserve the just mentioned
// relative order between absorbing trees and singletons. Note that a
// singleton that is found and merged into _some_ tree is `swap_remove`d
// from the `use_trees` vector.
for n in (0..use_trees.len()).rev() {
let singleton = &use_trees[n];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


if !singleton.is_singleton() || singleton.attrs.is_some() || singleton.contains_comment() {
continue;
}

let mut best_index_and_len = None;

// Search front-to-back for a tree to merge the singleton into. If
// multiple trees are equally good candidates, prefer the earliest one.
Comment on lines +282 to +283
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a unit test (with an explanation).

// Like above, these precautions help preserve the relative order
// between absorbing trees and singletons.
for curr_index in 0..n {
let curr_tree = &use_trees[curr_index];
let curr_len = curr_tree.shared_prefix_len(singleton);
if best_index_and_len.map_or(false, |(_, best_len)| best_len >= curr_len) {
continue;
}
if curr_len < 1
|| !(curr_tree.is_singleton() || curr_len + 1 == curr_tree.path.len())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so.

|| curr_tree.attrs.is_some()
|| curr_tree.contains_comment()
|| !curr_tree.same_visibility(singleton)
{
continue;
}
best_index_and_len = Some((curr_index, curr_len));
}

if let Some((best_index, _)) = best_index_and_len {
let singleton = use_trees.swap_remove(n);
use_trees[best_index].merge(&singleton, SharedPrefix::Crate);
}
}

// Put the trees back in their preferred order.
use_trees.sort();
}

fn flatten_use_trees(
use_trees: Vec<UseTree>,
import_granularity: ImportGranularity,
Expand Down Expand Up @@ -669,6 +725,18 @@ impl UseTree {
}
}

fn shared_prefix_len(&self, other: &UseTree) -> usize {
let mut n = 0;
while n < self.path.len() && n < other.path.len() && self.path[n] == other.path[n] {
n += 1;
}
n
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

fn flatten(self, import_granularity: ImportGranularity) -> Vec<UseTree> {
if self.path.is_empty() || self.contains_comment() {
return vec![self];
Expand Down
47 changes: 47 additions & 0 deletions tests/source/imports/imports_granularity_module_condensed.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// rustfmt-imports_granularity: ModuleCondensed

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};

#[cfg(test)]
use foo::{a::b, c::d};
use foo::e;

use bar::{
// comment
a::b,
// more comment
c::d,
e::f,
};

use b::{f::g, h::{i, j} /* After b::h group */};
use b::e;
use b::{/* Before b::l group */ l::{self, m, n::o, p::*}, q};
use b::d;
use b::r; // After b::r
use b::q::{self /* After b::q::self */};
use b::u::{
a,
b,
};
use b::t::{
// Before b::t::a
a,
b,
};
use b::s::{
a,
b, // After b::s::b
};
use b::v::{
// Before b::v::a
a,
// Before b::v::b
b,
};
use b::t::{/* Before b::t::self */ self};
use b::c;
50 changes: 50 additions & 0 deletions tests/target/imports/imports_granularity_module_condensed.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// rustfmt-imports_granularity: ModuleCondensed

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};
Comment on lines +3 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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};

Copy link
Author

@smoelius smoelius Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

(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.


use foo::e;
#[cfg(test)]
use foo::{a::b, c::d};

use bar::{
// comment
a::b,
// more comment
c::d,
e::f,
};

use b::q::{self /* After b::q::self */};
use b::r; // After b::r
use b::s::{
a,
b, // After b::s::b
};
use b::t::{/* Before b::t::self */ self};
use b::t::{
// Before b::t::a
a,
b,
};
use b::u::{a, b};
use b::v::{
// Before b::v::a
a,
// Before b::v::b
b,
};
use b::{c, d, e};
use b::{
f::g,
h::{i, j}, /* After b::h group */
};
use b::{
/* Before b::l group */ l::{self, m, n::o, p::*},
q,
};