Auto merge of #11980 - GuillaumeGomez:UNCONDITIONAL_RECURSION-tostring, r=llogiq

Extend UNCONDITIONAL_RECURSION to check for ToString implementations

Follow-up of https://github.com/rust-lang/rust-clippy/pull/11938.

r? `@llogiq`

changelog: Extend `UNCONDITIONAL_RECURSION` to check for `ToString` implementations

clippy_lints/src/unconditional_recursion.rs

@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::{get_trait_def_id, path_res};
+use clippy_utils::{expr_or_init, get_trait_def_id, path_res};
 use rustc_ast::BinOpKind;
 use rustc_hir::def::Res;
 use rustc_hir::def_id::{DefId, LocalDefId};
@@ -8,6 +8,7 @@ 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};
 
 declare_clippy_lint! {
@@ -55,8 +56,127 @@ fn is_local(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     matches!(path_res(cx, expr), Res::Local(_))
 }
 
+#[allow(clippy::unnecessary_def_path)]
+fn check_partial_eq(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident) {
+    let args = cx
+        .tcx
+        .instantiate_bound_regions_with_erased(cx.tcx.fn_sig(method_def_id).skip_binder())
+        .inputs();
+    // 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)
+        // The two arguments are of the same type.
+        && self_arg == other_arg
+        && let hir_id = cx.tcx.local_def_id_to_hir_id(method_def_id)
+        && let Some((
+            _,
+            Node::Item(Item {
+                kind: ItemKind::Impl(impl_),
+                owner_id,
+                ..
+            }),
+        )) = cx.tcx.hir().parent_iter(hir_id).next()
+        // We exclude `impl` blocks generated from rustc's proc macros.
+        && !cx.tcx.has_attr(*owner_id, sym::automatically_derived)
+        // It is a implementation of a trait.
+        && let Some(trait_) = impl_.of_trait
+        && let Some(trait_def_id) = trait_.trait_def_id()
+        // The trait is `PartialEq`.
+        && Some(trait_def_id) == get_trait_def_id(cx, &["core", "cmp", "PartialEq"])
+    {
+        let to_check_op = if name.name == sym::eq {
+            BinOpKind::Eq
+        } 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::MethodCall(segment, receiver, &[arg], _) if segment.ident.name == name.name => {
+                if is_local(cx, receiver)
+                    && 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
+                {
+                    true
+                } else {
+                    false
+                }
+            },
+            _ => false,
+        };
+        if is_bad {
+            span_lint_and_then(
+                cx,
+                UNCONDITIONAL_RECURSION,
+                method_span,
+                "function cannot return without recursing",
+                |diag| {
+                    diag.span_note(expr.span, "recursive call site");
+                },
+            );
+        }
+    }
+}
+
+#[allow(clippy::unnecessary_def_path)]
+fn check_to_string(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident) {
+    let args = cx
+        .tcx
+        .instantiate_bound_regions_with_erased(cx.tcx.fn_sig(method_def_id).skip_binder())
+        .inputs();
+    // That has one argument.
+    if let [_self_arg] = args
+        && let hir_id = cx.tcx.local_def_id_to_hir_id(method_def_id)
+        && let Some((
+            _,
+            Node::Item(Item {
+                kind: ItemKind::Impl(impl_),
+                owner_id,
+                ..
+            }),
+        )) = cx.tcx.hir().parent_iter(hir_id).next()
+        // We exclude `impl` blocks generated from rustc's proc macros.
+        && !cx.tcx.has_attr(*owner_id, sym::automatically_derived)
+        // It is a implementation of a trait.
+        && let Some(trait_) = impl_.of_trait
+        && let Some(trait_def_id) = trait_.trait_def_id()
+        // 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 => {
+                if is_local(cx, receiver)
+                    && 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
+                {
+                    true
+                } else {
+                    false
+                }
+            },
+            _ => false,
+        };
+        if is_bad {
+            span_lint_and_then(
+                cx,
+                UNCONDITIONAL_RECURSION,
+                method_span,
+                "function cannot return without recursing",
+                |diag| {
+                    diag.span_note(expr.span, "recursive call site");
+                },
+            );
+        }
+    }
+}
+
 impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion {
-    #[allow(clippy::unnecessary_def_path)]
     fn check_fn(
         &mut self,
         cx: &LateContext<'tcx>,
@@ -64,70 +184,14 @@ impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion {
         _decl: &'tcx FnDecl<'tcx>,
         body: &'tcx Body<'tcx>,
         method_span: Span,
-        def_id: LocalDefId,
+        method_def_id: LocalDefId,
     ) {
         // If the function is a method...
-        if let FnKind::Method(name, _) = kind
+        if let FnKind::Method(name, _) = kind {
-            // That has two arguments.
+            if name.name == sym::eq || name.name == sym::ne {
-            && let [self_arg, other_arg] = cx
+                check_partial_eq(cx, body, method_span, method_def_id, name);
-                .tcx
+            } else if name.name == sym::to_string {
-                .instantiate_bound_regions_with_erased(cx.tcx.fn_sig(def_id).skip_binder())
+                check_to_string(cx, body, method_span, method_def_id, name);
-                .inputs()
-            && let Some(self_arg) = get_ty_def_id(*self_arg)
-            && let Some(other_arg) = get_ty_def_id(*other_arg)
-            // The two arguments are of the same type.
-            && self_arg == other_arg
-            && let hir_id = cx.tcx.local_def_id_to_hir_id(def_id)
-            && let Some((
-                _,
-                Node::Item(Item {
-                    kind: ItemKind::Impl(impl_),
-                    owner_id,
-                    ..
-                }),
-            )) = cx.tcx.hir().parent_iter(hir_id).next()
-            // We exclude `impl` blocks generated from rustc's proc macros.
-            && !cx.tcx.has_attr(*owner_id, sym::automatically_derived)
-            // It is a implementation of a trait.
-            && let Some(trait_) = impl_.of_trait
-            && let Some(trait_def_id) = trait_.trait_def_id()
-            // The trait is `PartialEq`.
-            && Some(trait_def_id) == get_trait_def_id(cx, &["core", "cmp", "PartialEq"])
-        {
-            let to_check_op = if name.name == sym::eq {
-                BinOpKind::Eq
-            } 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::MethodCall(segment, receiver, &[arg], _) if segment.ident.name == name.name => {
-                    if is_local(cx, receiver)
-                        && 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
-                    {
-                        true
-                    } else {
-                        false
-                    }
-                },
-                _ => false,
-            };
-            if is_bad {
-                span_lint_and_then(
-                    cx,
-                    UNCONDITIONAL_RECURSION,
-                    method_span,
-                    "function cannot return without recursing",
-                    |diag| {
-                        diag.span_note(expr.span, "recursive call site");
-                    },
-                );
             }
         }
     }

tests/ui/unconditional_recursion.rs

@@ -158,6 +158,34 @@ struct S5;
 impl_partial_eq!(S5);
 //~^ ERROR: function cannot return without recursing
 
+struct S6;
+
+impl std::string::ToString for S6 {
+    fn to_string(&self) -> String {
+        //~^ ERROR: function cannot return without recursing
+        self.to_string()
+    }
+}
+
+struct S7;
+
+impl std::string::ToString for S7 {
+    fn to_string(&self) -> String {
+        //~^ ERROR: function cannot return without recursing
+        let x = self;
+        x.to_string()
+    }
+}
+
+struct S8;
+
+impl std::string::ToString for S8 {
+    fn to_string(&self) -> String {
+        //~^ ERROR: function cannot return without recursing
+        (self as &Self).to_string()
+    }
+}
+
 fn main() {
     // test code goes here
 }

tests/ui/unconditional_recursion.stderr

@@ -22,6 +22,39 @@ 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
+   |
+LL |     fn to_string(&self) -> String {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
+LL |
+LL |         self.to_string()
+   |         ---------------- recursive call site
+   |
+   = help: a `loop` may express intention better if this is on purpose
+
+error: function cannot return without recursing
+  --> $DIR/unconditional_recursion.rs:173:5
+   |
+LL |     fn to_string(&self) -> String {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
+...
+LL |         x.to_string()
+   |         ------------- recursive call site
+   |
+   = help: a `loop` may express intention better if this is on purpose
+
+error: function cannot return without recursing
+  --> $DIR/unconditional_recursion.rs:183:5
+   |
+LL |     fn to_string(&self) -> String {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
+LL |
+LL |         (self as &Self).to_string()
+   |         --------------------------- recursive call site
+   |
+   = help: a `loop` may express intention better if this is on purpose
+
 error: function cannot return without recursing
   --> $DIR/unconditional_recursion.rs:12:5
    |
@@ -247,5 +280,5 @@ 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 19 previous errors
+error: aborting due to 22 previous errors