Auto merge of #10283 - ParkMyCar:lint/pub_underscore_fields, r=blyxyas

feature: add new lint `pub_underscore_fields`

fixes: #10282

This PR introduces a new lint `pub_underscore_fields` that lints when a user has marked a field of a struct as public, but also prefixed it with an underscore (`_`). This is something users should avoid because the two ideas are contradictory. Prefixing a field with an `_` is inferred as the field being unused, but making a field public infers that it will be used.

- \[x] Followed [lint naming conventions][lint_naming]
  - I believe I followed the naming conventions, more than happy to update the naming if I did not :)
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[x] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`

---

changelog: new lint: [`pub_underscore_fields`]
[#10283](#10283)
<!-- changelog_checked -->

CHANGELOG.md

@@ -5483,6 +5483,7 @@ Released 2018-09-13
 [`ptr_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq
 [`ptr_offset_with_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_offset_with_cast
 [`pub_enum_variant_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#pub_enum_variant_names
+[`pub_underscore_fields`]: https://rust-lang.github.io/rust-clippy/master/index.html#pub_underscore_fields
 [`pub_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#pub_use
 [`pub_with_shorthand`]: https://rust-lang.github.io/rust-clippy/master/index.html#pub_with_shorthand
 [`pub_without_shorthand`]: https://rust-lang.github.io/rust-clippy/master/index.html#pub_without_shorthand
@@ -5810,4 +5811,5 @@ Released 2018-09-13
 [`allowed-dotfiles`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allowed-dotfiles
 [`enforce-iter-loop-reborrow`]: https://doc.rust-lang.org/clippy/lint_configuration.html#enforce-iter-loop-reborrow
 [`check-private-items`]: https://doc.rust-lang.org/clippy/lint_configuration.html#check-private-items
+[`pub-underscore-fields-behavior`]: https://doc.rust-lang.org/clippy/lint_configuration.html#pub-underscore-fields-behavior
 <!-- end autogenerated links to configuration documentation -->

book/src/lint_configuration.md

@@ -805,3 +805,13 @@ for _ in &mut *rmvec {}
 * [`missing_errors_doc`](https://rust-lang.github.io/rust-clippy/master/index.html#missing_errors_doc)
 
 
+## `pub-underscore-fields-behavior`
+
+
+**Default Value:** `"PublicallyExported"`
+
+---
+**Affected lints:**
+* [`pub_underscore_fields`](https://rust-lang.github.io/rust-clippy/master/index.html#pub_underscore_fields)
+
+

clippy_config/src/conf.rs

@@ -1,5 +1,5 @@
 use crate::msrvs::Msrv;
-use crate::types::{DisallowedPath, MacroMatcher, MatchLintBehaviour, Rename};
+use crate::types::{DisallowedPath, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour, Rename};
 use crate::ClippyConfiguration;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_session::Session;
@@ -547,6 +547,11 @@ define_Conf! {
     ///
     /// Whether to also run the listed lints on private items.
     (check_private_items: bool = false),
+    /// Lint: PUB_UNDERSCORE_FIELDS
+    ///
+    /// Lint "public" fields in a struct that are prefixed with an underscore based on their
+    /// exported visibility; or whether they are marked as "pub".
+    (pub_underscore_fields_behavior: PubUnderscoreFieldsBehaviour = PubUnderscoreFieldsBehaviour::PublicallyExported),
 }
 
 /// Search for the configuration file.

clippy_config/src/types.rs

@@ -126,3 +126,9 @@ unimplemented_serialize! {
     Rename,
     MacroMatcher,
 }
+
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)]
+pub enum PubUnderscoreFieldsBehaviour {
+    PublicallyExported,
+    AllPubFields,
+}

clippy_lints/src/declared_lints.rs

@@ -576,6 +576,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::ptr::MUT_FROM_REF_INFO,
     crate::ptr::PTR_ARG_INFO,
     crate::ptr_offset_with_cast::PTR_OFFSET_WITH_CAST_INFO,
+    crate::pub_underscore_fields::PUB_UNDERSCORE_FIELDS_INFO,
     crate::pub_use::PUB_USE_INFO,
     crate::question_mark::QUESTION_MARK_INFO,
     crate::question_mark_used::QUESTION_MARK_USED_INFO,

clippy_lints/src/lib.rs

@@ -272,6 +272,7 @@ mod permissions_set_readonly_false;
 mod precedence;
 mod ptr;
 mod ptr_offset_with_cast;
+mod pub_underscore_fields;
 mod pub_use;
 mod question_mark;
 mod question_mark_used;
@@ -571,6 +572,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
         verbose_bit_mask_threshold,
         warn_on_all_wildcard_imports,
         check_private_items,
+        pub_underscore_fields_behavior,
 
         blacklisted_names: _,
         cyclomatic_complexity_threshold: _,
@@ -1081,6 +1083,11 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(uninhabited_references::UninhabitedReferences));
     store.register_late_pass(|_| Box::new(ineffective_open_options::IneffectiveOpenOptions));
     store.register_late_pass(|_| Box::new(unconditional_recursion::UnconditionalRecursion));
+    store.register_late_pass(move |_| {
+        Box::new(pub_underscore_fields::PubUnderscoreFields {
+            behavior: pub_underscore_fields_behavior,
+        })
+    });
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/pub_underscore_fields.rs

@@ -0,0 +1,83 @@
+use clippy_config::types::PubUnderscoreFieldsBehaviour;
+use clippy_utils::attrs::is_doc_hidden;
+use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::is_path_lang_item;
+use rustc_hir::{FieldDef, Item, ItemKind, LangItem};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::impl_lint_pass;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks whether any field of the struct is prefixed with an `_` (underscore) and also marked
+    /// `pub` (public)
+    ///
+    /// ### Why is this bad?
+    /// Fields prefixed with an `_` are inferred as unused, which suggests it should not be marked
+    /// as `pub`, because marking it as `pub` infers it will be used.
+    ///
+    /// ### Example
+    /// ```rust
+    /// struct FileHandle {
+    ///     pub _descriptor: usize,
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// struct FileHandle {
+    ///     _descriptor: usize,
+    /// }
+    /// ```
+    ///
+    /// OR
+    ///
+    /// ```rust
+    /// struct FileHandle {
+    ///     pub descriptor: usize,
+    /// }
+    /// ```
+    #[clippy::version = "1.77.0"]
+    pub PUB_UNDERSCORE_FIELDS,
+    pedantic,
+    "struct field prefixed with underscore and marked public"
+}
+
+pub struct PubUnderscoreFields {
+    pub behavior: PubUnderscoreFieldsBehaviour,
+}
+impl_lint_pass!(PubUnderscoreFields => [PUB_UNDERSCORE_FIELDS]);
+
+impl<'tcx> LateLintPass<'tcx> for PubUnderscoreFields {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
+        // This lint only pertains to structs.
+        let ItemKind::Struct(variant_data, _) = &item.kind else {
+            return;
+        };
+
+        let is_visible = |field: &FieldDef<'_>| match self.behavior {
+            PubUnderscoreFieldsBehaviour::PublicallyExported => cx.effective_visibilities.is_reachable(field.def_id),
+            PubUnderscoreFieldsBehaviour::AllPubFields => {
+                // If there is a visibility span then the field is marked pub in some way.
+                !field.vis_span.is_empty()
+            },
+        };
+
+        for field in variant_data.fields() {
+            // Only pertains to fields that start with an underscore, and are public.
+            if field.ident.as_str().starts_with('_') && is_visible(field)
+                // We ignore fields that have `#[doc(hidden)]`.
+                && !is_doc_hidden(cx.tcx.hir().attrs(field.hir_id))
+                // We ignore fields that are `PhantomData`.
+                && !is_path_lang_item(cx, field.ty, LangItem::PhantomData)
+            {
+                span_lint_and_help(
+                    cx,
+                    PUB_UNDERSCORE_FIELDS,
+                    field.vis_span.to(field.ident.span),
+                    "field marked as public but also inferred as unused because it's prefixed with `_`",
+                    None,
+                    "consider removing the underscore, or making the field private",
+                );
+            }
+        }
+    }
+}

tests/ui-toml/pub_underscore_fields/all_pub_fields/clippy.toml

@@ -0,0 +1 @@
+pub-underscore-fields-behavior = "AllPubFields"

tests/ui-toml/pub_underscore_fields/exported/clippy.toml

@@ -0,0 +1 @@
+pub-underscore-fields-behavior = "PublicallyExported"

tests/ui-toml/pub_underscore_fields/pub_underscore_fields.all_pub_fields.stderr

@@ -0,0 +1,60 @@
+error: field marked as public but also inferred as unused because it's prefixed with `_`
+  --> $DIR/pub_underscore_fields.rs:15:9
+   |
+LL |         pub _b: u8,
+   |         ^^^^^^
+   |
+   = help: consider removing the underscore, or making the field private
+   = note: `-D clippy::pub-underscore-fields` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::pub_underscore_fields)]`
+
+error: field marked as public but also inferred as unused because it's prefixed with `_`
+  --> $DIR/pub_underscore_fields.rs:23:13
+   |
+LL |             pub(in crate::inner) _f: Option<()>,
+   |             ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider removing the underscore, or making the field private
+
+error: field marked as public but also inferred as unused because it's prefixed with `_`
+  --> $DIR/pub_underscore_fields.rs:27:13
+   |
+LL |             pub _g: String,
+   |             ^^^^^^
+   |
+   = help: consider removing the underscore, or making the field private
+
+error: field marked as public but also inferred as unused because it's prefixed with `_`
+  --> $DIR/pub_underscore_fields.rs:34:9
+   |
+LL |         pub _a: usize,
+   |         ^^^^^^
+   |
+   = help: consider removing the underscore, or making the field private
+
+error: field marked as public but also inferred as unused because it's prefixed with `_`
+  --> $DIR/pub_underscore_fields.rs:41:9
+   |
+LL |         pub _c: i64,
+   |         ^^^^^^
+   |
+   = help: consider removing the underscore, or making the field private
+
+error: field marked as public but also inferred as unused because it's prefixed with `_`
+  --> $DIR/pub_underscore_fields.rs:44:9
+   |
+LL |         pub _e: Option<u8>,
+   |         ^^^^^^
+   |
+   = help: consider removing the underscore, or making the field private
+
+error: field marked as public but also inferred as unused because it's prefixed with `_`
+  --> $DIR/pub_underscore_fields.rs:57:9
+   |
+LL |         pub(crate) _b: Option<String>,
+   |         ^^^^^^^^^^^^^
+   |
+   = help: consider removing the underscore, or making the field private
+
+error: aborting due to 7 previous errors
+

tests/ui-toml/pub_underscore_fields/pub_underscore_fields.exported.stderr

@@ -0,0 +1,12 @@
+error: field marked as public but also inferred as unused because it's prefixed with `_`
+  --> $DIR/pub_underscore_fields.rs:15:9
+   |
+LL |         pub _b: u8,
+   |         ^^^^^^
+   |
+   = help: consider removing the underscore, or making the field private
+   = note: `-D clippy::pub-underscore-fields` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::pub_underscore_fields)]`
+
+error: aborting due to 1 previous error
+

tests/ui-toml/pub_underscore_fields/pub_underscore_fields.rs

@@ -0,0 +1,66 @@
+//@revisions: exported all_pub_fields
+//@[all_pub_fields] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/pub_underscore_fields/all_pub_fields
+//@[exported] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/pub_underscore_fields/exported
+
+#![allow(unused)]
+#![warn(clippy::pub_underscore_fields)]
+
+use std::marker::PhantomData;
+
+pub mod inner {
+    use std::marker;
+
+    pub struct PubSuper {
+        pub(super) a: usize,
+        pub _b: u8,
+        _c: i32,
+        pub _mark: marker::PhantomData<u8>,
+    }
+
+    mod inner2 {
+        pub struct PubModVisibility {
+            pub(in crate::inner) e: bool,
+            pub(in crate::inner) _f: Option<()>,
+        }
+
+        struct PrivateStructPubField {
+            pub _g: String,
+        }
+    }
+}
+
+fn main() {
+    pub struct StructWithOneViolation {
+        pub _a: usize,
+    }
+
+    // should handle structs with multiple violations
+    pub struct StructWithMultipleViolations {
+        a: u8,
+        _b: usize,
+        pub _c: i64,
+        #[doc(hidden)]
+        pub d: String,
+        pub _e: Option<u8>,
+    }
+
+    // shouldn't warn on anonymous fields
+    pub struct AnonymousFields(pub usize, i32);
+
+    // don't warn on empty structs
+    pub struct Empty1;
+    pub struct Empty2();
+    pub struct Empty3 {};
+
+    pub struct PubCrate {
+        pub(crate) a: String,
+        pub(crate) _b: Option<String>,
+    }
+
+    // shouldn't warn on fields named pub
+    pub struct NamedPub {
+        r#pub: bool,
+        _pub: String,
+        pub(crate) _mark: PhantomData<u8>,
+    }
+}

tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

@@ -49,6 +49,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
            missing-docs-in-crate-items
            msrv
            pass-by-value-size-limit
+           pub-underscore-fields-behavior
            semicolon-inside-block-ignore-singleline
            semicolon-outside-block-ignore-multiline
            single-char-binding-names-threshold
@@ -124,6 +125,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
            missing-docs-in-crate-items
            msrv
            pass-by-value-size-limit
+           pub-underscore-fields-behavior
            semicolon-inside-block-ignore-singleline
            semicolon-outside-block-ignore-multiline
            single-char-binding-names-threshold