Condense `use` sections #125062

nnethercote · 2024-05-13T06:50:03Z

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

rustbot · 2024-05-13T06:50:12Z

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

nnethercote · 2024-05-13T07:11:42Z

@bors rollup=never p=1

Because this is highly conflict-prone.

RalfJung · 2024-05-13T09:10:03Z

The interpreter changes LGTM; I don't currently have the capacity to review the entire PR unfortunately.

r? compiler

klensy · 2024-05-13T09:30:08Z

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 :-(

nnethercote · 2024-05-13T09:31:30Z

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

RalfJung · 2024-05-13T09:48:18Z

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.

Zalathar · 2024-05-13T12:19:50Z

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.)

Zalathar · 2024-05-13T12:20:53Z

compiler/rustc_borrowck/src/lib.rs

+use rustc_mir_dataflow::move_paths::{InitIndex, MoveOutIndex, MovePathIndex};
+use rustc_mir_dataflow::move_paths::{InitLocation, LookupResult, MoveData};
+use rustc_mir_dataflow::Analysis;
+use rustc_mir_dataflow::MoveDataParamEnv;


Two pairs of same-module imports that can potentially be merged.

Zalathar · 2024-05-13T12:23:30Z

compiler/rustc_metadata/src/rmeta/mod.rs

+use rustc_macros::{Decodable, Encodable, TyDecodable, TyEncodable};
 use rustc_macros::{MetadataDecodable, MetadataEncodable};


Another pair of same-module imports here.

Zalathar · 2024-05-13T12:24:06Z

compiler/rustc_middle/src/mir/mod.rs

+use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::fx::FxHashSet;


Another pair of same-module imports here.

Zalathar · 2024-05-13T12:30:59Z

compiler/rustc_resolve/src/check_unused.rs

 use crate::module_to_string;
-use crate::Resolver;
+use crate::{LexicalScopeBinding, NameBindingKind, Resolver};


Another pair of same-module imports here.

Zalathar · 2024-05-13T12:31:42Z

compiler/rustc_span/src/lib.rs

+use rustc_data_structures::stable_hasher::HashingControls;
+use rustc_data_structures::stable_hasher::{Hash128, Hash64, HashStable, StableHasher};


Another pair of same-module imports here.

saethlin · 2024-05-13T12:31:57Z

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.

Zalathar · 2024-05-13T12:32:28Z

compiler/rustc_trait_selection/src/traits/project.rs

+use crate::traits::normalize::normalize_with_depth;
+use crate::traits::normalize::normalize_with_depth_to;


Another pair of same-module imports here.

Zalathar · 2024-05-13T12:32:50Z

compiler/rustc_trait_selection/src/traits/select/mod.rs

+use rustc_middle::ty::print::with_no_trimmed_paths;
 use rustc_middle::ty::print::PrintTraitRefExt as _;


Another pair of same-module imports here.

nnethercote · 2024-05-13T22:17:03Z

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?)

saethlin · 2024-05-13T23:18:58Z

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.

nnethercote · 2024-05-13T23:24:54Z

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 :(

saethlin · 2024-05-13T23:39:43Z

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.

bors · 2024-05-14T01:26:02Z

☔ The latest upstream changes (presumably #125076) made this pull request unmergeable. Please resolve the merge conflicts.

saethlin · 2024-05-14T02:01:23Z

as if on cue

RalfJung · 2024-05-14T05:52:54Z

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.

nnethercote · 2024-05-14T06:07:38Z

but rustfmt won't let us.

That is imports_layout="Vertical", AFAICT.

nnethercote · 2024-05-14T06:10:28Z

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.

RalfJung · 2024-05-14T06:10:41Z

That is imports_layout="Vertical", AFAICT.

Ah, TIL! Thanks.

saethlin · 2024-05-14T13:40:06Z

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.

nnethercote · 2024-05-14T21:31:45Z

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.

saethlin · 2024-05-20T14:29:51Z

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.

oli-obk · 2024-05-21T06:21:42Z

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.

nnethercote added 5 commits May 13, 2024 15:07

Put all use rustc_foo::* lines together.

7a4e908

Put all use crate::* lines together.

328f924

Put all use self::* lines together.

65b3577

Put all use super::* lines together.

9db0634

Put all use std::* lines together.

986f2c0

rustbot assigned RalfJung May 13, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 13, 2024

rustbot assigned jieyouxu and unassigned RalfJung May 13, 2024

rustbot assigned saethlin and unassigned jieyouxu May 13, 2024

Zalathar reviewed May 13, 2024

View reviewed changes

nnethercote mentioned this pull request May 23, 2024

Fully rustfmt use declarations rust-lang/compiler-team#750

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Condense `use` sections #125062

Condense `use` sections #125062

nnethercote commented May 13, 2024

rustbot commented May 13, 2024

nnethercote commented May 13, 2024

RalfJung commented May 13, 2024

klensy commented May 13, 2024

nnethercote commented May 13, 2024

RalfJung commented May 13, 2024 •

edited

Zalathar commented May 13, 2024 •

edited

Zalathar May 13, 2024

Zalathar May 13, 2024

Zalathar May 13, 2024

Zalathar May 13, 2024

Zalathar May 13, 2024

saethlin commented May 13, 2024

Zalathar May 13, 2024

Zalathar May 13, 2024

nnethercote commented May 13, 2024

saethlin commented May 13, 2024

nnethercote commented May 13, 2024

saethlin commented May 13, 2024

bors commented May 14, 2024

saethlin commented May 14, 2024

RalfJung commented May 14, 2024 •

edited

nnethercote commented May 14, 2024

nnethercote commented May 14, 2024

RalfJung commented May 14, 2024

saethlin commented May 14, 2024

nnethercote commented May 14, 2024

saethlin commented May 20, 2024

oli-obk commented May 21, 2024

		use rustc_macros::{Decodable, Encodable, TyDecodable, TyEncodable};
		use rustc_macros::{MetadataDecodable, MetadataEncodable};

		use rustc_data_structures::fx::FxHashMap;
		use rustc_data_structures::fx::FxHashSet;

		use rustc_data_structures::stable_hasher::HashingControls;
		use rustc_data_structures::stable_hasher::{Hash128, Hash64, HashStable, StableHasher};

		use crate::traits::normalize::normalize_with_depth;
		use crate::traits::normalize::normalize_with_depth_to;

		use rustc_middle::ty::print::with_no_trimmed_paths;
		use rustc_middle::ty::print::PrintTraitRefExt as _;

Condense use sections #125062

Are you sure you want to change the base?

Condense use sections #125062

Conversation

nnethercote commented May 13, 2024

rustbot commented May 13, 2024

nnethercote commented May 13, 2024

RalfJung commented May 13, 2024

klensy commented May 13, 2024

nnethercote commented May 13, 2024

RalfJung commented May 13, 2024 • edited

Zalathar commented May 13, 2024 • edited

Zalathar May 13, 2024

Choose a reason for hiding this comment

Zalathar May 13, 2024

Choose a reason for hiding this comment

Zalathar May 13, 2024

Choose a reason for hiding this comment

Zalathar May 13, 2024

Choose a reason for hiding this comment

Zalathar May 13, 2024

Choose a reason for hiding this comment

saethlin commented May 13, 2024

Zalathar May 13, 2024

Choose a reason for hiding this comment

Zalathar May 13, 2024

Choose a reason for hiding this comment

nnethercote commented May 13, 2024

saethlin commented May 13, 2024

nnethercote commented May 13, 2024

saethlin commented May 13, 2024

bors commented May 14, 2024

saethlin commented May 14, 2024

RalfJung commented May 14, 2024 • edited

nnethercote commented May 14, 2024

nnethercote commented May 14, 2024

RalfJung commented May 14, 2024

saethlin commented May 14, 2024

nnethercote commented May 14, 2024

saethlin commented May 20, 2024

oli-obk commented May 21, 2024

Condense `use` sections #125062

Condense `use` sections #125062

RalfJung commented May 13, 2024 •

edited

Zalathar commented May 13, 2024 •

edited

RalfJung commented May 14, 2024 •

edited