-
Notifications
You must be signed in to change notification settings - Fork 969
Fix duplicate import removal for imports_granularity="Module" #6677
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
Fix duplicate import removal for imports_granularity="Module" #6677
Conversation
Manishearth
left a comment
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.
In any situation where rustfmt produces a duplicate list, that resultant code will not compile. Thus this change cannot be breaking.
| extern crate foo; | ||
|
|
||
| use std::cell::*; | ||
| use std::{ |
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.
Hmm. Should we have this test at all? This original code won't compile, and now the use tree is much smaller. We should tweak this to import a lot of items
cc @calebcartwright if you have an idea as to what this test was doing before
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.
I still think this should be fixed: let's change this to a test that imports a lot of different items.
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.
rustfmt does still have to work on code that doesn't compile, provided it can still be parsed.
I'm not sure what the thinking was for making that distinction back in the day, but it made sense to me in the context of an inner dev-loop and format on save in editors
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.
Yeah, makes sense. We shouldn't have any stability around that but we should test it I guess.
| borrow, borrow, boxed, boxed, boxed, boxed, boxed, boxed, boxed, boxed, boxed, boxed, char, | ||
| char, char, char, char, char, char, char, char, char, | ||
| }; | ||
| use std::{self, any, ascii, borrow, boxed, char}; |
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.
Wait, this is changing behavior without this flag.
However, this code is broken, so perhaps that's fine?
Fixes #6243 - Strange behavior for duplicate imports with imports_granularity="Module"
Problem
When using
imports_granularity="Module", duplicate imports were being merged but not deduplicated, requiring multiple formatting passes to reach a stable state. This was inconsistent with other granularity settings which properly remove duplicates.Solution
Added
.dedup()calls after sorting import lists in three locations:merge_rest()- when creating merged import listsUseTree::normalize()- when normalizing nested importsmerge_use_trees_inner()- when merging use treesThis ensures duplicate imports are removed in a single formatting pass, matching the behavior of
CrateandItemgranularity settings.Test Changes
Updated
tests/target/multiple.rsto expect deduplicated imports. I believe the previous test was documenting the buggy behavior rather than the expected behavior.Example
Before (unstable, requires 2 passes):
After (stable, single pass):