fix: make imports_granularity One preserve aliases - #7007
Conversation
Fix `imports_granularity = "One"` to preserve aliases.
In the following example by sivizius:
```rust
pub use foo::x;
pub use foo::x as x2;
pub use foo::y;
use bar::a;
use bar::b;
use bar::b::f;
use bar::b::f as f2;
use bar::b::g;
use bar::c;
use bar::d::e;
use bar::d::e as e2;
use qux::h;
use qux::i;
use qux::i as j;
```
`bar::b::f as f2`; and `qux::i as j`; were silently dropped, returning
this merged result:
```rust
pub use foo::{x, x as x2, y};
use {
bar::{
a,
b::{self, f, g},
c,
d::{e, e as e2},
},
qux::{h, i},
};
```
Two import paths that only differ by the alias of their last segment were
being treated as equal when merging, keeping only one of the two names. They
now get merged into a list containing both, e.g., `qux::{h, i, i as j}`.
This also fixes two related problems:
- Merging `use qux::h;` followed by `use qux as Q;` produced the
invalid `use qux as Q::{self as Q, h};` because the merged root kept
the alias of the shorter path.
- The result of merging depends on the order in which the `use`
trees get visited, and once aliases are preserved a single pass over
inputs like `use a; use a as b; use a::c;` did not, produce a stable
result. `a::{self as b}` is now normalized to the equivalent
`a as b` when flattening, and merging is repeated until it reaches a
fixed point, so that formatting stays idempotent.
Fixes: rust-lang#6027
rustbot
added
the
S-waiting-on-review
|
@rustbot ready |
| // The result of merging can depend on the order in which the trees | ||
| // were encountered, in which case a single pass is not stable | ||
| // (see #6027). Keep re-merging until we reach a fixed point so that | ||
| // formatting stays idempotent. This normally converges after one | ||
| // extra pass. The cap at four iterations is just a safeguard against | ||
| // oscillation. |
There was a problem hiding this comment.
I'm not sure I fully understand what this comment means. Since we're walking the AST don't we encounter the trees in the order that they're listed in the source code? Can you give me an example in which the order changes the output.
Also, I personally think we should come up with an approach that doesn't involve running merge_use_trees multiple times until we reach some stable formatting. We should come up with a way so that we have stable formatting after one pass.
There was a problem hiding this comment.
What I mean by the order in which trees are encountered is that the instability is across formatting runs, not during a single AST walk. Here's an example from my initial testing:
use a as b;
use a;
use a::c;Running once (to be fair, in an intermediate version of my code) produced:
use {
a as b,
a::{self, c},
};Running again on this already-formatted (by the first run) code produced:
use a::{self as b, self, c};After the first run, use a; only exists inside a::{self, c}, so the second run is merging a::self, a different tree than the a the first run saw, and lands on a different grouping.
Either way, I agree this is fragile and I'm working on a different approach to canonicalize when a batch contains both use a as b; and a deeper use a::... where we rewrite as a::{self as b} before merging and then do it in one pass. I'll try to get something up tonight. Thanks!
There was a problem hiding this comment.
No rush to get this out tonight. I probably won't be able to look at this again until later this week. Thanks for continuing to look into this.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
3 participants
Fix
imports_granularity = "One"to preserve aliases.In the following example by sivizius:
bar::b::f as f2; andqux::i as j; were silently dropped, returning this merged result:Two import paths that only differ by the alias of their last segment were being treated as equal when merging, keeping only one of the two names. They now get merged into a list containing both, e.g.,
qux::{h, i, i as j}.This also fixes two related problems:
use qux::h;followed byuse qux as Q;produced the invaliduse qux as Q::{self as Q, h};because the merged root kept the alias of the shorter path.usetrees get visited, and once aliases are preserved a single pass over inputs likeuse a; use a as b; use a::c;did not, produce a stable result.a::{self as b}is now normalized to the equivalenta as bwhen flattening, and merging is repeated until it reaches a fixed point, so that formatting stays idempotent.Fixes: #6027