Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up more comments near use declarations #126776

Merged
merged 3 commits into from
Jul 17, 2024
Merged

Clean up more comments near use declarations #126776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9fde25
Merge some `core::iter` entries.
nnethercote Jun 20, 2024
75b6ec9
Avoid comments that describe multiple `use` items.
nnethercote Jun 20, 2024
7f0ccfe
Adjust some comments on individual `use` declarations.
nnethercote Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion compiler/rustc_const_eval/src/interpret/validity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use super::{
Pointer, Projectable, Scalar, ValueVisitor,
};

// for the validation errors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this comment have to go? Can't we have separate import blocks separated by comments any more?

It's not a super important comment, but being able to apply custom grouping via comments seems quite valuable so it would be concerning for rustfmt to just flat-out ignore that. Previously on Zulip it was said that rustmft would only regroup within a contiguous sequence of use statements, which led me to believe it wouldn't regroup across comments here?

Copy link
Contributor Author

@nnethercote nnethercote Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustmft would only regroup within a contiguous sequence of use statements

Depends what you mean by "contiguous sequence". Blank lines and comments don't act as separators between use statements. Other items (e.g. mod, type declarations) do.

And "for the validation errors" seems like a low-value comment to me.

Copy link
Member

@RalfJung RalfJung Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank lines and comments don't act as separators between use statements.

For blank lines that makes sense since the entire point is for rustfmt to do the grouping. But for comments I find this highly disruptive.

And "for the validation errors" seems like a low-value comment to me.

As a maintainer and one of the main authors of this file, I disagree. The imports that carry this comment are unusual, usually we'd not have * imports of an enum at a global level, nor as aliases. So it's worth explaining why we do something strange like that.

Copy link
Member

@RalfJung RalfJung Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it rather concerning that we should entirely lose the ability to add a comment to a group of use statements. The standard library also has several of these cases show up in this PR, where comments that are helpful to structure the file are now being erased because we have a tool that can't deal with them. That's a bad reason to remove comments from code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could take or leave this particular comment, but I do agree that commented groups are useful and would be a shame to lose. Is there any rustfmt setting we can use (or request) to keep that ability?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. rustfmt will, with group_imports = "StdExternalCrate", put all the use declarations into three sections, with a single blank line between them. The whole concept of a single comment applying to multiple lines is suspect within this model. That's the whole point of the first commit in this PR, to get rid of such comments.

The example in question looks like this:

use super::{
    err_ub, format_interp_error, machine::AllocMap, throw_ub, AllocId, AllocKind, CheckInAllocMsg,
    GlobalAlloc, ImmTy, Immediate, InterpCx, InterpResult, MPlaceTy, Machine, MemPlaceMeta, OpTy,
    Pointer, Projectable, Scalar, ValueVisitor,
};

// for the validation errors
use super::InterpError::UndefinedBehavior as Ub; 
use super::InterpError::Unsupported as Unsup;
use super::UndefinedBehaviorInfo::*;
use super::UnsupportedOpInfo::*;

After reformatting, it looks like this:

use super::machine::AllocMap;
// for the validation errors
use super::InterpError::UndefinedBehavior as Ub;
use super::InterpError::Unsupported as Unsup;
use super::UndefinedBehaviorInfo::*;
use super::UnsupportedOpInfo::*;
use super::{
    err_ub, format_interp_error, throw_ub, AllocId, AllocKind, CheckInAllocMsg, GlobalAlloc, ImmTy,
    Immediate, InterpCx, InterpResult, MPlaceTy, Machine, MemPlaceMeta, OpTy, Pointer, Projectable,
    Scalar, ValueVisitor,
};

The comment sticks to the use super::InterpError::UndefinedBehavior as Ub; line, which is a reasonable choice and would be fine if the comment only applied to that line. But rustfmt can't know that it actually applies to four lines. (That fact isn't even perfectly clear to a human reader.) Maybe you could argue that the blank line before the comment should make it clear. But the formatting will be getting rid of unnecessary blank lines. Maybe you could argue that a blank line followed by a comment should be interpreted as the comment applying to multiple lines. Perhaps? But at this point it's worth mentioning that the number of comments on use items in the codebase is actually really small, and the number of comments that apply to multiple use items is even smaller. This PR deals with all of them. It's a couple of dozen comments out of many thousands of use items.

In summary, rustfmt does just fine with like 99.99% of cases and for 0.01% of cases we have a slightly suboptimal experience. It's a standard autoformatting trade-off: we accept the small number of suboptimal cases for all the other benefits, such as consistency and not having to make decisions about how to style things. For this particular case, I could do something like this:

use super::machine::AllocMap;
use super::InterpError::UndefinedBehavior as Ub; // for the validation errors
use super::InterpError::Unsupported as Unsup; // for the validation errors
use super::UndefinedBehaviorInfo::*; // for the validation errors
use super::UnsupportedOpInfo::*; // for the validation errors
use super::{
    err_ub, format_interp_error, throw_ub, AllocId, AllocKind, CheckInAllocMsg, GlobalAlloc, ImmTy,
    Immediate, InterpCx, InterpResult, MPlaceTy, Machine, MemPlaceMeta, OpTy, Pointer, Projectable,
    Scalar, ValueVisitor,
};

which is totally unambiguous, more so than the original comment.

Copy link
Member

@RalfJung RalfJung Jul 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having a group of code with empty lines before and after it, and a comment at the top of it, makes it fairly clear to a human reader that the comment applies to the entire group. But rustfmt is frequently bad at dealing with comments and likes to move them around in ways that change their meaning (rust-lang/rustfmt#3255, rust-lang/rustfmt#3994, rust-lang/rustfmt#5485), so it is no surprise that this happens here, too. It is a constant frustration of mine that issues like that are ignored in the name of the Grand Goal Of Consistency. For basic rustfmt formatting the advantages it provides are so big that the overall tradeoff still comes out in favor of rustfmt (though it is unfortunate that fixing these rustfmt issues is nobody's priority), but for use statements I am not convinced that there's a significant enough problem here worth solving.

In summary, rustfmt does just fine with like 99.99% of cases and for 0.01% of cases we have a slightly suboptimal experience

That "one in 10000" number is complete guesswork on your end. A couple dozen cases in around 3k source files also shows that you are off by around 2 orders of magnitude.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are something like 18,000 use items in compiler/ and library/ alone. Maybe the correct number is 99.9% or 99.5%. No matter what the exact number is, we're having an argument about suboptimal handling of one or two actual comments in the entire codebase. Is that worth blocking this entire change from happening? In my opinion, no.

use super::InterpError::UndefinedBehavior as Ub;
use super::InterpError::Unsupported as Unsup;
use super::UndefinedBehaviorInfo::*;
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_lint/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ use types::*;
use unit_bindings::*;
use unused::*;

/// Useful for other parts of the compiler / Clippy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does seem like a valuable comment being lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. That exact comment could be attached to many re-exports. Why would you re-export something if not because it's useful elsewhere in the compiler or other tools?

pub use builtin::{MissingDoc, SoftLints};
pub use context::{CheckLintNameResult, FindLintError, LintStore};
pub use context::{EarlyContext, LateContext, LintContext};
Expand Down
4 changes: 1 addition & 3 deletions compiler/rustc_pattern_analysis/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ rustc_fluent_macro::fluent_messages! { "../messages.ftl" }

use std::fmt;

// Re-exports to avoid rustc_index version issues.
pub use rustc_index::Idx;
pub use rustc_index::IndexVec;
pub use rustc_index::{Idx, IndexVec}; // re-exported to avoid rustc_index version issues

#[cfg(feature = "rustc")]
use rustc_middle::ty::Ty;
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_smir/src/rustc_internal/internal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//! due to incomplete stable coverage.

// Prefer importing stable_mir over internal rustc constructs to make this file more readable.

use crate::rustc_smir::Tables;
use rustc_middle::ty::{self as rustc_ty, Const as InternalConst, Ty as InternalTy, TyCtxt};
use rustc_span::Symbol;
Expand Down
3 changes: 1 addition & 2 deletions library/alloc/src/testing/crash_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// We avoid relying on anything else in the crate, apart from the `Debug` trait.
use crate::fmt::Debug;
use crate::fmt::Debug; // the `Debug` trait is the only thing we use from `crate::fmt`
use std::cmp::Ordering;
use std::sync::atomic::{AtomicUsize, Ordering::SeqCst};

Expand Down
6 changes: 2 additions & 4 deletions library/core/src/char/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,17 @@ mod convert;
mod decode;
mod methods;

// stable re-exports
#[stable(feature = "try_from", since = "1.34.0")]
pub use self::convert::CharTryFromError;
#[stable(feature = "char_from_str", since = "1.20.0")]
pub use self::convert::ParseCharError;
#[stable(feature = "decode_utf16", since = "1.9.0")]
pub use self::decode::{DecodeUtf16, DecodeUtf16Error};

// perma-unstable re-exports
#[unstable(feature = "char_internals", reason = "exposed only for libstd", issue = "none")]
pub use self::methods::encode_utf16_raw;
pub use self::methods::encode_utf16_raw; // perma-unstable
#[unstable(feature = "char_internals", reason = "exposed only for libstd", issue = "none")]
pub use self::methods::encode_utf8_raw;
pub use self::methods::encode_utf8_raw; // perma-unstable

use crate::ascii;
use crate::error::Error;
Expand Down
8 changes: 4 additions & 4 deletions library/core/src/prelude/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
//!
//! See the [module-level documentation](super) for more.

// No formatting: this file is nothing but re-exports, and their order is worth preserving.
#![cfg_attr(rustfmt, rustfmt::skip)]

// Re-exported core operators
#[stable(feature = "core_prelude", since = "1.4.0")]
#[doc(no_inline)]
Expand Down Expand Up @@ -33,10 +36,7 @@ pub use crate::convert::{AsMut, AsRef, From, Into};
pub use crate::default::Default;
#[stable(feature = "core_prelude", since = "1.4.0")]
#[doc(no_inline)]
pub use crate::iter::{DoubleEndedIterator, ExactSizeIterator};
#[stable(feature = "core_prelude", since = "1.4.0")]
#[doc(no_inline)]
pub use crate::iter::{Extend, IntoIterator, Iterator};
pub use crate::iter::{DoubleEndedIterator, ExactSizeIterator, Extend, IntoIterator, Iterator};
#[stable(feature = "core_prelude", since = "1.4.0")]
#[doc(no_inline)]
pub use crate::option::Option::{self, None, Some};
Expand Down
3 changes: 3 additions & 0 deletions library/core/src/prelude/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
//! This module is imported by default when `#![no_std]` is used in the same
//! manner as the standard library's prelude.

// No formatting: this file is nothing but re-exports, and their order is worth preserving.
#![cfg_attr(rustfmt, rustfmt::skip)]

#![stable(feature = "core_prelude", since = "1.4.0")]

mod common;
Expand Down
5 changes: 2 additions & 3 deletions library/core/src/ptr/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1809,10 +1809,9 @@ pub(crate) const unsafe fn align_offset<T: Sized>(p: *const T, a: usize) -> usiz
// FIXME(#75598): Direct use of these intrinsics improves codegen significantly at opt-level <=
// 1, where the method versions of these operations are not inlined.
use intrinsics::{
assume, cttz_nonzero, exact_div, mul_with_overflow, unchecked_rem, unchecked_sub,
wrapping_add, wrapping_mul, wrapping_sub,
assume, cttz_nonzero, exact_div, mul_with_overflow, unchecked_rem, unchecked_shl,
unchecked_shr, unchecked_sub, wrapping_add, wrapping_mul, wrapping_sub,
};
use intrinsics::{unchecked_shl, unchecked_shr};

/// Calculate multiplicative modular inverse of `x` modulo `m`.
///
Expand Down
26 changes: 13 additions & 13 deletions library/core/src/unicode/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,19 @@
#![unstable(feature = "unicode_internals", issue = "none")]
#![allow(missing_docs)]

// The `pub use` ones are for use in alloc, and are not re-exported in std.

pub(crate) use unicode_data::alphabetic::lookup as Alphabetic;
pub use unicode_data::case_ignorable::lookup as Case_Ignorable;
pub use unicode_data::cased::lookup as Cased;
pub(crate) use unicode_data::cc::lookup as Cc;
pub use unicode_data::conversions;
pub(crate) use unicode_data::grapheme_extend::lookup as Grapheme_Extend;
pub(crate) use unicode_data::lowercase::lookup as Lowercase;
pub(crate) use unicode_data::n::lookup as N;
pub(crate) use unicode_data::uppercase::lookup as Uppercase;
pub(crate) use unicode_data::white_space::lookup as White_Space;
Comment on lines +4 to +15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Is it not possible to separate these between the pub(crate) and pub use set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, rustfmt doesn't consider the visibility when grouping and ordering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it is a requested feature, it seems: rust-lang/rustfmt#5083 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I discovered you can use #[rustfmt::skip] to create arbitrary custom groups, which allows separation of pub use from use if desired. See #127950 for an update to this exact piece of code.


pub(crate) mod printable;
mod unicode_data;

Expand All @@ -16,16 +29,3 @@ mod unicode_data;
/// [Unicode 11.0 or later, Section 3.1 Versions of the Unicode Standard](https://www.unicode.org/versions/Unicode11.0.0/ch03.pdf#page=4).
#[stable(feature = "unicode_version", since = "1.45.0")]
pub const UNICODE_VERSION: (u8, u8, u8) = unicode_data::UNICODE_VERSION;

// For use in alloc, not re-exported in std.
pub use unicode_data::{
case_ignorable::lookup as Case_Ignorable, cased::lookup as Cased, conversions,
};

pub(crate) use unicode_data::alphabetic::lookup as Alphabetic;
pub(crate) use unicode_data::cc::lookup as Cc;
pub(crate) use unicode_data::grapheme_extend::lookup as Grapheme_Extend;
pub(crate) use unicode_data::lowercase::lookup as Lowercase;
pub(crate) use unicode_data::n::lookup as N;
pub(crate) use unicode_data::uppercase::lookup as Uppercase;
pub(crate) use unicode_data::white_space::lookup as White_Space;
1 change: 0 additions & 1 deletion library/std/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,6 @@ pub mod rt;
// The Rust prelude
pub mod prelude;

// Public module declarations and re-exports
#[stable(feature = "rust1", since = "1.0.0")]
pub use alloc_crate::borrow;
#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
1 change: 0 additions & 1 deletion library/std/src/os/fortanix_sgx/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ pub mod usercalls {
pub use crate::sys::abi::usercalls::raw::{do_usercall, Usercalls as UsercallNrs};
pub use crate::sys::abi::usercalls::raw::{Register, RegisterArgument, ReturnValue};

// fortanix-sgx-abi re-exports
pub use crate::sys::abi::usercalls::raw::Error;
pub use crate::sys::abi::usercalls::raw::{
ByteBuffer, Cancel, FifoDescriptor, Return, Usercall,
Expand Down
3 changes: 3 additions & 0 deletions library/std/src/prelude/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
//!
//! See the [module-level documentation](super) for more.

// No formatting: this file is nothing but re-exports, and their order is worth preserving.
#![cfg_attr(rustfmt, rustfmt::skip)]

// Re-exported core operators
#[stable(feature = "rust1", since = "1.0.0")]
#[doc(no_inline)]
Expand Down
3 changes: 3 additions & 0 deletions library/std/src/prelude/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@
//! [book-enums]: ../../book/ch06-01-defining-an-enum.html
//! [book-iter]: ../../book/ch13-02-iterators.html

// No formatting: this file is nothing but re-exports, and their order is worth preserving.
#![cfg_attr(rustfmt, rustfmt::skip)]
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved

#![stable(feature = "rust1", since = "1.0.0")]

mod common;
Expand Down
1 change: 0 additions & 1 deletion library/std/src/rt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#![deny(unsafe_op_in_unsafe_fn)]
#![allow(unused_macros)]

// Re-export some of our utilities which are expected by other crates.
pub use crate::panicking::{begin_panic, panic_count};
pub use core::panicking::{panic_display, panic_fmt};

Expand Down
9 changes: 6 additions & 3 deletions library/std/src/sys/pal/wasi/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,15 @@ pub mod time;
#[deny(unsafe_op_in_unsafe_fn)]
#[allow(unused)]
mod common;

pub use common::*;

mod helpers;
// These exports are listed individually to work around Rust's glob import
// conflict rules. If we glob export `helpers` and `common` together, then
// the compiler complains about conflicts.

// The following exports are listed individually to work around Rust's glob
// import conflict rules. If we glob export `helpers` and `common` together,
// then the compiler complains about conflicts.

pub use helpers::abort_internal;
pub use helpers::decode_error_kind;
use helpers::err2io;
Expand Down
9 changes: 6 additions & 3 deletions library/std/src/sys/pal/wasip2/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,16 @@ pub mod time;
#[deny(unsafe_op_in_unsafe_fn)]
#[allow(unused)]
mod common;

pub use common::*;

#[path = "../wasi/helpers.rs"]
mod helpers;
// These exports are listed individually to work around Rust's glob import
// conflict rules. If we glob export `helpers` and `common` together, then
// the compiler complains about conflicts.

// The following exports are listed individually to work around Rust's glob
// import conflict rules. If we glob export `helpers` and `common` together,
// then the compiler complains about conflicts.

pub use helpers::abort_internal;
pub use helpers::decode_error_kind;
use helpers::err2io;
Expand Down
1 change: 0 additions & 1 deletion library/test/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
#![feature(test)]
#![allow(internal_features)]

// Public reexports
pub use self::bench::{black_box, Bencher};
pub use self::console::run_tests_console;
pub use self::options::{ColorConfig, Options, OutputFormat, RunIgnored, ShouldPanic};
Expand Down
Loading