Auto merge of #7653 - lengyijun:same_name_method_crate, r=llogiq

bors · bors · commit ed7a82ef053d · 2021-09-17T22:02:04.000Z
New lint: `same_name_method` changelog: ``[`same_name_method`]`` fix: #7632 It only compares a method in `impl` with another in `impl trait for` It doesn't lint two methods in two traits. I'm not sure my approach is the best way. I meet difficulty in other approaches.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2906,6 +2906,7 @@ Released 2018-09-13
 [`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges
 [`same_functions_in_if_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_functions_in_if_condition
 [`same_item_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push
+[`same_name_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_name_method
 [`search_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#search_is_some
 [`self_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_assignment
 [`self_named_constructors`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_named_constructors
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -331,6 +331,7 @@ mod reference;
 mod regex;
 mod repeat_once;
 mod returns;
+mod same_name_method;
 mod self_assignment;
 mod self_named_constructors;
 mod semicolon_if_nothing_returned;
@@ -910,6 +911,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         repeat_once::REPEAT_ONCE,
         returns::LET_AND_RETURN,
         returns::NEEDLESS_RETURN,
+        same_name_method::SAME_NAME_METHOD,
         self_assignment::SELF_ASSIGNMENT,
         self_named_constructors::SELF_NAMED_CONSTRUCTORS,
         semicolon_if_nothing_returned::SEMICOLON_IF_NOTHING_RETURNED,
@@ -1053,6 +1055,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(panic_unimplemented::UNIMPLEMENTED),
         LintId::of(panic_unimplemented::UNREACHABLE),
         LintId::of(pattern_type_mismatch::PATTERN_TYPE_MISMATCH),
+        LintId::of(same_name_method::SAME_NAME_METHOD),
         LintId::of(shadow::SHADOW_REUSE),
         LintId::of(shadow::SHADOW_SAME),
         LintId::of(strings::STRING_ADD),
@@ -1922,6 +1925,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(move || Box::new(unnested_or_patterns::UnnestedOrPatterns::new(msrv)));
 
     store.register_late_pass(|| Box::new(size_of_in_element_count::SizeOfInElementCount));
+    store.register_late_pass(|| Box::new(same_name_method::SameNameMethod));
     store.register_late_pass(|| Box::new(map_clone::MapClone));
     store.register_late_pass(|| Box::new(map_err_ignore::MapErrIgnore));
     store.register_late_pass(|| Box::new(shadow::Shadow));
diff --git a/clippy_lints/src/same_name_method.rs b/clippy_lints/src/same_name_method.rs
@@ -0,0 +1,160 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use rustc_data_structures::fx::FxHashMap;
+use rustc_hir::def::{DefKind, Res};
+use rustc_hir::{Crate, Impl, ItemKind, Node, Path, QPath, TraitRef, TyKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::AssocKind;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::symbol::Symbol;
+use rustc_span::Span;
+use std::collections::{BTreeMap, BTreeSet};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// It lints if a struct has two method with same time:
+    /// one from a trait, another not from trait.
+    ///
+    /// ### Why is this bad?
+    /// Confusing.
+    ///
+    /// ### Example
+    /// ```rust
+    /// trait T {
+    ///     fn foo(&self) {}
+    /// }
+    ///
+    /// struct S;
+    ///
+    /// impl T for S {
+    ///     fn foo(&self) {}
+    /// }
+    ///
+    /// impl S {
+    ///     fn foo(&self) {}
+    /// }
+    /// ```
+    pub SAME_NAME_METHOD,
+    restriction,
+    "two method with same name"
+}
+
+declare_lint_pass!(SameNameMethod => [SAME_NAME_METHOD]);
+
+struct ExistingName {
+    impl_methods: BTreeMap<Symbol, Span>,
+    trait_methods: BTreeMap<Symbol, Vec<Span>>,
+}
+
+impl<'tcx> LateLintPass<'tcx> for SameNameMethod {
+    fn check_crate_post(&mut self, cx: &LateContext<'tcx>, krate: &'tcx Crate<'tcx>) {
+        let mut map = FxHashMap::<Res, ExistingName>::default();
+
+        for item in krate.items() {
+            if let ItemKind::Impl(Impl {
+                items,
+                of_trait,
+                self_ty,
+                ..
+            }) = &item.kind
+            {
+                if let TyKind::Path(QPath::Resolved(_, Path { res, .. })) = self_ty.kind {
+                    if !map.contains_key(res) {
+                        map.insert(
+                            *res,
+                            ExistingName {
+                                impl_methods: BTreeMap::new(),
+                                trait_methods: BTreeMap::new(),
+                            },
+                        );
+                    }
+                    let existing_name = map.get_mut(res).unwrap();
+
+                    match of_trait {
+                        Some(trait_ref) => {
+                            let mut methods_in_trait: BTreeSet<Symbol> = if_chain! {
+                                if let Some(Node::TraitRef(TraitRef { path, .. })) =
+                                    cx.tcx.hir().find(trait_ref.hir_ref_id);
+                                if let Res::Def(DefKind::Trait, did) = path.res;
+                                then{
+                                    // FIXME: if
+                                    // `rustc_middle::ty::assoc::AssocItems::items` is public,
+                                    // we can iterate its keys instead of `in_definition_order`,
+                                    // which's more efficient
+                                    cx.tcx
+                                        .associated_items(did)
+                                        .in_definition_order()
+                                        .filter(|assoc_item| {
+                                            matches!(assoc_item.kind, AssocKind::Fn)
+                                        })
+                                        .map(|assoc_item| assoc_item.ident.name)
+                                        .collect()
+                                }else{
+                                    BTreeSet::new()
+                                }
+                            };
+
+                            let mut check_trait_method = |method_name: Symbol, trait_method_span: Span| {
+                                if let Some(impl_span) = existing_name.impl_methods.get(&method_name) {
+                                    span_lint_and_then(
+                                        cx,
+                                        SAME_NAME_METHOD,
+                                        *impl_span,
+                                        "method's name is same to an existing method in a trait",
+                                        |diag| {
+                                            diag.span_note(
+                                                trait_method_span,
+                                                &format!("existing `{}` defined here", method_name),
+                                            );
+                                        },
+                                    );
+                                }
+                                if let Some(v) = existing_name.trait_methods.get_mut(&method_name) {
+                                    v.push(trait_method_span);
+                                } else {
+                                    existing_name.trait_methods.insert(method_name, vec![trait_method_span]);
+                                }
+                            };
+
+                            for impl_item_ref in (*items).iter().filter(|impl_item_ref| {
+                                matches!(impl_item_ref.kind, rustc_hir::AssocItemKind::Fn { .. })
+                            }) {
+                                let method_name = impl_item_ref.ident.name;
+                                methods_in_trait.remove(&method_name);
+                                check_trait_method(method_name, impl_item_ref.span);
+                            }
+
+                            for method_name in methods_in_trait {
+                                check_trait_method(method_name, item.span);
+                            }
+                        },
+                        None => {
+                            for impl_item_ref in (*items).iter().filter(|impl_item_ref| {
+                                matches!(impl_item_ref.kind, rustc_hir::AssocItemKind::Fn { .. })
+                            }) {
+                                let method_name = impl_item_ref.ident.name;
+                                let impl_span = impl_item_ref.span;
+                                if let Some(trait_spans) = existing_name.trait_methods.get(&method_name) {
+                                    span_lint_and_then(
+                                        cx,
+                                        SAME_NAME_METHOD,
+                                        impl_span,
+                                        "method's name is same to an existing method in a trait",
+                                        |diag| {
+                                            // TODO should we `span_note` on every trait?
+                                            // iterate on trait_spans?
+                                            diag.span_note(
+                                                trait_spans[0],
+                                                &format!("existing `{}` defined here", method_name),
+                                            );
+                                        },
+                                    );
+                                }
+                                existing_name.impl_methods.insert(method_name, impl_span);
+                            }
+                        },
+                    }
+                }
+            }
+        }
+    }
+}
diff --git a/tests/ui/same_name_method.rs b/tests/ui/same_name_method.rs
@@ -0,0 +1,111 @@
+#![warn(clippy::same_name_method)]
+#![allow(dead_code, non_camel_case_types)]
+
+trait T1 {
+    fn foo() {}
+}
+
+trait T2 {
+    fn foo() {}
+}
+
+mod should_lint {
+
+    mod test_basic_case {
+        use crate::T1;
+
+        struct S;
+
+        impl S {
+            fn foo() {}
+        }
+
+        impl T1 for S {
+            fn foo() {}
+        }
+    }
+
+    mod test_derive {
+
+        #[derive(Clone)]
+        struct S;
+
+        impl S {
+            fn clone() {}
+        }
+    }
+
+    mod with_generic {
+        use crate::T1;
+
+        struct S<U>(U);
+
+        impl<U> S<U> {
+            fn foo() {}
+        }
+
+        impl<U: Copy> T1 for S<U> {
+            fn foo() {}
+        }
+    }
+
+    mod default_method {
+        use crate::T1;
+
+        struct S;
+
+        impl S {
+            fn foo() {}
+        }
+
+        impl T1 for S {}
+    }
+
+    mod mulitply_conflicit_trait {
+        use crate::{T1, T2};
+
+        struct S;
+
+        impl S {
+            fn foo() {}
+        }
+
+        impl T1 for S {}
+
+        impl T2 for S {}
+    }
+}
+
+mod should_not_lint {
+
+    mod not_lint_two_trait_method {
+        use crate::{T1, T2};
+
+        struct S;
+
+        impl T1 for S {
+            fn foo() {}
+        }
+
+        impl T2 for S {
+            fn foo() {}
+        }
+    }
+
+    mod only_lint_on_method {
+        trait T3 {
+            type foo;
+        }
+
+        struct S;
+
+        impl S {
+            fn foo() {}
+        }
+        impl T3 for S {
+            type foo = usize;
+        }
+    }
+}
+
+fn main() {}
diff --git a/tests/ui/same_name_method.stderr b/tests/ui/same_name_method.stderr
@@ -0,0 +1,64 @@
+error: method's name is same to an existing method in a trait
+  --> $DIR/same_name_method.rs:20:13
+   |
+LL |             fn foo() {}
+   |             ^^^^^^^^^^^
+   |
+   = note: `-D clippy::same-name-method` implied by `-D warnings`
+note: existing `foo` defined here
+  --> $DIR/same_name_method.rs:24:13
+   |
+LL |             fn foo() {}
+   |             ^^^^^^^^^^^
+
+error: method's name is same to an existing method in a trait
+  --> $DIR/same_name_method.rs:44:13
+   |
+LL |             fn foo() {}
+   |             ^^^^^^^^^^^
+   |
+note: existing `foo` defined here
+  --> $DIR/same_name_method.rs:48:13
+   |
+LL |             fn foo() {}
+   |             ^^^^^^^^^^^
+
+error: method's name is same to an existing method in a trait
+  --> $DIR/same_name_method.rs:58:13
+   |
+LL |             fn foo() {}
+   |             ^^^^^^^^^^^
+   |
+note: existing `foo` defined here
+  --> $DIR/same_name_method.rs:61:9
+   |
+LL |         impl T1 for S {}
+   |         ^^^^^^^^^^^^^^^^
+
+error: method's name is same to an existing method in a trait
+  --> $DIR/same_name_method.rs:70:13
+   |
+LL |             fn foo() {}
+   |             ^^^^^^^^^^^
+   |
+note: existing `foo` defined here
+  --> $DIR/same_name_method.rs:73:9
+   |
+LL |         impl T1 for S {}
+   |         ^^^^^^^^^^^^^^^^
+
+error: method's name is same to an existing method in a trait
+  --> $DIR/same_name_method.rs:34:13
+   |
+LL |             fn clone() {}
+   |             ^^^^^^^^^^^^^
+   |
+note: existing `clone` defined here
+  --> $DIR/same_name_method.rs:30:18
+   |
+LL |         #[derive(Clone)]
+   |                  ^^^^^
+   = note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 5 previous errors
+
