-
Notifications
You must be signed in to change notification settings - Fork 888
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
imports_granularity = "Module"
with path contains self
#4716
imports_granularity = "Module"
with path contains self
#4716
Conversation
@@ -0,0 +1,8 @@ | |||
// rustfmt-imports_granularity: Item |
There was a problem hiding this comment.
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)
// rustfmt-imports_granularity: Item | |
// rustfmt-imports_granularity: Module |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Will go ahead and merge so we can get the issue fixed, but if you're interested in take a follow up look I think it might be worthwhile to see if there's pieces that can be leveraged between the Module
and Crate
variant, since the latter obviously doesn't need this post correction
@@ -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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.?
Suggested fix for #4681: when
imports_granularity = "Module"
, the formatting output ofuse crate::lexer::{self, tokens::TokenData}
includes nowcrate::lexer::{self}
instead ofcrate::lexer::self
.The change in
merge_use_trees()
is based on the code inflatten_use_trees()
that handles similar issue.