Skip to content

Commit

Permalink
Auto merge of #12062 - GuillaumeGomez:fix-false-positive-unconditiona…
Browse files Browse the repository at this point in the history 
…l_recursion, r=xFrednet

Fix false positive `unconditional_recursion`

Fixes #12052.

Only checking if both variables are `local` was not enough, we also need to confirm they have the same type as `Self`.

changelog: Fix false positive for `unconditional_recursion` lint
  • Loading branch information
@bors
bors committed Jan 3, 2024
2 parents 2eb13d3 + 7107aa2 commit 0153ca9
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 31 deletions.
60 changes: 39 additions & 21 deletions clippy_lints/src/unconditional_recursion.rs
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -52,12 +52,19 @@ fn get_ty_def_id(ty: Ty<'_>) -> Option<DefId> {
}
}

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())
Expand Down Expand Up @@ -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
{
Expand All @@ -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())
Expand All @@ -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
{
Expand Down Expand Up @@ -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);
}
}
}
Expand Down
58 changes: 52 additions & 6 deletions tests/ui/unconditional_recursion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,28 +158,74 @@ struct S5;
impl_partial_eq!(S5);
//~^ ERROR: function cannot return without recursing

struct S6;
struct S6 {
field: String,
}

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<Box<S8>>,
}

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 S6 {
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;
x.to_string()
}
}

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()
Expand Down
53 changes: 49 additions & 4 deletions tests/ui/unconditional_recursion.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
|
Expand Down Expand Up @@ -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

0 comments on commit 0153ca9

Please sign in to comment.