Skip to content

Fix false positive unconditional_recursion #12062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Fix false positive unconditional_recursion #12062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3bde85
Fix false positive in `PartialEq` implementation
GuillaumeGomez Dec 30, 2023
7107aa2
Add regression tests for `unconditional_recursion`
GuillaumeGomez Dec 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 39 additions & 21 deletions clippy_lints/src/unconditional_recursion.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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