Skip to content

imports_granularity = "Module" with path contains self #4716

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 12, 2021
Merged

imports_granularity = "Module" with path contains self #4716

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35da0c1
Fix for issue 4681 - Module imports_granularity and self in path
davidBar-On Feb 22, 2021
21b59a7
Change wrong rustfmt-imports_granularity Item in test file to Module
davidBar-On Feb 24, 2021
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
14 changes: 13 additions & 1 deletion src/formatting/imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,19 @@ pub(crate) fn merge_use_trees(use_trees: Vec<UseTree>, merge_by: SharedPrefix) -
continue;
}

for flattened in use_tree.flatten() {
for mut flattened in use_tree.flatten() {
if merge_by == SharedPrefix::Module {
Copy link
Member

Choose a reason for hiding this comment

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

Any ideas why this isn't needed for the Crate variant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand, the following code transforms ahead foo::self to foo:

rustfmt/src/formatting/imports.rs

Lines 474 to 477 in 0e90855

// Normalise foo::self -> foo.
if let UseSegment::Slf(None) = last {
if !self.path.is_empty() {
return self;

Therefore, the only reason foo::self can be present in the code you asked about, is that flatten() created foo::self from foo::{self, bar}, in addition to creating foo::bar. For SharedPrefix::Module these too imports will remain separate, so {} should be added to self. However for SharedPrefix::Crate they will be merged again. This is because how share_prefix() identify shared prefix:

rustfmt/src/formatting/imports.rs

Lines 570 to 573 in 0e90855

match shared_prefix {
SharedPrefix::Crate => self.path[0] == other.path[0],
SharedPrefix::Module => {
self.path[..self.path.len() - 1] == other.path[..other.path.len() - 1]

When checking this I found two examples of imports formatting which I am not sure are correct. The first is when using imports_granularity = "Crate":

use foo;
use foo::{self};

is formatted as:

use foo::{
    self, {self},
};

Is that o.k. or it should have been use foo::{self}?

The second possible formatting issue is when using imports_granularity = "Module":

use foo;
use bar;

is formatted as:

use {bar, foo};

Is this o.k.?

// If a path ends in `::self`, rewrite it to `::{self}`.
if let Some(UseSegment::Slf(..)) = flattened.path.last() {
let self_segment = flattened.path.pop().unwrap();
flattened
.path
.push(UseSegment::List(vec![UseTree::from_path(
vec![self_segment],
DUMMY_SP,
)]));
}
}
if let Some(tree) = result
.iter_mut()
.find(|tree| tree.share_prefix(&flattened, merge_by))
Expand Down
6 changes: 6 additions & 0 deletions tests/source/issue-4681-imports_granularity_default.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
use crate::lexer;
use crate::lexer::tokens::TokenData;
use crate::lexer::{tokens::TokenData};
use crate::lexer::self;
use crate::lexer::{self};
use crate::lexer::{self, tokens::TokenData};
8 changes: 8 additions & 0 deletions tests/source/issue-4681-imports_granularity_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// rustfmt-imports_granularity: Item
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, although it kinda turned out well that you did this as it highlights a lack of dedupe we have with the Item variant (something separate)

Suggested change
// rustfmt-imports_granularity: Item
// rustfmt-imports_granularity: Module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Fixed. (Probably was a last minute change as I did most of the tests by setting the configuration in the .toml file)

Note the test file changes, as with Module duplicates are removed in the formatted output, so made sure the related output is from the use crate::lexer::{self, tokens::TokenData} source line.


use crate::lexer;
use crate::lexer::tokens::TokenData;
use crate::lexer::{tokens::TokenData};
use crate::lexer::self;
use crate::lexer::{self};
use crate::lexer::{self, tokens::TokenData};
6 changes: 6 additions & 0 deletions tests/target/issue-4681-imports_granularity_default.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
use crate::lexer;
use crate::lexer;
use crate::lexer::tokens::TokenData;
use crate::lexer::tokens::TokenData;
use crate::lexer::{self};
use crate::lexer::{self, tokens::TokenData};
9 changes: 9 additions & 0 deletions tests/target/issue-4681-imports_granularity_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// rustfmt-imports_granularity: Item

use crate::lexer;
use crate::lexer;
use crate::lexer::tokens::TokenData;
use crate::lexer::tokens::TokenData;
use crate::lexer::tokens::TokenData;
use crate::lexer::{self};
use crate::lexer::{self};