Make `struct_field_names` check private fields of public structs. (#14076)

Currently, If a struct is `pub` and its field is private, and
`avoid-breaking-exported-api = true` (default), then
`struct_field_names` will not lint the field, even though changing the
field’s name is not a breaking change. This is because the
breaking-exported-api condition was checking the visibility of the
struct, not its fields (perhaps because the same code was used for
enums). With this change, Clippy will check the field’s effective
visibility only.

Note: This change is large because some functions were moved into an
`impl` to be able to access more configuration. Consider viewing the
diff with whitespace ignored.

changelog: [`struct_field_names`]: also check private fields of public
structs

1 files changed, 153 insertions, 122 deletions

diff --git a/clippy_lints/src/item_name_repetitions.rs b/clippy_lints/src/item_name_repetitions.rs
index 6363f717a5c..1107946f538 100644
--- a/clippy_lints/src/item_name_repetitions.rs
+++ b/clippy_lints/src/item_name_repetitions.rs
@@ -8,7 +8,6 @@ use rustc_data_structures::fx::FxHashSet;
 use rustc_hir::{EnumDef, FieldDef, Item, ItemKind, OwnerId, Variant, VariantData};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
-use rustc_span::Span;
 use rustc_span::symbol::Symbol;
 
 declare_clippy_lint! {
@@ -196,80 +195,169 @@ fn have_no_extra_prefix(prefixes: &[&str]) -> bool {
     prefixes.iter().all(|p| p == &"" || p == &"_")
 }
 
-fn check_fields(cx: &LateContext<'_>, threshold: u64, item: &Item<'_>, fields: &[FieldDef<'_>]) {
-    if (fields.len() as u64) < threshold {
-        return;
-    }
+impl ItemNameRepetitions {
+    /// Lint the names of enum variants against the name of the enum.
+    fn check_variants(&self, cx: &LateContext<'_>, item: &Item<'_>, def: &EnumDef<'_>) {
+        if self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(item.owner_id.def_id) {
+            return;
+        }
+
+        if (def.variants.len() as u64) < self.enum_threshold {
+            return;
+        }
 
-    check_struct_name_repetition(cx, item, fields);
+        let item_name = item.ident.name.as_str();
+        for var in def.variants {
+            check_enum_start(cx, item_name, var);
+            check_enum_end(cx, item_name, var);
+        }
 
-    // if the SyntaxContext of the identifiers of the fields and struct differ dont lint them.
-    // this prevents linting in macros in which the location of the field identifier names differ
-    if !fields.iter().all(|field| item.ident.span.eq_ctxt(field.ident.span)) {
-        return;
+        Self::check_enum_common_affix(cx, item, def);
     }
 
-    let mut pre: Vec<&str> = match fields.first() {
-        Some(first_field) => first_field.ident.name.as_str().split('_').collect(),
-        None => return,
-    };
-    let mut post = pre.clone();
-    post.reverse();
-    for field in fields {
-        let field_split: Vec<&str> = field.ident.name.as_str().split('_').collect();
-        if field_split.len() == 1 {
+    /// Lint the names of struct fields against the name of the struct.
+    fn check_fields(&self, cx: &LateContext<'_>, item: &Item<'_>, fields: &[FieldDef<'_>]) {
+        if (fields.len() as u64) < self.struct_threshold {
             return;
         }
 
-        pre = pre
-            .into_iter()
-            .zip(field_split.iter())
-            .take_while(|(a, b)| &a == b)
-            .map(|e| e.0)
-            .collect();
-        post = post
-            .into_iter()
-            .zip(field_split.iter().rev())
-            .take_while(|(a, b)| &a == b)
-            .map(|e| e.0)
-            .collect();
+        self.check_struct_name_repetition(cx, item, fields);
+        self.check_struct_common_affix(cx, item, fields);
     }
-    let prefix = pre.join("_");
-    post.reverse();
-    let postfix = match post.last() {
-        Some(&"") => post.join("_") + "_",
-        Some(_) | None => post.join("_"),
-    };
-    if fields.len() > 1 {
-        let (what, value) = match (
-            prefix.is_empty() || prefix.chars().all(|c| c == '_'),
-            postfix.is_empty(),
-        ) {
-            (true, true) => return,
-            (false, _) => ("pre", prefix),
-            (true, false) => ("post", postfix),
+
+    fn check_enum_common_affix(cx: &LateContext<'_>, item: &Item<'_>, def: &EnumDef<'_>) {
+        let first = match def.variants.first() {
+            Some(variant) => variant.ident.name.as_str(),
+            None => return,
         };
-        if fields.iter().all(|field| is_bool(field.ty)) {
-            // If all fields are booleans, we don't want to emit this lint.
-            return;
+        let mut pre = camel_case_split(first);
+        let mut post = pre.clone();
+        post.reverse();
+        for var in def.variants {
+            let name = var.ident.name.as_str();
+
+            let variant_split = camel_case_split(name);
+            if variant_split.len() == 1 {
+                return;
+            }
+
+            pre = pre
+                .iter()
+                .zip(variant_split.iter())
+                .take_while(|(a, b)| a == b)
+                .map(|e| *e.0)
+                .collect();
+            post = post
+                .iter()
+                .zip(variant_split.iter().rev())
+                .take_while(|(a, b)| a == b)
+                .map(|e| *e.0)
+                .collect();
         }
+        let (what, value) = match (have_no_extra_prefix(&pre), post.is_empty()) {
+            (true, true) => return,
+            (false, _) => ("pre", pre.join("")),
+            (true, false) => {
+                post.reverse();
+                ("post", post.join(""))
+            },
+        };
         span_lint_and_help(
             cx,
-            STRUCT_FIELD_NAMES,
+            ENUM_VARIANT_NAMES,
             item.span,
-            format!("all fields have the same {what}fix: `{value}`"),
+            format!("all variants have the same {what}fix: `{value}`"),
             None,
-            format!("remove the {what}fixes"),
+            format!(
+                "remove the {what}fixes and use full paths to \
+                 the variants instead of glob imports"
+            ),
         );
     }
-}
 
-fn check_struct_name_repetition(cx: &LateContext<'_>, item: &Item<'_>, fields: &[FieldDef<'_>]) {
-    let snake_name = to_snake_case(item.ident.name.as_str());
-    let item_name_words: Vec<&str> = snake_name.split('_').collect();
-    for field in fields {
-        if field.ident.span.eq_ctxt(item.ident.span) {
-            //consider linting only if the field identifier has the same SyntaxContext as the item(struct)
+    fn check_struct_common_affix(&self, cx: &LateContext<'_>, item: &Item<'_>, fields: &[FieldDef<'_>]) {
+        // if the SyntaxContext of the identifiers of the fields and struct differ dont lint them.
+        // this prevents linting in macros in which the location of the field identifier names differ
+        if !fields.iter().all(|field| item.ident.span.eq_ctxt(field.ident.span)) {
+            return;
+        }
+
+        if self.avoid_breaking_exported_api
+            && fields
+                .iter()
+                .any(|field| cx.effective_visibilities.is_exported(field.def_id))
+        {
+            return;
+        }
+
+        let mut pre: Vec<&str> = match fields.first() {
+            Some(first_field) => first_field.ident.name.as_str().split('_').collect(),
+            None => return,
+        };
+        let mut post = pre.clone();
+        post.reverse();
+        for field in fields {
+            let field_split: Vec<&str> = field.ident.name.as_str().split('_').collect();
+            if field_split.len() == 1 {
+                return;
+            }
+
+            pre = pre
+                .into_iter()
+                .zip(field_split.iter())
+                .take_while(|(a, b)| &a == b)
+                .map(|e| e.0)
+                .collect();
+            post = post
+                .into_iter()
+                .zip(field_split.iter().rev())
+                .take_while(|(a, b)| &a == b)
+                .map(|e| e.0)
+                .collect();
+        }
+        let prefix = pre.join("_");
+        post.reverse();
+        let postfix = match post.last() {
+            Some(&"") => post.join("_") + "_",
+            Some(_) | None => post.join("_"),
+        };
+        if fields.len() > 1 {
+            let (what, value) = match (
+                prefix.is_empty() || prefix.chars().all(|c| c == '_'),
+                postfix.is_empty(),
+            ) {
+                (true, true) => return,
+                (false, _) => ("pre", prefix),
+                (true, false) => ("post", postfix),
+            };
+            if fields.iter().all(|field| is_bool(field.ty)) {
+                // If all fields are booleans, we don't want to emit this lint.
+                return;
+            }
+            span_lint_and_help(
+                cx,
+                STRUCT_FIELD_NAMES,
+                item.span,
+                format!("all fields have the same {what}fix: `{value}`"),
+                None,
+                format!("remove the {what}fixes"),
+            );
+        }
+    }
+
+    fn check_struct_name_repetition(&self, cx: &LateContext<'_>, item: &Item<'_>, fields: &[FieldDef<'_>]) {
+        let snake_name = to_snake_case(item.ident.name.as_str());
+        let item_name_words: Vec<&str> = snake_name.split('_').collect();
+        for field in fields {
+            if self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(field.def_id) {
+                continue;
+            }
+
+            if !field.ident.span.eq_ctxt(item.ident.span) {
+                // consider linting only if the field identifier has the same SyntaxContext as the item(struct)
+                continue;
+            }
+
             let field_words: Vec<&str> = field.ident.name.as_str().split('_').collect();
             if field_words.len() >= item_name_words.len() {
                 // if the field name is shorter than the struct name it cannot contain it
@@ -337,65 +425,6 @@ fn check_enum_end(cx: &LateContext<'_>, item_name: &str, variant: &Variant<'_>)
     }
 }
 
-fn check_variant(cx: &LateContext<'_>, threshold: u64, def: &EnumDef<'_>, item_name: &str, span: Span) {
-    if (def.variants.len() as u64) < threshold {
-        return;
-    }
-
-    for var in def.variants {
-        check_enum_start(cx, item_name, var);
-        check_enum_end(cx, item_name, var);
-    }
-
-    let first = match def.variants.first() {
-        Some(variant) => variant.ident.name.as_str(),
-        None => return,
-    };
-    let mut pre = camel_case_split(first);
-    let mut post = pre.clone();
-    post.reverse();
-    for var in def.variants {
-        let name = var.ident.name.as_str();
-
-        let variant_split = camel_case_split(name);
-        if variant_split.len() == 1 {
-            return;
-        }
-
-        pre = pre
-            .iter()
-            .zip(variant_split.iter())
-            .take_while(|(a, b)| a == b)
-            .map(|e| *e.0)
-            .collect();
-        post = post
-            .iter()
-            .zip(variant_split.iter().rev())
-            .take_while(|(a, b)| a == b)
-            .map(|e| *e.0)
-            .collect();
-    }
-    let (what, value) = match (have_no_extra_prefix(&pre), post.is_empty()) {
-        (true, true) => return,
-        (false, _) => ("pre", pre.join("")),
-        (true, false) => {
-            post.reverse();
-            ("post", post.join(""))
-        },
-    };
-    span_lint_and_help(
-        cx,
-        ENUM_VARIANT_NAMES,
-        span,
-        format!("all variants have the same {what}fix: `{value}`"),
-        None,
-        format!(
-            "remove the {what}fixes and use full paths to \
-             the variants instead of glob imports"
-        ),
-    );
-}
-
 impl LateLintPass<'_> for ItemNameRepetitions {
     fn check_item_post(&mut self, _cx: &LateContext<'_>, _item: &Item<'_>) {
         let last = self.modules.pop();
@@ -458,17 +487,19 @@ impl LateLintPass<'_> for ItemNameRepetitions {
                 }
             }
         }
-        if !(self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(item.owner_id.def_id))
-            && span_is_local(item.span)
-        {
+
+        if span_is_local(item.span) {
             match item.kind {
-                ItemKind::Enum(def, _) => check_variant(cx, self.enum_threshold, &def, item_name, item.span),
+                ItemKind::Enum(def, _) => {
+                    self.check_variants(cx, item, &def);
+                },
                 ItemKind::Struct(VariantData::Struct { fields, .. }, _) => {
-                    check_fields(cx, self.struct_threshold, item, fields);
+                    self.check_fields(cx, item, fields);
                 },
                 _ => (),
             }
         }
+
         self.modules.push((item.ident.name, item_camel, item.owner_id));
     }
 }