diff --git a/CHANGELOG.md b/CHANGELOG.md index f593966c0459..1e0ff5db0ee7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4134,6 +4134,7 @@ Released 2018-09-13 [`panic_in_result_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_in_result_fn [`panic_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_params [`panicking_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#panicking_unwrap +[`partial_pub_fields`]: https://rust-lang.github.io/rust-clippy/master/index.html#partial_pub_fields [`partialeq_ne_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_ne_impl [`partialeq_to_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_to_none [`path_buf_push_overwrite`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_buf_push_overwrite diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index de1253c8510a..799c62743aaa 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -479,6 +479,7 @@ store.register_lints(&[ panic_unimplemented::TODO, panic_unimplemented::UNIMPLEMENTED, panic_unimplemented::UNREACHABLE, + partial_pub_fields::PARTIAL_PUB_FIELDS, partialeq_ne_impl::PARTIALEQ_NE_IMPL, partialeq_to_none::PARTIALEQ_TO_NONE, pass_by_ref_or_value::LARGE_TYPES_PASSED_BY_VALUE, diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs index 6eb9b3d3b9b7..9edced28408f 100644 --- a/clippy_lints/src/lib.register_restriction.rs +++ b/clippy_lints/src/lib.register_restriction.rs @@ -61,6 +61,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(panic_unimplemented::TODO), LintId::of(panic_unimplemented::UNIMPLEMENTED), LintId::of(panic_unimplemented::UNREACHABLE), + LintId::of(partial_pub_fields::PARTIAL_PUB_FIELDS), LintId::of(pattern_type_mismatch::PATTERN_TYPE_MISMATCH), LintId::of(pub_use::PUB_USE), LintId::of(redundant_slicing::DEREF_BY_SLICING), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ebb0f14fef52..bf5688829e22 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -324,6 +324,7 @@ mod option_if_let_else; mod overflow_check_conditional; mod panic_in_result_fn; mod panic_unimplemented; +mod partial_pub_fields; mod partialeq_ne_impl; mod partialeq_to_none; mod pass_by_ref_or_value; @@ -908,6 +909,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(bool_to_int_with_if::BoolToIntWithIf)); store.register_late_pass(|_| Box::new(box_default::BoxDefault)); store.register_late_pass(|_| Box::new(implicit_saturating_add::ImplicitSaturatingAdd)); + store.register_early_pass(|| Box::new(partial_pub_fields::PartialPubFields)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/partial_pub_fields.rs b/clippy_lints/src/partial_pub_fields.rs new file mode 100644 index 000000000000..f60d9d65b120 --- /dev/null +++ b/clippy_lints/src/partial_pub_fields.rs @@ -0,0 +1,81 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use rustc_ast::ast::{Item, ItemKind}; +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// ### What it does + /// Checks whether partial fields of a struct are public. + /// + /// Either make all fields of a type public, or make none of them public + /// + /// ### Why is this bad? + /// Most types should either be: + /// * Abstract data types: complex objects with opaque implementation which guard + /// interior invariants and expose intentionally limited API to the outside world. + /// * Data: relatively simple objects which group a bunch of related attributes together. + /// + /// ### Example + /// ```rust + /// pub struct Color { + /// pub r: u8, + /// pub g: u8, + /// b: u8, + /// } + /// ``` + /// Use instead: + /// ```rust + /// pub struct Color { + /// pub r: u8, + /// pub g: u8, + /// pub b: u8, + /// } + /// ``` + #[clippy::version = "1.66.0"] + pub PARTIAL_PUB_FIELDS, + restriction, + "partial fields of a struct are public" +} +declare_lint_pass!(PartialPubFields => [PARTIAL_PUB_FIELDS]); + +impl EarlyLintPass for PartialPubFields { + fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + let ItemKind::Struct(ref st, _) = item.kind else { + return; + }; + + let mut fields = st.fields().iter(); + let Some(first_field) = fields.next() else { + // Empty struct. + return; + }; + let all_pub = first_field.vis.kind.is_pub(); + let all_priv = !all_pub; + + let msg = "mixed usage of pub and non-pub fields"; + + for field in fields { + if all_priv && field.vis.kind.is_pub() { + span_lint_and_help( + cx, + PARTIAL_PUB_FIELDS, + field.vis.span, + msg, + None, + "consider using private field here", + ); + return; + } else if all_pub && !field.vis.kind.is_pub() { + span_lint_and_help( + cx, + PARTIAL_PUB_FIELDS, + field.vis.span, + msg, + None, + "consider using public field here", + ); + return; + } + } + } +} diff --git a/src/docs.rs b/src/docs.rs index 41c31f91bca5..b8b4286b488a 100644 --- a/src/docs.rs +++ b/src/docs.rs @@ -395,6 +395,7 @@ docs! { "panic", "panic_in_result_fn", "panicking_unwrap", + "partial_pub_fields", "partialeq_ne_impl", "partialeq_to_none", "path_buf_push_overwrite", diff --git a/src/docs/partial_pub_fields.txt b/src/docs/partial_pub_fields.txt new file mode 100644 index 000000000000..b529adf1547d --- /dev/null +++ b/src/docs/partial_pub_fields.txt @@ -0,0 +1,27 @@ +### What it does +Checks whether partial fields of a struct are public. + +Either make all fields of a type public, or make none of them public + +### Why is this bad? +Most types should either be: +* Abstract data types: complex objects with opaque implementation which guard +interior invariants and expose intentionally limited API to the outside world. +* Data: relatively simple objects which group a bunch of related attributes together. + +### Example +``` +pub struct Color { + pub r: u8, + pub g: u8, + b: u8, +} +``` +Use instead: +``` +pub struct Color { + pub r: u8, + pub g: u8, + pub b: u8, +} +``` \ No newline at end of file diff --git a/tests/ui/partial_pub_fields.rs b/tests/ui/partial_pub_fields.rs new file mode 100644 index 000000000000..668545da8441 --- /dev/null +++ b/tests/ui/partial_pub_fields.rs @@ -0,0 +1,40 @@ +#![allow(unused)] +#![warn(clippy::partial_pub_fields)] + +fn main() { + use std::collections::HashMap; + + #[derive(Default)] + pub struct FileSet { + files: HashMap, + pub paths: HashMap, + } + + pub struct Color { + pub r: u8, + pub g: u8, + b: u8, + } + + pub struct Point(i32, pub i32); + + pub struct Visibility { + r#pub: bool, + pub pos: u32, + } + + // Don't lint on empty structs; + pub struct Empty1; + pub struct Empty2(); + pub struct Empty3 {}; + + // Don't lint on structs with one field. + pub struct Single1(i32); + pub struct Single2(pub i32); + pub struct Single3 { + v1: i32, + } + pub struct Single4 { + pub v1: i32, + } +} diff --git a/tests/ui/partial_pub_fields.stderr b/tests/ui/partial_pub_fields.stderr new file mode 100644 index 000000000000..84cfc1a91940 --- /dev/null +++ b/tests/ui/partial_pub_fields.stderr @@ -0,0 +1,35 @@ +error: mixed usage of pub and non-pub fields + --> $DIR/partial_pub_fields.rs:10:9 + | +LL | pub paths: HashMap, + | ^^^ + | + = help: consider using private field here + = note: `-D clippy::partial-pub-fields` implied by `-D warnings` + +error: mixed usage of pub and non-pub fields + --> $DIR/partial_pub_fields.rs:16:9 + | +LL | b: u8, + | ^ + | + = help: consider using public field here + +error: mixed usage of pub and non-pub fields + --> $DIR/partial_pub_fields.rs:19:27 + | +LL | pub struct Point(i32, pub i32); + | ^^^ + | + = help: consider using private field here + +error: mixed usage of pub and non-pub fields + --> $DIR/partial_pub_fields.rs:23:9 + | +LL | pub pos: u32, + | ^^^ + | + = help: consider using private field here + +error: aborting due to 4 previous errors +