Auto merge of rust-lang#9658 - TennyZhuang:partial-pub-fields, r=llogiq

Add new lint `partial_pub_fields`

Signed-off-by: TennyZhuang <[email protected]>

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: `partial_pub_fields`: new lint to disallow partial fields of a struct be pub

Resolve rust-lang#9604

Author: bors

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

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,

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),

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;
@@ -914,6 +915,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`
 }
 

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;
+            }
+        }
+    }
+}

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",

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,
+}
+```

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<String, u32>,
+        pub paths: HashMap<u32, String>,
+    }
+
+    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,
+    }
+}

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<u32, String>,
+   |         ^^^
+   |
+   = 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
+