From c9a2555a577f63608cbbeb41c553f302d3d6a5d2 Mon Sep 17 00:00:00 2001 From: Parker Timmerman Date: Mon, 16 Oct 2023 09:22:48 -0400 Subject: [PATCH] add a new lint, pub_underscore_fields - add a new late pass lint - add ui tests - update CHANGELOG.md --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/pub_underscore_fields.rs | 82 +++++++++++++++++++++++ tests/ui/pub_underscore_fields.rs | 58 ++++++++++++++++ tests/ui/pub_underscore_fields.stderr | 52 ++++++++++++++ 6 files changed, 196 insertions(+) create mode 100644 clippy_lints/src/pub_underscore_fields.rs create mode 100644 tests/ui/pub_underscore_fields.rs create mode 100644 tests/ui/pub_underscore_fields.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index fef25ad8635a..b7b3feb25e0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5357,6 +5357,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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 481c44031cf7..65fbb41445b6 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -561,6 +561,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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0f35ec276657..01b28e86ab8e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -271,6 +271,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; @@ -1123,6 +1124,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv()))); store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter)); + store.register_late_pass(|_| Box::new(pub_underscore_fields::PubUnderscoreFields)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/pub_underscore_fields.rs b/clippy_lints/src/pub_underscore_fields.rs new file mode 100644 index 000000000000..7fe03f2e2f4f --- /dev/null +++ b/clippy_lints/src/pub_underscore_fields.rs @@ -0,0 +1,82 @@ +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_middle::ty::Visibility; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +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.75.0"] + pub PUB_UNDERSCORE_FIELDS, + pedantic, + "struct field prefixed with underscore and marked public" +} +declare_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<'_>| { + let parent = cx.tcx.parent_module_from_def_id(field.def_id); + let grandparent = cx.tcx.parent_module_from_def_id(parent.into()); + let visibility = cx.tcx.visibility(field.def_id); + + let case_1 = parent == grandparent && !field.vis_span.is_empty(); + let case_2 = visibility != Visibility::Restricted(parent.to_def_id()); + + case_1 || case_2 + }; + + 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", + ); + } + } + } +} diff --git a/tests/ui/pub_underscore_fields.rs b/tests/ui/pub_underscore_fields.rs new file mode 100644 index 000000000000..60d9822f5243 --- /dev/null +++ b/tests/ui/pub_underscore_fields.rs @@ -0,0 +1,58 @@ +#![allow(unused)] +#![warn(clippy::pub_underscore_fields)] + +use std::marker::PhantomData; + +mod inner { + use std::marker; + + pub struct PubSuper { + pub(super) a: usize, + pub(super) _b: u8, + _c: i32, + pub _mark: marker::PhantomData, + } + + mod inner2 { + pub struct PubModVisibility { + pub(in crate::inner) e: bool, + pub(in crate::inner) _f: Option<()>, + } + } +} + +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, + } + + // 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, + } + + // shouldn't warn on fields named pub + pub struct NamedPub { + r#pub: bool, + _pub: String, + pub(crate) _mark: PhantomData, + } +} diff --git a/tests/ui/pub_underscore_fields.stderr b/tests/ui/pub_underscore_fields.stderr new file mode 100644 index 000000000000..b24119a17816 --- /dev/null +++ b/tests/ui/pub_underscore_fields.stderr @@ -0,0 +1,52 @@ +error: field marked as public but also inferred as unused because it's prefixed with `_` + --> $DIR/pub_underscore_fields.rs:11:9 + | +LL | pub(super) _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:19: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:26: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:33: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:36:9 + | +LL | pub _e: 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:49:9 + | +LL | pub(crate) _b: Option, + | ^^^^^^^^^^^^^ + | + = help: consider removing the underscore, or making the field private + +error: aborting due to 6 previous errors +