diff --git a/clippy_lints/src/unconditional_recursion.rs b/clippy_lints/src/unconditional_recursion.rs index 4036ad78a..5366b5513 100644 --- a/clippy_lints/src/unconditional_recursion.rs +++ b/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 { } } -fn is_local(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - matches!(path_res(cx, expr), Res::Local(_)) +fn has_conditional_return(body: &Body<'_>, expr: &Expr<'_>) -> bool { + 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::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) + ExprKind::Binary(op, left, right) if op.node == to_check_op => { + // Then we check if the left-hand element is of the same type as `self`. + if let Some(left_ty) = cx.typeck_results().expr_ty_opt(left) + && let Some(left_id) = get_ty_def_id(left_ty) + && 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 => { - if is_local(cx, receiver) - && is_local(cx, &arg) - && let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + 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 { @@ -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); } } } diff --git a/tests/ui/unconditional_recursion.rs b/tests/ui/unconditional_recursion.rs index 36fb7e08d..19cd553b3 100644 --- a/tests/ui/unconditional_recursion.rs +++ b/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>, +} + +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() diff --git a/tests/ui/unconditional_recursion.stderr b/tests/ui/unconditional_recursion.stderr index 040cc4a85..364dd5728 100644 --- a/tests/ui/unconditional_recursion.stderr +++ b/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