diff --git a/CHANGELOG.md b/CHANGELOG.md index 7beb4eb17d46..9d7894e87f11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5423,6 +5423,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_config/src/conf.rs b/clippy_config/src/conf.rs index 88611eb70878..53abb359f95a 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 38fda36c58ac..097bacf190e0 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -574,6 +574,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 61935dc7e3c5..38079cd0b5e4 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; @@ -570,6 +571,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: _, @@ -1080,6 +1082,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..21a7b46d6b02 --- /dev/null +++ b/clippy_lints/src/pub_underscore_fields.rs @@ -0,0 +1,88 @@ +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 clippy_utils::source::snippet_opt; +use rustc_hir::{FieldDef, Item, ItemKind, LangItem}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::impl_lint_pass; +use rustc_span::Span; + +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.76.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 => start_with_pub(cx, field.vis_span), + }; + + 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", + ); + } + } + } +} + +fn start_with_pub(cx: &LateContext<'_>, span: Span) -> bool { + snippet_opt(cx, span) + .map(|s| s.as_str().starts_with("pub")) + .unwrap_or(false) +} 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/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.publically_exported.stderr b/tests/ui-toml/pub_underscore_fields/pub_underscore_fields.publically_exported.stderr new file mode 100644 index 000000000000..b76f6b3d3882 --- /dev/null +++ b/tests/ui-toml/pub_underscore_fields/pub_underscore_fields.publically_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..1590e68e13ee --- /dev/null +++ b/tests/ui-toml/pub_underscore_fields/pub_underscore_fields.rs @@ -0,0 +1,67 @@ +//@revisions: publically_exported all_pub_fields +//@[publically_exported] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/pub_underscore_fields/ +//@[publically_exported] publically_exported +//@[all_pub_fields] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/pub_underscore_fields/all_pub_fields + +#![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/pub_underscore_fields/publically_exported/clippy.toml b/tests/ui-toml/pub_underscore_fields/publically_exported/clippy.toml new file mode 100644 index 000000000000..94a0d3554bc7 --- /dev/null +++ b/tests/ui-toml/pub_underscore_fields/publically_exported/clippy.toml @@ -0,0 +1 @@ +pub-underscore-fields-behavior = "PublicallyExported" \ No newline at end of file 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