diff --git a/clippy_lints/src/unconditional_recursion.rs b/clippy_lints/src/unconditional_recursion.rs index b1fa30aa0..4036ad78a 100644 --- a/clippy_lints/src/unconditional_recursion.rs +++ b/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 - // 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"); - }, - ); + if let FnKind::Method(name, _) = kind { + if name.name == sym::eq || name.name == sym::ne { + check_partial_eq(cx, body, method_span, method_def_id, name); + } else if name.name == sym::to_string { + check_to_string(cx, body, method_span, method_def_id, name); } } } diff --git a/tests/ui/unconditional_recursion.rs b/tests/ui/unconditional_recursion.rs index 1169118de..36fb7e08d 100644 --- a/tests/ui/unconditional_recursion.rs +++ b/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 } diff --git a/tests/ui/unconditional_recursion.stderr b/tests/ui/unconditional_recursion.stderr index 1fb01c00f..040cc4a85 100644 --- a/tests/ui/unconditional_recursion.stderr +++ b/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