Add new lints: [method_shadow_field, method_shadow_public_field]

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

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,

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(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);

clippy_lints/src/method_shadow_field.rs

@@ -0,0 +1,172 @@
+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::impl_lint_pass;
+use rustc_span::symbol::Symbol;
+
+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"
+}
+
+impl_lint_pass!(MethodShadowField => [
+    METHOD_SHADOW_FIELD,
+    METHOD_SHADOW_PUBLIC_FIELD,
+]);
+
+pub struct MethodShadowField {
+    avoid_breaking_exported_api: bool,
+    field_cache: FxHashMap<DefId, FxHashMap<Symbol, (bool, DefId)>>,
+}
+
+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() {
+            // 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;
+        }
+
+        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 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 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),
+                        "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",
+                    );
+                }
+            }
+        }
+    }
+}

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
     }

tests/ui/method_shadow_field.rs

@@ -0,0 +1,178 @@
+#![warn(clippy::method_shadow_field)]
+
+use std::ops::Deref;
+
+// Struct
+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
+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
+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) {}
+}
+
+// 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();
+}