diff --git a/CHANGELOG.md b/CHANGELOG.md index db54bfbf0b32..3bd2defbc7a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4776,6 +4776,7 @@ Released 2018-09-13 [`almost_complete_letter_range`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_complete_letter_range [`almost_complete_range`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_complete_range [`almost_swapped`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_swapped +[`ambiguous_method_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#ambiguous_method_names [`approx_constant`]: https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant [`arc_with_non_send_sync`]: https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync [`arithmetic_side_effects`]: https://rust-lang.github.io/rust-clippy/master/index.html#arithmetic_side_effects diff --git a/clippy_lints/src/ambiguous_method_names.rs b/clippy_lints/src/ambiguous_method_names.rs new file mode 100644 index 000000000000..a5e83f81cc05 --- /dev/null +++ b/clippy_lints/src/ambiguous_method_names.rs @@ -0,0 +1,142 @@ +use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::is_trait_impl_item; +use clippy_utils::ty::implements_trait; +use hir::{ImplItem, Item}; +use rustc_hir as hir; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::def_id::DefId; +use rustc_span::symbol::Ident; + +declare_clippy_lint! { + /// ### What it does + /// Looks for methods in trait impls and struct impls with the same name, + /// as well as method call sites. + /// + /// ### Why is this bad? + /// It is easy to accidentally override a trait method impl with a method + /// of the same name in a struct impl. Inherent methods are preferred if + /// the call site is unqualified, and naming conflicts will happen silently. + /// + /// ### Example + /// ```rust + /// trait MyTrait { + /// fn ambiguous(&self); + /// } + /// + /// struct Base; + /// + /// impl Base { + /// fn ambiguous(&self) { + /// println!("struct impl"); + /// } + /// } + /// + /// impl MyTrait for Base { + /// fn ambiguous(&self) { + /// println!("trait impl"); + /// } + /// } + /// + /// Base.ambiguous(); // prints "struct impl" + /// ``` + /// Use instead: + /// ```rust + /// trait MyTrait { + /// fn ambiguous(&self); + /// } + /// + /// struct Base; + /// + /// impl Base { + /// fn unambiguous(&self) { + /// println!("struct impl"); + /// } + /// } + /// + /// impl MyTrait for Base { + /// fn ambiguous(&self) { + /// println!("trait impl"); + /// } + /// } + /// + /// Base.unambiguous(); // prints "struct impl" + /// Base.ambiguous(); // prints "trait impl" + /// ``` + #[clippy::version = "1.74.0"] + pub AMBIGUOUS_METHOD_NAMES, + pedantic, + "declarations and calls for same-named methods in struct impls and trait impls" +} + +#[derive(Clone)] +pub struct AmbiguousMethodNames { + // Keeps track of trait methods + trait_methods: Vec<(DefId, Ident)>, + // Keeps track of inherent methods + inherent_methods: Vec<(DefId, Ident)>, +} + +impl AmbiguousMethodNames { + pub fn new() -> Self { + Self { + trait_methods: Vec::default(), + inherent_methods: Vec::default(), + } + } +} + +impl_lint_pass!(AmbiguousMethodNames => [AMBIGUOUS_METHOD_NAMES]); + +impl<'tcx> LateLintPass<'tcx> for AmbiguousMethodNames { + // Check trait impls + fn check_item(&mut self, _: &LateContext<'tcx>, item: &'tcx Item<'_>) { + if let hir::ItemKind::Trait(_, _, _, _, tr_items) = item.kind { + for tr_item in tr_items { + if let hir::AssocItemKind::Fn { .. } = tr_item.kind { + self.trait_methods + .push((item.owner_id.def_id.to_def_id(), tr_item.ident)); + } + } + } + } + + // Check inherent methods + fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'_>) { + if let hir::ImplItemKind::Fn(..) = impl_item.kind { + let hir_id = cx.tcx.hir().local_def_id_to_hir_id(impl_item.owner_id.def_id); + if !is_trait_impl_item(cx, hir_id) { + let struct_id = cx.tcx.hir().get_parent_item(hir_id); + self.inherent_methods + .push((struct_id.def_id.to_def_id(), impl_item.ident)); + } + } + } + + fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { + for (r#trait, ident) in &self.trait_methods { + for (r#struct, inherent_ident) in &self.inherent_methods { + let struct_ty = cx.tcx.type_of(r#struct).skip_binder(); + let trait_ref = ty::TraitRef::identity(cx.tcx, *r#trait); + let args = &trait_ref.args[1..]; + if implements_trait( + cx, + struct_ty, + *r#trait, + &args[..cx.tcx.generics_of(r#trait).params.len() - 1], + ) && ident.name == inherent_ident.name + { + span_lint_and_note( + cx, + AMBIGUOUS_METHOD_NAMES, + inherent_ident.span, + "ambiguous inherent method name", + Some(ident.span), + "trait method defined here", + ); + } + } + } + } +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index b4b84b36044c..9e0eabddbab5 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -40,6 +40,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::absolute_paths::ABSOLUTE_PATHS_INFO, crate::allow_attributes::ALLOW_ATTRIBUTES_INFO, crate::almost_complete_range::ALMOST_COMPLETE_RANGE_INFO, + crate::ambiguous_method_names::AMBIGUOUS_METHOD_NAMES_INFO, crate::approx_const::APPROX_CONSTANT_INFO, crate::arc_with_non_send_sync::ARC_WITH_NON_SEND_SYNC_INFO, crate::as_conversions::AS_CONVERSIONS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 63a3fbcb8976..c20a24ac3b76 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -70,6 +70,7 @@ mod renamed_lints; mod absolute_paths; mod allow_attributes; mod almost_complete_range; +mod ambiguous_method_names; mod approx_const; mod arc_with_non_send_sync; mod as_conversions; @@ -1121,6 +1122,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: )) }); store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv()))); + store.register_late_pass(|_| Box::new(ambiguous_method_names::AmbiguousMethodNames::new())); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/ambiguous_method_names.rs b/tests/ui/ambiguous_method_names.rs new file mode 100644 index 000000000000..4b1963494963 --- /dev/null +++ b/tests/ui/ambiguous_method_names.rs @@ -0,0 +1,81 @@ +#![allow(dead_code)] +#![warn(clippy::ambiguous_method_names)] + +fn main() {} + +fn ambiguous() {} + +trait MyTrait { + fn ambiguous(&self); + fn also_ambiguous(&self); + fn ambiguous_default(&self) {} +} + +trait Another { + fn another(&self); +} + +trait G { + fn g(&self, a: T, b: U); +} + +struct A; + +impl A { + fn ambiguous(&self) {} + fn also_ambiguous(&self) {} + fn ambiguous_default(&self) {} + fn unambiguous(&self) {} + fn another(&self) {} +} + +impl MyTrait for A { + fn ambiguous(&self) {} + fn also_ambiguous(&self) {} +} + +impl Another for A { + fn another(&self) {} +} + +struct B; + +impl B { + fn ambiguous(&self) {} + fn also_ambiguous(&self) {} + fn ambiguous_default(&self) {} + fn another(&self) {} +} + +impl MyTrait for B { + fn ambiguous(&self) {} + fn also_ambiguous(&self) {} +} + +impl Another for B { + fn another(&self) {} +} + +struct C; + +impl MyTrait for C { + fn ambiguous(&self) {} + fn also_ambiguous(&self) {} +} + +struct D; + +impl D { + fn ambiguous(&self) {} + fn also_ambiguous(&self) {} +} + +struct S(T); + +impl S { + fn f(&self) {} +} + +impl S { + fn f(&self) {} +} diff --git a/tests/ui/ambiguous_method_names.stderr b/tests/ui/ambiguous_method_names.stderr new file mode 100644 index 000000000000..bc1dbb6e95b8 --- /dev/null +++ b/tests/ui/ambiguous_method_names.stderr @@ -0,0 +1,100 @@ +error: ambiguous inherent method name + --> $DIR/ambiguous_method_names.rs:25:8 + | +LL | fn ambiguous(&self) {} + | ^^^^^^^^^ + | +note: trait method defined here + --> $DIR/ambiguous_method_names.rs:9:8 + | +LL | fn ambiguous(&self); + | ^^^^^^^^^ + = note: `-D clippy::ambiguous-method-names` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::ambiguous_method_names)]` + +error: ambiguous inherent method name + --> $DIR/ambiguous_method_names.rs:44:8 + | +LL | fn ambiguous(&self) {} + | ^^^^^^^^^ + | +note: trait method defined here + --> $DIR/ambiguous_method_names.rs:9:8 + | +LL | fn ambiguous(&self); + | ^^^^^^^^^ + +error: ambiguous inherent method name + --> $DIR/ambiguous_method_names.rs:26:8 + | +LL | fn also_ambiguous(&self) {} + | ^^^^^^^^^^^^^^ + | +note: trait method defined here + --> $DIR/ambiguous_method_names.rs:10:8 + | +LL | fn also_ambiguous(&self); + | ^^^^^^^^^^^^^^ + +error: ambiguous inherent method name + --> $DIR/ambiguous_method_names.rs:45:8 + | +LL | fn also_ambiguous(&self) {} + | ^^^^^^^^^^^^^^ + | +note: trait method defined here + --> $DIR/ambiguous_method_names.rs:10:8 + | +LL | fn also_ambiguous(&self); + | ^^^^^^^^^^^^^^ + +error: ambiguous inherent method name + --> $DIR/ambiguous_method_names.rs:27:8 + | +LL | fn ambiguous_default(&self) {} + | ^^^^^^^^^^^^^^^^^ + | +note: trait method defined here + --> $DIR/ambiguous_method_names.rs:11:8 + | +LL | fn ambiguous_default(&self) {} + | ^^^^^^^^^^^^^^^^^ + +error: ambiguous inherent method name + --> $DIR/ambiguous_method_names.rs:46:8 + | +LL | fn ambiguous_default(&self) {} + | ^^^^^^^^^^^^^^^^^ + | +note: trait method defined here + --> $DIR/ambiguous_method_names.rs:11:8 + | +LL | fn ambiguous_default(&self) {} + | ^^^^^^^^^^^^^^^^^ + +error: ambiguous inherent method name + --> $DIR/ambiguous_method_names.rs:29:8 + | +LL | fn another(&self) {} + | ^^^^^^^ + | +note: trait method defined here + --> $DIR/ambiguous_method_names.rs:15:8 + | +LL | fn another(&self); + | ^^^^^^^ + +error: ambiguous inherent method name + --> $DIR/ambiguous_method_names.rs:47:8 + | +LL | fn another(&self) {} + | ^^^^^^^ + | +note: trait method defined here + --> $DIR/ambiguous_method_names.rs:15:8 + | +LL | fn another(&self); + | ^^^^^^^ + +error: aborting due to 8 previous errors +