Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make struct_field_names check private fields of public structs. #14076

Merged
merged 2 commits into from
Mar 5, 2025
Merged

Make struct_field_names check private fields of public structs. #14076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2067375
Make `struct_field_names` lint on private fields of public structs.
kpreid Jan 25, 2025
8c73b76
Move `check_variant()` code into a method of `ItemNameRepetitions` too.
kpreid Feb 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
275 changes: 153 additions & 122 deletions clippy_lints/src/item_name_repetitions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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));
}
}
27 changes: 27 additions & 0 deletions tests/ui/struct_fields.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
21 changes: 20 additions & 1 deletion tests/ui/struct_fields.stderr
View file
Open in desktop
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: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