Fully rustfmt use declarations

[nnethercote]

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::*;

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};

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:

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

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.

You can read more about Major Change Proposals on forge.

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.

[rustbot]

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.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

@rustbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rustbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler @rust-lang/compiler-contributors

[nnethercote]

rust-lang/rust#125443 has a draft implementation with the recommended options, for those who want to see what it looks like on the whole repository.

[oli-obk]

@rustbot second

[RalfJung]

I think there have been enough discussions on Zulip that should be considered before just accepting this proposal as-is.

[RalfJung]

Ah, I see the MCP has been updated based on the discussion. A shame that we don't get diffs for that.

[oli-obk]

you can click the edited button on github to see history, but yea, it doesn't allow aggregating the diffs if there are many small ones

[apiraino]

@rustbot label -final-comment-period +major-change-accepted

[nnethercote]

Update: just before I originally planned to merge this change in rust-lang/rust#125443, I found a minor issue with comments on use declarations. Imagine you have this:

// A lovely import.
use crate::ast;
use rustc_parser::parser;
use std::fmt;

rustfmt will rearrange this to this with the configuration we've chosen:

use std::fmt;

use rustc_parser::parser;

// A lovely import.
use crate::ast;

The comment that applied to the use crate::ast; sticks to it, which is good.

Now what about this?

// Three lovely imports.
use crate::ast;
use rustc_parser::parser;
use std::fmt;

This is almost identical to the previous case, but the comment clearly is intended to apply to multiple use declarations, not just the one immediately below. rustfmt can't be expected to understand this, so it does the same rearrangement as before, which mucks up the comment's intention.

There are many tens of thousands of use declarations in the codebase, and in rust-lang/rust#126776 I rearranged the ~15 affected comments of this kind. Comments relating to multiple use declarations are more or less incompatible with automated reordering of use declarations. (It's a little similar to how doc comments can only apply to a single item.) Well, perhaps rustfmt could be taught to handle some special cases, e.g. if comments are used in combination with blank lines. But currently it doesn't have that, and an ideal design seems difficult and not worth waiting for. It's the standard auto-formatting trade-off: it does a fine job the vast majority of the time; occasionally you get a sub-optimal case; but overall it's well worth it for the consistency and time saved from not having to format by hand and then argue about that formatting.

Based on all the above, I intend to update and merge rust-lang/rust#125443 soon, hopefully at a time when the merge queue is quiet.

[RalfJung]

That's this rustfmt issue: rust-lang/rustfmt#6241. I think it is very unfortunate that we should lose the ability to comment a group of imports. Seems like something around 1 in a 100 files is affected by this. (Counting individual use statements here is a misleading metric IMO.)

overall it's well worth it for the consistency and time saved from not having to format by hand and then argue about that formatting.

I don't recall ever losing noticeable time to format this by hand, and there were no arguments that I am aware of until you started them by deciding that we should set group_imports -- an unstable rustfmt flag that's considered experimental for a reason. As far as I am concerned, there was no problem with our use imports that needed fixing, it's all nice-to-have. Was the situation perfect? No. But it wasn't nearly as bad as you make it sound. The time spent on the arguments you started is around 100x the time I spent worrying about use in the compiler in the last 5 years.

So there's a very clear alternative that also basically entirely avoids having to argue about use formatting, which is to just keep doing what we did for the last years. No need to get out the big hammer of imperfect formatting tools that impede our ability to structure and comment the code in a meaningful way, for the situations where that is appropriate.

You are repeating the usual arguments for doing any formatting at all, but I don't think they apply here. This is about a small subset of the code that's rarely read carefully; our current inconsistent use order is not a hurdle for readability -- making this very different from poorly formatted regular code. The auto-formatting options we have available are not even ready for stable Rust yet. If the formatting had no major downside, I wouldn't mind very much, but we discovered a major downside now that IMO clearly outweighs the benefits.

[nnethercote]

Ralf: on the question of motivation, I described that in the MCP description above. Inconsistent use declarations might not be a problem for you, but your experience is not universal. I touch a lot of different files, and it the inconsistent formatting has been an ongoing papercut for me for years. I only learned that rustfmt had options for formatting use declarations recently, and I filed this MCP shortly after learning about it because it will be a clear quality of life improvement for me.

More generally, I have followed (and gone beyond) the required procedures for making a change of this kind.

• I filed this MCP. At the time of writing this is has seven "thumbs up" reactions and two "heart" reactions.
• I filed rustfmt use declarations rust#125443 on the same day so everybody could see waht the changes looked like in advance. It touched so many files that almost every who could be notified was notified. At the time of writing it has three "heart" reactions.
• The Zulip thread was auto-posted. There were multiple favourable responses, e.g.:
  • @davidtwco: "I think this is a good change... I've often wanted this to be uniform and consistent throughout the codebase."
  • @jieyouxu: "I like this. I really don't care which use style is picked, having one consistent style that can be automatically formatted saves me from having to think about it."
  • @oli-obk: "I for one love the One style, but I understand it's not everybody's preferred option (to state it mildly). StdExternalCrate seems like a good compromise that is already used manually a lot in some form."
• I posted the polls on the options in the Zulip thread, which 20 people voted in. I then update the group_imports proposal in the MCP from One to StdExternalModule (something you argued for strongly at the time).
• The MCP was approved.
• The PR received r+ from three different people, covering compiler, libs, and rustdoc.
• Just before merging, I noticed minor issues with comments. I postponed the merge and addressed them in Clean up some comments near use declarations rust#126717 and Clean up more comments near use declarations rust#126776, both of which received r+ and were merged.
• I gave a status update. At the time of writing, that comment has three "thumbs up" reactions and one "thumbs down" reaction (from you).
• I linked to that explanation in the PR.
• All this has played out over almost two months.

Your opposition to this has been strong from the start, at first about the original group_imports = "One" proposal (which was later changed to "StdExternalCrate", which you had advocated for), and now about doing anything at all. But there is no other notable opposition and plenty of support, and the project does not run on unanimity.

I contend that all procedures have been completed and this is appropriate to merge. I expect you will say that the comment-on-multiple-declarations issue is a showstopper that invalidates much or all of what went before. I disagree; although we have lost one way to write a comment describing a group of imports, rust-lang/rust#126776 demonstrates there are multiple good alternatives, and comments like that are rare.

The two of us have been doing most of the talking. I would like to hear thoughts from other people.

[saethlin]

The two of us have been doing most of the talking. I would like to hear thoughts from other people.

I'm not surprised that a formatting change has provoked so much arguing, but I'm still disappointed at how much energy has been spent on this topic. There are decisions made in the Rust organization that I do not fully agree with, but I try to defer to the wisdom of my peers and the group's overall preferences, because that's just part of being on a team.

The time spent on the arguments you started is around 100x the time I spent worrying about use in the compiler in the last 5 years.

Every time I touch a use statement (which is quite often) I spend some time adjusting it, wondering if someone will ask me to change it to one style or another. Same when I add an import. For the amount of time I've invested in this topic, the formatting will on net save me time and mental effort. In my mind this is the fundamental benefit of an autoformatter: I don't always like what it does, but I get to stop thinking about formatting and dealing with code review comments about it.

but we discovered a major downside

I do not agree that this is a major downside. I looked at the comments that @nnethercote deleted, and I didn't see anything of significant value being lost.

[nnethercote]

I looked at the comments that @nnethercote deleted

(I want to let others talk, but I will say one brief thing to pre-empt possible confusion: I did delete two comments in rust-lang/rust#126776 that I thought were low-value, but deleting them was not the only option. There are ways they could have been preserved. See the "post-merge update" in this comment for details on the comment-on-multiple-declarations issue.)

[RalfJung]

your experience is not universal

Fully agreed. The same goes for you.

I touch a lot of different files, and it the inconsistent formatting has been an ongoing papercut for me for years. I only learned that rustfmt had options for formatting use declarations recently, and I filed this MCP shortly after learning about it because it will be a clear quality of life improvement for me.

I also touch a lot of files and never spent any noticeable time thinking about this. I find it hard to say which of these positions is more representative.

on the question of motivation, I described that in the MCP description above

Yes, and I notice that both of the PRs you are referencing for concrete evidence that there is a problem in practice here are by you. I understand that you are bothered by this inconsistency. All else being equal, I would also prefer consistent ordering over the current situation! But as rust-lang/rust#126717 and rust-lang/rust#126717 show, all else is not equal. Increasingly it becomes clear why this is still an experimental option in rustfmt.

More generally, I have followed (and gone beyond) the required procedures for making a change of this kind.

Yes you did, I never claimed anything else. I appreciate that this is a lot of work, and I thank you for constructively engaging in discussion throughout this. I was okay with the decision that was eventually made -- I may have still preferred another outcome, but the downsides I was aware of at the time were not significant enough to justify spending more time on this.

But most of the discussion happened without us being aware that we'd lose the ability to comment blocks of imports, and for me that tipped the scale back from "harmless" to "rather not". I am not the only one who considers that issue relevant -- @cuviper also spoke up in rust-lang/rust#126776 with similar concerns. @workingjubilee was disappointed by another aspect of rustfmt formatting, one that would also be a non-issue if we could just have a comment above the pub(crate) use block and therefore prevent it from being mixed with the rest of the use statements.

I contend that all procedures have been completed and this is appropriate to merge. I expect you will say that the comment-on-multiple-declarations issue is a showstopper that invalidates much or all of what went before. I disagree;

No, I was not going to say that. I was going to say that this is new evidence and we should make sure that there is still consensus in the compiler team to do this. If that is the case, I will concede that I am in a minority position here and accept that the team has different priorities/preferences.

although we have lost one way to write a comment describing a group of imports, rust-lang/rust#126776 demonstrates there are multiple good alternatives, and comments like that are rare.

I do not think that you have presented a good alternative. The one alternative I recall you suggesting is to duplicate the comment on each use line; I don't consider that acceptable. It breaks up the group and spreads the use statements across the entire import section. (In that particular case it happened that the four affected use statements are adjacent alphabetically, but there's no reason to assume that will be the case in other situations.)

The only good alternative I could come up with so far is to add a mod spacer {} dummy module between the block of use statements with the comment, and the rest, to prevent rustfmt from reordering things it should not reorder. I am not sure if people would accept such a patch though.

Yes, comments like that are rare, but we're talking about formatting code that almost everyone scrolls over most of the time anyway. The benefit of formatting that is, in my view, very small, so it doesn't take a big downside to tip the scale.

@saethlin

I'm not surprised that a formatting change has provoked so much arguing, but I'm still disappointed at how much energy has been spent on this topic. There are decisions made in the Rust organization that I do not fully agree with, but I try to defer to the wisdom of my peers and the group's overall preferences, because that's just part of being on a team.

When, after making a decision, new evidence relevant for this decision surfaces, then we should make sure that that does not change the group's opinion. This is part of our normal decision-making process -- the lang team may accept an RFC, but then during implementation new information surfaces, and that may well change the team's position. So I don't think it is fair to characterize my opposition here as trying to overthrow established team consensus; I am saying there is new evidence and so we shouldn't blindly go ahead with a decision made before this evidence was known.

[workingjubilee]

tbh I feel like most of this discussion is a symptom of the thought of PRing rustfmt being kinda anxiety inducing, because it has simultaneously very low review capacity and very strict stability promises. probably even stricter than the actual language's, honestly.

[Noratrieb]

May I ask y'all to move to the zulip thread that rustbot explicitly asks y'all to move to?

[kornelski]

This feature has worsened merge conflicts caused by use statements, because different PRs adding lines in different places are now more likely to be forced into the same multi-line merged statement.

[oli-obk]

May I ask y'all to move to the zulip thread that rustbot explicitly asks y'all to move to?