Skip to content

Commit ac4c69f

Browse files
AlexendooAlexendoo
authored
implicit_return: better handling of asynchronous code (#14446)
Blocks created by desugaring will not contain an explicit `return`. Do not suggest to add it when the user has no control over the desugared code. Also, ensure that in a `xxx.await` expression, the suggested `return` is emitted before the whole expression, not before the `await` keyword. Fix #14411 changelog: [`implicit_return`]: fix proposed `return` position in the presence of asynchronous code
2 parents fad1bfc + 4ac3611 commit ac4c69f

File tree

6 files changed

+222
-37
lines changed

6 files changed

+222
-37
lines changed

‎clippy_lints/src/implicit_return.rs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use clippy_utils::diagnostics::span_lint_hir_and_then;
22
use clippy_utils::source::{snippet_with_applicability, snippet_with_context, walk_span_to_context};
33
use clippy_utils::visitors::for_each_expr_without_closures;
4-
use clippy_utils::{get_async_fn_body, is_async_fn, is_from_proc_macro};
4+
use clippy_utils::{desugar_await, get_async_closure_expr, get_async_fn_body, is_async_fn, is_from_proc_macro};
55
use core::ops::ControlFlow;
66
use rustc_errors::Applicability;
77
use rustc_hir::intravisit::FnKind;
@@ -134,6 +134,10 @@ fn lint_implicit_returns(
134134
},
135135

136136
ExprKind::Match(_, arms, _) => {
137+
if let Some(await_expr) = desugar_await(expr) {
138+
lint_return(cx, await_expr.hir_id, await_expr.span);
139+
return LintLocation::Inner;
140+
}
137141
for arm in arms {
138142
let res = lint_implicit_returns(
139143
cx,
@@ -241,6 +245,8 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitReturn {
241245
Some(e) => e,
242246
None => return,
243247
}
248+
} else if let Some(expr) = get_async_closure_expr(cx.tcx, body.value) {
249+
expr
244250
} else {
245251
body.value
246252
};

‎clippy_lints/src/redundant_async_block.rs‎

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,9 @@
1-
use std::ops::ControlFlow;
2-
31
use clippy_utils::diagnostics::span_lint_and_sugg;
4-
use clippy_utils::peel_blocks;
52
use clippy_utils::source::{snippet, walk_span_to_context};
63
use clippy_utils::ty::implements_trait;
7-
use clippy_utils::visitors::for_each_expr_without_closures;
4+
use clippy_utils::{desugar_await, peel_blocks};
85
use rustc_errors::Applicability;
9-
use rustc_hir::{
10-
Closure, ClosureKind, CoroutineDesugaring, CoroutineKind, CoroutineSource, Expr, ExprKind, MatchSource,
11-
};
6+
use rustc_hir::{Closure, ClosureKind, CoroutineDesugaring, CoroutineKind, CoroutineSource, Expr, ExprKind};
127
use rustc_lint::{LateContext, LateLintPass};
138
use rustc_middle::ty::UpvarCapture;
149
use rustc_session::declare_lint_pass;
@@ -99,20 +94,3 @@ fn desugar_async_block<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Op
9994
None
10095
}
10196
}
102-
103-
/// If `expr` is a desugared `.await`, return the original expression if it does not come from a
104-
/// macro expansion.
105-
fn desugar_await<'tcx>(expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
106-
if let ExprKind::Match(match_value, _, MatchSource::AwaitDesugar) = expr.kind
107-
&& let ExprKind::Call(_, [into_future_arg]) = match_value.kind
108-
&& let ctxt = expr.span.ctxt()
109-
&& for_each_expr_without_closures(into_future_arg, |e| {
110-
walk_span_to_context(e.span, ctxt).map_or(ControlFlow::Break(()), |_| ControlFlow::Continue(()))
111-
})
112-
.is_none()
113-
{
114-
Some(into_future_arg)
115-
} else {
116-
None
117-
}
118-
}

‎clippy_utils/src/lib.rs‎

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -104,10 +104,10 @@ use rustc_hir::hir_id::{HirIdMap, HirIdSet};
104104
use rustc_hir::intravisit::{FnKind, Visitor, walk_expr};
105105
use rustc_hir::{
106106
self as hir, Arm, BindingMode, Block, BlockCheckMode, Body, ByRef, Closure, ConstArgKind, ConstContext,
107-
Destination, Expr, ExprField, ExprKind, FnDecl, FnRetTy, GenericArg, GenericArgs, HirId, Impl, ImplItem,
108-
ImplItemKind, ImplItemRef, Item, ItemKind, LangItem, LetStmt, MatchSource, Mutability, Node, OwnerId, OwnerNode,
109-
Param, Pat, PatExpr, PatExprKind, PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitFn, TraitItem,
110-
TraitItemKind, TraitItemRef, TraitRef, TyKind, UnOp, def,
107+
CoroutineDesugaring, CoroutineKind, Destination, Expr, ExprField, ExprKind, FnDecl, FnRetTy, GenericArg,
108+
GenericArgs, HirId, Impl, ImplItem, ImplItemKind, ImplItemRef, Item, ItemKind, LangItem, LetStmt, MatchSource,
109+
Mutability, Node, OwnerId, OwnerNode, Param, Pat, PatExpr, PatExprKind, PatKind, Path, PathSegment, PrimTy, QPath,
110+
Stmt, StmtKind, TraitFn, TraitItem, TraitItemKind, TraitItemRef, TraitRef, TyKind, UnOp, def,
111111
};
112112
use rustc_lexer::{TokenKind, tokenize};
113113
use rustc_lint::{LateContext, Level, Lint, LintContext};
@@ -124,6 +124,7 @@ use rustc_span::hygiene::{ExpnKind, MacroKind};
124124
use rustc_span::source_map::SourceMap;
125125
use rustc_span::symbol::{Ident, Symbol, kw};
126126
use rustc_span::{InnerSpan, Span, sym};
127+
use source::walk_span_to_context;
127128
use visitors::{Visitable, for_each_unconsumed_temporary};
128129

129130
use crate::consts::{ConstEvalCtxt, Constant, mir_to_const};
@@ -2131,25 +2132,34 @@ pub fn is_async_fn(kind: FnKind<'_>) -> bool {
21312132
}
21322133
}
21332134

2134-
/// Peels away all the compiler generated code surrounding the body of an async function,
2135-
pub fn get_async_fn_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'_>) -> Option<&'tcx Expr<'tcx>> {
2136-
if let ExprKind::Closure(&Closure { body, .. }) = body.value.kind
2135+
/// Peels away all the compiler generated code surrounding the body of an async closure.
2136+
pub fn get_async_closure_expr<'tcx>(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
2137+
if let ExprKind::Closure(&Closure {
2138+
body,
2139+
kind: hir::ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::Async, _)),
2140+
..
2141+
}) = expr.kind
21372142
&& let ExprKind::Block(
21382143
Block {
2139-
stmts: [],
21402144
expr:
21412145
Some(Expr {
2142-
kind: ExprKind::DropTemps(expr),
2146+
kind: ExprKind::DropTemps(inner_expr),
21432147
..
21442148
}),
21452149
..
21462150
},
21472151
_,
21482152
) = tcx.hir_body(body).value.kind
21492153
{
2150-
return Some(expr);
2154+
Some(inner_expr)
2155+
} else {
2156+
None
21512157
}
2152-
None
2158+
}
2159+
2160+
/// Peels away all the compiler generated code surrounding the body of an async function,
2161+
pub fn get_async_fn_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'_>) -> Option<&'tcx Expr<'tcx>> {
2162+
get_async_closure_expr(tcx, body.value)
21532163
}
21542164

21552165
// check if expr is calling method or function with #[must_use] attribute
@@ -3716,3 +3726,20 @@ pub fn peel_hir_ty_options<'tcx>(cx: &LateContext<'tcx>, mut hir_ty: &'tcx hir::
37163726
}
37173727
hir_ty
37183728
}
3729+
3730+
/// If `expr` is a desugared `.await`, return the original expression if it does not come from a
3731+
/// macro expansion.
3732+
pub fn desugar_await<'tcx>(expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
3733+
if let ExprKind::Match(match_value, _, MatchSource::AwaitDesugar) = expr.kind
3734+
&& let ExprKind::Call(_, [into_future_arg]) = match_value.kind
3735+
&& let ctxt = expr.span.ctxt()
3736+
&& for_each_expr_without_closures(into_future_arg, |e| {
3737+
walk_span_to_context(e.span, ctxt).map_or(ControlFlow::Break(()), |_| ControlFlow::Continue(()))
3738+
})
3739+
.is_none()
3740+
{
3741+
Some(into_future_arg)
3742+
} else {
3743+
None
3744+
}
3745+
}

‎tests/ui/implicit_return.fixed‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,3 +165,46 @@ with_span!(
165165
x
166166
}
167167
);
168+
169+
fn desugared_closure_14446() {
170+
let _ = async || return 0;
171+
//~^ implicit_return
172+
#[rustfmt::skip]
173+
let _ = async || -> i32 { return 0 };
174+
//~^ implicit_return
175+
let _ = async |a: i32| return a;
176+
//~^ implicit_return
177+
#[rustfmt::skip]
178+
let _ = async |a: i32| { return a };
179+
//~^ implicit_return
180+
181+
let _ = async || return 0;
182+
let _ = async || -> i32 { return 0 };
183+
let _ = async |a: i32| return a;
184+
#[rustfmt::skip]
185+
let _ = async |a: i32| { return a; };
186+
187+
let _ = async || return foo().await;
188+
//~^ implicit_return
189+
let _ = async || {
190+
foo().await;
191+
return foo().await
192+
};
193+
//~^^ implicit_return
194+
#[rustfmt::skip]
195+
let _ = async || { return foo().await };
196+
//~^ implicit_return
197+
let _ = async || -> bool { return foo().await };
198+
//~^ implicit_return
199+
200+
let _ = async || return foo().await;
201+
let _ = async || {
202+
foo().await;
203+
return foo().await;
204+
};
205+
#[rustfmt::skip]
206+
let _ = async || { return foo().await; };
207+
let _ = async || -> bool {
208+
return foo().await;
209+
};
210+
}

‎tests/ui/implicit_return.rs‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,3 +165,46 @@ with_span!(
165165
x
166166
}
167167
);
168+
169+
fn desugared_closure_14446() {
170+
let _ = async || 0;
171+
//~^ implicit_return
172+
#[rustfmt::skip]
173+
let _ = async || -> i32 { 0 };
174+
//~^ implicit_return
175+
let _ = async |a: i32| a;
176+
//~^ implicit_return
177+
#[rustfmt::skip]
178+
let _ = async |a: i32| { a };
179+
//~^ implicit_return
180+
181+
let _ = async || return 0;
182+
let _ = async || -> i32 { return 0 };
183+
let _ = async |a: i32| return a;
184+
#[rustfmt::skip]
185+
let _ = async |a: i32| { return a; };
186+
187+
let _ = async || foo().await;
188+
//~^ implicit_return
189+
let _ = async || {
190+
foo().await;
191+
foo().await
192+
};
193+
//~^^ implicit_return
194+
#[rustfmt::skip]
195+
let _ = async || { foo().await };
196+
//~^ implicit_return
197+
let _ = async || -> bool { foo().await };
198+
//~^ implicit_return
199+
200+
let _ = async || return foo().await;
201+
let _ = async || {
202+
foo().await;
203+
return foo().await;
204+
};
205+
#[rustfmt::skip]
206+
let _ = async || { return foo().await; };
207+
let _ = async || -> bool {
208+
return foo().await;
209+
};
210+
}

‎tests/ui/implicit_return.stderr‎

Lines changed: 89 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,5 +183,93 @@ help: add `return` as shown
183183
LL | return true
184184
| ++++++
185185

186-
error: aborting due to 16 previous errors
186+
error: missing `return` statement
187+
--> tests/ui/implicit_return.rs:170:22
188+
|
189+
LL | let _ = async || 0;
190+
| ^
191+
|
192+
help: add `return` as shown
193+
|
194+
LL | let _ = async || return 0;
195+
| ++++++
196+
197+
error: missing `return` statement
198+
--> tests/ui/implicit_return.rs:173:31
199+
|
200+
LL | let _ = async || -> i32 { 0 };
201+
| ^
202+
|
203+
help: add `return` as shown
204+
|
205+
LL | let _ = async || -> i32 { return 0 };
206+
| ++++++
207+
208+
error: missing `return` statement
209+
--> tests/ui/implicit_return.rs:175:28
210+
|
211+
LL | let _ = async |a: i32| a;
212+
| ^
213+
|
214+
help: add `return` as shown
215+
|
216+
LL | let _ = async |a: i32| return a;
217+
| ++++++
218+
219+
error: missing `return` statement
220+
--> tests/ui/implicit_return.rs:178:30
221+
|
222+
LL | let _ = async |a: i32| { a };
223+
| ^
224+
|
225+
help: add `return` as shown
226+
|
227+
LL | let _ = async |a: i32| { return a };
228+
| ++++++
229+
230+
error: missing `return` statement
231+
--> tests/ui/implicit_return.rs:187:22
232+
|
233+
LL | let _ = async || foo().await;
234+
| ^^^^^
235+
|
236+
help: add `return` as shown
237+
|
238+
LL | let _ = async || return foo().await;
239+
| ++++++
240+
241+
error: missing `return` statement
242+
--> tests/ui/implicit_return.rs:191:9
243+
|
244+
LL | foo().await
245+
| ^^^^^
246+
|
247+
help: add `return` as shown
248+
|
249+
LL | return foo().await
250+
| ++++++
251+
252+
error: missing `return` statement
253+
--> tests/ui/implicit_return.rs:195:24
254+
|
255+
LL | let _ = async || { foo().await };
256+
| ^^^^^
257+
|
258+
help: add `return` as shown
259+
|
260+
LL | let _ = async || { return foo().await };
261+
| ++++++
262+
263+
error: missing `return` statement
264+
--> tests/ui/implicit_return.rs:197:32
265+
|
266+
LL | let _ = async || -> bool { foo().await };
267+
| ^^^^^
268+
|
269+
help: add `return` as shown
270+
|
271+
LL | let _ = async || -> bool { return foo().await };
272+
| ++++++
273+
274+
error: aborting due to 24 previous errors
187275

0 commit comments

Comments
 (0)