Skip to content

Commit 8f896fc

Browse files
samueltardieusamueltardieu
committed
Properly check that an expression might be the one returned
The `TyCtxt::hir_get_fn_id_for_return_block()` function was too broad, as it will return positively even when given part of an expression that can be used as a return value. A new `potential_return_of_enclosing_body()` utility function has been made to represent the fact that an expression might be directly returned from its enclosing body.
1 parent 76118ec commit 8f896fc

File tree

5 files changed

+258
-7
lines changed

5 files changed

+258
-7
lines changed

‎clippy_lints/src/methods/return_and_then.rs‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use rustc_errors::Applicability;
2-
use rustc_hir as hir;
2+
use rustc_hir::{self as hir, Node};
33
use rustc_lint::LateContext;
44
use rustc_middle::ty::{self, GenericArg, Ty};
55
use rustc_span::sym;
@@ -9,7 +9,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg;
99
use clippy_utils::source::{indent_of, reindent_multiline, snippet_with_applicability};
1010
use clippy_utils::ty::get_type_diagnostic_name;
1111
use clippy_utils::visitors::for_each_unconsumed_temporary;
12-
use clippy_utils::{get_parent_expr, peel_blocks};
12+
use clippy_utils::{peel_blocks, potential_return_of_enclosing_body};
1313

1414
use super::RETURN_AND_THEN;
1515

@@ -21,7 +21,7 @@ pub(super) fn check<'tcx>(
2121
recv: &'tcx hir::Expr<'tcx>,
2222
arg: &'tcx hir::Expr<'_>,
2323
) {
24-
if cx.tcx.hir_get_fn_id_for_return_block(expr.hir_id).is_none() {
24+
if !potential_return_of_enclosing_body(cx, expr) {
2525
return;
2626
}
2727

@@ -55,9 +55,15 @@ pub(super) fn check<'tcx>(
5555
None => &body_snip,
5656
};
5757

58-
// If suggestion is going to get inserted as part of a `return` expression, it must be blockified.
59-
let sugg = if let Some(parent_expr) = get_parent_expr(cx, expr) {
60-
let base_indent = indent_of(cx, parent_expr.span);
58+
// If suggestion is going to get inserted as part of a `return` expression or as a match expression
59+
// arm, it must be blockified.
60+
let parent_span_for_indent = match cx.tcx.parent_hir_node(expr.hir_id) {
61+
Node::Expr(parent_expr) => Some(parent_expr.span),
62+
Node::Arm(arm) => Some(arm.span),
63+
_ => None,
64+
};
65+
let sugg = if let Some(span) = parent_span_for_indent {
66+
let base_indent = indent_of(cx, span);
6167
let inner_indent = base_indent.map(|i| i + 4);
6268
format!(
6369
"{}\n{}\n{}",

‎clippy_utils/src/lib.rs‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3497,3 +3497,47 @@ pub fn is_expr_default<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) ->
34973497
false
34983498
}
34993499
}
3500+
3501+
/// Checks if `expr` may be directly used as the return value of its enclosing body.
3502+
/// The following cases are covered:
3503+
/// - `expr` as the last expression of the body, or of a block that can be used as the return value
3504+
/// - `return expr`
3505+
/// - then or else part of a `if` in return position
3506+
/// - arm body of a `match` in a return position
3507+
///
3508+
/// Contrary to [`TyCtxt::hir_get_fn_id_for_return_block()`], if `expr` is part of a
3509+
/// larger expression, for example a field expression of a `struct`, it will not be
3510+
/// considered as matching the condition and will return `false`.
3511+
///
3512+
/// Also, even if `expr` is assigned to a variable which is later returned, this function
3513+
/// will still return `false` because `expr` is not used *directly* as the return value
3514+
/// as it goes through the intermediate variable.
3515+
pub fn potential_return_of_enclosing_body(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
3516+
let enclosing_body_owner = cx
3517+
.tcx
3518+
.local_def_id_to_hir_id(cx.tcx.hir_enclosing_body_owner(expr.hir_id));
3519+
let mut prev_id = expr.hir_id;
3520+
for (hir_id, node) in cx.tcx.hir_parent_iter(expr.hir_id) {
3521+
if hir_id == enclosing_body_owner {
3522+
return true;
3523+
}
3524+
match node {
3525+
Node::Block(Block { expr, .. }) if expr.is_some_and(|expr| expr.hir_id == prev_id) => {},
3526+
Node::Arm(arm) if arm.body.hir_id == prev_id => {},
3527+
Node::Expr(expr) => match expr.kind {
3528+
ExprKind::Ret(_) => return true,
3529+
ExprKind::If(_, then, opt_else)
3530+
if then.hir_id == prev_id || opt_else.is_some_and(|els| els.hir_id == prev_id) => {},
3531+
ExprKind::Match(_, arms, _) if arms.iter().any(|arm| arm.hir_id == prev_id) => {},
3532+
ExprKind::Block(block, _) if block.hir_id == prev_id => {},
3533+
_ => break,
3534+
},
3535+
_ => break,
3536+
}
3537+
prev_id = hir_id;
3538+
}
3539+
3540+
// `expr` is used as part of "something" and is not returned directly from its
3541+
// enclosing body.
3542+
false
3543+
}

‎tests/ui/return_and_then.fixed‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,45 @@ fn main() {
9999
};
100100
None
101101
}
102+
103+
fn inside_if(a: bool, i: Option<u32>) -> Option<u32> {
104+
if a {
105+
let i = i?;
106+
if i > 3 { Some(i) } else { None }
107+
//~^ return_and_then
108+
} else {
109+
Some(42)
110+
}
111+
}
112+
113+
fn inside_match(a: u32, i: Option<u32>) -> Option<u32> {
114+
match a {
115+
1 | 2 => {
116+
let i = i?;
117+
if i > 3 { Some(i) } else { None }
118+
},
119+
//~^ return_and_then
120+
3 | 4 => Some(42),
121+
_ => None,
122+
}
123+
}
124+
125+
fn inside_match_and_block_and_if(a: u32, i: Option<u32>) -> Option<u32> {
126+
match a {
127+
1 | 2 => {
128+
let a = a * 3;
129+
if a.is_multiple_of(2) {
130+
let i = i?;
131+
if i > 3 { Some(i) } else { None }
132+
//~^ return_and_then
133+
} else {
134+
Some(10)
135+
}
136+
},
137+
3 | 4 => Some(42),
138+
_ => None,
139+
}
140+
}
102141
}
103142

104143
fn gen_option(n: i32) -> Option<i32> {
@@ -124,3 +163,48 @@ mod issue14781 {
124163
Ok(())
125164
}
126165
}
166+
167+
mod issue15111 {
168+
#[derive(Debug)]
169+
struct EvenOdd {
170+
even: Option<u32>,
171+
odd: Option<u32>,
172+
}
173+
174+
impl EvenOdd {
175+
fn new(i: Option<u32>) -> Self {
176+
Self {
177+
even: i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }),
178+
odd: i.and_then(|i| if i.is_multiple_of(2) { None } else { Some(i) }),
179+
}
180+
}
181+
}
182+
183+
fn with_if_let(i: Option<u32>) -> u32 {
184+
if let Some(x) = i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }) {
185+
x
186+
} else {
187+
std::hint::black_box(0)
188+
}
189+
}
190+
191+
fn main() {
192+
let _ = EvenOdd::new(Some(2));
193+
}
194+
}
195+
196+
mod issue14927 {
197+
use std::path::Path;
198+
struct A {
199+
pub func: fn(check: bool, a: &Path, b: Option<&Path>),
200+
}
201+
const MY_A: A = A {
202+
func: |check, a, b| {
203+
if check {
204+
let _ = ();
205+
} else if let Some(parent) = b.and_then(|p| p.parent()) {
206+
let _ = ();
207+
}
208+
},
209+
};
210+
}

‎tests/ui/return_and_then.rs‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,40 @@ fn main() {
9090
};
9191
None
9292
}
93+
94+
fn inside_if(a: bool, i: Option<u32>) -> Option<u32> {
95+
if a {
96+
i.and_then(|i| if i > 3 { Some(i) } else { None })
97+
//~^ return_and_then
98+
} else {
99+
Some(42)
100+
}
101+
}
102+
103+
fn inside_match(a: u32, i: Option<u32>) -> Option<u32> {
104+
match a {
105+
1 | 2 => i.and_then(|i| if i > 3 { Some(i) } else { None }),
106+
//~^ return_and_then
107+
3 | 4 => Some(42),
108+
_ => None,
109+
}
110+
}
111+
112+
fn inside_match_and_block_and_if(a: u32, i: Option<u32>) -> Option<u32> {
113+
match a {
114+
1 | 2 => {
115+
let a = a * 3;
116+
if a.is_multiple_of(2) {
117+
i.and_then(|i| if i > 3 { Some(i) } else { None })
118+
//~^ return_and_then
119+
} else {
120+
Some(10)
121+
}
122+
},
123+
3 | 4 => Some(42),
124+
_ => None,
125+
}
126+
}
93127
}
94128

95129
fn gen_option(n: i32) -> Option<i32> {
@@ -115,3 +149,48 @@ mod issue14781 {
115149
Ok(())
116150
}
117151
}
152+
153+
mod issue15111 {
154+
#[derive(Debug)]
155+
struct EvenOdd {
156+
even: Option<u32>,
157+
odd: Option<u32>,
158+
}
159+
160+
impl EvenOdd {
161+
fn new(i: Option<u32>) -> Self {
162+
Self {
163+
even: i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }),
164+
odd: i.and_then(|i| if i.is_multiple_of(2) { None } else { Some(i) }),
165+
}
166+
}
167+
}
168+
169+
fn with_if_let(i: Option<u32>) -> u32 {
170+
if let Some(x) = i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }) {
171+
x
172+
} else {
173+
std::hint::black_box(0)
174+
}
175+
}
176+
177+
fn main() {
178+
let _ = EvenOdd::new(Some(2));
179+
}
180+
}
181+
182+
mod issue14927 {
183+
use std::path::Path;
184+
struct A {
185+
pub func: fn(check: bool, a: &Path, b: Option<&Path>),
186+
}
187+
const MY_A: A = A {
188+
func: |check, a, b| {
189+
if check {
190+
let _ = ();
191+
} else if let Some(parent) = b.and_then(|p| p.parent()) {
192+
let _ = ();
193+
}
194+
},
195+
};
196+
}

‎tests/ui/return_and_then.stderr‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,5 +146,43 @@ LL + if x.len() > 2 { Some(3) } else { None }
146146
LL ~ };
147147
|
148148

149-
error: aborting due to 10 previous errors
149+
error: use the `?` operator instead of an `and_then` call
150+
--> tests/ui/return_and_then.rs:96:13
151+
|
152+
LL | i.and_then(|i| if i > 3 { Some(i) } else { None })
153+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
154+
|
155+
help: try
156+
|
157+
LL ~ let i = i?;
158+
LL + if i > 3 { Some(i) } else { None }
159+
|
160+
161+
error: use the `?` operator instead of an `and_then` call
162+
--> tests/ui/return_and_then.rs:105:22
163+
|
164+
LL | 1 | 2 => i.and_then(|i| if i > 3 { Some(i) } else { None }),
165+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
166+
|
167+
help: try
168+
|
169+
LL ~ 1 | 2 => {
170+
LL + let i = i?;
171+
LL + if i > 3 { Some(i) } else { None }
172+
LL ~ },
173+
|
174+
175+
error: use the `?` operator instead of an `and_then` call
176+
--> tests/ui/return_and_then.rs:117:21
177+
|
178+
LL | i.and_then(|i| if i > 3 { Some(i) } else { None })
179+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
180+
|
181+
help: try
182+
|
183+
LL ~ let i = i?;
184+
LL + if i > 3 { Some(i) } else { None }
185+
|
186+
187+
error: aborting due to 13 previous errors
150188

0 commit comments

Comments
 (0)