From e2cdaec98437ea317eead6c90336d3462c779608 Mon Sep 17 00:00:00 2001 From: lengyijun Date: Wed, 8 Sep 2021 10:36:51 +0800 Subject: [PATCH] same_name_method --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 4 + clippy_lints/src/same_name_method.rs | 160 +++++++++++++++++++++++++++ tests/ui/same_name_method.rs | 111 +++++++++++++++++++ tests/ui/same_name_method.stderr | 64 +++++++++++ 5 files changed, 340 insertions(+) create mode 100644 clippy_lints/src/same_name_method.rs create mode 100644 tests/ui/same_name_method.rs create mode 100644 tests/ui/same_name_method.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 71383e8116b..72fe6ae3173 100644 --- 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 index e3dd43d6982..60e48728dc1 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -331,6 +331,7 @@ macro_rules! declare_clippy_lint { 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), @@ -1923,6 +1926,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 new file mode 100644 index 00000000000..014898e6dab --- /dev/null +++ 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, + trait_methods: BTreeMap>, +} + +impl<'tcx> LateLintPass<'tcx> for SameNameMethod { + fn check_crate_post(&mut self, cx: &LateContext<'tcx>, krate: &'tcx Crate<'tcx>) { + let mut map = FxHashMap::::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 = 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 new file mode 100644 index 00000000000..12e10ba6c49 --- /dev/null +++ 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); + + impl S { + fn foo() {} + } + + impl T1 for S { + 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 new file mode 100644 index 00000000000..0f9139b41b9 --- /dev/null +++ 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 +