rust
rust copied to clipboard
rust-lang
Condense `use` sections
use items can be broken (or not) into sections in many ways: crate items, self items, std items, rustc_* items, third-party crate items. Also pub vs non-pub, and items with/without attributes (e.g. #[cfg(parallel_compiler)]. rustfmt is very unopinionated about this and, as a result, there is little consistency within the rustc codebase on the right way to do it.
However, for vanilla use items (i.e. no pub or attributes) it never makes sense for use crate::a to be in a different section to use crate::b, or use rustc_foo::a to be in a different section to use rustc_bar::b, etc. This PR fixes all such cases that I could find. I tried to each change in the simple/obvious way -- e.g. if there is a single use std::foo by itself in a section and then three use std::... items in another section then I moved the single one into the group of three -- but there were a few cases where there was no clear move and judgment was needed.
r? @RalfJung
Some changes occurred in coverage instrumentation.
cc @Zalathar
Some changes occurred in compiler/rustc_codegen_gcc
cc @antoyo, @GuillaumeGomez
This PR changes MIR
cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras
This PR changes Stable MIR
cc @oli-obk, @celinval, @ouz-a
Some changes occurred in compiler/rustc_codegen_cranelift
cc @bjorn3
Some changes occurred in exhaustiveness checking
cc @Nadrieril
Some changes occurred to the CTFE / Miri engine
cc @rust-lang/miri
Some changes occurred to the CTFE / Miri engine
cc @rust-lang/miri
Some changes occurred to MIR optimizations
cc @rust-lang/wg-mir-opt
The interpreter changes LGTM; I don't currently have the capacity to review the entire PR unfortunately.
r? compiler
Once i tried to use https://rust-lang.github.io/rustfmt/?version=master&search=#group_imports StdExternalCrate but that given truly big diff, so i given up :-(
I don't currently have the capacity to review the entire PR unfortunately.
I have seen you say this on other PRs. I admit I find it surprising. You are regularly in the top three contributors to each Rust release as measured by number of commits, and often the top contributor. Oh well.
r? @saethlin
All commits in Miri get mirrored over to rustc, so this isn't painting an adequate picture. Most of my work is on Miri.
I'm also trying to reduce the amount of time I spend on Rust, for my sanity's sake. It's not working out too well so far, but certainly I shouldn't increase the scope of what I review. Instead I should focus on contributing where I can make the biggest difference -- Miri and unsafe code things.
Each individual change looks reasonable to me.
I spotted a few more opportunities to merge same-module imports, but I appreciate the desire to not scope-creep too much, especially for such a rot-prone change. 😅
(EDIT: This is on the assumption that merging same-module imports in this PR is OK. If it turns out to not be, then disregard my suggestions.)
I am fine with moving entire lines around, but I do not want to see changes that turn this style:
use rustc_crate::a;
use rustc_crate::b;
Into this:
use rustc_crate::{a, b};
The latter style is extremely prone to merge conflicts, because any two PRs that want to add or remove an import from rust_crate will conflict with each other. Importing each item on its own line uses a considerable amount of space at the top of each file for imports, but really does result in less merge conflicts.
I am fine with moving entire lines around, but I do not want to see changes that turn this style:
use rustc_crate::a; use rustc_crate::b;Into this:
use rustc_crate::{a, b};
Huh, I didn't expect that. Just to make sure I'm understanding, in general your preferred style would be for something like this:
use rustc_span::{sym, Span, SpanDecoder, SpanEncoder, Symbol, DUMMY_SP};
To instead be this:
use rustc_span::sym;
use rustc_span::Span;
use rustc_span::SpanDecoder;
use rustc_span::SpanEncoder;
use rustc_span::Symbol;
use rustc_span::DUMMY_SP;
Is that right? (I understand you're not advocating for proactively splitting up existing lines, but over time as natural use churn occurs is that the style you would prefer to emerge?)
Is that right? (I understand you're not advocating for proactively splitting up existing lines, but over time as natural use churn occurs is that the style you would prefer to emerge?)
Yes you understand me completely.
Huh. The one-import-per-line style is very far from the current state of rustc!
So on one side I have @saethlin asking for no merging of mergeable lines, and on the other side I have @Zalathar pointing out all the places where I missed merging mergeable lines.
rustfmt really missed a trick in allowing so much flexibility here :(
Yeah. For what it's worth, I prefer the look of imports_granularity = "Crate", but getting kicked out of the queue because I wanted to add an import from the same module as someone else just wins out.
:umbrella: The latest upstream changes (presumably #125076) made this pull request unmergeable. Please resolve the merge conflicts.
I did have to fix my share of merge conflicts in import lines... these are usually fairly trivial to fix. I don't think it is so bad as to warrant splitting all the imports, which makes them a lot more verbose and harder to parse.
We could do this
use rustc_span::{
sym,
Span,
SpanDecoder,
SpanEncoder,
Symbol,
DUMMY_SP,
};
to reduce verbosity and merge conflicts, but rustfmt won't let us.
as if on cue
You are right that one-import-line greatly reduces the likelihood of merge commits. But multiple-imports-per-line is the style used in the overwhelming majority of cases in the compiler's existing code. It feels odd to have changes rejected for conforming to the predominant style.
It would be really nice if imports_layout and group_imports were stable and reliable and we could just choose a value for each and let this be handled automatically.
It feels odd to have changes rejected for conforming to the predominant style.
Has the team ever agreed on a style? I'm willing to +1 the majority opinion (as opposed to consensus), I'm just not aware of any discussion of the matter.
There is no style guide because rustfmt and tidy theoretically avoid the need for one, though theory and practice don't align entirely, as this PR indicates.
When I say "predominant" style I just mean what the existing code does. When it comes to section ordering, for example, there are a variety of styles and no single one dominates. But within sections, multiple imports per line is overwhelmingly common. Single import per line is extremely rare, and you can see in this PR both @Zalathar and I naturally gravitated to merging imports where possible. And the PR description is premised on multiple imports per line being non-controversial.
I'm not going to sign off on changes which I think are net-negative on the basis of "it looks like that's how things are being done right now" as opposed to a conscious team decision. I've seen way too many things like this result in an outcome the team does not like because individuals keep going with "it looks like this is how we do things".
If you want someone to sign off on this, re-roll for another reviewer or start a team discussion.
I also don't think we should do this. Either we decide on rustfmt settings as a team, or we keep the current chaotic system. But doing a manual reformatting towards some sort of goal is not improving things in my eyes.
