From 206737569741970865cdc6d5347994b971aa2af8 Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Fri, 24 Jan 2025 22:00:17 -0800 Subject: [PATCH 1/2] Make `struct_field_names` lint on private fields of public structs. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- clippy_lints/src/item_name_repetitions.rs | 172 +++++++++++++--------- tests/ui/struct_fields.rs | 27 ++++ tests/ui/struct_fields.stderr | 21 ++- 3 files changed, 146 insertions(+), 74 deletions(-) diff --git a/clippy_lints/src/item_name_repetitions.rs b/clippy_lints/src/item_name_repetitions.rs index 6363f717a5c2..fbde2ff799e8 100644 --- a/clippy_lints/src/item_name_repetitions.rs +++ b/clippy_lints/src/item_name_repetitions.rs @@ -196,80 +196,100 @@ 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; - } - - check_struct_name_repetition(cx, item, fields); - - // 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; - } - - 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 { +impl ItemNameRepetitions { + /// 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_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_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, }; - 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"), - ); - } -} + 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; + } -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) 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 @@ -458,17 +478,23 @@ 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, _) => { + if !(self.avoid_breaking_exported_api + && cx.effective_visibilities.is_exported(item.owner_id.def_id)) + { + check_variant(cx, self.enum_threshold, &def, item_name, item.span); + } + }, 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)); } } diff --git a/tests/ui/struct_fields.rs b/tests/ui/struct_fields.rs index 3dce530efffa..e7ff2e6573b2 100644 --- a/tests/ui/struct_fields.rs +++ b/tests/ui/struct_fields.rs @@ -344,4 +344,31 @@ struct Use { //~^ struct_field_names } +// should lint on private fields of public structs (renaming them is not breaking-exported-api) +pub struct PubStructFieldNamedAfterStruct { + pub_struct_field_named_after_struct: bool, + //~^ ERROR: field name starts with the struct's name + other1: bool, + other2: bool, +} +pub struct PubStructFieldPrefix { + //~^ ERROR: all fields have the same prefix: `field` + field_foo: u8, + field_bar: u8, + field_baz: u8, +} +// ...but should not lint on structs with public fields. +pub struct PubStructPubAndPrivateFields { + /// One could argue that this field should be linted, but currently, any public field stops all + /// checking. + pub_struct_pub_and_private_fields_1: bool, + pub pub_struct_pub_and_private_fields_2: bool, +} +// nor on common prefixes if one of the involved fields is public +pub struct PubStructPubAndPrivateFieldPrefix { + pub field_foo: u8, + field_bar: u8, + field_baz: u8, +} + fn main() {} diff --git a/tests/ui/struct_fields.stderr b/tests/ui/struct_fields.stderr index 79186cc1cfd8..a5ff1b125907 100644 --- a/tests/ui/struct_fields.stderr +++ b/tests/ui/struct_fields.stderr @@ -281,5 +281,24 @@ error: field name starts with the struct's name LL | use_baz: bool, | ^^^^^^^^^^^^^ -error: aborting due to 24 previous errors +error: field name starts with the struct's name + --> tests/ui/struct_fields.rs:349:5 + | +LL | pub_struct_field_named_after_struct: bool, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: all fields have the same prefix: `field` + --> tests/ui/struct_fields.rs:354:1 + | +LL | / pub struct PubStructFieldPrefix { +LL | | +LL | | field_foo: u8, +LL | | field_bar: u8, +LL | | field_baz: u8, +LL | | } + | |_^ + | + = help: remove the prefixes + +error: aborting due to 26 previous errors From 8c73b76fb2f32674f76254e07474a596afee8120 Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Sun, 16 Feb 2025 13:26:27 -0800 Subject: [PATCH 2/2] Move `check_variant()` code into a method of `ItemNameRepetitions` too. --- clippy_lints/src/item_name_repetitions.rs | 139 +++++++++++----------- 1 file changed, 72 insertions(+), 67 deletions(-) diff --git a/clippy_lints/src/item_name_repetitions.rs b/clippy_lints/src/item_name_repetitions.rs index fbde2ff799e8..1107946f538b 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! { @@ -197,6 +196,25 @@ fn have_no_extra_prefix(prefixes: &[&str]) -> bool { } 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; + } + + 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); + } + + Self::check_enum_common_affix(cx, item, def); + } + /// 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 { @@ -204,10 +222,60 @@ impl ItemNameRepetitions { } self.check_struct_name_repetition(cx, item, fields); - self.check_common_affix(cx, item, fields); + self.check_struct_common_affix(cx, item, fields); } - fn check_common_affix(&self, cx: &LateContext<'_>, item: &Item<'_>, fields: &[FieldDef<'_>]) { + 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, + }; + 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, + item.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" + ), + ); + } + + 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)) { @@ -357,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(); @@ -482,11 +491,7 @@ impl LateLintPass<'_> for ItemNameRepetitions { if span_is_local(item.span) { match item.kind { ItemKind::Enum(def, _) => { - if !(self.avoid_breaking_exported_api - && cx.effective_visibilities.is_exported(item.owner_id.def_id)) - { - check_variant(cx, self.enum_threshold, &def, item_name, item.span); - } + self.check_variants(cx, item, &def); }, ItemKind::Struct(VariantData::Struct { fields, .. }, _) => { self.check_fields(cx, item, fields);