Skip to content

Commit

Permalink
Make struct_name_repetitions lint on private fields of public structs.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@kpreid
kpreid committed Jan 25, 2025
1 parent d7e20a9 commit 454bd77
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 70 deletions.
164 changes: 95 additions & 69 deletions clippy_lints/src/item_name_repetitions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
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;
}

// 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_struct_name_repetition(cx, item, fields);
self.check_common_affix(cx, item, fields);
}

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

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.
if self.avoid_breaking_exported_api
&& fields
.iter()
.any(|field| cx.effective_visibilities.is_exported(field.def_id))
{
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 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(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_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
Expand Down Expand Up @@ -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));
}
}
14 changes: 14 additions & 0 deletions tests/ui/struct_fields.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,4 +342,18 @@ struct Use {
use_baz: bool,
}

// 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,
}

fn main() {}
21 changes: 20 additions & 1 deletion tests/ui/struct_fields.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:347:5
|
LL | pub_struct_field_named_after_struct: bool,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: all fields have the same prefix: `field`
--> tests/ui/struct_fields.rs:352: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

0 comments on commit 454bd77

Please sign in to comment.