Auto merge of #11938 - GuillaumeGomez:unconditional_recursion, r=llogiq

bors · bors · commit 9907b90b1e01 · 2023-12-16T18:21:01.000Z
Add new `unconditional_recursion` lint Currently, rustc `unconditional_recursion` doesn't detect cases like: ```rust enum Foo { A, B, } impl PartialEq for Foo { fn eq(&self, other: &Self) -> bool { self == other } } ``` This is because the lint is currently implemented only for one level, and in the above code, `self == other` will then call `impl PartialEq for &T`, escaping from the detection. The fix for it seems to be a bit tricky (I started investigating potential solution to add one extra level of recursion [here](https://github.com/rust-lang/rust/compare/master...GuillaumeGomez:rust:trait-impl-recursion?expand=1) but completely broken at the moment). I expect that this situation will remain for a while. In the meantime, I think it's acceptable to check it directly into clippy for the time being as a lot of easy cases like this one can be easily checked (next I plan to extend it to cover other traits like `ToString`). changelog: Add new `unconditional_recursion` lint
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5582,6 +5582,7 @@ Released 2018-09-13
 [`type_id_on_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_id_on_box
 [`type_repetition_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds
 [`unchecked_duration_subtraction`]: https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_duration_subtraction
+[`unconditional_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#unconditional_recursion
 [`undocumented_unsafe_blocks`]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
 [`undropped_manually_drops`]: https://rust-lang.github.io/rust-clippy/master/index.html#undropped_manually_drops
 [`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -676,6 +676,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::types::REDUNDANT_ALLOCATION_INFO,
     crate::types::TYPE_COMPLEXITY_INFO,
     crate::types::VEC_BOX_INFO,
+    crate::unconditional_recursion::UNCONDITIONAL_RECURSION_INFO,
     crate::undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS_INFO,
     crate::undocumented_unsafe_blocks::UNNECESSARY_SAFETY_COMMENT_INFO,
     crate::unicode::INVISIBLE_CHARACTERS_INFO,
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -327,6 +327,7 @@ mod trait_bounds;
 mod transmute;
 mod tuple_array_conversions;
 mod types;
+mod unconditional_recursion;
 mod undocumented_unsafe_blocks;
 mod unicode;
 mod uninhabited_references;
@@ -1078,6 +1079,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(repeat_vec_with_capacity::RepeatVecWithCapacity));
     store.register_late_pass(|_| Box::new(uninhabited_references::UninhabitedReferences));
     store.register_late_pass(|_| Box::new(ineffective_open_options::IneffectiveOpenOptions));
+    store.register_late_pass(|_| Box::new(unconditional_recursion::UnconditionalRecursion));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 
diff --git a/clippy_lints/src/unconditional_recursion.rs b/clippy_lints/src/unconditional_recursion.rs
@@ -0,0 +1,134 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::{get_trait_def_id, path_res};
+use rustc_ast::BinOpKind;
+use rustc_hir::def::Res;
+use rustc_hir::def_id::{DefId, LocalDefId};
+use rustc_hir::intravisit::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::{sym, Span};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks that there isn't an infinite recursion in `PartialEq` trait
+    /// implementation.
+    ///
+    /// ### Why is this bad?
+    /// This is a hard to find infinite recursion which will crashing any code
+    /// using it.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// enum Foo {
+    ///     A,
+    ///     B,
+    /// }
+    ///
+    /// impl PartialEq for Foo {
+    ///     fn eq(&self, other: &Self) -> bool {
+    ///         self == other // bad!
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    ///
+    /// In such cases, either use `#[derive(PartialEq)]` or don't implement it.
+    #[clippy::version = "1.76.0"]
+    pub UNCONDITIONAL_RECURSION,
+    suspicious,
+    "detect unconditional recursion in some traits implementation"
+}
+
+declare_lint_pass!(UnconditionalRecursion => [UNCONDITIONAL_RECURSION]);
+
+fn get_ty_def_id(ty: Ty<'_>) -> Option<DefId> {
+    match ty.peel_refs().kind() {
+        ty::Adt(adt, _) => Some(adt.did()),
+        ty::Foreign(def_id) => Some(*def_id),
+        _ => None,
+    }
+}
+
+fn is_local(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    matches!(path_res(cx, expr), Res::Local(_))
+}
+
+impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion {
+    #[allow(clippy::unnecessary_def_path)]
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'tcx>,
+        _decl: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        method_span: Span,
+        def_id: LocalDefId,
+    ) {
+        // If the function is a method...
+        if let FnKind::Method(name, _) = kind
+            // That has two arguments.
+            && let [self_arg, other_arg] = cx
+                .tcx
+                .instantiate_bound_regions_with_erased(cx.tcx.fn_sig(def_id).skip_binder())
+                .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");
+                    },
+                );
+            }
+        }
+    }
+}
diff --git a/tests/ui/unconditional_recursion.rs b/tests/ui/unconditional_recursion.rs
@@ -0,0 +1,163 @@
+//@no-rustfix
+
+#![warn(clippy::unconditional_recursion)]
+#![allow(clippy::partialeq_ne_impl)]
+
+enum Foo {
+    A,
+    B,
+}
+
+impl PartialEq for Foo {
+    fn ne(&self, other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        self != other
+    }
+    fn eq(&self, other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        self == other
+    }
+}
+
+enum Foo2 {
+    A,
+    B,
+}
+
+impl PartialEq for Foo2 {
+    fn ne(&self, other: &Self) -> bool {
+        self != &Foo2::B // no error here
+    }
+    fn eq(&self, other: &Self) -> bool {
+        self == &Foo2::B // no error here
+    }
+}
+
+enum Foo3 {
+    A,
+    B,
+}
+
+impl PartialEq for Foo3 {
+    fn ne(&self, other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        self.ne(other)
+    }
+    fn eq(&self, other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        self.eq(other)
+    }
+}
+
+enum Foo4 {
+    A,
+    B,
+}
+
+impl PartialEq for Foo4 {
+    fn ne(&self, other: &Self) -> bool {
+        self.eq(other) // no error
+    }
+    fn eq(&self, other: &Self) -> bool {
+        self.ne(other) // no error
+    }
+}
+
+enum Foo5 {
+    A,
+    B,
+}
+
+impl Foo5 {
+    fn a(&self) -> bool {
+        true
+    }
+}
+
+impl PartialEq for Foo5 {
+    fn ne(&self, other: &Self) -> bool {
+        self.a() // no error
+    }
+    fn eq(&self, other: &Self) -> bool {
+        self.a() // no error
+    }
+}
+
+struct S;
+
+// Check the order doesn't matter.
+impl PartialEq for S {
+    fn ne(&self, other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        other != self
+    }
+    fn eq(&self, other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        other == self
+    }
+}
+
+struct S2;
+
+// Check that if the same element is compared, it's also triggering the lint.
+impl PartialEq for S2 {
+    fn ne(&self, other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        other != other
+    }
+    fn eq(&self, other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        other == other
+    }
+}
+
+struct S3;
+
+impl PartialEq for S3 {
+    fn ne(&self, _other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        self != self
+    }
+    fn eq(&self, _other: &Self) -> bool {
+        //~^ ERROR: function cannot return without recursing
+        self == self
+    }
+}
+
+// There should be no warning here!
+#[derive(PartialEq)]
+enum E {
+    A,
+    B,
+}
+
+#[derive(PartialEq)]
+struct Bar<T: PartialEq>(T);
+
+struct S4;
+
+impl PartialEq for S4 {
+    fn eq(&self, other: &Self) -> bool {
+        // No warning here.
+        Bar(self) == Bar(other)
+    }
+}
+
+macro_rules! impl_partial_eq {
+    ($ty:ident) => {
+        impl PartialEq for $ty {
+            fn eq(&self, other: &Self) -> bool {
+                self == other
+            }
+        }
+    };
+}
+
+struct S5;
+
+impl_partial_eq!(S5);
+//~^ ERROR: function cannot return without recursing
+
+fn main() {
+    // test code goes here
+}
diff --git a/tests/ui/unconditional_recursion.stderr b/tests/ui/unconditional_recursion.stderr
