Fully rustfmt `use` declarations · Issue #750 · rust-lang/compiler-team (original) (raw)

Proposal

[**Edit**: the MCP has been updated after polling people on Zulip about the options, which uncovered some fairly clear preferences.]

TL;DR This MCP proposes adding these lines to rustfmt.toml and reformatting use declarations for the entire repository:

group_imports = "StdExternalCrate" imports_granularity = "Module"

Problem: inadequate auto-formatting of `use` decls

rustc doesn't have a style guide because rustfmt and tidy theoretically obviate the need for one. In practice there are some style choices left under-specified. One of them is use declarations. By default rustfmt touches use declarations only lightly.

It sorts use declarations within a section, where a section is a series of use declarations without any intervening blank lines. The sorting is self, super, crate, then alphabetical.
It sorts identifiers within any use declaration with braces.
It breaks long lines.

However, it doesn't express any opinion on two matters.

How to use braces, i.e. how much "factoring" of common prefixes is done.
How use declarations should be divided into sections.

This leaves a lot of flexibility and the repository uses a wide variety of styles. Many use item additions can occur in more than one place and require a choice from the author.

For example, if I need to use fruit::Orange and a file already has these lines:

use fruit::{Apple, Grape, Pear}; use fruit::{Kiwi, Plum, Strawberry};

should I insert in the first line, the second, or merge them?

Alternatively has use use sections for crate, rustc_*, and std imports, where should I add something like use tracing::debug;?

These questions get particularly annoying in some large changes like rust-lang/rust#125434.

And attempts to improve even "obviously" badly arranged use declarations can be controversial.

These are all problems that auto-formatting is supposed to solve!

Solution: enable more opinionated rustfmt options

Fortunately rustfmt has some options that can fix the problems. We just need to make some decisions about how to set them, resulting a particular style.

There are various characteristics to consider for this style.

Normalcy: how similar is it to existing code?
Conflict avoidance: i.e. how likely are git conflicts?
Greppability: i.e. can you grep for use.*foo and get reliable hits?
Line-count: how many lines are used?
Char-count: how many characters are used?

Scope

rustfmt.toml applies to the entire repository. Although you can tell rustfmt to ignore certain files or directories, there is no practical way for part of the repository to opt into a subset of rustfmt options. Therefore, this change would apply to everything that rustfmt doesn't ignore, i.e.:

compiler/
library/
src/
- includes bootstrap, librustdoc, compiletest, tidy, plus a few other small tools
- excludes cargo, clippy, miri, rust-analyzer, rustfmt
tests/
- includes only rmake.rs files within this directory

An MCP is appropriate for the compiler changes. I don't know what the equivalent mechanism would be for the other components, or if there even is one. Hopefully this MCP will suffice.

The choices

I have deliberately limited the discussion to options that are implemented.

There are four relevant rustfmt options. They are all unstable, but the relevant tracking issues are all three to five years old so there is some level of maturity.

The proposal is to add group_imports = "One" + imports_granularity = "Module", and leave imports_layout and imports_indent with their default values. Discussion below.

group_imports

This controls how sections are used.

Currently there are two common sectioning strategies in the codebase.

A single section.
Multiple sections. The most common sections are crate, rustc_*, and std, and when sectioning is used they are typically separate, though the order varies. Third-party items (e.g. use tracing::debug;) are less common, and when present they are arranged inconsistently: sometimes in a separate section, sometimes with rustc_*, and sometimes with std. Sometimes pub use items are in their own section. Sometimes use declarations with attributes are in their own section.

There are two choices beyond the unsatisfactory "Preserve" default.

"One" uses a single section for everything. self/super/crate are sorted to the top, everything else is alphabetical. This is ultra-simple and has good normalcy, matching a lot of existing code.
"StdExternalCrate" is a sectioned strategy. It puts std in the first section, self/super/crate in the last section, and everything else in the second section. This has reasonably good normalcy (though a crate/external/std ordering is probably more common in the codebase). The second section ends up with a mixture of local modules, local crates, and external crates because rustfmt doesn't have (and never will have) have enough information to distinguish these cases. #125645

The choice of option doesn't affect conflict avoidance, greppability, or char-count, and barely affects line-count. So it's mostly a matter of normalcy and personal preference.

The proposal is to change to "StdExternalCrate", which received 9 votes. "One" received 4 votes, "no preference" received 3 votes, and "Preserve" (the existing default) used 1 vote. (This was the closest of the four votes.)

It's also worth noting that if there is one or more use sections, followed by one or more other kind of item, followed by one or more additional use section, rustfmt won't rearrange the use declarations around that other kind of item. This happens in numerous places with mod items. It's unclear if such cases should be fixed by moving all the use declarations together, and if so, whether they should come before or after mod items. I recommend leaving such cases as a follow-up.

imports_granularity

This option dictates how much "factoring" of common prefixes is done, and how many things are imported by each individual use item.

This choice is very important, affecting every characteristic. There are four choices beyond the unsatisfactory "Preserve" default.

"One" puts every import into a single use declaration, as long as they have the same visibility and attributes. This ranks terribly for normalcy and greppability, though it is the best for char-count.
"Module" easily ranks highest for normalcy. It's also good for greppability and line-count.
"Crate" is worse on that "Module" on normalcy and and greppability, and quite a bit worse on line-count.
"Item" is an interestingly extreme case. It's optimal for conflict avoidance and greppability, but terrible for normalcy, line-count and char-count, with a lot of repetition that affects readability.

The proposal is to change to "Module", which received 13 votes. "Crate", "Item", and "Preserve" (the default) each received 1 vote.

imports_layout

This only affects how lengthy use declarations are split across multiple lines.

"Mixed" is the default. It is optimal for normalcy.
"Horizontal" can lead to arbitrarily long lines. Highly greppable but deeply non-normal.
"HorizontalVertical" is similar to "Mixed", but if a use doesn't fit on one line then every entry gets its own line. More like the "Block" styling used everywhere else by rustfmt. Slightly worse for line-count, slightly better for conflict avoidance.
"Vertical" puts the final part of every import on its own line. Similar to imports_granularity = "Item", except better for char-count and even worse for line-count -- requires four lines to import two items from the same module!

The proposal is to keep using "Mixed", which received 12 votes. The only other vote was for an option that rustfmt currently doesn't implement: "Horizontal" + splitting long items into multiple items.

imports_indent

This only affects how multi-line use declarations are indented. There are two options.

"Block" is the default. It is optimal for normalcy, is consistent with indent_style and is very Rust-y.
"Visual" is very non-normal, non-Rust-y, and gives inconsistent indentation depths.

The proposal is to keep using "Block", which was the unanimous choice with 13 votes.

Examples

Here are a few examples of the more plausible combinations. I have used a moderately complex collection of real use declarations from rustc, taken from a mixture of files, that demonstrate all the interesting cases.

`group_imports = "StdExternalCrate"` + `imports_granularity = "Module"`

This shows how the "external" section contains local crates (rustc_*), modules (borrow_set) and enums (Nonterminal::*), alongside external crates (smallvec and tracing).

use std::ffi::OsString; use std::io::{BufWriter, Write}; #[cfg(feature = "nightly")] use std::iter::Step; use std::{env, fs, io, iter};

use borrow_set::{BorrowData, BorrowSet}; use rustc_ast as ast; use rustc_ast::visit; use rustc_data_structures::parallel; use rustc_data_structures::steal::Steal; use rustc_errors::PResult; use rustc_expand::base::{ExtCtxt, LintStoreExpand}; use rustc_lint::{unerased_lint_store, BufferedEarlyLint, EarlyCheckNode, LintStore}; use rustc_metadata::creader::CStore; use rustc_middle::dep_graph::DepGraph; use rustc_middle::mir::interpret::{ CheckInAllocMsg, ExpectedKind, InterpError, InvalidMetaKind, InvalidProgramInfo, Misalignment, PointerKind, ResourceExhaustionInfo, UndefinedBehaviorInfo, UnsupportedOpInfo, ValidationErrorInfo, }; use rustc_middle::ty; use rustc_middle::ty::{GlobalCtxt, RegisteredTools, TyCtxt}; use rustc_session::config::{CrateType, Input, OutFileName, OutputFilenames, OutputType}; use rustc_session::cstore::Untracked; use rustc_session::{Limit, Session}; pub use rustc_span::AttrId; use rustc_span::Symbol; use smallvec::{smallvec, SmallVec}; use tracing::debug; pub use Nonterminal::*;

`group_imports = "One"` + `imports_granularity = "Module"`

Note the self/super/crate at the top.

use self::diagnostics::{AccessKind, IllegalMoveOriginKind, MoveError, RegionName}; use self::location::LocationTable; use super::{ImplTraitContext, LoweringContext, ParamMode}; use crate::interface::{Compiler, Result}; use crate::{errors, proc_macro_decls, util}; use borrow_set::{BorrowData, BorrowSet}; use rustc_ast as ast; use rustc_ast::visit; use rustc_data_structures::parallel; use rustc_data_structures::steal::Steal; use rustc_errors::PResult; use rustc_expand::base::{ExtCtxt, LintStoreExpand}; use rustc_lint::{unerased_lint_store, BufferedEarlyLint, EarlyCheckNode, LintStore}; use rustc_metadata::creader::CStore; use rustc_middle::dep_graph::DepGraph; use rustc_middle::mir::interpret::{ CheckInAllocMsg, ExpectedKind, InterpError, InvalidMetaKind, InvalidProgramInfo, Misalignment, PointerKind, ResourceExhaustionInfo, UndefinedBehaviorInfo, UnsupportedOpInfo, ValidationErrorInfo, };
use rustc_middle::ty; use rustc_middle::ty::{GlobalCtxt, RegisteredTools, TyCtxt}; use rustc_session::config::{CrateType, Input, OutFileName, OutputFilenames, OutputType}; use rustc_session::cstore::Untracked; use rustc_session::{Limit, Session}; pub use rustc_span::AttrId; use rustc_span::Symbol; use smallvec::{smallvec, SmallVec}; use std::ffi::OsString; use std::io::{BufWriter, Write}; #[cfg(feature = "nightly")] use std::iter::Step; use std::{env, fs, io, iter}; use tracing::debug; pub use Nonterminal::*;

`group_imports = "One"` + `imports_granularity = "Crate"`

This is more indent-heavy and has a higher line-count than the previous example.

use self::{ diagnostics::{AccessKind, IllegalMoveOriginKind, MoveError, RegionName}, location::LocationTable, }; use super::{ImplTraitContext, LoweringContext, ParamMode}; use crate::{ errors, interface::{Compiler, Result}, proc_macro_decls, util, }; use borrow_set::{BorrowData, BorrowSet}; use rustc_ast as ast; use rustc_ast::visit; use rustc_data_structures::{parallel, steal::Steal}; use rustc_errors::PResult; use rustc_expand::base::{ExtCtxt, LintStoreExpand}; use rustc_lint::{unerased_lint_store, BufferedEarlyLint, EarlyCheckNode, LintStore}; use rustc_metadata::creader::CStore; use rustc_middle::{ dep_graph::DepGraph, mir::interpret::{ CheckInAllocMsg, ExpectedKind, InterpError, InvalidMetaKind, InvalidProgramInfo, Misalignment, PointerKind, ResourceExhaustionInfo, UndefinedBehaviorInfo, UnsupportedOpInfo, ValidationErrorInfo, }, ty, ty::{GlobalCtxt, RegisteredTools, TyCtxt}, }; use rustc_session::{ config::{CrateType, Input, OutFileName, OutputFilenames, OutputType}, cstore::Untracked, Limit, Session, }; pub use rustc_span::AttrId; use rustc_span::Symbol; use smallvec::{smallvec, SmallVec}; #[cfg(feature = "nightly")] use std::iter::Step; use std::{ env, ffi::OsString, fs, io, io::{BufWriter, Write}, iter, }; use tracing::debug; pub use Nonterminal::*;

`group_imports = "One"` + `imports_granularity = "Item"`

This one is really verbose.

use self::diagnostics::AccessKind; use self::diagnostics::IllegalMoveOriginKind; use self::diagnostics::MoveError; use self::diagnostics::RegionName; use self::location::LocationTable; use super::ImplTraitContext; use super::LoweringContext; use super::ParamMode; use crate::errors; use crate::interface::Compiler; use crate::interface::Result; use crate::proc_macro_decls; use crate::util; use borrow_set::BorrowData; use borrow_set::BorrowSet; use rustc_ast::visit; use rustc_ast as ast; use rustc_data_structures::parallel; use rustc_data_structures::steal::Steal; use rustc_errors::PResult; use rustc_expand::base::ExtCtxt; use rustc_expand::base::LintStoreExpand; use rustc_lint::unerased_lint_store; use rustc_lint::BufferedEarlyLint; use rustc_lint::EarlyCheckNode; use rustc_lint::LintStore; use rustc_metadata::creader::CStore; use rustc_middle::dep_graph::DepGraph; use rustc_middle::mir::interpret::CheckInAllocMsg; use rustc_middle::mir::interpret::ExpectedKind; use rustc_middle::mir::interpret::InterpError; use rustc_middle::mir::interpret::InvalidMetaKind; use rustc_middle::mir::interpret::InvalidProgramInfo; use rustc_middle::mir::interpret::Misalignment; use rustc_middle::mir::interpret::PointerKind; use rustc_middle::mir::interpret::ResourceExhaustionInfo; use rustc_middle::mir::interpret::UndefinedBehaviorInfo; use rustc_middle::mir::interpret::UnsupportedOpInfo; use rustc_middle::mir::interpret::ValidationErrorInfo; use rustc_middle::ty::GlobalCtxt; use rustc_middle::ty::RegisteredTools; use rustc_middle::ty::TyCtxt; use rustc_middle::ty; use rustc_session::config::CrateType; use rustc_session::config::Input; use rustc_session::config::OutFileName; use rustc_session::config::OutputFilenames; use rustc_session::config::OutputType; use rustc_session::cstore::Untracked; use rustc_session::Limit; use rustc_session::Session; pub use rustc_span::AttrId; use rustc_span::Symbol; use smallvec::smallvec; use smallvec::SmallVec; use std::env; use std::ffi::OsString; use std::fs; use std::io::BufWriter; use std::io::Write; use std::io; use std::iter; #[cfg(feature = "nightly")] use std::iter::Step; use tracing::debug; pub use Nonterminal::*;

`group_imports = "One"` + `imports_granularity = "Module"` + `imports_layout = "HorizontalVertical"`

Similar to group_imports = "One" + imports_granularity = "Module", but this:

use rustc_middle::mir::interpret::{ CheckInAllocMsg, ExpectedKind, InterpError, InvalidMetaKind, InvalidProgramInfo, Misalignment, PointerKind, ResourceExhaustionInfo, UndefinedBehaviorInfo, UnsupportedOpInfo, ValidationErrorInfo, };

is changed to this:

Mentors or Reviewers

No mentors needed. The resulting PR will be enormous but fully automated, so anyone could review it.

Process

The main points of the Major Change Process are as follows:

File an issue describing the proposal.
A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
- Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
- Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Fully rustfmt use declarations · Issue #750 · rust-lang/compiler-team (original) (raw)