From 42d13f8eb0b5ed893ffd92c81f10c43499cb3cad Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 20 Jan 2024 20:26:06 +0100 Subject: [PATCH 1/2] [`unconditional_recursion`]: compare by types instead of `DefId`s --- clippy_lints/src/unconditional_recursion.rs | 86 +++++++++++---------- tests/ui/unconditional_recursion.rs | 35 +++++++++ 2 files changed, 82 insertions(+), 39 deletions(-) diff --git a/clippy_lints/src/unconditional_recursion.rs b/clippy_lints/src/unconditional_recursion.rs index 209035804e4..224ec475c51 100644 --- a/clippy_lints/src/unconditional_recursion.rs +++ b/clippy_lints/src/unconditional_recursion.rs @@ -69,14 +69,6 @@ fn span_error(cx: &LateContext<'_>, method_span: Span, expr: &Expr<'_>) { ); } -fn get_ty_def_id(ty: Ty<'_>) -> Option { - match ty.peel_refs().kind() { - ty::Adt(adt, _) => Some(adt.did()), - ty::Foreign(def_id) => Some(*def_id), - _ => None, - } -} - fn get_hir_ty_def_id<'tcx>(tcx: TyCtxt<'tcx>, hir_ty: rustc_hir::Ty<'tcx>) -> Option { let TyKind::Path(qpath) = hir_ty.kind else { return None }; match qpath { @@ -131,21 +123,49 @@ fn get_impl_trait_def_id(cx: &LateContext<'_>, method_def_id: LocalDefId) -> Opt } } -#[allow(clippy::unnecessary_def_path)] +/// When we have `x == y` where `x = &T` and `y = &T`, then that resolves to +/// `<&T as PartialEq<&T>>::eq`, which is not the same as `>::eq`, +/// however we still would want to treat it the same, because we know that it's a blanket impl +/// that simply delegates to the `PartialEq` impl with one reference removed. +/// +/// Still, we can't just do `lty.peel_refs() == rty.peel_refs()` because when we have `x = &T` and +/// `y = &&T`, this is not necessarily the same as `>::eq` +/// +/// So to avoid these FNs and FPs, we keep removing a layer of references from *both* sides +/// until both sides match the expected LHS and RHS type (or they don't). +fn matches_ty<'tcx>( + mut left: Ty<'tcx>, + mut right: Ty<'tcx>, + expected_left: Ty<'tcx>, + expected_right: Ty<'tcx>, +) -> bool { + while let (&ty::Ref(_, lty, _), &ty::Ref(_, rty, _)) = (left.kind(), right.kind()) { + if lty == expected_left && rty == expected_right { + return true; + } + left = lty; + right = rty; + } + false +} + fn check_partial_eq(cx: &LateContext<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident, expr: &Expr<'_>) { - let args = cx - .tcx - .instantiate_bound_regions_with_erased(cx.tcx.fn_sig(method_def_id).skip_binder()) - .inputs(); + let Some(sig) = cx + .typeck_results() + .liberated_fn_sigs() + .get(cx.tcx.local_def_id_to_hir_id(method_def_id)) + else { + return; + }; + // That has two arguments. - if let [self_arg, other_arg] = args - && let Some(self_arg) = get_ty_def_id(*self_arg) - && let Some(other_arg) = get_ty_def_id(*other_arg) + if let [self_arg, other_arg] = sig.inputs() + && let &ty::Ref(_, self_arg, _) = self_arg.kind() + && let &ty::Ref(_, other_arg, _) = other_arg.kind() // The two arguments are of the same type. - && self_arg == other_arg && let Some(trait_def_id) = get_impl_trait_def_id(cx, method_def_id) // The trait is `PartialEq`. - && Some(trait_def_id) == get_trait_def_id(cx, &["core", "cmp", "PartialEq"]) + && cx.tcx.is_diagnostic_item(sym::PartialEq, trait_def_id) { let to_check_op = if name.name == sym::eq { BinOpKind::Eq @@ -154,31 +174,19 @@ fn check_partial_eq(cx: &LateContext<'_>, method_span: Span, method_def_id: Loca }; let is_bad = match expr.kind { ExprKind::Binary(op, left, right) if op.node == to_check_op => { - // Then we check if the left-hand element is of the same type as `self`. - if let Some(left_ty) = cx.typeck_results().expr_ty_opt(left) - && let Some(left_id) = get_ty_def_id(left_ty) - && self_arg == left_id - && let Some(right_ty) = cx.typeck_results().expr_ty_opt(right) - && let Some(right_id) = get_ty_def_id(right_ty) - && other_arg == right_id - { - true - } else { - false - } + // Then we check if the LHS matches self_arg and RHS matches other_arg + let left_ty = cx.typeck_results().expr_ty_adjusted(left); + let right_ty = cx.typeck_results().expr_ty_adjusted(right); + matches_ty(left_ty, right_ty, self_arg, other_arg) }, - ExprKind::MethodCall(segment, receiver, &[_arg], _) if segment.ident.name == name.name => { - if let Some(ty) = cx.typeck_results().expr_ty_opt(receiver) - && let Some(ty_id) = get_ty_def_id(ty) - && self_arg != ty_id - { - // Since this called on a different type, the lint should not be - // triggered here. - return; - } + ExprKind::MethodCall(segment, receiver, [arg], _) if segment.ident.name == name.name => { + let receiver_ty = cx.typeck_results().expr_ty_adjusted(receiver); + let arg_ty = cx.typeck_results().expr_ty_adjusted(arg); + if let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) && let Some(trait_id) = cx.tcx.trait_of_item(fn_id) && trait_id == trait_def_id + && matches_ty(receiver_ty, arg_ty, self_arg, other_arg) { true } else { diff --git a/tests/ui/unconditional_recursion.rs b/tests/ui/unconditional_recursion.rs index 7b898a6e0e7..2ad7c410da5 100644 --- a/tests/ui/unconditional_recursion.rs +++ b/tests/ui/unconditional_recursion.rs @@ -288,4 +288,39 @@ impl PartialEq for S15<'_> { } } +mod issue12154 { + struct MyBox(T); + + impl std::ops::Deref for MyBox { + type Target = T; + fn deref(&self) -> &T { + &self.0 + } + } + + impl PartialEq for MyBox { + fn eq(&self, other: &Self) -> bool { + (**self).eq(&**other) + } + } + + // Not necessarily related to the issue but another FP from the http crate that was fixed with it: + // https://docs.rs/http/latest/src/http/header/name.rs.html#1424 + // We used to simply peel refs from the LHS and RHS, so we couldn't differentiate + // between `PartialEq for &T` and `PartialEq<&T> for T` impls. + #[derive(PartialEq)] + struct HeaderName; + impl<'a> PartialEq<&'a HeaderName> for HeaderName { + fn eq(&self, other: &&'a HeaderName) -> bool { + *self == **other + } + } + + impl<'a> PartialEq for &'a HeaderName { + fn eq(&self, other: &HeaderName) -> bool { + *other == *self + } + } +} + fn main() {} From 87a6300b22725bfea57f0e4fb95ccf70ed7d8cc8 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 21 Jan 2024 13:33:03 +0100 Subject: [PATCH 2/2] add a test for rust-lang/rust-clippy#12181 --- tests/ui/unconditional_recursion.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/ui/unconditional_recursion.rs b/tests/ui/unconditional_recursion.rs index 2ad7c410da5..263fdf26d4d 100644 --- a/tests/ui/unconditional_recursion.rs +++ b/tests/ui/unconditional_recursion.rs @@ -321,6 +321,30 @@ mod issue12154 { *other == *self } } + + // Issue #12181 but also fixed by the same PR + struct Foo; + + impl Foo { + fn as_str(&self) -> &str { + "Foo" + } + } + + impl PartialEq for Foo { + fn eq(&self, other: &Self) -> bool { + self.as_str().eq(other.as_str()) + } + } + + impl PartialEq for Foo + where + for<'a> &'a str: PartialEq, + { + fn eq(&self, other: &T) -> bool { + (&self.as_str()).eq(other) + } + } } fn main() {}