From 7ac97b69fc81fcd5cbd2a7c862187f6a6c6ea354 Mon Sep 17 00:00:00 2001 From: TennyZhuang Date: Sun, 16 Oct 2022 16:02:23 +0800 Subject: [PATCH 1/4] Add new lint `partial_pub_fields` Signed-off-by: TennyZhuang --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_restriction.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/partial_pub_fields.rs | 81 ++++++++++++++++++++ src/docs.rs | 1 + src/docs/partial_pub_fields.txt | 27 +++++++ tests/ui/partial_pub_fields.rs | 20 +++++ tests/ui/partial_pub_fields.stderr | 19 +++++ 9 files changed, 153 insertions(+) create mode 100644 clippy_lints/src/partial_pub_fields.rs create mode 100644 src/docs/partial_pub_fields.txt create mode 100644 tests/ui/partial_pub_fields.rs create mode 100644 tests/ui/partial_pub_fields.stderr 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..085ee08afca9 --- /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::*; +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, + /// pub g, + /// b, + /// } + /// ``` + /// Use instead: + /// ```rust + /// pub struct Color { + /// pub r, + /// pub g, + /// pub b, + /// } + /// ``` + #[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..a332ec8c28a6 --- /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, + pub g, + b, +} +``` +Use instead: +``` +pub struct Color { + pub r, + pub g, + pub b, +} +``` \ 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..e1a4b4827991 --- /dev/null +++ b/tests/ui/partial_pub_fields.rs @@ -0,0 +1,20 @@ +#![allow(unused)] +#![warn(clippy::partial_pub_fields)] + +fn main() { + // test code goes here + + 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, + } +} diff --git a/tests/ui/partial_pub_fields.stderr b/tests/ui/partial_pub_fields.stderr new file mode 100644 index 000000000000..f7f23c85a67e --- /dev/null +++ b/tests/ui/partial_pub_fields.stderr @@ -0,0 +1,19 @@ +error: mixed usage of pub and non-pub fields + --> $DIR/partial_pub_fields.rs:12: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:18:9 + | +LL | b: u8, + | ^ + | + = help: consider using public field here + +error: aborting due to 2 previous errors + From b10882ab9153733249f95d63150724aa5f50ce59 Mon Sep 17 00:00:00 2001 From: TennyZhuang Date: Sun, 16 Oct 2022 16:21:48 +0800 Subject: [PATCH 2/4] fix dogfood Signed-off-by: TennyZhuang --- clippy_lints/src/partial_pub_fields.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/partial_pub_fields.rs b/clippy_lints/src/partial_pub_fields.rs index 085ee08afca9..42f892e3652a 100644 --- a/clippy_lints/src/partial_pub_fields.rs +++ b/clippy_lints/src/partial_pub_fields.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_help; -use rustc_ast::ast::*; +use rustc_ast::ast::{Item, ItemKind}; use rustc_lint::{EarlyContext, EarlyLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -58,7 +58,7 @@ impl EarlyLintPass for PartialPubFields { if all_priv && field.vis.kind.is_pub() { span_lint_and_help( cx, - &PARTIAL_PUB_FIELDS, + PARTIAL_PUB_FIELDS, field.vis.span, msg, None, @@ -68,7 +68,7 @@ impl EarlyLintPass for PartialPubFields { } else if all_pub && !field.vis.kind.is_pub() { span_lint_and_help( cx, - &PARTIAL_PUB_FIELDS, + PARTIAL_PUB_FIELDS, field.vis.span, msg, None, From abd5db332173a5ef76a56f4fd18af421cf551c17 Mon Sep 17 00:00:00 2001 From: TennyZhuang Date: Sun, 16 Oct 2022 16:57:31 +0800 Subject: [PATCH 3/4] add many tests Signed-off-by: TennyZhuang --- tests/ui/partial_pub_fields.rs | 24 ++++++++++++++++++++++-- tests/ui/partial_pub_fields.stderr | 22 +++++++++++++++++++--- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/tests/ui/partial_pub_fields.rs b/tests/ui/partial_pub_fields.rs index e1a4b4827991..668545da8441 100644 --- a/tests/ui/partial_pub_fields.rs +++ b/tests/ui/partial_pub_fields.rs @@ -2,8 +2,6 @@ #![warn(clippy::partial_pub_fields)] fn main() { - // test code goes here - use std::collections::HashMap; #[derive(Default)] @@ -17,4 +15,26 @@ fn main() { 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 index f7f23c85a67e..84cfc1a91940 100644 --- a/tests/ui/partial_pub_fields.stderr +++ b/tests/ui/partial_pub_fields.stderr @@ -1,5 +1,5 @@ error: mixed usage of pub and non-pub fields - --> $DIR/partial_pub_fields.rs:12:9 + --> $DIR/partial_pub_fields.rs:10:9 | LL | pub paths: HashMap, | ^^^ @@ -8,12 +8,28 @@ LL | pub paths: HashMap, = note: `-D clippy::partial-pub-fields` implied by `-D warnings` error: mixed usage of pub and non-pub fields - --> $DIR/partial_pub_fields.rs:18:9 + --> $DIR/partial_pub_fields.rs:16:9 | LL | b: u8, | ^ | = help: consider using public field here -error: aborting due to 2 previous errors +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 From 360b48b1ab97471c0d122732a027b65f46980447 Mon Sep 17 00:00:00 2001 From: TennyZhuang Date: Sun, 16 Oct 2022 17:10:27 +0800 Subject: [PATCH 4/4] fix a doctest Signed-off-by: TennyZhuang --- clippy_lints/src/partial_pub_fields.rs | 12 ++++++------ src/docs/partial_pub_fields.txt | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/partial_pub_fields.rs b/clippy_lints/src/partial_pub_fields.rs index 42f892e3652a..f60d9d65b120 100644 --- a/clippy_lints/src/partial_pub_fields.rs +++ b/clippy_lints/src/partial_pub_fields.rs @@ -18,17 +18,17 @@ declare_clippy_lint! { /// ### Example /// ```rust /// pub struct Color { - /// pub r, - /// pub g, - /// b, + /// pub r: u8, + /// pub g: u8, + /// b: u8, /// } /// ``` /// Use instead: /// ```rust /// pub struct Color { - /// pub r, - /// pub g, - /// pub b, + /// pub r: u8, + /// pub g: u8, + /// pub b: u8, /// } /// ``` #[clippy::version = "1.66.0"] diff --git a/src/docs/partial_pub_fields.txt b/src/docs/partial_pub_fields.txt index a332ec8c28a6..b529adf1547d 100644 --- a/src/docs/partial_pub_fields.txt +++ b/src/docs/partial_pub_fields.txt @@ -12,16 +12,16 @@ interior invariants and expose intentionally limited API to the outside world. ### Example ``` pub struct Color { - pub r, - pub g, - b, + pub r: u8, + pub g: u8, + b: u8, } ``` Use instead: ``` pub struct Color { - pub r, - pub g, - pub b, + pub r: u8, + pub g: u8, + pub b: u8, } ``` \ No newline at end of file