Skip to content

Commit 0153ca9

Browse files
borsbors
committed
Auto merge of rust-lang#12062 - GuillaumeGomez:fix-false-positive-unconditional_recursion, r=xFrednet
Fix false positive `unconditional_recursion` Fixes rust-lang#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
2 parents 2eb13d3 + 7107aa2 commit 0153ca9

File tree

3 files changed

+140
-31
lines changed

3 files changed

+140
-31
lines changed

‎clippy_lints/src/unconditional_recursion.rs

+39-21
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
2-
use clippy_utils::{expr_or_init, get_trait_def_id, path_res};
2+
use clippy_utils::{expr_or_init, get_trait_def_id};
33
use rustc_ast::BinOpKind;
4-
use rustc_hir::def::Res;
54
use rustc_hir::def_id::{DefId, LocalDefId};
6-
use rustc_hir::intravisit::FnKind;
5+
use rustc_hir::intravisit::{walk_body, FnKind};
76
use rustc_hir::{Body, Expr, ExprKind, FnDecl, Item, ItemKind, Node};
87
use rustc_lint::{LateContext, LateLintPass};
98
use rustc_middle::ty::{self, Ty};
109
use rustc_session::declare_lint_pass;
1110
use rustc_span::symbol::Ident;
1211
use rustc_span::{sym, Span};
12+
use rustc_trait_selection::traits::error_reporting::suggestions::ReturnsVisitor;
1313

1414
declare_clippy_lint! {
1515
/// ### What it does
@@ -52,12 +52,19 @@ fn get_ty_def_id(ty: Ty<'_>) -> Option<DefId> {
5252
}
5353
}
5454

55-
fn is_local(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
56-
matches!(path_res(cx, expr), Res::Local(_))
55+
fn has_conditional_return(body: &Body<'_>, expr: &Expr<'_>) -> bool {
56+
let mut visitor = ReturnsVisitor::default();
57+
58+
walk_body(&mut visitor, body);
59+
match visitor.returns.as_slice() {
60+
[] => false,
61+
[return_expr] => return_expr.hir_id != expr.hir_id,
62+
_ => true,
63+
}
5764
}
5865

5966
#[allow(clippy::unnecessary_def_path)]
60-
fn check_partial_eq(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident) {
67+
fn check_partial_eq(cx: &LateContext<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident, expr: &Expr<'_>) {
6168
let args = cx
6269
.tcx
6370
.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
9097
} else {
9198
BinOpKind::Ne
9299
};
93-
let expr = body.value.peel_blocks();
94100
let is_bad = match expr.kind {
95-
ExprKind::Binary(op, left, right) if op.node == to_check_op => is_local(cx, left) && is_local(cx, right),
96-
ExprKind::MethodCall(segment, receiver, &[arg], _) if segment.ident.name == name.name => {
97-
if is_local(cx, receiver)
98-
&& is_local(cx, &arg)
99-
&& let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
101+
ExprKind::Binary(op, left, right) if op.node == to_check_op => {
102+
// Then we check if the left-hand element is of the same type as `self`.
103+
if let Some(left_ty) = cx.typeck_results().expr_ty_opt(left)
104+
&& let Some(left_id) = get_ty_def_id(left_ty)
105+
&& self_arg == left_id
106+
&& let Some(right_ty) = cx.typeck_results().expr_ty_opt(right)
107+
&& let Some(right_id) = get_ty_def_id(right_ty)
108+
&& other_arg == right_id
109+
{
110+
true
111+
} else {
112+
false
113+
}
114+
},
115+
ExprKind::MethodCall(segment, _receiver, &[_arg], _) if segment.ident.name == name.name => {
116+
if let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
100117
&& let Some(trait_id) = cx.tcx.trait_of_item(fn_id)
101118
&& trait_id == trait_def_id
102119
{
@@ -122,7 +139,7 @@ fn check_partial_eq(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, me
122139
}
123140

124141
#[allow(clippy::unnecessary_def_path)]
125-
fn check_to_string(cx: &LateContext<'_>, body: &Body<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident) {
142+
fn check_to_string(cx: &LateContext<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident, expr: &Expr<'_>) {
126143
let args = cx
127144
.tcx
128145
.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
146163
// The trait is `ToString`.
147164
&& Some(trait_def_id) == get_trait_def_id(cx, &["alloc", "string", "ToString"])
148165
{
149-
let expr = expr_or_init(cx, body.value.peel_blocks());
150166
let is_bad = match expr.kind {
151-
ExprKind::MethodCall(segment, receiver, &[arg], _) if segment.ident.name == name.name => {
152-
if is_local(cx, receiver)
153-
&& is_local(cx, &arg)
154-
&& let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
167+
ExprKind::MethodCall(segment, _receiver, &[_arg], _) if segment.ident.name == name.name => {
168+
if let Some(fn_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
155169
&& let Some(trait_id) = cx.tcx.trait_of_item(fn_id)
156170
&& trait_id == trait_def_id
157171
{
@@ -187,11 +201,15 @@ impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion {
187201
method_def_id: LocalDefId,
188202
) {
189203
// If the function is a method...
190-
if let FnKind::Method(name, _) = kind {
204+
if let FnKind::Method(name, _) = kind
205+
&& let expr = expr_or_init(cx, body.value).peel_blocks()
206+
// Doesn't have a conditional return.
207+
&& !has_conditional_return(body, expr)
208+
{
191209
if name.name == sym::eq || name.name == sym::ne {
192-
check_partial_eq(cx, body, method_span, method_def_id, name);
210+
check_partial_eq(cx, method_span, method_def_id, name, expr);
193211
} else if name.name == sym::to_string {
194-
check_to_string(cx, body, method_span, method_def_id, name);
212+
check_to_string(cx, method_span, method_def_id, name, expr);
195213
}
196214
}
197215
}

‎tests/ui/unconditional_recursion.rs

+52-6
Original file line numberDiff line numberDiff line change
@@ -158,28 +158,74 @@ struct S5;
158158
impl_partial_eq!(S5);
159159
//~^ ERROR: function cannot return without recursing
160160

161-
struct S6;
161+
struct S6 {
162+
field: String,
163+
}
164+
165+
impl PartialEq for S6 {
166+
fn eq(&self, other: &Self) -> bool {
167+
let mine = &self.field;
168+
let theirs = &other.field;
169+
mine == theirs // Should not warn!
170+
}
171+
}
172+
173+
struct S7<'a> {
174+
field: &'a S7<'a>,
175+
}
176+
177+
impl<'a> PartialEq for S7<'a> {
178+
fn eq(&self, other: &Self) -> bool {
179+
//~^ ERROR: function cannot return without recursing
180+
let mine = &self.field;
181+
let theirs = &other.field;
182+
mine == theirs
183+
}
184+
}
185+
186+
struct S8 {
187+
num: i32,
188+
field: Option<Box<S8>>,
189+
}
190+
191+
impl PartialEq for S8 {
192+
fn eq(&self, other: &Self) -> bool {
193+
if self.num != other.num {
194+
return false;
195+
}
196+
197+
let (this, other) = match (self.field.as_deref(), other.field.as_deref()) {
198+
(Some(x1), Some(x2)) => (x1, x2),
199+
(None, None) => return true,
200+
_ => return false,
201+
};
202+
203+
this == other
204+
}
205+
}
206+
207+
struct S9;
162208

163-
impl std::string::ToString for S6 {
209+
impl std::string::ToString for S9 {
164210
fn to_string(&self) -> String {
165211
//~^ ERROR: function cannot return without recursing
166212
self.to_string()
167213
}
168214
}
169215

170-
struct S7;
216+
struct S10;
171217

172-
impl std::string::ToString for S7 {
218+
impl std::string::ToString for S10 {
173219
fn to_string(&self) -> String {
174220
//~^ ERROR: function cannot return without recursing
175221
let x = self;
176222
x.to_string()
177223
}
178224
}
179225

180-
struct S8;
226+
struct S11;
181227

182-
impl std::string::ToString for S8 {
228+
impl std::string::ToString for S11 {
183229
fn to_string(&self) -> String {
184230
//~^ ERROR: function cannot return without recursing
185231
(self as &Self).to_string()

‎tests/ui/unconditional_recursion.stderr

+49-4
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ LL | self.eq(other)
2323
= help: a `loop` may express intention better if this is on purpose
2424

2525
error: function cannot return without recursing
26-
--> $DIR/unconditional_recursion.rs:164:5
26+
--> $DIR/unconditional_recursion.rs:210:5
2727
|
2828
LL | fn to_string(&self) -> String {
2929
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@@ -34,7 +34,7 @@ LL | self.to_string()
3434
= help: a `loop` may express intention better if this is on purpose
3535

3636
error: function cannot return without recursing
37-
--> $DIR/unconditional_recursion.rs:173:5
37+
--> $DIR/unconditional_recursion.rs:219:5
3838
|
3939
LL | fn to_string(&self) -> String {
4040
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@@ -45,7 +45,7 @@ LL | x.to_string()
4545
= help: a `loop` may express intention better if this is on purpose
4646

4747
error: function cannot return without recursing
48-
--> $DIR/unconditional_recursion.rs:183:5
48+
--> $DIR/unconditional_recursion.rs:229:5
4949
|
5050
LL | fn to_string(&self) -> String {
5151
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
@@ -87,6 +87,34 @@ note: recursive call site
8787
LL | self == other
8888
| ^^^^^^^^^^^^^
8989

90+
error: function cannot return without recursing
91+
--> $DIR/unconditional_recursion.rs:28:5
92+
|
93+
LL | / fn ne(&self, other: &Self) -> bool {
94+
LL | | self != &Foo2::B // no error here
95+
LL | | }
96+
| |_____^
97+
|
98+
note: recursive call site
99+
--> $DIR/unconditional_recursion.rs:29:9
100+
|
101+
LL | self != &Foo2::B // no error here
102+
| ^^^^^^^^^^^^^^^^
103+
104+
error: function cannot return without recursing
105+
--> $DIR/unconditional_recursion.rs:31:5
106+
|
107+
LL | / fn eq(&self, other: &Self) -> bool {
108+
LL | | self == &Foo2::B // no error here
109+
LL | | }
110+
| |_____^
111+
|
112+
note: recursive call site
113+
--> $DIR/unconditional_recursion.rs:32:9
114+
|
115+
LL | self == &Foo2::B // no error here
116+
| ^^^^^^^^^^^^^^^^
117+
90118
error: function cannot return without recursing
91119
--> $DIR/unconditional_recursion.rs:42:5
92120
|
@@ -280,5 +308,22 @@ LL | impl_partial_eq!(S5);
280308
| -------------------- in this macro invocation
281309
= note: this error originates in the macro `impl_partial_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
282310

283-
error: aborting due to 22 previous errors
311+
error: function cannot return without recursing
312+
--> $DIR/unconditional_recursion.rs:178:5
313+
|
314+
LL | / fn eq(&self, other: &Self) -> bool {
315+
LL | |
316+
LL | | let mine = &self.field;
317+
LL | | let theirs = &other.field;
318+
LL | | mine == theirs
319+
LL | | }
320+
| |_____^
321+
|
322+
note: recursive call site
323+
--> $DIR/unconditional_recursion.rs:182:9
324+
|
325+
LL | mine == theirs
326+
| ^^^^^^^^^^^^^^
327+
328+
error: aborting due to 25 previous errors
284329

0 commit comments

Comments
 (0)