Auto merge of #12459 - y21:unconditional_recursion_from_into, r=Jarcho

lint when calling the blanket `Into` impl from a `From` impl

Closes #11150
```
warning: function cannot return without recursing
  --> x.rs:9:9
   |
9  | /         fn from(value: f32) -> Self {
10 | |             value.into()
11 | |         }
   | |_________^
   |
note: recursive call site
  --> x.rs:10:13
   |
10 |             value.into()
   |             ^^^^^^^^^^^^
```

I'm also thinking that we can probably generalize this lint to #11032 at some point (instead of hardcoding a bunch of impls), like how rustc's `unconditional_recursion` works, at least up to one indirect call, but this still seems useful for now :)

I've also noticed that we use `fn_def_id` in a bunch of lints and then try to get the node args of the call in a separate step, so I made a helper function that does both in one. I intend to refactor a bunch of uses of `fn_def_id` to use this later

I can add more test cases, but this is already using much of the same logic that exists for the other impls that this lint looks for (e.g. making sure that there are no conditional returns).

changelog: [`unconditional_recursion`]: emit a warning inside of `From::from` when unconditionally calling the blanket `.into()` impl

Author: bors

clippy_lints/src/unconditional_recursion.rs

@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::{expr_or_init, get_trait_def_id, path_def_id};
+use clippy_utils::{expr_or_init, fn_def_id_with_node_args, path_def_id};
 use rustc_ast::BinOpKind;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_hir as hir;
@@ -19,11 +19,11 @@ use rustc_trait_selection::traits::error_reporting::suggestions::ReturnsVisitor;
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks that there isn't an infinite recursion in `PartialEq` trait
-    /// implementation.
+    /// Checks that there isn't an infinite recursion in trait
+    /// implementations.
     ///
     /// ### Why is this bad?
-    /// This is a hard to find infinite recursion which will crashing any code
+    /// This is a hard to find infinite recursion that will crash any code.
     /// using it.
     ///
     /// ### Example
@@ -201,7 +201,6 @@ fn check_partial_eq(cx: &LateContext<'_>, method_span: Span, method_def_id: Loca
     }
 }
 
-#[allow(clippy::unnecessary_def_path)]
 fn check_to_string(cx: &LateContext<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident, expr: &Expr<'_>) {
     let args = cx
         .tcx
@@ -224,7 +223,7 @@ fn check_to_string(cx: &LateContext<'_>, method_span: Span, method_def_id: Local
         && 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"])
+        && cx.tcx.is_diagnostic_item(sym::ToString, trait_def_id)
     {
         let is_bad = match expr.kind {
             ExprKind::MethodCall(segment, _receiver, &[_arg], _) if segment.ident.name == name.name => {
@@ -291,7 +290,6 @@ where
         self.map
     }
 
-    #[allow(clippy::unnecessary_def_path)]
     fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
         if self.found_default_call {
             return;
@@ -303,7 +301,7 @@ where
             && is_default_method_on_current_ty(self.cx.tcx, qpath, self.implemented_ty_id)
             && let Some(method_def_id) = path_def_id(self.cx, f)
             && let Some(trait_def_id) = self.cx.tcx.trait_of_item(method_def_id)
-            && Some(trait_def_id) == get_trait_def_id(self.cx, &["core", "default", "Default"])
+            && self.cx.tcx.is_diagnostic_item(sym::Default, trait_def_id)
         {
             self.found_default_call = true;
             span_error(self.cx, self.method_span, expr);
@@ -312,10 +310,9 @@ where
 }
 
 impl UnconditionalRecursion {
-    #[allow(clippy::unnecessary_def_path)]
     fn init_default_impl_for_type_if_needed(&mut self, cx: &LateContext<'_>) {
         if self.default_impl_for_type.is_empty()
-            && let Some(default_trait_id) = get_trait_def_id(cx, &["core", "default", "Default"])
+            && let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
         {
             let impls = cx.tcx.trait_impls_of(default_trait_id);
             for (ty, impl_def_ids) in impls.non_blanket_impls() {
@@ -394,6 +391,34 @@ impl UnconditionalRecursion {
     }
 }
 
+fn check_from(cx: &LateContext<'_>, method_span: Span, method_def_id: LocalDefId, expr: &Expr<'_>) {
+    let Some(sig) = cx
+        .typeck_results()
+        .liberated_fn_sigs()
+        .get(cx.tcx.local_def_id_to_hir_id(method_def_id))
+    else {
+        return;
+    };
+
+    // Check if we are calling `Into::into` where the node args match with our `From::from` signature:
+    // From::from signature: fn(S1) -> S2
+    // <S1 as Into<S2>>::into(s1), node_args=[S1, S2]
+    // If they do match, then it must mean that it is the blanket impl,
+    // which calls back into our `From::from` again (`Into` is not specializable).
+    // rustc's unconditional_recursion already catches calling `From::from` directly
+    if let Some((fn_def_id, node_args)) = fn_def_id_with_node_args(cx, expr)
+        && let [s1, s2] = **node_args
+        && let (Some(s1), Some(s2)) = (s1.as_type(), s2.as_type())
+        && let Some(trait_def_id) = cx.tcx.trait_of_item(fn_def_id)
+        && cx.tcx.is_diagnostic_item(sym::Into, trait_def_id)
+        && get_impl_trait_def_id(cx, method_def_id) == cx.tcx.get_diagnostic_item(sym::From)
+        && s1 == sig.inputs()[0]
+        && s2 == sig.output()
+    {
+        span_error(cx, method_span, expr);
+    }
+}
+
 impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion {
     fn check_fn(
         &mut self,
@@ -410,10 +435,11 @@ impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion {
             // Doesn't have a conditional return.
             && !has_conditional_return(body, expr)
         {
-            if name.name == sym::eq || name.name == sym::ne {
-                check_partial_eq(cx, method_span, method_def_id, name, expr);
-            } else if name.name == sym::to_string {
-                check_to_string(cx, method_span, method_def_id, name, expr);
+            match name.name {
+                sym::eq | sym::ne => check_partial_eq(cx, method_span, method_def_id, name, expr),
+                sym::to_string => check_to_string(cx, method_span, method_def_id, name, expr),
+                sym::from => check_from(cx, method_span, method_def_id, expr),
+                _ => {},
             }
             self.check_default_new(cx, decl, body, method_span, method_def_id);
         }

clippy_utils/src/lib.rs

@@ -2249,8 +2249,21 @@ pub fn fn_has_unsatisfiable_preds(cx: &LateContext<'_>, did: DefId) -> bool {
 
 /// Returns the `DefId` of the callee if the given expression is a function or method call.
 pub fn fn_def_id(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<DefId> {
+    fn_def_id_with_node_args(cx, expr).map(|(did, _)| did)
+}
+
+/// Returns the `DefId` of the callee if the given expression is a function or method call,
+/// as well as its node args.
+pub fn fn_def_id_with_node_args<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &Expr<'_>,
+) -> Option<(DefId, rustc_ty::GenericArgsRef<'tcx>)> {
+    let typeck = cx.typeck_results();
     match &expr.kind {
-        ExprKind::MethodCall(..) => cx.typeck_results().type_dependent_def_id(expr.hir_id),
+        ExprKind::MethodCall(..) => Some((
+            typeck.type_dependent_def_id(expr.hir_id)?,
+            typeck.node_args(expr.hir_id),
+        )),
         ExprKind::Call(
             Expr {
                 kind: ExprKind::Path(qpath),
@@ -2262,9 +2275,9 @@ pub fn fn_def_id(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<DefId> {
             // Only return Fn-like DefIds, not the DefIds of statics/consts/etc that contain or
             // deref to fn pointers, dyn Fn, impl Fn - #8850
             if let Res::Def(DefKind::Fn | DefKind::Ctor(..) | DefKind::AssocFn, id) =
-                cx.typeck_results().qpath_res(qpath, *path_hir_id)
+                typeck.qpath_res(qpath, *path_hir_id)
             {
-                Some(id)
+                Some((id, typeck.node_args(*path_hir_id)))
             } else {
                 None
             }

tests/ui/unconditional_recursion.rs

@@ -1,7 +1,11 @@
 //@no-rustfix
 
 #![warn(clippy::unconditional_recursion)]
-#![allow(clippy::partialeq_ne_impl, clippy::default_constructed_unit_structs)]
+#![allow(
+    clippy::partialeq_ne_impl,
+    clippy::default_constructed_unit_structs,
+    clippy::only_used_in_recursion
+)]
 
 enum Foo {
     A,
@@ -350,4 +354,48 @@ mod issue12154 {
     }
 }
 
+// From::from -> Into::into -> From::from
+struct BadFromTy1<'a>(&'a ());
+struct BadIntoTy1<'b>(&'b ());
+impl<'a> From<BadFromTy1<'a>> for BadIntoTy1<'static> {
+    fn from(f: BadFromTy1<'a>) -> Self {
+        f.into()
+    }
+}
+
+// Using UFCS syntax
+struct BadFromTy2<'a>(&'a ());
+struct BadIntoTy2<'b>(&'b ());
+impl<'a> From<BadFromTy2<'a>> for BadIntoTy2<'static> {
+    fn from(f: BadFromTy2<'a>) -> Self {
+        Into::into(f)
+    }
+}
+
+// Different Into impl (<i16 as Into<i32>>), so no infinite recursion
+struct BadFromTy3;
+impl From<BadFromTy3> for i32 {
+    fn from(f: BadFromTy3) -> Self {
+        Into::into(1i16)
+    }
+}
+
+// A conditional return that ends the recursion
+struct BadFromTy4;
+impl From<BadFromTy4> for i32 {
+    fn from(f: BadFromTy4) -> Self {
+        if true {
+            return 42;
+        }
+        f.into()
+    }
+}
+
+// Types differ in refs, don't lint
+impl From<&BadFromTy4> for i32 {
+    fn from(f: &BadFromTy4) -> Self {
+        BadFromTy4.into()
+    }
+}
+
 fn main() {}