-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add a lint for an async block/closure that yields a type that is itself awaitable. #5909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
use crate::utils::{implements_trait, snippet, span_lint_and_then}; | ||
use rustc_errors::Applicability; | ||
use rustc_hir::{AsyncGeneratorKind, Body, BodyId, ExprKind, GeneratorKind, QPath}; | ||
use rustc_lint::{LateContext, LateLintPass}; | ||
use rustc_session::{declare_lint_pass, declare_tool_lint}; | ||
|
||
declare_clippy_lint! { | ||
/// **What it does:** Checks for async blocks that yield values of types | ||
/// that can themselves be awaited. | ||
/// | ||
/// **Why is this bad?** An await is likely missing. | ||
/// | ||
/// **Known problems:** None. | ||
/// | ||
/// **Example:** | ||
/// | ||
/// ```rust | ||
/// async fn foo() {} | ||
/// | ||
/// fn bar() { | ||
/// let x = async { | ||
/// foo() | ||
/// }; | ||
/// } | ||
/// ``` | ||
/// Use instead: | ||
/// ```rust | ||
/// async fn foo() {} | ||
/// | ||
/// fn bar() { | ||
/// let x = async { | ||
/// foo().await | ||
/// }; | ||
/// } | ||
/// ``` | ||
pub ASYNC_YIELDS_ASYNC, | ||
correctness, | ||
"async blocks that return a type that can be awaited" | ||
} | ||
|
||
declare_lint_pass!(AsyncYieldsAsync => [ASYNC_YIELDS_ASYNC]); | ||
|
||
impl<'tcx> LateLintPass<'tcx> for AsyncYieldsAsync { | ||
fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) { | ||
use AsyncGeneratorKind::{Block, Closure}; | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// For functions, with explicitly defined types, don't warn. | ||
khuey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// XXXkhuey maybe we should? | ||
if let Some(GeneratorKind::Async(Block | Closure)) = body.generator_kind { | ||
if let Some(future_trait_def_id) = cx.tcx.lang_items().future_trait() { | ||
let body_id = BodyId { | ||
hir_id: body.value.hir_id, | ||
}; | ||
let def_id = cx.tcx.hir().body_owner_def_id(body_id); | ||
let typeck_results = cx.tcx.typeck(def_id); | ||
let expr_ty = typeck_results.expr_ty(&body.value); | ||
|
||
if implements_trait(cx, expr_ty, future_trait_def_id, &[]) { | ||
let return_expr_span = match &body.value.kind { | ||
// XXXkhuey there has to be a better way. | ||
yaahc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ExprKind::Block(block, _) => block.expr.map(|e| e.span), | ||
ExprKind::Path(QPath::Resolved(_, path)) => Some(path.span), | ||
_ => None, | ||
}; | ||
if let Some(return_expr_span) = return_expr_span { | ||
span_lint_and_then( | ||
cx, | ||
ASYNC_YIELDS_ASYNC, | ||
return_expr_span, | ||
"an async construct yields a type which is itself awaitable", | ||
|db| { | ||
db.span_label(body.value.span, "outer async construct"); | ||
db.span_label(return_expr_span, "awaitable value not awaited"); | ||
db.span_suggestion( | ||
return_expr_span, | ||
"consider awaiting this value", | ||
format!("{}.await", snippet(cx, return_expr_span, "..")), | ||
Applicability::MaybeIncorrect, | ||
); | ||
}, | ||
); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
// run-rustfix | ||
// edition:2018 | ||
|
||
#![feature(async_closure)] | ||
#![warn(clippy::async_yields_async)] | ||
|
||
use core::future::Future; | ||
use core::pin::Pin; | ||
use core::task::{Context, Poll}; | ||
|
||
struct CustomFutureType; | ||
|
||
impl Future for CustomFutureType { | ||
type Output = u8; | ||
|
||
fn poll(self: Pin<&mut Self>, _: &mut Context) -> Poll<Self::Output> { | ||
Poll::Ready(3) | ||
} | ||
} | ||
|
||
fn custom_future_type_ctor() -> CustomFutureType { | ||
CustomFutureType | ||
} | ||
|
||
async fn f() -> CustomFutureType { | ||
// Don't warn for functions since you have to explicitly declare their | ||
// return types. | ||
CustomFutureType | ||
} | ||
|
||
#[rustfmt::skip] | ||
fn main() { | ||
let _f = { | ||
3 | ||
}; | ||
let _g = async { | ||
3 | ||
}; | ||
let _h = async { | ||
async { | ||
3 | ||
}.await | ||
}; | ||
let _i = async { | ||
CustomFutureType.await | ||
}; | ||
let _i = async || { | ||
3 | ||
}; | ||
let _j = async || { | ||
async { | ||
3 | ||
}.await | ||
}; | ||
let _k = async || { | ||
CustomFutureType.await | ||
}; | ||
let _l = async || CustomFutureType.await; | ||
let _m = async || { | ||
println!("I'm bored"); | ||
// Some more stuff | ||
|
||
// Finally something to await | ||
CustomFutureType.await | ||
}; | ||
let _n = async || custom_future_type_ctor(); | ||
let _o = async || f(); | ||
Comment on lines
+66
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why aren't these two triggering the lint here? I can understand that their definitions shouldn't trigger the lint but these usages seem like they should. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an example of the limitations of having to match a zillion AST types to deduce There's no case for these so they don't warn. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, do you mean the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea it feels like there has to be a way to handle this... |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
// run-rustfix | ||
// edition:2018 | ||
|
||
#![feature(async_closure)] | ||
#![warn(clippy::async_yields_async)] | ||
|
||
use core::future::Future; | ||
use core::pin::Pin; | ||
use core::task::{Context, Poll}; | ||
|
||
struct CustomFutureType; | ||
|
||
impl Future for CustomFutureType { | ||
type Output = u8; | ||
|
||
fn poll(self: Pin<&mut Self>, _: &mut Context) -> Poll<Self::Output> { | ||
Poll::Ready(3) | ||
} | ||
} | ||
|
||
fn custom_future_type_ctor() -> CustomFutureType { | ||
CustomFutureType | ||
} | ||
|
||
async fn f() -> CustomFutureType { | ||
// Don't warn for functions since you have to explicitly declare their | ||
// return types. | ||
CustomFutureType | ||
} | ||
|
||
#[rustfmt::skip] | ||
fn main() { | ||
let _f = { | ||
3 | ||
}; | ||
let _g = async { | ||
3 | ||
}; | ||
let _h = async { | ||
async { | ||
3 | ||
} | ||
}; | ||
let _i = async { | ||
CustomFutureType | ||
}; | ||
let _i = async || { | ||
3 | ||
}; | ||
let _j = async || { | ||
async { | ||
3 | ||
} | ||
}; | ||
let _k = async || { | ||
CustomFutureType | ||
}; | ||
let _l = async || CustomFutureType; | ||
let _m = async || { | ||
println!("I'm bored"); | ||
// Some more stuff | ||
|
||
// Finally something to await | ||
CustomFutureType | ||
}; | ||
let _n = async || custom_future_type_ctor(); | ||
let _o = async || f(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
error: an async construct yields a type which is itself awaitable | ||
--> $DIR/async_yields_async.rs:40:9 | ||
| | ||
LL | let _h = async { | ||
| ____________________- | ||
LL | | async { | ||
| |_________^ | ||
LL | || 3 | ||
LL | || } | ||
| ||_________^ awaitable value not awaited | ||
LL | | }; | ||
| |_____- outer async construct | ||
| | ||
= note: `-D clippy::async-yields-async` implied by `-D warnings` | ||
help: consider awaiting this value | ||
| | ||
LL | async { | ||
LL | 3 | ||
LL | }.await | ||
| | ||
|
||
error: an async construct yields a type which is itself awaitable | ||
--> $DIR/async_yields_async.rs:45:9 | ||
| | ||
LL | let _i = async { | ||
| ____________________- | ||
LL | | CustomFutureType | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | | ||
| | awaitable value not awaited | ||
| | help: consider awaiting this value: `CustomFutureType.await` | ||
LL | | }; | ||
| |_____- outer async construct | ||
|
||
error: an async construct yields a type which is itself awaitable | ||
--> $DIR/async_yields_async.rs:51:9 | ||
| | ||
LL | let _j = async || { | ||
| _______________________- | ||
LL | | async { | ||
| |_________^ | ||
LL | || 3 | ||
LL | || } | ||
| ||_________^ awaitable value not awaited | ||
LL | | }; | ||
| |_____- outer async construct | ||
| | ||
help: consider awaiting this value | ||
| | ||
LL | async { | ||
LL | 3 | ||
LL | }.await | ||
| | ||
|
||
error: an async construct yields a type which is itself awaitable | ||
--> $DIR/async_yields_async.rs:56:9 | ||
| | ||
LL | let _k = async || { | ||
| _______________________- | ||
LL | | CustomFutureType | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | | ||
| | awaitable value not awaited | ||
| | help: consider awaiting this value: `CustomFutureType.await` | ||
LL | | }; | ||
| |_____- outer async construct | ||
|
||
error: an async construct yields a type which is itself awaitable | ||
--> $DIR/async_yields_async.rs:58:23 | ||
| | ||
LL | let _l = async || CustomFutureType; | ||
| ^^^^^^^^^^^^^^^^ | ||
| | | ||
| outer async construct | ||
| awaitable value not awaited | ||
| help: consider awaiting this value: `CustomFutureType.await` | ||
|
||
error: an async construct yields a type which is itself awaitable | ||
--> $DIR/async_yields_async.rs:64:9 | ||
| | ||
LL | let _m = async || { | ||
| _______________________- | ||
LL | | println!("I'm bored"); | ||
LL | | // Some more stuff | ||
LL | | | ||
LL | | // Finally something to await | ||
LL | | CustomFutureType | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | | ||
| | awaitable value not awaited | ||
| | help: consider awaiting this value: `CustomFutureType.await` | ||
LL | | }; | ||
| |_____- outer async construct | ||
|
||
error: aborting due to 6 previous errors | ||
|
Uh oh!
There was an error while loading. Please reload this page.