Skip to content

Fix/12035 silence struct field names #12190

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.

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 22 additions & 5 deletions clippy_lints/src/item_name_repetitions.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
//! lint on enum variants that are prefixed or suffixed by the same characters

use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_hir};
use clippy_utils::is_bool;
use clippy_utils::macros::span_is_local;
use clippy_utils::source::is_present_in_source;
use clippy_utils::str_utils::{camel_case_split, count_match_end, count_match_start, to_camel_case, to_snake_case};
use clippy_utils::{any_impl_has_lint_allowed, is_bool};
use rustc_hir::{EnumDef, FieldDef, Item, ItemKind, OwnerId, Variant, VariantData};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::impl_lint_pass;
use rustc_span::def_id::DefId;
use rustc_span::symbol::Symbol;
use rustc_span::Span;

Expand Down Expand Up @@ -180,7 +181,8 @@ fn have_no_extra_prefix(prefixes: &[&str]) -> bool {
}

fn check_fields(cx: &LateContext<'_>, threshold: u64, item: &Item<'_>, fields: &[FieldDef<'_>]) {
if (fields.len() as u64) < threshold {
if (fields.len() as u64) < threshold || any_impl_has_lint_allowed(cx, STRUCT_FIELD_NAMES, item.owner_id.to_def_id())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: unsure if we should perform this check before or after the field name check

this is a bunch of queries, that is some string comparisons, so probably before makes sense.

{
return;
}

Expand Down Expand Up @@ -320,8 +322,15 @@ 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 {
fn check_variant(
cx: &LateContext<'_>,
threshold: u64,
def: &EnumDef<'_>,
item_name: &str,
item_did: DefId,
span: Span,
) {
if (def.variants.len() as u64) < threshold || any_impl_has_lint_allowed(cx, ENUM_VARIANT_NAMES, item_did) {
return;
}

Expand Down Expand Up @@ -388,6 +397,7 @@ impl LateLintPass<'_> for ItemNameRepetitions {
#[expect(clippy::similar_names)]
fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
let item_name = item.ident.name.as_str();

let item_camel = to_camel_case(item_name);
if !item.span.from_expansion() && is_present_in_source(cx, item.span) {
if let [.., (mod_name, mod_camel, owner_id)] = &*self.modules {
Expand Down Expand Up @@ -440,7 +450,14 @@ impl LateLintPass<'_> for ItemNameRepetitions {
&& 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, _) => check_variant(
cx,
self.enum_threshold,
&def,
item_name,
item.owner_id.to_def_id(),
item.span,
),
ItemKind::Struct(VariantData::Struct { fields, .. }, _) => {
check_fields(cx, self.struct_threshold, item, fields);
},
Expand Down
8 changes: 8 additions & 0 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1846,6 +1846,14 @@ pub fn is_lint_allowed(cx: &LateContext<'_>, lint: &'static Lint, id: HirId) ->
cx.tcx.lint_level_at_node(lint, id).0 == Level::Allow
}

/// Returns `true` if any of the `impl`s for the given `item` has the lint allowed
pub fn any_impl_has_lint_allowed(cx: &LateContext<'_>, lint: &'static Lint, id: DefId) -> bool {
cx.tcx
.inherent_impls(id)
Copy link
Member

@y21 y21 Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, a bit late, but does this also return trait impls? tcx.inherent_impls only contains those impls with no trait reference, right? If it does work correctly, I think it should at least have a test, since that's usually what derive macros expand to and AIUI what the issue asked for ?

Though reading #12035 (comment), it doesn't seem uncontroversial that serde should annotate its impls with this anyway and maybe deserves more discussion in general for the ser/de case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah good point.

perhaps we should wait on this PR for now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also looking up all trait impls can be an expensive operation

.iter()
.any(|imp| is_lint_allowed(cx, lint, cx.tcx.hir().expect_item(imp.expect_local()).hir_id()))
}

pub fn strip_pat_refs<'hir>(mut pat: &'hir Pat<'hir>) -> &'hir Pat<'hir> {
while let PatKind::Ref(subpat, _) = pat.kind {
pat = subpat;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,28 @@ enum Foo2 {
EFoo,
}

// This should not trigger the lint as one of it's impl has allow attribute
struct Data3 {
a_data: u8,
b_data: u8,
c_data: u8,
d_data: u8,
e_data: u8,
}

#[allow(clippy::struct_field_names)]
impl Data3 {}

// This should not trigger the lint as one of it's impl has allow attribute
enum Foo3 {
AFoo,
BFoo,
CFoo,
DFoo,
EFoo,
}

#[allow(clippy::enum_variant_names)]
impl Foo3 {}

fn main() {}