From fa7dd1c4e0106ce0346236801653188689dbaa43 Mon Sep 17 00:00:00 2001 From: Parker Timmerman Date: Sun, 17 Dec 2023 15:55:06 -0500 Subject: [PATCH] add new lint, pub_underscore_fields - add a new late pass lint, with config options - add ui tests for both variations of config option - update CHANGELOG.md github feedback bump version to 1.77 and run cargo collect-metadata Change `,` to `;` in `conf.rs` --- CHANGELOG.md | 2 + book/src/lint_configuration.md | 10 +++ clippy_config/src/conf.rs | 7 +- clippy_config/src/types.rs | 6 ++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 7 ++ clippy_lints/src/pub_underscore_fields.rs | 83 +++++++++++++++++++ .../all_pub_fields/clippy.toml | 1 + .../exported/clippy.toml | 1 + ...ub_underscore_fields.all_pub_fields.stderr | 60 ++++++++++++++ .../pub_underscore_fields.exported.stderr | 12 +++ .../pub_underscore_fields.rs | 66 +++++++++++++++ .../toml_unknown_key/conf_unknown_key.stderr | 2 + 13 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/pub_underscore_fields.rs create mode 100644 tests/ui-toml/pub_underscore_fields/all_pub_fields/clippy.toml create mode 100644 tests/ui-toml/pub_underscore_fields/exported/clippy.toml create mode 100644 tests/ui-toml/pub_underscore_fields/pub_underscore_fields.all_pub_fields.stderr create mode 100644 tests/ui-toml/pub_underscore_fields/pub_underscore_fields.exported.stderr create mode 100644 tests/ui-toml/pub_underscore_fields/pub_underscore_fields.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index f82421a687b8..0ac7297f8f1b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 7c9a8eb1bfba..3b62ae0524ab 100644 --- a/book/src/lint_configuration.md +++ b/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) + + diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 67ea0af07e6f..77d7f1760c99 100644 --- a/clippy_config/src/conf.rs +++ b/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. diff --git a/clippy_config/src/types.rs b/clippy_config/src/types.rs index df48cc3f5e39..baee09629ac4 100644 --- a/clippy_config/src/types.rs +++ b/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, +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index eae9dfac064e..9dd22135d95c 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 755a4ff525d2..712fe73f6a16 100644 --- a/clippy_lints/src/lib.rs +++ b/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` } diff --git a/clippy_lints/src/pub_underscore_fields.rs b/clippy_lints/src/pub_underscore_fields.rs new file mode 100644 index 000000000000..00465ce43813 --- /dev/null +++ b/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", + ); + } + } + } +} diff --git a/tests/ui-toml/pub_underscore_fields/all_pub_fields/clippy.toml b/tests/ui-toml/pub_underscore_fields/all_pub_fields/clippy.toml new file mode 100644 index 000000000000..95835d101b1d --- /dev/null +++ b/tests/ui-toml/pub_underscore_fields/all_pub_fields/clippy.toml @@ -0,0 +1 @@ +pub-underscore-fields-behavior = "AllPubFields" \ No newline at end of file diff --git a/tests/ui-toml/pub_underscore_fields/exported/clippy.toml b/tests/ui-toml/pub_underscore_fields/exported/clippy.toml new file mode 100644 index 000000000000..94a0d3554bc7 --- /dev/null +++ b/tests/ui-toml/pub_underscore_fields/exported/clippy.toml @@ -0,0 +1 @@ +pub-underscore-fields-behavior = "PublicallyExported" \ No newline at end of file diff --git a/tests/ui-toml/pub_underscore_fields/pub_underscore_fields.all_pub_fields.stderr b/tests/ui-toml/pub_underscore_fields/pub_underscore_fields.all_pub_fields.stderr new file mode 100644 index 000000000000..c6bd499fd8cb --- /dev/null +++ b/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, + | ^^^^^^ + | + = 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, + | ^^^^^^^^^^^^^ + | + = help: consider removing the underscore, or making the field private + +error: aborting due to 7 previous errors + diff --git a/tests/ui-toml/pub_underscore_fields/pub_underscore_fields.exported.stderr b/tests/ui-toml/pub_underscore_fields/pub_underscore_fields.exported.stderr new file mode 100644 index 000000000000..b76f6b3d3882 --- /dev/null +++ b/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 + diff --git a/tests/ui-toml/pub_underscore_fields/pub_underscore_fields.rs b/tests/ui-toml/pub_underscore_fields/pub_underscore_fields.rs new file mode 100644 index 000000000000..1d8fee7daad2 --- /dev/null +++ b/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, + } + + 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, + } + + // 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-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 12828cf9dec5..ed4fb84df1ae 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/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