Auto merge of #12838 - kpreid:restriction-doc, r=llogiq

For restriction lints, replace “Why is this bad?” with “Why restrict this?”

The `restriction` group contains many lints which are not about necessarily “bad” things, but style choices — perhaps even style choices which contradict conventional Rust style — or are otherwise very situational. This results in silly wording like “Why is this bad? It isn't, but ...”, which I’ve seen confuse and distress a newcomer at least once.

To improve this situation, this PR replaces the “Why is this bad?” section heading with “Why restrict this?”, for most, but not all, restriction lints. I left alone the ones whose placement in the restriction group is more incidental.

In order to make this make sense, I had to remove the “It isn't, but” texts from the contents of the sections. Sometimes further changes were needed, or there were obvious fixes to make, and I went ahead and made those changes without attempting to split them into another commit, even though many of them are not strictly necessary for the “Why restrict this?” project; it seemed to me that it was more valuable to grab the low-hanging fruit than to be careful about it.

changelog: rephrased the documentation of `restriction` lints for clarity about their nature

Author: bors

book/src/development/adding_lints.md

@@ -587,6 +587,11 @@ declare_clippy_lint! {
 }
 ```
 
+If the lint is in the `restriction` group because it lints things that are not
+necessarily “bad” but are more of a style choice, then replace the
+“Why is this bad?” section heading with “Why restrict this?”, to avoid writing
+“Why is this bad? It isn't, but ...”.
+
 Once your lint is merged, this documentation will show up in the [lint
 list][lint_list].
 

clippy_dev/src/new_lint.rs

@@ -331,12 +331,17 @@ fn get_lint_file_contents(lint: &LintData<'_>, enable_msrv: bool) -> String {
 }
 
 fn get_lint_declaration(name_upper: &str, category: &str) -> String {
+    let justification_heading = if category == "restriction" {
+        "Why restrict this?"
+    } else {
+        "Why is this bad?"
+    };
     formatdoc!(
         r#"
             declare_clippy_lint! {{
                 /// ### What it does
                 ///
-                /// ### Why is this bad?
+                /// ### {justification_heading}
                 ///
                 /// ### Example
                 /// ```no_run

clippy_lints/src/absolute_paths.rs

@@ -12,7 +12,7 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for usage of items through absolute paths, like `std::env::current_dir`.
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// Many codebases have their own style when it comes to importing, but one that is seldom used
     /// is using absolute paths *everywhere*. This is generally considered unidiomatic, and you
     /// should add a `use` statement.

clippy_lints/src/allow_attributes.rs

@@ -19,10 +19,11 @@ declare_clippy_lint! {
     /// This lint only warns outer attributes (`#[allow]`), as inner attributes
     /// (`#![allow]`) are usually used to enable or disable lints on a global scale.
     ///
-    /// ### Why is this bad?
-    /// `#[expect]` attributes suppress the lint emission, but emit a warning, if
+    /// ### Why restrict this?
+    /// `#[allow]` attributes can linger after their reason for existence is gone.
+    /// `#[expect]` attributes suppress the lint emission, but emit a warning if
     /// the expectation is unfulfilled. This can be useful to be notified when the
-    /// lint is no longer triggered.
+    /// lint is no longer triggered, which may indicate the attribute can be removed.
     ///
     /// ### Example
     /// ```rust,ignore

clippy_lints/src/as_conversions.rs

@@ -17,7 +17,7 @@ declare_clippy_lint! {
     /// There is a good explanation the reason why this lint should work in this way and how it is useful
     /// [in this issue](https://github.com/rust-lang/rust-clippy/issues/5122).
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// `as` conversions will perform many kinds of
     /// conversions, including silently lossy conversions and dangerous coercions.
     /// There are cases when it makes sense to use `as`, so the lint is

clippy_lints/src/asm_syntax.rs

@@ -65,9 +65,8 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for usage of Intel x86 assembly syntax.
     ///
-    /// ### Why is this bad?
-    /// The lint has been enabled to indicate a preference
-    /// for AT&T x86 assembly syntax.
+    /// ### Why restrict this?
+    /// To enforce consistent use of AT&T x86 assembly syntax.
     ///
     /// ### Example
     ///
@@ -114,9 +113,8 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for usage of AT&T x86 assembly syntax.
     ///
-    /// ### Why is this bad?
-    /// The lint has been enabled to indicate a preference
-    /// for Intel x86 assembly syntax.
+    /// ### Why restrict this?
+    /// To enforce consistent use of Intel x86 assembly syntax.
     ///
     /// ### Example
     ///

clippy_lints/src/assertions_on_result_states.rs

@@ -16,23 +16,33 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for `assert!(r.is_ok())` or `assert!(r.is_err())` calls.
     ///
-    /// ### Why is this bad?
-    /// An assertion failure cannot output an useful message of the error.
+    /// ### Why restrict this?
+    /// This form of assertion does not show any of the information present in the `Result`
+    /// other than which variant it isn’t.
     ///
     /// ### Known problems
     /// The suggested replacement decreases the readability of code and log output.
     ///
     /// ### Example
-    /// ```rust,ignore
+    /// ```rust,no_run
     /// # let r = Ok::<_, ()>(());
     /// assert!(r.is_ok());
-    /// # let r = Err::<_, ()>(());
+    /// # let r = Err::<(), _>(());
     /// assert!(r.is_err());
     /// ```
+    ///
+    /// Use instead:
+    ///
+    /// ```rust,no_run
+    /// # let r = Ok::<_, ()>(());
+    /// r.unwrap();
+    /// # let r = Err::<(), _>(());
+    /// r.unwrap_err();
+    /// ```
     #[clippy::version = "1.64.0"]
     pub ASSERTIONS_ON_RESULT_STATES,
     restriction,
-    "`assert!(r.is_ok())`/`assert!(r.is_err())` gives worse error message than directly calling `r.unwrap()`/`r.unwrap_err()`"
+    "`assert!(r.is_ok())` or `assert!(r.is_err())` gives worse panic messages than directly calling `r.unwrap()` or `r.unwrap_err()`"
 }
 
 declare_lint_pass!(AssertionsOnResultStates => [ASSERTIONS_ON_RESULT_STATES]);

clippy_lints/src/attrs/mod.rs

@@ -309,9 +309,9 @@ declare_clippy_lint! {
     ///
     /// (This requires the `lint_reasons` feature)
     ///
-    /// ### Why is this bad?
-    /// Allowing a lint should always have a reason. This reason should be documented to
-    /// ensure that others understand the reasoning
+    /// ### Why restrict this?
+    /// Justifying each `allow` helps readers understand the reasoning,
+    /// and may allow removing `allow` attributes if their purpose is obsolete.
     ///
     /// ### Example
     /// ```no_run

clippy_lints/src/casts/mod.rs

@@ -303,7 +303,7 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for casts of a function pointer to any integer type.
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// Casting a function pointer to an integer can have surprising results and can occur
     /// accidentally if parentheses are omitted from a function call. If you aren't doing anything
     /// low-level with function pointers then you can opt-out of casting functions to integers in
@@ -535,8 +535,8 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for the usage of `as _` conversion using inferred type.
     ///
-    /// ### Why is this bad?
-    /// The conversion might include lossy conversion and dangerous cast that might go
+    /// ### Why restrict this?
+    /// The conversion might include lossy conversion or a dangerous cast that might go
     /// undetected due to the type being inferred.
     ///
     /// The lint is allowed by default as using `_` is less wordy than always specifying the type.

clippy_lints/src/create_dir.rs

@@ -10,8 +10,10 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks usage of `std::fs::create_dir` and suggest using `std::fs::create_dir_all` instead.
     ///
-    /// ### Why is this bad?
-    /// Sometimes `std::fs::create_dir` is mistakenly chosen over `std::fs::create_dir_all`.
+    /// ### Why restrict this?
+    /// Sometimes `std::fs::create_dir` is mistakenly chosen over `std::fs::create_dir_all`,
+    /// resulting in failure when more than one directory needs to be created or when the directory already exists.
+    /// Crates which never need to specifically create a single directory may wish to prevent this mistake.
     ///
     /// ### Example
     /// ```rust,ignore

clippy_lints/src/dbg_macro.rs

@@ -14,7 +14,7 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for usage of the [`dbg!`](https://doc.rust-lang.org/std/macro.dbg.html) macro.
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// The `dbg!` macro is intended as a debugging tool. It should not be present in released
     /// software or committed to a version control system.
     ///

clippy_lints/src/default_numeric_fallback.rs

@@ -22,9 +22,8 @@ declare_clippy_lint! {
     ///
     /// See [RFC0212](https://github.com/rust-lang/rfcs/blob/master/text/0212-restore-int-fallback.md) for more information about the fallback.
     ///
-    /// ### Why is this bad?
-    /// For those who are very careful about types, default numeric fallback
-    /// can be a pitfall that cause unexpected runtime behavior.
+    /// ### Why restrict this?
+    /// To ensure that every numeric type is chosen explicitly rather than implicitly.
     ///
     /// ### Known problems
     /// This lint can only be allowed at the function level or above.

clippy_lints/src/default_union_representation.rs

@@ -10,7 +10,7 @@ declare_clippy_lint! {
     /// ### What it does
     /// Displays a warning when a union is declared with the default representation (without a `#[repr(C)]` attribute).
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// Unions in Rust have unspecified layout by default, despite many people thinking that they
     /// lay out each field at the start of the union (like C does). That is, there are no guarantees
     /// about the offset of the fields for unions with multiple non-ZST fields without an explicitly

clippy_lints/src/disallowed_script_idents.rs

@@ -20,11 +20,11 @@ declare_clippy_lint! {
     /// [aliases]: http://www.unicode.org/reports/tr24/tr24-31.html#Script_Value_Aliases
     /// [supported_scripts]: https://www.unicode.org/iso15924/iso15924-codes.html
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// It may be not desired to have many different scripts for
     /// identifiers in the codebase.
     ///
-    /// Note that if you only want to allow plain English, you might want to use
+    /// Note that if you only want to allow typical English, you might want to use
     /// built-in [`non_ascii_idents`] lint instead.
     ///
     /// [`non_ascii_idents`]: https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#non-ascii-idents

clippy_lints/src/doc/mod.rs

@@ -261,7 +261,7 @@ declare_clippy_lint! {
     /// Checks for the doc comments of publicly visible
     /// safe functions and traits and warns if there is a `# Safety` section.
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// Safe functions and traits are safe to implement and therefore do not
     /// need to describe safety preconditions that users are required to uphold.
     ///

clippy_lints/src/drop_forget_ref.rs

@@ -52,9 +52,10 @@ declare_clippy_lint! {
     /// Checks for usage of `std::mem::forget(t)` where `t` is
     /// `Drop` or has a field that implements `Drop`.
     ///
-    /// ### Why is this bad?
-    /// `std::mem::forget(t)` prevents `t` from running its
-    /// destructor, possibly causing leaks.
+    /// ### Why restrict this?
+    /// `std::mem::forget(t)` prevents `t` from running its destructor, possibly causing leaks.
+    /// It is not possible to detect all means of creating leaks, but it may be desirable to
+    /// prohibit the simple ones.
     ///
     /// ### Example
     /// ```no_run

clippy_lints/src/else_if_without_else.rs

@@ -11,7 +11,7 @@ declare_clippy_lint! {
     /// Checks for usage of if expressions with an `else if` branch,
     /// but without a final `else` branch.
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// Some coding guidelines require this (e.g., MISRA-C:2004 Rule 14.10).
     ///
     /// ### Example

clippy_lints/src/empty_drop.rs

@@ -9,7 +9,7 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for empty `Drop` implementations.
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// Empty `Drop` implementations have no effect when dropping an instance of the type. They are
     /// most likely useless. However, an empty `Drop` implementation prevents a type from being
     /// destructured, which might be the intention behind adding the implementation as a marker.

clippy_lints/src/empty_with_brackets.rs

@@ -11,16 +11,23 @@ declare_clippy_lint! {
     /// ### What it does
     /// Finds structs without fields (a so-called "empty struct") that are declared with brackets.
     ///
-    /// ### Why is this bad?
-    /// Empty brackets after a struct declaration can be omitted.
+    /// ### Why restrict this?
+    /// Empty brackets after a struct declaration can be omitted,
+    /// and it may be desirable to do so consistently for style.
+    ///
+    /// However, removing the brackets also introduces a public constant named after the struct,
+    /// so this is not just a syntactic simplification but an an API change, and adding them back
+    /// is a *breaking* API change.
     ///
     /// ### Example
     /// ```no_run
     /// struct Cookie {}
+    /// struct Biscuit();
     /// ```
     /// Use instead:
     /// ```no_run
     /// struct Cookie;
+    /// struct Biscuit;
     /// ```
     #[clippy::version = "1.62.0"]
     pub EMPTY_STRUCTS_WITH_BRACKETS,
@@ -32,14 +39,20 @@ declare_clippy_lint! {
     /// ### What it does
     /// Finds enum variants without fields that are declared with empty brackets.
     ///
-    /// ### Why is this bad?
-    /// Empty brackets while defining enum variants are redundant and can be omitted.
+    /// ### Why restrict this?
+    /// Empty brackets after a enum variant declaration are redundant and can be omitted,
+    /// and it may be desirable to do so consistently for style.
+    ///
+    /// However, removing the brackets also introduces a public constant named after the variant,
+    /// so this is not just a syntactic simplification but an an API change, and adding them back
+    /// is a *breaking* API change.
     ///
     /// ### Example
     /// ```no_run
     /// enum MyEnum {
     ///     HasData(u8),
-    ///     HasNoData(), // redundant parentheses
+    ///     HasNoData(),       // redundant parentheses
+    ///     NoneHereEither {}, // redundant braces
     /// }
     /// ```
     ///
@@ -48,6 +61,7 @@ declare_clippy_lint! {
     /// enum MyEnum {
     ///     HasData(u8),
     ///     HasNoData,
+    ///     NoneHereEither,
     /// }
     /// ```
     #[clippy::version = "1.77.0"]

clippy_lints/src/endian_bytes.rs

@@ -13,8 +13,9 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for the usage of the `to_ne_bytes` method and/or the function `from_ne_bytes`.
     ///
-    /// ### Why is this bad?
-    /// It's not, but some may prefer to specify the target endianness explicitly.
+    /// ### Why restrict this?
+    /// To ensure use of explicitly chosen endianness rather than the target’s endianness,
+    /// such as when implementing network protocols or file formats rather than FFI.
     ///
     /// ### Example
     /// ```rust,ignore
@@ -31,9 +32,8 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for the usage of the `to_le_bytes` method and/or the function `from_le_bytes`.
     ///
-    /// ### Why is this bad?
-    /// It's not, but some may wish to lint usage of this method, either to suggest using the host
-    /// endianness or big endian.
+    /// ### Why restrict this?
+    /// To ensure use of big endian or the target’s endianness rather than little endian.
     ///
     /// ### Example
     /// ```rust,ignore
@@ -50,9 +50,8 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for the usage of the `to_be_bytes` method and/or the function `from_be_bytes`.
     ///
-    /// ### Why is this bad?
-    /// It's not, but some may wish to lint usage of this method, either to suggest using the host
-    /// endianness or little endian.
+    /// ### Why restrict this?
+    /// To ensure use of little endian or the target’s endianness rather than big endian.
     ///
     /// ### Example
     /// ```rust,ignore

clippy_lints/src/error_impl_error.rs

@@ -12,7 +12,7 @@ declare_clippy_lint! {
     /// ### What it does
     /// Checks for types named `Error` that implement `Error`.
     ///
-    /// ### Why is this bad?
+    /// ### Why restrict this?
     /// It can become confusing when a codebase has 20 types all named `Error`, requiring either
     /// aliasing them in the `use` statement or qualifying them like `my_module::Error`. This
     /// hinders comprehension, as it requires you to memorize every variation of importing `Error`

clippy_lints/src/exhaustive_items.rs

@@ -10,10 +10,10 @@ declare_clippy_lint! {
     /// ### What it does
     /// Warns on any exported `enum`s that are not tagged `#[non_exhaustive]`
     ///
-    /// ### Why is this bad?
-    /// Exhaustive enums are typically fine, but a project which does
-    /// not wish to make a stability commitment around exported enums may wish to
-    /// disable them by default.
+    /// ### Why restrict this?
+    /// Making an `enum` exhaustive is a stability commitment: adding a variant is a breaking change.
+    /// A project may wish to ensure that there are no exhaustive enums or that every exhaustive
+    /// `enum` is explicitly `#[allow]`ed.
     ///
     /// ### Example
     /// ```no_run
@@ -40,10 +40,10 @@ declare_clippy_lint! {
     /// ### What it does
     /// Warns on any exported `struct`s that are not tagged `#[non_exhaustive]`
     ///
-    /// ### Why is this bad?
-    /// Exhaustive structs are typically fine, but a project which does
-    /// not wish to make a stability commitment around exported structs may wish to
-    /// disable them by default.
+    /// ### Why restrict this?
+    /// Making a `struct` exhaustive is a stability commitment: adding a field is a breaking change.
+    /// A project may wish to ensure that there are no exhaustive structs or that every exhaustive
+    /// `struct` is explicitly `#[allow]`ed.
     ///
     /// ### Example
     /// ```no_run