Make redundant_async_block a more complete late pass

This lets us detect more complex situations: `async { x.await }` is
simplified into `x` if:

- `x` is an expression without side-effect
- or `x` is an async block itself

In both cases, no part of the `async` expression can be part of a macro
expansion.
This commit is contained in:
Samuel "Sam" Tardieu 2023-03-26 17:11:34 +02:00
parent 5ed64d4c61
commit 2891d8f72f
5 changed files with 338 additions and 151 deletions

View File

@ -935,7 +935,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|_| Box::new(no_mangle_with_rust_abi::NoMangleWithRustAbi));
store.register_late_pass(|_| Box::new(collection_is_never_read::CollectionIsNeverRead));
store.register_late_pass(|_| Box::new(missing_assert_message::MissingAssertMessage));
store.register_early_pass(|| Box::new(redundant_async_block::RedundantAsyncBlock));
store.register_late_pass(|_| Box::new(redundant_async_block::RedundantAsyncBlock));
store.register_late_pass(|_| Box::new(let_with_type_underscore::UnderscoreTyped));
store.register_late_pass(|_| Box::new(allow_attributes::AllowAttribute));
store.register_late_pass(move |_| Box::new(manual_main_separator_str::ManualMainSeparatorStr::new(msrv())));

View File

@ -1,8 +1,15 @@
use clippy_utils::{diagnostics::span_lint_and_sugg, source::snippet};
use rustc_ast::ast::{Expr, ExprKind, Stmt, StmtKind};
use rustc_ast::visit::Visitor as AstVisitor;
use std::ops::ControlFlow;
use clippy_utils::{
diagnostics::span_lint_and_sugg,
peel_blocks,
source::{snippet, walk_span_to_context},
visitors::for_each_expr,
};
use rustc_errors::Applicability;
use rustc_lint::{EarlyContext, EarlyLintPass};
use rustc_hir::{AsyncGeneratorKind, Closure, Expr, ExprKind, GeneratorKind, MatchSource};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::{lint::in_external_macro, ty::UpvarCapture};
use rustc_session::{declare_lint_pass, declare_tool_lint};
declare_clippy_lint! {
@ -14,106 +21,88 @@ declare_clippy_lint! {
///
/// ### Example
/// ```rust
/// async fn f() -> i32 {
/// 1 + 2
/// }
///
/// let f = async {
/// 1 + 2
/// };
/// let fut = async {
/// f().await
/// f.await
/// };
/// ```
/// Use instead:
/// ```rust
/// async fn f() -> i32 {
/// 1 + 2
/// }
///
/// let fut = f();
/// let f = async {
/// 1 + 2
/// };
/// let fut = f;
/// ```
#[clippy::version = "1.69.0"]
pub REDUNDANT_ASYNC_BLOCK,
nursery,
complexity,
"`async { future.await }` can be replaced by `future`"
}
declare_lint_pass!(RedundantAsyncBlock => [REDUNDANT_ASYNC_BLOCK]);
impl EarlyLintPass for RedundantAsyncBlock {
fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
if expr.span.from_expansion() {
return;
}
if let ExprKind::Async(_, _, block) = &expr.kind && block.stmts.len() == 1 &&
let Some(Stmt { kind: StmtKind::Expr(last), .. }) = block.stmts.last() &&
let ExprKind::Await(future) = &last.kind &&
!future.span.from_expansion() &&
!await_in_expr(future)
impl<'tcx> LateLintPass<'tcx> for RedundantAsyncBlock {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
let span = expr.span;
if !in_external_macro(cx.tcx.sess, span) &&
let Some(body_expr) = desugar_async_block(cx, expr) &&
let Some(expr) = desugar_await(peel_blocks(body_expr)) &&
// The await prefix must not come from a macro as its content could change in the future.
expr.span.ctxt() == body_expr.span.ctxt() &&
// An async block does not have immediate side-effects from a `.await` point-of-view.
(!expr.can_have_side_effects() || desugar_async_block(cx, expr).is_some()) &&
let Some(shortened_span) = walk_span_to_context(expr.span, span.ctxt())
{
if captures_value(last) {
// If the async block captures variables then there is no equivalence.
return;
}
span_lint_and_sugg(
cx,
REDUNDANT_ASYNC_BLOCK,
expr.span,
span,
"this async expression only awaits a single future",
"you can reduce it to",
snippet(cx, future.span, "..").into_owned(),
snippet(cx, shortened_span, "..").into_owned(),
Applicability::MachineApplicable,
);
}
}
}
/// Check whether an expression contains `.await`
fn await_in_expr(expr: &Expr) -> bool {
let mut detector = AwaitDetector::default();
detector.visit_expr(expr);
detector.await_found
}
#[derive(Default)]
struct AwaitDetector {
await_found: bool,
}
impl<'ast> AstVisitor<'ast> for AwaitDetector {
fn visit_expr(&mut self, ex: &'ast Expr) {
match (&ex.kind, self.await_found) {
(ExprKind::Await(_), _) => self.await_found = true,
(_, false) => rustc_ast::visit::walk_expr(self, ex),
_ => (),
}
/// If `expr` is a desugared `async` block, return the original expression if it does not capture
/// any variable by ref.
fn desugar_async_block<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
if let ExprKind::Closure(Closure { body, def_id, .. }) = expr.kind &&
let body = cx.tcx.hir().body(*body) &&
matches!(body.generator_kind, Some(GeneratorKind::Async(AsyncGeneratorKind::Block)))
{
cx
.typeck_results()
.closure_min_captures
.get(def_id)
.map_or(true, |m| {
m.values().all(|places| {
places
.iter()
.all(|place| matches!(place.info.capture_kind, UpvarCapture::ByValue))
})
})
.then_some(body.value)
} else {
None
}
}
/// Check whether an expression may have captured a local variable.
/// This is done by looking for paths with only one segment, except as
/// a prefix of `.await` since this would be captured by value.
///
/// This function will sometimes return `true` even tough there are no
/// captures happening: at the AST level, it is impossible to
/// dinstinguish a function call from a call to a closure which comes
/// from the local environment.
fn captures_value(expr: &Expr) -> bool {
let mut detector = CaptureDetector::default();
detector.visit_expr(expr);
detector.capture_found
}
#[derive(Default)]
struct CaptureDetector {
capture_found: bool,
}
impl<'ast> AstVisitor<'ast> for CaptureDetector {
fn visit_expr(&mut self, ex: &'ast Expr) {
match (&ex.kind, self.capture_found) {
(ExprKind::Await(fut), _) if matches!(fut.kind, ExprKind::Path(..)) => (),
(ExprKind::Path(_, path), _) if path.segments.len() == 1 => self.capture_found = true,
(_, false) => rustc_ast::visit::walk_expr(self, ex),
_ => (),
}
/// If `expr` is a desugared `.await`, return the original expression if it does not come from a
/// macro expansion.
fn desugar_await<'tcx>(expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
if let ExprKind::Match(match_value, _, MatchSource::AwaitDesugar) = expr.kind &&
let ExprKind::Call(_, [into_future_arg]) = match_value.kind &&
let ctxt = expr.span.ctxt() &&
for_each_expr(into_future_arg, |e|
walk_span_to_context(e.span, ctxt)
.map_or(ControlFlow::Break(()), |_| ControlFlow::Continue(()))).is_none()
{
Some(into_future_arg)
} else {
None
}
}

View File

@ -1,6 +1,6 @@
// run-rustfix
#![allow(unused)]
#![allow(unused, clippy::manual_async_fn)]
#![warn(clippy::redundant_async_block)]
use std::future::Future;
@ -16,40 +16,16 @@ async fn func2() -> String {
x.await
}
macro_rules! await_in_macro {
($e:expr) => {
std::convert::identity($e).await
};
}
async fn func3(n: usize) -> usize {
// Do not lint (suggestion would be `std::convert::identity(func1(n))`
// which copies code from inside the macro)
async move { await_in_macro!(func1(n)) }.await
}
// This macro should never be linted as `$e` might contain `.await`
macro_rules! async_await_parameter_in_macro {
($e:expr) => {
async { $e.await }
};
}
// MISSED OPPORTUNITY: this macro could be linted as the `async` block does not
// contain code coming from the parameters
macro_rules! async_await_in_macro {
($f:expr) => {
($f)(async { func2().await })
};
}
fn main() {
let fut1 = async { 17 };
// Lint
let fut2 = fut1;
let fut1 = async { 25 };
// Lint
let fut2 = fut1;
// Lint
let fut = async { 42 };
// Do not lint: not a single expression
@ -60,15 +36,12 @@ fn main() {
// Do not lint: expression contains `.await`
let fut = async { func1(func2().await.len()).await };
let fut = async_await_parameter_in_macro!(func2());
let fut = async_await_in_macro!(std::convert::identity);
}
#[allow(clippy::let_and_return)]
fn capture_local() -> impl Future<Output = i32> {
// Lint
let fut = async { 17 };
// Lint
fut
}
@ -80,11 +53,39 @@ fn capture_local_closure(s: &str) -> impl Future<Output = &str> {
#[allow(clippy::let_and_return)]
fn capture_arg(s: &str) -> impl Future<Output = &str> {
// Lint
let fut = async move { s };
// Lint
fut
}
fn capture_future_arg<T>(f: impl Future<Output = T>) -> impl Future<Output = T> {
// Lint
f
}
fn capture_func_result<FN, F, T>(f: FN) -> impl Future<Output = T>
where
F: Future<Output = T>,
FN: FnOnce() -> F,
{
// Do not lint, as f() would be evaluated prematurely
async { f().await }
}
fn double_future(f: impl Future<Output = impl Future<Output = u32>>) -> impl Future<Output = u32> {
// Do not lint, we will get a `.await` outside a `.async`
async { f.await.await }
}
fn await_in_async<F, R>(f: F) -> impl Future<Output = u32>
where
F: FnOnce() -> R,
R: Future<Output = u32>,
{
// Lint
async { f().await + 1 }
}
#[derive(Debug, Clone)]
struct F {}
@ -109,3 +110,84 @@ fn capture() {
// Do not lint: `val` would not live long enough
spawn(async { work(&{ val }).await });
}
fn await_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
($e:expr) => {
$e.await
};
}
// Do not lint: the macro may change in the future
// or return different things depending on its argument
async { mac!(async { 42 }) }
}
fn async_expr_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
() => {
async { 42 }
};
}
// Do not lint: the macro may change in the future
async { mac!().await }
}
fn async_expr_from_macro_deep() -> impl Future<Output = u32> {
macro_rules! mac {
() => {
async { 42 }
};
}
// Do not lint: the macro may change in the future
async { ({ mac!() }).await }
}
fn all_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
() => {
// Lint
async { 42 }
};
}
mac!()
}
fn parts_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
($e: expr) => {
// Do not lint: `$e` might not always be side-effect free
async { $e.await }
};
}
mac!(async { 42 })
}
fn safe_parts_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
($e: expr) => {
// Lint
async { $e }
};
}
mac!(42)
}
fn parts_from_macro_deep() -> impl Future<Output = u32> {
macro_rules! mac {
($e: expr) => {
// Do not lint: `$e` might not always be side-effect free
async { ($e,).0.await }
};
}
let f = std::future::ready(42);
mac!(f)
}
fn await_from_macro_deep() -> impl Future<Output = u32> {
macro_rules! mac {
($e:expr) => {{ $e }.await};
}
// Do not lint: the macro may change in the future
// or return different things depending on its argument
async { mac!(async { 42 }) }
}

View File

@ -1,6 +1,6 @@
// run-rustfix
#![allow(unused)]
#![allow(unused, clippy::manual_async_fn)]
#![warn(clippy::redundant_async_block)]
use std::future::Future;
@ -16,40 +16,16 @@ async fn func2() -> String {
x.await
}
macro_rules! await_in_macro {
($e:expr) => {
std::convert::identity($e).await
};
}
async fn func3(n: usize) -> usize {
// Do not lint (suggestion would be `std::convert::identity(func1(n))`
// which copies code from inside the macro)
async move { await_in_macro!(func1(n)) }.await
}
// This macro should never be linted as `$e` might contain `.await`
macro_rules! async_await_parameter_in_macro {
($e:expr) => {
async { $e.await }
};
}
// MISSED OPPORTUNITY: this macro could be linted as the `async` block does not
// contain code coming from the parameters
macro_rules! async_await_in_macro {
($f:expr) => {
($f)(async { func2().await })
};
}
fn main() {
let fut1 = async { 17 };
// Lint
let fut2 = async { fut1.await };
let fut1 = async { 25 };
// Lint
let fut2 = async move { fut1.await };
// Lint
let fut = async { async { 42 }.await };
// Do not lint: not a single expression
@ -60,15 +36,12 @@ fn main() {
// Do not lint: expression contains `.await`
let fut = async { func1(func2().await.len()).await };
let fut = async_await_parameter_in_macro!(func2());
let fut = async_await_in_macro!(std::convert::identity);
}
#[allow(clippy::let_and_return)]
fn capture_local() -> impl Future<Output = i32> {
// Lint
let fut = async { 17 };
// Lint
async move { fut.await }
}
@ -80,11 +53,39 @@ fn capture_local_closure(s: &str) -> impl Future<Output = &str> {
#[allow(clippy::let_and_return)]
fn capture_arg(s: &str) -> impl Future<Output = &str> {
// Lint
let fut = async move { s };
// Lint
async move { fut.await }
}
fn capture_future_arg<T>(f: impl Future<Output = T>) -> impl Future<Output = T> {
// Lint
async { f.await }
}
fn capture_func_result<FN, F, T>(f: FN) -> impl Future<Output = T>
where
F: Future<Output = T>,
FN: FnOnce() -> F,
{
// Do not lint, as f() would be evaluated prematurely
async { f().await }
}
fn double_future(f: impl Future<Output = impl Future<Output = u32>>) -> impl Future<Output = u32> {
// Do not lint, we will get a `.await` outside a `.async`
async { f.await.await }
}
fn await_in_async<F, R>(f: F) -> impl Future<Output = u32>
where
F: FnOnce() -> R,
R: Future<Output = u32>,
{
// Lint
async { async { f().await + 1 }.await }
}
#[derive(Debug, Clone)]
struct F {}
@ -109,3 +110,84 @@ fn capture() {
// Do not lint: `val` would not live long enough
spawn(async { work(&{ val }).await });
}
fn await_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
($e:expr) => {
$e.await
};
}
// Do not lint: the macro may change in the future
// or return different things depending on its argument
async { mac!(async { 42 }) }
}
fn async_expr_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
() => {
async { 42 }
};
}
// Do not lint: the macro may change in the future
async { mac!().await }
}
fn async_expr_from_macro_deep() -> impl Future<Output = u32> {
macro_rules! mac {
() => {
async { 42 }
};
}
// Do not lint: the macro may change in the future
async { ({ mac!() }).await }
}
fn all_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
() => {
// Lint
async { async { 42 }.await }
};
}
mac!()
}
fn parts_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
($e: expr) => {
// Do not lint: `$e` might not always be side-effect free
async { $e.await }
};
}
mac!(async { 42 })
}
fn safe_parts_from_macro() -> impl Future<Output = u32> {
macro_rules! mac {
($e: expr) => {
// Lint
async { async { $e }.await }
};
}
mac!(42)
}
fn parts_from_macro_deep() -> impl Future<Output = u32> {
macro_rules! mac {
($e: expr) => {
// Do not lint: `$e` might not always be side-effect free
async { ($e,).0.await }
};
}
let f = std::future::ready(42);
mac!(f)
}
fn await_from_macro_deep() -> impl Future<Output = u32> {
macro_rules! mac {
($e:expr) => {{ $e }.await};
}
// Do not lint: the macro may change in the future
// or return different things depending on its argument
async { mac!(async { 42 }) }
}

View File

@ -7,34 +7,68 @@ LL | let x = async { f.await };
= note: `-D clippy::redundant-async-block` implied by `-D warnings`
error: this async expression only awaits a single future
--> $DIR/redundant_async_block.rs:48:16
--> $DIR/redundant_async_block.rs:22:16
|
LL | let fut2 = async { fut1.await };
| ^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `fut1`
error: this async expression only awaits a single future
--> $DIR/redundant_async_block.rs:51:16
--> $DIR/redundant_async_block.rs:26:16
|
LL | let fut2 = async move { fut1.await };
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `fut1`
error: this async expression only awaits a single future
--> $DIR/redundant_async_block.rs:53:15
--> $DIR/redundant_async_block.rs:29:15
|
LL | let fut = async { async { 42 }.await };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `async { 42 }`
error: this async expression only awaits a single future
--> $DIR/redundant_async_block.rs:72:5
--> $DIR/redundant_async_block.rs:45:5
|
LL | async move { fut.await }
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `fut`
error: this async expression only awaits a single future
--> $DIR/redundant_async_block.rs:85:5
--> $DIR/redundant_async_block.rs:58:5
|
LL | async move { fut.await }
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `fut`
error: aborting due to 6 previous errors
error: this async expression only awaits a single future
--> $DIR/redundant_async_block.rs:63:5
|
LL | async { f.await }
| ^^^^^^^^^^^^^^^^^ help: you can reduce it to: `f`
error: this async expression only awaits a single future
--> $DIR/redundant_async_block.rs:86:5
|
LL | async { async { f().await + 1 }.await }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `async { f().await + 1 }`
error: this async expression only awaits a single future
--> $DIR/redundant_async_block.rs:149:13
|
LL | async { async { 42 }.await }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `async { 42 }`
...
LL | mac!()
| ------ in this macro invocation
|
= note: this error originates in the macro `mac` (in Nightly builds, run with -Z macro-backtrace for more info)
error: this async expression only awaits a single future
--> $DIR/redundant_async_block.rs:169:13
|
LL | async { async { $e }.await }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `async { $e }`
...
LL | mac!(42)
| -------- in this macro invocation
|
= note: this error originates in the macro `mac` (in Nightly builds, run with -Z macro-backtrace for more info)
error: aborting due to 10 previous errors