Auto merge of #12062 - GuillaumeGomez:fix-false-positive-unconditional_recursion, r=xFrednet

Fix false positive `unconditional_recursion`

Fixes #12052.

Only checking if both variables are `local` was not enough, we also need to confirm they have the same type as `Self`.

changelog: Fix false positive for `unconditional_recursion` lint

clippy_lints/src/unconditional_recursion.rs

@@ -1,15 +1,15 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::{expr_or_init, get_trait_def_id, path_res};
+use clippy_utils::{expr_or_init, get_trait_def_id};
 use rustc_ast::BinOpKind;
-use rustc_hir::def::Res;
 use rustc_hir::def_id::{DefId, LocalDefId};
-use rustc_hir::intravisit::FnKind;
+use rustc_hir::intravisit::{walk_body, FnKind};
 use rustc_hir::{Body, Expr, ExprKind, FnDecl, Item, ItemKind, Node};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::{self, Ty};
 use rustc_session::declare_lint_pass;
 use rustc_span::symbol::Ident;
 use rustc_span::{sym, Span};
+use rustc_trait_selection::traits::error_reporting::suggestions::ReturnsVisitor;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -52,12 +52,19 @@ fn get_ty_def_id(ty: Ty<'_>) -> Option<DefId> {
     }
 }
 
-fn is_local(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+fn has_conditional_return(body: &Body<'_>, expr: &Expr<'_>) -> bool {
-    matches!(path_res(cx, expr), Res::Local(_))
+    let mut visitor = ReturnsVisitor::default();
+
+    walk_body(&mut visitor, body);
+    match visitor.returns.as_slice() {
+        [] => false,
+        [return_expr] => return_expr.hir_id != expr.hir_id,
+        _ => true,
+    }
 }
 
 #[allow(clippy::unnecessary_def_path)]
-fn check_partial_eq(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident) {
+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())
@@ -90,13 +97,23 @@ fn check_partial_eq(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, me
         } else {
             BinOpKind::Ne
         };
-        let expr = body.value.peel_blocks();
         let is_bad = match expr.kind {
-            ExprKind::Binary(op, left, right) if op.node == to_check_op => is_local(cx, left) && is_local(cx, right),
+            ExprKind::Binary(op, left, right) if op.node == to_check_op => {
-            ExprKind::MethodCall(segment, receiver, &[arg], _) if segment.ident.name == name.name => {
+                // Then we check if the left-hand element is of the same type as `self`.
-                if is_local(cx, receiver)
+                if let Some(left_ty) = cx.typeck_results().expr_ty_opt(left)
-                    && is_local(cx, &arg)
+                    && let Some(left_id) = get_ty_def_id(left_ty)
-                    && let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
+                    && 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
+                }
+            },
+            ExprKind::MethodCall(segment, _receiver, &[_arg], _) if segment.ident.name == name.name => {
+                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
                 {
@@ -122,7 +139,7 @@ fn check_partial_eq(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, me
 }
 
 #[allow(clippy::unnecessary_def_path)]
-fn check_to_string(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident) {
+fn check_to_string(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())
@@ -146,12 +163,9 @@ fn check_to_string(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, met
         // The trait is `ToString`.
         && Some(trait_def_id) == get_trait_def_id(cx, &["alloc", "string", "ToString"])
     {
-        let expr = expr_or_init(cx, body.value.peel_blocks());
         let is_bad = match expr.kind {
-            ExprKind::MethodCall(segment, receiver, &[arg], _) if segment.ident.name == name.name => {
+            ExprKind::MethodCall(segment, _receiver, &[_arg], _) if segment.ident.name == name.name => {
-                if is_local(cx, receiver)
+                if let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
-                    && is_local(cx, &arg)
-                    && 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
                 {
@@ -187,11 +201,15 @@ impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion {
         method_def_id: LocalDefId,
     ) {
         // If the function is a method...
-        if let FnKind::Method(name, _) = kind {
+        if let FnKind::Method(name, _) = kind
+            && let expr = expr_or_init(cx, body.value).peel_blocks()
+            // Doesn't have a conditional return.
+            && !has_conditional_return(body, expr)
+        {
             if name.name == sym::eq || name.name == sym::ne {
-                check_partial_eq(cx, body, method_span, method_def_id, name);
+                check_partial_eq(cx, method_span, method_def_id, name, expr);
             } else if name.name == sym::to_string {
-                check_to_string(cx, body, method_span, method_def_id, name);
+                check_to_string(cx, method_span, method_def_id, name, expr);
             }
         }
     }

tests/ui/unconditional_recursion.rs

@@ -158,18 +158,64 @@ struct S5;
 impl_partial_eq!(S5);
 //~^ ERROR: function cannot return without recursing
 
-struct S6;
+struct S6 {
+    field: String,
+}
 
-impl std::string::ToString for S6 {
+impl PartialEq for S6 {
+    fn eq(&self, other: &Self) -> bool {
+        let mine = &self.field;
+        let theirs = &other.field;
+        mine == theirs // Should not warn!
+    }
+}
+
+struct S7<'a> {
+    field: &'a S7<'a>,
+}
+
+impl<'a> PartialEq for S7<'a> {
+    fn eq(&self, other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        let mine = &self.field;
+        let theirs = &other.field;
+        mine == theirs
+    }
+}
+
+struct S8 {
+    num: i32,
+    field: Option<Box<S8>>,
+}
+
+impl PartialEq for S8 {
+    fn eq(&self, other: &Self) -> bool {
+        if self.num != other.num {
+            return false;
+        }
+
+        let (this, other) = match (self.field.as_deref(), other.field.as_deref()) {
+            (Some(x1), Some(x2)) => (x1, x2),
+            (None, None) => return true,
+            _ => return false,
+        };
+
+        this == other
+    }
+}
+
+struct S9;
+
+impl std::string::ToString for S9 {
     fn to_string(&self) -> String {
         //~^ ERROR: function cannot return without recursing
         self.to_string()
     }
 }
 
-struct S7;
+struct S10;
 
-impl std::string::ToString for S7 {
+impl std::string::ToString for S10 {
     fn to_string(&self) -> String {
         //~^ ERROR: function cannot return without recursing
         let x = self;
@@ -177,9 +223,9 @@ impl std::string::ToString for S7 {
     }
 }
 
-struct S8;
+struct S11;
 
-impl std::string::ToString for S8 {
+impl std::string::ToString for S11 {
     fn to_string(&self) -> String {
         //~^ ERROR: function cannot return without recursing
         (self as &Self).to_string()

tests/ui/unconditional_recursion.stderr

@@ -23,7 +23,7 @@ LL |         self.eq(other)
    = help: a `loop` may express intention better if this is on purpose
 
 error: function cannot return without recursing
-  --> $DIR/unconditional_recursion.rs:164:5
+  --> $DIR/unconditional_recursion.rs:210:5
    |
 LL |     fn to_string(&self) -> String {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@@ -34,7 +34,7 @@ LL |         self.to_string()
    = help: a `loop` may express intention better if this is on purpose
 
 error: function cannot return without recursing
-  --> $DIR/unconditional_recursion.rs:173:5
+  --> $DIR/unconditional_recursion.rs:219:5
    |
 LL |     fn to_string(&self) -> String {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@@ -45,7 +45,7 @@ LL |         x.to_string()
    = help: a `loop` may express intention better if this is on purpose
 
 error: function cannot return without recursing
-  --> $DIR/unconditional_recursion.rs:183:5
+  --> $DIR/unconditional_recursion.rs:229:5
    |
 LL |     fn to_string(&self) -> String {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@@ -87,6 +87,34 @@ note: recursive call site
 LL |         self == other
    |         ^^^^^^^^^^^^^
 
+error: function cannot return without recursing
+  --> $DIR/unconditional_recursion.rs:28:5
+   |
+LL | /     fn ne(&self, other: &Self) -> bool {
+LL | |         self != &Foo2::B // no error here
+LL | |     }
+   | |_____^
+   |
+note: recursive call site
+  --> $DIR/unconditional_recursion.rs:29:9
+   |
+LL |         self != &Foo2::B // no error here
+   |         ^^^^^^^^^^^^^^^^
+
+error: function cannot return without recursing
+  --> $DIR/unconditional_recursion.rs:31:5
+   |
+LL | /     fn eq(&self, other: &Self) -> bool {
+LL | |         self == &Foo2::B // no error here
+LL | |     }
+   | |_____^
+   |
+note: recursive call site
+  --> $DIR/unconditional_recursion.rs:32:9
+   |
+LL |         self == &Foo2::B // no error here
+   |         ^^^^^^^^^^^^^^^^
+
 error: function cannot return without recursing
   --> $DIR/unconditional_recursion.rs:42:5
    |
@@ -280,5 +308,22 @@ LL | impl_partial_eq!(S5);
    | -------------------- in this macro invocation
    = note: this error originates in the macro `impl_partial_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error: aborting due to 22 previous errors
+error: function cannot return without recursing
+  --> $DIR/unconditional_recursion.rs:178:5
+   |
+LL | /     fn eq(&self, other: &Self) -> bool {
+LL | |
+LL | |         let mine = &self.field;
+LL | |         let theirs = &other.field;
+LL | |         mine == theirs
+LL | |     }
+   | |_____^
+   |
+note: recursive call site
+  --> $DIR/unconditional_recursion.rs:182:9
+   |
+LL |         mine == theirs
+   |         ^^^^^^^^^^^^^^
+
+error: aborting due to 25 previous errors