From 6b18b8d1c3e67c4d8c46b4673a74581cb9c75358 Mon Sep 17 00:00:00 2001 From: Ray Droplet <262660609+raydroplet@users.noreply.github.com> Date: Mon, 30 Mar 2026 16:43:11 -0300 Subject: [PATCH 1/2] Add new lints: [method_shadow_field, method_shadow_public_field] --- CHANGELOG.md | 2 + clippy_lints/src/declared_lints.rs | 2 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/method_shadow_field.rs | 138 ++++++++++++++++++++++++ clippy_utils/src/macros.rs | 1 + tests/ui/method_shadow_field.rs | 58 ++++++++++ tests/ui/method_shadow_field.stderr | 54 ++++++++++ 7 files changed, 257 insertions(+) create mode 100644 clippy_lints/src/method_shadow_field.rs create mode 100644 tests/ui/method_shadow_field.rs create mode 100644 tests/ui/method_shadow_field.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 748e283edffb..a343f621bc88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6853,6 +6853,8 @@ Released 2018-09-13 [`mem_replace_option_with_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_some [`mem_replace_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default [`mem_replace_with_uninit`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_uninit +[`method_shadow_field`]: https://rust-lang.github.io/rust-clippy/master/index.html#method_shadow_field +[`method_shadow_public_field`]: https://rust-lang.github.io/rust-clippy/master/index.html#method_shadow_public_field [`min_ident_chars`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_ident_chars [`min_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_max [`misaligned_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#misaligned_transmute diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index c164241673a3..9eb96250a251 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -356,6 +356,8 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::mem_replace::MEM_REPLACE_OPTION_WITH_SOME_INFO, crate::mem_replace::MEM_REPLACE_WITH_DEFAULT_INFO, crate::mem_replace::MEM_REPLACE_WITH_UNINIT_INFO, + crate::method_shadow_field::METHOD_SHADOW_FIELD_INFO, + crate::method_shadow_field::METHOD_SHADOW_PUBLIC_FIELD_INFO, crate::methods::BIND_INSTEAD_OF_MAP_INFO, crate::methods::BYTES_COUNT_TO_LEN_INFO, crate::methods::BYTES_NTH_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 61c54678c4b2..dee470479220 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -224,6 +224,7 @@ mod map_unit_fn; mod match_result_ok; mod matches; mod mem_replace; +mod method_shadow_field; mod methods; mod min_ident_chars; mod minmax; @@ -867,6 +868,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co Box::new(|_| Box::new(manual_checked_ops::ManualCheckedOps)), Box::new(move |tcx| Box::new(manual_pop_if::ManualPopIf::new(tcx, conf))), Box::new(move |_| Box::new(manual_noop_waker::ManualNoopWaker::new(conf))), + Box::new(|_| Box::new(method_shadow_field::MethodShadowField)), // add late passes here, used by `cargo dev new_lint` ]; store.late_passes.extend(late_lints); diff --git a/clippy_lints/src/method_shadow_field.rs b/clippy_lints/src/method_shadow_field.rs new file mode 100644 index 000000000000..f45b84fdf7e2 --- /dev/null +++ b/clippy_lints/src/method_shadow_field.rs @@ -0,0 +1,138 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use rustc_hir::{ImplItem, ImplItemKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Checks for struct/union methods that shadow private struct/union fields + /// + /// ### Why is this bad? + /// Having a method and a field with the same name can cause confusion + /// + /// ### Example + /// ```no_run + /// struct Foo { + /// bar: i32 + /// } + /// + /// impl Foo { + /// fn bar(self) {} + /// } + /// ``` + /// + /// Instead use distinct identifiers: + /// ```no_run + /// struct Foo { + /// bar: i32, + /// } + /// + /// impl Foo { + /// fn other(self) {} + /// } + /// ``` + #[clippy::version = "1.96.0"] + pub METHOD_SHADOW_FIELD, + restriction, + "method shadows the name of a field" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for public struct/union methods that shadow public struct/union fields + /// + /// ### Why is this bad? + /// Having a method and a field with the same name can cause confusion + /// + /// ### Example + /// ```no_run + /// struct Foo { + /// pub bar: i32 + /// } + /// + /// impl Foo { + /// fn bar(self) {} + /// } + /// ``` + /// + /// Instead make the field private or use distinct identifiers: + /// ```no_run + /// struct Foo { + /// bar: i32, + /// } + /// + /// impl Foo { + /// fn bar(self) {} + /// } + /// ``` + #[clippy::version = "1.96.0"] + pub METHOD_SHADOW_PUBLIC_FIELD, + style, + "method shadows the name of a public field" +} + +declare_lint_pass!(MethodShadowField => [ + METHOD_SHADOW_FIELD, + METHOD_SHADOW_PUBLIC_FIELD, +]); + +impl<'tcx> LateLintPass<'tcx> for MethodShadowField { + fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'tcx>) { + if impl_item.span.from_expansion() { + // it's from a macro + return; + } + + // queries function signature + let ImplItemKind::Fn(ref signature, _) = impl_item.kind else { + // not a function + return; + }; + + if !signature.decl.implicit_self.has_implicit_self() { + // not a method + return; + } + + let method_name = &impl_item.ident.name; + + // get the ty of the parent + let impl_def_id = cx.tcx.local_parent(impl_item.owner_id.def_id); + let ty = cx.tcx.type_of(impl_def_id).instantiate_identity(); + + // we filter out enums + if let ty::Adt(adt_def, _) = ty.kind() + && (adt_def.is_struct() || adt_def.is_union()) + { + let variant = adt_def.non_enum_variant(); + // query fields and checks name collision, public/private + for field in &variant.fields { + if field.name != *method_name { + continue; + } + + if cx.tcx.visibility(field.did).is_public() { + span_lint_and_help( + cx, + METHOD_SHADOW_PUBLIC_FIELD, + impl_item.ident.span, + "method shadows a public field", + cx.tcx.def_ident_span(field.did), + "consider making the field private or use a distinct name", + ); + } else { + span_lint_and_help( + cx, + METHOD_SHADOW_FIELD, + impl_item.ident.span, + "method shadows a field", + cx.tcx.def_ident_span(field.did), + "consider renaming the method or field", + ); + } + return; + } + } + } +} diff --git a/clippy_utils/src/macros.rs b/clippy_utils/src/macros.rs index 4162595ffe81..eb09415ef520 100644 --- a/clippy_utils/src/macros.rs +++ b/clippy_utils/src/macros.rs @@ -554,6 +554,7 @@ impl HirNode for hir::Item<'_> { self.hir_id() } + #[allow(clippy::method_shadow_public_field)] fn span(&self) -> Span { self.span } diff --git a/tests/ui/method_shadow_field.rs b/tests/ui/method_shadow_field.rs new file mode 100644 index 000000000000..97ac8b395b4c --- /dev/null +++ b/tests/ui/method_shadow_field.rs @@ -0,0 +1,58 @@ +#![warn(clippy::method_shadow_field)] + +struct Circle { + radius: f32, + pub diameter: f32, + // + bar: i32, + pub var: i32, + // + extra: i32, +} + +impl Circle { + // Shadows pub or non-pub field names: + fn radius(self) {} + //~^ method_shadow_field + fn diameter(self) {} + //~^ method_shadow_public_field + + // Do not shadow anything + fn other(self) {} + fn another(self) {} + + // Only methods are linted + fn extra() {} +} + +union Counter { + small: i8, + pub large: i64, +} + +impl Counter { + // Shadows pub or non-pub field names: + fn small(self) {} + //~^ method_shadow_field + fn large(self) {} + //~^ method_shadow_public_field + + // Do not shadow anything + fn tick(self) {} +} + +enum Numbers { + One, + Two, + Last, +} + +impl Numbers { + // fields differ in naming from methods: One vs one(), so they don't clash + + // Do not shadow anything + fn smallest(self) {} + fn last(self) {} +} + +fn main() {} diff --git a/tests/ui/method_shadow_field.stderr b/tests/ui/method_shadow_field.stderr new file mode 100644 index 000000000000..3d7381c692aa --- /dev/null +++ b/tests/ui/method_shadow_field.stderr @@ -0,0 +1,54 @@ +error: method shadows a field + --> tests/ui/method_shadow_field.rs:15:8 + | +LL | fn radius(self) {} + | ^^^^^^ + | +help: consider renaming the method or field + --> tests/ui/method_shadow_field.rs:4:5 + | +LL | radius: f32, + | ^^^^^^ + = note: `-D clippy::method-shadow-field` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::method_shadow_field)]` + +error: method shadows a public field + --> tests/ui/method_shadow_field.rs:17:8 + | +LL | fn diameter(self) {} + | ^^^^^^^^ + | +help: consider making the field private or use a distinct name + --> tests/ui/method_shadow_field.rs:5:9 + | +LL | pub diameter: f32, + | ^^^^^^^^ + = note: `-D clippy::method-shadow-public-field` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::method_shadow_public_field)]` + +error: method shadows a field + --> tests/ui/method_shadow_field.rs:35:8 + | +LL | fn small(self) {} + | ^^^^^ + | +help: consider renaming the method or field + --> tests/ui/method_shadow_field.rs:29:5 + | +LL | small: i8, + | ^^^^^ + +error: method shadows a public field + --> tests/ui/method_shadow_field.rs:37:8 + | +LL | fn large(self) {} + | ^^^^^ + | +help: consider making the field private or use a distinct name + --> tests/ui/method_shadow_field.rs:30:9 + | +LL | pub large: i64, + | ^^^^^ + +error: aborting due to 4 previous errors + From 364f24b4a0bea643420c082e41cffcbf4390508b Mon Sep 17 00:00:00 2001 From: Ray Droplet <262660609+raydroplet@users.noreply.github.com> Date: Sat, 11 Apr 2026 15:47:25 -0300 Subject: [PATCH 2/2] implement requested changes --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/method_shadow_field.rs | 58 ++++++++--- tests/ui/method_shadow_field.rs | 128 +++++++++++++++++++++++- tests/ui/method_shadow_field.stderr | 90 +++++++++++++++-- 4 files changed, 252 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index dee470479220..c185bbbe9d81 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -868,7 +868,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co Box::new(|_| Box::new(manual_checked_ops::ManualCheckedOps)), Box::new(move |tcx| Box::new(manual_pop_if::ManualPopIf::new(tcx, conf))), Box::new(move |_| Box::new(manual_noop_waker::ManualNoopWaker::new(conf))), - Box::new(|_| Box::new(method_shadow_field::MethodShadowField)), + Box::new(move |_| Box::new(method_shadow_field::MethodShadowField::new(conf))), // add late passes here, used by `cargo dev new_lint` ]; store.late_passes.extend(late_lints); diff --git a/clippy_lints/src/method_shadow_field.rs b/clippy_lints/src/method_shadow_field.rs index f45b84fdf7e2..a1d27344ca7e 100644 --- a/clippy_lints/src/method_shadow_field.rs +++ b/clippy_lints/src/method_shadow_field.rs @@ -1,8 +1,12 @@ +use clippy_config::Conf; use clippy_utils::diagnostics::span_lint_and_help; +use rustc_data_structures::fx::FxHashMap; +use rustc_hir::def_id::DefId; use rustc_hir::{ImplItem, ImplItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; -use rustc_session::declare_lint_pass; +use rustc_session::impl_lint_pass; +use rustc_span::symbol::Symbol; declare_clippy_lint! { /// ### What it does @@ -72,11 +76,25 @@ declare_clippy_lint! { "method shadows the name of a public field" } -declare_lint_pass!(MethodShadowField => [ +impl_lint_pass!(MethodShadowField => [ METHOD_SHADOW_FIELD, METHOD_SHADOW_PUBLIC_FIELD, ]); +pub struct MethodShadowField { + avoid_breaking_exported_api: bool, + field_cache: FxHashMap>, +} + +impl MethodShadowField { + pub fn new(conf: &'static Conf) -> Self { + Self { + avoid_breaking_exported_api: conf.avoid_breaking_exported_api, + field_cache: FxHashMap::default(), + } + } +} + impl<'tcx> LateLintPass<'tcx> for MethodShadowField { fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'tcx>) { if impl_item.span.from_expansion() { @@ -95,30 +113,47 @@ impl<'tcx> LateLintPass<'tcx> for MethodShadowField { return; } - let method_name = &impl_item.ident.name; + if self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(impl_item.owner_id.def_id) { + // the api is exported + return; + } // get the ty of the parent let impl_def_id = cx.tcx.local_parent(impl_item.owner_id.def_id); let ty = cx.tcx.type_of(impl_def_id).instantiate_identity(); + if cx.tcx.impl_opt_trait_ref(impl_def_id).is_some() { + // it's a trait impl + return; + } + // we filter out enums if let ty::Adt(adt_def, _) = ty.kind() && (adt_def.is_struct() || adt_def.is_union()) { - let variant = adt_def.non_enum_variant(); - // query fields and checks name collision, public/private - for field in &variant.fields { - if field.name != *method_name { - continue; + let adt_did = adt_def.did(); + + // per impl caching for fast lookup + let field_map = self.field_cache.entry(adt_did).or_insert_with(|| { + let mut map = FxHashMap::default(); + let variant = adt_def.non_enum_variant(); + for field in &variant.fields { + let is_pub = cx.tcx.visibility(field.did).is_public(); + map.insert(field.name, (is_pub, field.did)); } + map + }); + + let method_name = impl_item.ident.name; - if cx.tcx.visibility(field.did).is_public() { + if let Some(&(is_public, field_did)) = field_map.get(&method_name) { + if is_public { span_lint_and_help( cx, METHOD_SHADOW_PUBLIC_FIELD, impl_item.ident.span, "method shadows a public field", - cx.tcx.def_ident_span(field.did), + cx.tcx.def_ident_span(field_did), "consider making the field private or use a distinct name", ); } else { @@ -127,11 +162,10 @@ impl<'tcx> LateLintPass<'tcx> for MethodShadowField { METHOD_SHADOW_FIELD, impl_item.ident.span, "method shadows a field", - cx.tcx.def_ident_span(field.did), + cx.tcx.def_ident_span(field_did), "consider renaming the method or field", ); } - return; } } } diff --git a/tests/ui/method_shadow_field.rs b/tests/ui/method_shadow_field.rs index 97ac8b395b4c..852d7b5641f3 100644 --- a/tests/ui/method_shadow_field.rs +++ b/tests/ui/method_shadow_field.rs @@ -1,5 +1,8 @@ #![warn(clippy::method_shadow_field)] +use std::ops::Deref; + +// Struct struct Circle { radius: f32, pub diameter: f32, @@ -25,22 +28,24 @@ impl Circle { fn extra() {} } +// Union union Counter { small: i8, pub large: i64, } impl Counter { - // Shadows pub or non-pub field names: + // shadows pub or non-pub field names: fn small(self) {} //~^ method_shadow_field fn large(self) {} //~^ method_shadow_public_field - // Do not shadow anything + // do not shadow anything fn tick(self) {} } +// Enum enum Numbers { One, Two, @@ -50,9 +55,124 @@ enum Numbers { impl Numbers { // fields differ in naming from methods: One vs one(), so they don't clash - // Do not shadow anything + // do not shadow anything fn smallest(self) {} fn last(self) {} } -fn main() {} +// Deref +struct Inner { + foo: String, + pub pub_foo: String, +} + +struct Wrapper { + inner: Inner, +} + +impl Wrapper { + fn foo(&self) {} // not linted + fn pub_foo(&self) {} // not linted +} + +impl Deref for Wrapper { + type Target = Inner; + + fn deref(&self) -> &Self::Target { + &self.inner + } +} + +fn deref_test() { + let inner = Inner { + foo: String::from("inner"), + pub_foo: String::from("inner_pub"), + }; + let wrapper = Wrapper { inner }; + + wrapper.foo(); + wrapper.pub_foo(); +} + +// Trait +trait DefaultName { + fn bar(&self) {} +} + +impl DefaultName for Circle { + fn bar(&self) {} // not linted +} + +// Generics and lifetimes +struct GenericStruct<'a, T> { + data: &'a T, +} + +impl<'a, T> GenericStruct<'a, T> { + fn data(&self) {} + //~^ method_shadow_field +} + +// Multiple impl blocks +struct MultiImpl { + a: i32, + b: i32, +} + +impl MultiImpl { + fn a(&self) {} + //~^ method_shadow_field +} + +impl MultiImpl { + fn b(&self) {} + //~^ method_shadow_field +} + +// Macros +struct MacroStruct { + mac_field: i32, +} + +macro_rules! generate_method { + () => { + impl MacroStruct { + // should not lint because it is generated by a macro + fn mac_field(&self) {} + } + }; +} + +generate_method!(); + +// pub(crate), pub(super) +mod internal { + pub struct RestrictedVisibility { + pub(crate) crate_field: i32, + pub(super) super_field: i32, + } + + impl RestrictedVisibility { + pub fn crate_field(&self) {} + //~^ method_shadow_field + + pub fn super_field(&self) {} + //~^ method_shadow_field + } +} + +// type alias +struct Original { + data: i32, +} + +type Aliased = Original; + +impl Aliased { + fn data(&self) {} + //~^ method_shadow_field +} + +fn main() { + deref_test(); +} diff --git a/tests/ui/method_shadow_field.stderr b/tests/ui/method_shadow_field.stderr index 3d7381c692aa..e83df3dcfc03 100644 --- a/tests/ui/method_shadow_field.stderr +++ b/tests/ui/method_shadow_field.stderr @@ -1,11 +1,11 @@ error: method shadows a field - --> tests/ui/method_shadow_field.rs:15:8 + --> tests/ui/method_shadow_field.rs:18:8 | LL | fn radius(self) {} | ^^^^^^ | help: consider renaming the method or field - --> tests/ui/method_shadow_field.rs:4:5 + --> tests/ui/method_shadow_field.rs:7:5 | LL | radius: f32, | ^^^^^^ @@ -13,13 +13,13 @@ LL | radius: f32, = help: to override `-D warnings` add `#[allow(clippy::method_shadow_field)]` error: method shadows a public field - --> tests/ui/method_shadow_field.rs:17:8 + --> tests/ui/method_shadow_field.rs:20:8 | LL | fn diameter(self) {} | ^^^^^^^^ | help: consider making the field private or use a distinct name - --> tests/ui/method_shadow_field.rs:5:9 + --> tests/ui/method_shadow_field.rs:8:9 | LL | pub diameter: f32, | ^^^^^^^^ @@ -27,28 +27,100 @@ LL | pub diameter: f32, = help: to override `-D warnings` add `#[allow(clippy::method_shadow_public_field)]` error: method shadows a field - --> tests/ui/method_shadow_field.rs:35:8 + --> tests/ui/method_shadow_field.rs:39:8 | LL | fn small(self) {} | ^^^^^ | help: consider renaming the method or field - --> tests/ui/method_shadow_field.rs:29:5 + --> tests/ui/method_shadow_field.rs:33:5 | LL | small: i8, | ^^^^^ error: method shadows a public field - --> tests/ui/method_shadow_field.rs:37:8 + --> tests/ui/method_shadow_field.rs:41:8 | LL | fn large(self) {} | ^^^^^ | help: consider making the field private or use a distinct name - --> tests/ui/method_shadow_field.rs:30:9 + --> tests/ui/method_shadow_field.rs:34:9 | LL | pub large: i64, | ^^^^^ -error: aborting due to 4 previous errors +error: method shadows a field + --> tests/ui/method_shadow_field.rs:112:8 + | +LL | fn data(&self) {} + | ^^^^ + | +help: consider renaming the method or field + --> tests/ui/method_shadow_field.rs:108:5 + | +LL | data: &'a T, + | ^^^^ + +error: method shadows a field + --> tests/ui/method_shadow_field.rs:123:8 + | +LL | fn a(&self) {} + | ^ + | +help: consider renaming the method or field + --> tests/ui/method_shadow_field.rs:118:5 + | +LL | a: i32, + | ^ + +error: method shadows a field + --> tests/ui/method_shadow_field.rs:128:8 + | +LL | fn b(&self) {} + | ^ + | +help: consider renaming the method or field + --> tests/ui/method_shadow_field.rs:119:5 + | +LL | b: i32, + | ^ + +error: method shadows a field + --> tests/ui/method_shadow_field.rs:156:16 + | +LL | pub fn crate_field(&self) {} + | ^^^^^^^^^^^ + | +help: consider renaming the method or field + --> tests/ui/method_shadow_field.rs:151:20 + | +LL | pub(crate) crate_field: i32, + | ^^^^^^^^^^^ + +error: method shadows a field + --> tests/ui/method_shadow_field.rs:159:16 + | +LL | pub fn super_field(&self) {} + | ^^^^^^^^^^^ + | +help: consider renaming the method or field + --> tests/ui/method_shadow_field.rs:152:20 + | +LL | pub(super) super_field: i32, + | ^^^^^^^^^^^ + +error: method shadows a field + --> tests/ui/method_shadow_field.rs:172:8 + | +LL | fn data(&self) {} + | ^^^^ + | +help: consider renaming the method or field + --> tests/ui/method_shadow_field.rs:166:5 + | +LL | data: i32, + | ^^^^ + +error: aborting due to 10 previous errors