mirror of
https://github.com/rust-lang/rust.git
synced 2026-05-07 01:05:39 +03:00
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.
This commit is contained in:
Samuel Tardieu
@@ -1,5 +1,5 @@
|
||||
use rustc_errors::Applicability;
|
||||
use rustc_hir as hir;
|
||||
use rustc_hir::{self as hir, Node};
|
||||
use rustc_lint::LateContext;
|
||||
use rustc_middle::ty::{self, GenericArg, Ty};
|
||||
use rustc_span::sym;
|
||||
@@ -9,7 +9,7 @@
|
||||
use clippy_utils::source::{indent_of, reindent_multiline, snippet_with_applicability};
|
||||
use clippy_utils::ty::get_type_diagnostic_name;
|
||||
use clippy_utils::visitors::for_each_unconsumed_temporary;
|
||||
use clippy_utils::{get_parent_expr, peel_blocks};
|
||||
use clippy_utils::{peel_blocks, potential_return_of_enclosing_body};
|
||||
|
||||
use super::RETURN_AND_THEN;
|
||||
|
||||
@@ -21,7 +21,7 @@ pub(super) fn check<'tcx>(
|
||||
recv: &'tcx hir::Expr<'tcx>,
|
||||
arg: &'tcx hir::Expr<'_>,
|
||||
) {
|
||||
if cx.tcx.hir_get_fn_id_for_return_block(expr.hir_id).is_none() {
|
||||
if !potential_return_of_enclosing_body(cx, expr) {
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -55,15 +55,28 @@ pub(super) fn check<'tcx>(
|
||||
None => &body_snip,
|
||||
};
|
||||
|
||||
// If suggestion is going to get inserted as part of a `return` expression, it must be blockified.
|
||||
let sugg = if let Some(parent_expr) = get_parent_expr(cx, expr) {
|
||||
let base_indent = indent_of(cx, parent_expr.span);
|
||||
// If suggestion is going to get inserted as part of a `return` expression or as a match expression
|
||||
// arm, it must be blockified.
|
||||
let (parent_span_for_indent, opening_paren, closing_paren) = match cx.tcx.parent_hir_node(expr.hir_id) {
|
||||
Node::Expr(parent_expr) if matches!(parent_expr.kind, hir::ExprKind::Break(..)) => {
|
||||
(Some(parent_expr.span), "(", ")")
|
||||
},
|
||||
Node::Expr(parent_expr) => (Some(parent_expr.span), "", ""),
|
||||
Node::Arm(match_arm) => (Some(match_arm.span), "", ""),
|
||||
_ => (None, "", ""),
|
||||
};
|
||||
let sugg = if let Some(span) = parent_span_for_indent {
|
||||
let base_indent = indent_of(cx, span);
|
||||
let inner_indent = base_indent.map(|i| i + 4);
|
||||
format!(
|
||||
"{}\n{}\n{}",
|
||||
reindent_multiline(&format!("{{\nlet {arg_snip} = {recv_snip}?;"), true, inner_indent),
|
||||
reindent_multiline(
|
||||
&format!("{opening_paren}{{\nlet {arg_snip} = {recv_snip}?;"),
|
||||
true,
|
||||
inner_indent
|
||||
),
|
||||
reindent_multiline(inner, false, inner_indent),
|
||||
reindent_multiline("}", false, base_indent),
|
||||
reindent_multiline(&format!("}}{closing_paren}"), false, base_indent),
|
||||
)
|
||||
} else {
|
||||
format!(
|
||||
|
||||
@@ -3497,3 +3497,64 @@ pub fn is_expr_default<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) ->
|
||||
false
|
||||
}
|
||||
}
|
||||
|
||||
/// Checks if `expr` may be directly used as the return value of its enclosing body.
|
||||
/// The following cases are covered:
|
||||
/// - `expr` as the last expression of the body, or of a block that can be used as the return value
|
||||
/// - `return expr`
|
||||
/// - then or else part of a `if` in return position
|
||||
/// - arm body of a `match` in a return position
|
||||
/// - `break expr` or `break 'label expr` if the loop or block being exited is used as a return
|
||||
/// value
|
||||
///
|
||||
/// Contrary to [`TyCtxt::hir_get_fn_id_for_return_block()`], if `expr` is part of a
|
||||
/// larger expression, for example a field expression of a `struct`, it will not be
|
||||
/// considered as matching the condition and will return `false`.
|
||||
///
|
||||
/// Also, even if `expr` is assigned to a variable which is later returned, this function
|
||||
/// will still return `false` because `expr` is not used *directly* as the return value
|
||||
/// as it goes through the intermediate variable.
|
||||
pub fn potential_return_of_enclosing_body(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
|
||||
let enclosing_body_owner = cx
|
||||
.tcx
|
||||
.local_def_id_to_hir_id(cx.tcx.hir_enclosing_body_owner(expr.hir_id));
|
||||
let mut prev_id = expr.hir_id;
|
||||
let mut skip_until_id = None;
|
||||
for (hir_id, node) in cx.tcx.hir_parent_iter(expr.hir_id) {
|
||||
if hir_id == enclosing_body_owner {
|
||||
return true;
|
||||
}
|
||||
if let Some(id) = skip_until_id {
|
||||
prev_id = hir_id;
|
||||
if id == hir_id {
|
||||
skip_until_id = None;
|
||||
}
|
||||
continue;
|
||||
}
|
||||
match node {
|
||||
Node::Block(Block { expr, .. }) if expr.is_some_and(|expr| expr.hir_id == prev_id) => {},
|
||||
Node::Arm(arm) if arm.body.hir_id == prev_id => {},
|
||||
Node::Expr(expr) => match expr.kind {
|
||||
ExprKind::Ret(_) => return true,
|
||||
ExprKind::If(_, then, opt_else)
|
||||
if then.hir_id == prev_id || opt_else.is_some_and(|els| els.hir_id == prev_id) => {},
|
||||
ExprKind::Match(_, arms, _) if arms.iter().any(|arm| arm.hir_id == prev_id) => {},
|
||||
ExprKind::Block(block, _) if block.hir_id == prev_id => {},
|
||||
ExprKind::Break(
|
||||
Destination {
|
||||
target_id: Ok(target_id),
|
||||
..
|
||||
},
|
||||
_,
|
||||
) => skip_until_id = Some(target_id),
|
||||
_ => break,
|
||||
},
|
||||
_ => break,
|
||||
}
|
||||
prev_id = hir_id;
|
||||
}
|
||||
|
||||
// `expr` is used as part of "something" and is not returned directly from its
|
||||
// enclosing body.
|
||||
false
|
||||
}
|
||||
|
||||
@@ -99,6 +99,92 @@ fn main() {
|
||||
};
|
||||
None
|
||||
}
|
||||
|
||||
#[expect(clippy::diverging_sub_expression)]
|
||||
fn with_return_in_expression() -> Option<i32> {
|
||||
_ = (
|
||||
return {
|
||||
let x = Some("")?;
|
||||
if x.len() > 2 { Some(3) } else { None }
|
||||
},
|
||||
//~^ return_and_then
|
||||
10,
|
||||
);
|
||||
}
|
||||
|
||||
fn inside_if(a: bool, i: Option<u32>) -> Option<u32> {
|
||||
if a {
|
||||
let i = i?;
|
||||
if i > 3 { Some(i) } else { None }
|
||||
//~^ return_and_then
|
||||
} else {
|
||||
Some(42)
|
||||
}
|
||||
}
|
||||
|
||||
fn inside_match(a: u32, i: Option<u32>) -> Option<u32> {
|
||||
match a {
|
||||
1 | 2 => {
|
||||
let i = i?;
|
||||
if i > 3 { Some(i) } else { None }
|
||||
},
|
||||
//~^ return_and_then
|
||||
3 | 4 => Some(42),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
fn inside_match_and_block_and_if(a: u32, i: Option<u32>) -> Option<u32> {
|
||||
match a {
|
||||
1 | 2 => {
|
||||
let a = a * 3;
|
||||
if a.is_multiple_of(2) {
|
||||
let i = i?;
|
||||
if i > 3 { Some(i) } else { None }
|
||||
//~^ return_and_then
|
||||
} else {
|
||||
Some(10)
|
||||
}
|
||||
},
|
||||
3 | 4 => Some(42),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
#[expect(clippy::never_loop)]
|
||||
fn with_break(i: Option<u32>) -> Option<u32> {
|
||||
match i {
|
||||
Some(1) => loop {
|
||||
break ({
|
||||
let i = i?;
|
||||
if i > 3 { Some(i) } else { None }
|
||||
});
|
||||
//~^ return_and_then
|
||||
},
|
||||
Some(2) => 'foo: loop {
|
||||
loop {
|
||||
break 'foo ({
|
||||
let i = i?;
|
||||
if i > 3 { Some(i) } else { None }
|
||||
});
|
||||
//~^ return_and_then
|
||||
}
|
||||
},
|
||||
Some(3) => 'bar: {
|
||||
break 'bar ({
|
||||
let i = i?;
|
||||
if i > 3 { Some(i) } else { None }
|
||||
});
|
||||
//~^ return_and_then
|
||||
},
|
||||
Some(4) => 'baz: loop {
|
||||
_ = loop {
|
||||
break i.and_then(|i| if i > 3 { Some(i) } else { None });
|
||||
};
|
||||
},
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn gen_option(n: i32) -> Option<i32> {
|
||||
@@ -124,3 +210,48 @@ mod issue14781 {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
mod issue15111 {
|
||||
#[derive(Debug)]
|
||||
struct EvenOdd {
|
||||
even: Option<u32>,
|
||||
odd: Option<u32>,
|
||||
}
|
||||
|
||||
impl EvenOdd {
|
||||
fn new(i: Option<u32>) -> Self {
|
||||
Self {
|
||||
even: i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }),
|
||||
odd: i.and_then(|i| if i.is_multiple_of(2) { None } else { Some(i) }),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn with_if_let(i: Option<u32>) -> u32 {
|
||||
if let Some(x) = i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }) {
|
||||
x
|
||||
} else {
|
||||
std::hint::black_box(0)
|
||||
}
|
||||
}
|
||||
|
||||
fn main() {
|
||||
let _ = EvenOdd::new(Some(2));
|
||||
}
|
||||
}
|
||||
|
||||
mod issue14927 {
|
||||
use std::path::Path;
|
||||
struct A {
|
||||
pub func: fn(check: bool, a: &Path, b: Option<&Path>),
|
||||
}
|
||||
const MY_A: A = A {
|
||||
func: |check, a, b| {
|
||||
if check {
|
||||
let _ = ();
|
||||
} else if let Some(parent) = b.and_then(|p| p.parent()) {
|
||||
let _ = ();
|
||||
}
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
@@ -90,6 +90,75 @@ fn with_return_multiline(shortcut: bool) -> Option<i32> {
|
||||
};
|
||||
None
|
||||
}
|
||||
|
||||
#[expect(clippy::diverging_sub_expression)]
|
||||
fn with_return_in_expression() -> Option<i32> {
|
||||
_ = (
|
||||
return Some("").and_then(|x| if x.len() > 2 { Some(3) } else { None }),
|
||||
//~^ return_and_then
|
||||
10,
|
||||
);
|
||||
}
|
||||
|
||||
fn inside_if(a: bool, i: Option<u32>) -> Option<u32> {
|
||||
if a {
|
||||
i.and_then(|i| if i > 3 { Some(i) } else { None })
|
||||
//~^ return_and_then
|
||||
} else {
|
||||
Some(42)
|
||||
}
|
||||
}
|
||||
|
||||
fn inside_match(a: u32, i: Option<u32>) -> Option<u32> {
|
||||
match a {
|
||||
1 | 2 => i.and_then(|i| if i > 3 { Some(i) } else { None }),
|
||||
//~^ return_and_then
|
||||
3 | 4 => Some(42),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
fn inside_match_and_block_and_if(a: u32, i: Option<u32>) -> Option<u32> {
|
||||
match a {
|
||||
1 | 2 => {
|
||||
let a = a * 3;
|
||||
if a.is_multiple_of(2) {
|
||||
i.and_then(|i| if i > 3 { Some(i) } else { None })
|
||||
//~^ return_and_then
|
||||
} else {
|
||||
Some(10)
|
||||
}
|
||||
},
|
||||
3 | 4 => Some(42),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
#[expect(clippy::never_loop)]
|
||||
fn with_break(i: Option<u32>) -> Option<u32> {
|
||||
match i {
|
||||
Some(1) => loop {
|
||||
break i.and_then(|i| if i > 3 { Some(i) } else { None });
|
||||
//~^ return_and_then
|
||||
},
|
||||
Some(2) => 'foo: loop {
|
||||
loop {
|
||||
break 'foo i.and_then(|i| if i > 3 { Some(i) } else { None });
|
||||
//~^ return_and_then
|
||||
}
|
||||
},
|
||||
Some(3) => 'bar: {
|
||||
break 'bar i.and_then(|i| if i > 3 { Some(i) } else { None });
|
||||
//~^ return_and_then
|
||||
},
|
||||
Some(4) => 'baz: loop {
|
||||
_ = loop {
|
||||
break i.and_then(|i| if i > 3 { Some(i) } else { None });
|
||||
};
|
||||
},
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn gen_option(n: i32) -> Option<i32> {
|
||||
@@ -115,3 +184,48 @@ fn bug(_: Option<&str>) -> Result<(), ()> {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
mod issue15111 {
|
||||
#[derive(Debug)]
|
||||
struct EvenOdd {
|
||||
even: Option<u32>,
|
||||
odd: Option<u32>,
|
||||
}
|
||||
|
||||
impl EvenOdd {
|
||||
fn new(i: Option<u32>) -> Self {
|
||||
Self {
|
||||
even: i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }),
|
||||
odd: i.and_then(|i| if i.is_multiple_of(2) { None } else { Some(i) }),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn with_if_let(i: Option<u32>) -> u32 {
|
||||
if let Some(x) = i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }) {
|
||||
x
|
||||
} else {
|
||||
std::hint::black_box(0)
|
||||
}
|
||||
}
|
||||
|
||||
fn main() {
|
||||
let _ = EvenOdd::new(Some(2));
|
||||
}
|
||||
}
|
||||
|
||||
mod issue14927 {
|
||||
use std::path::Path;
|
||||
struct A {
|
||||
pub func: fn(check: bool, a: &Path, b: Option<&Path>),
|
||||
}
|
||||
const MY_A: A = A {
|
||||
func: |check, a, b| {
|
||||
if check {
|
||||
let _ = ();
|
||||
} else if let Some(parent) = b.and_then(|p| p.parent()) {
|
||||
let _ = ();
|
||||
}
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
@@ -146,5 +146,99 @@ LL + if x.len() > 2 { Some(3) } else { None }
|
||||
LL ~ };
|
||||
|
|
||||
|
||||
error: aborting due to 10 previous errors
|
||||
error: use the `?` operator instead of an `and_then` call
|
||||
--> tests/ui/return_and_then.rs:97:20
|
||||
|
|
||||
LL | return Some("").and_then(|x| if x.len() > 2 { Some(3) } else { None }),
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
|
|
||||
help: try
|
||||
|
|
||||
LL ~ return {
|
||||
LL + let x = Some("")?;
|
||||
LL + if x.len() > 2 { Some(3) } else { None }
|
||||
LL ~ },
|
||||
|
|
||||
|
||||
error: use the `?` operator instead of an `and_then` call
|
||||
--> tests/ui/return_and_then.rs:105:13
|
||||
|
|
||||
LL | i.and_then(|i| if i > 3 { Some(i) } else { None })
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
|
|
||||
help: try
|
||||
|
|
||||
LL ~ let i = i?;
|
||||
LL + if i > 3 { Some(i) } else { None }
|
||||
|
|
||||
|
||||
error: use the `?` operator instead of an `and_then` call
|
||||
--> tests/ui/return_and_then.rs:114:22
|
||||
|
|
||||
LL | 1 | 2 => i.and_then(|i| if i > 3 { Some(i) } else { None }),
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
|
|
||||
help: try
|
||||
|
|
||||
LL ~ 1 | 2 => {
|
||||
LL + let i = i?;
|
||||
LL + if i > 3 { Some(i) } else { None }
|
||||
LL ~ },
|
||||
|
|
||||
|
||||
error: use the `?` operator instead of an `and_then` call
|
||||
--> tests/ui/return_and_then.rs:126:21
|
||||
|
|
||||
LL | i.and_then(|i| if i > 3 { Some(i) } else { None })
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
|
|
||||
help: try
|
||||
|
|
||||
LL ~ let i = i?;
|
||||
LL + if i > 3 { Some(i) } else { None }
|
||||
|
|
||||
|
||||
error: use the `?` operator instead of an `and_then` call
|
||||
--> tests/ui/return_and_then.rs:141:23
|
||||
|
|
||||
LL | break i.and_then(|i| if i > 3 { Some(i) } else { None });
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
|
|
||||
help: try
|
||||
|
|
||||
LL ~ break ({
|
||||
LL + let i = i?;
|
||||
LL + if i > 3 { Some(i) } else { None }
|
||||
LL ~ });
|
||||
|
|
||||
|
||||
error: use the `?` operator instead of an `and_then` call
|
||||
--> tests/ui/return_and_then.rs:146:32
|
||||
|
|
||||
LL | break 'foo i.and_then(|i| if i > 3 { Some(i) } else { None });
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
|
|
||||
help: try
|
||||
|
|
||||
LL ~ break 'foo ({
|
||||
LL + let i = i?;
|
||||
LL + if i > 3 { Some(i) } else { None }
|
||||
LL ~ });
|
||||
|
|
||||
|
||||
error: use the `?` operator instead of an `and_then` call
|
||||
--> tests/ui/return_and_then.rs:151:28
|
||||
|
|
||||
LL | break 'bar i.and_then(|i| if i > 3 { Some(i) } else { None });
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
|
|
||||
help: try
|
||||
|
|
||||
LL ~ break 'bar ({
|
||||
LL + let i = i?;
|
||||
LL + if i > 3 { Some(i) } else { None }
|
||||
LL ~ });
|
||||
|
|
||||
|
||||
error: aborting due to 17 previous errors
|
||||
|
||||
|
||||
Reference in New Issue
Block a user