Skip to content

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

Merged
merged 3 commits into from
Aug 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1512,6 +1512,7 @@ Released 2018-09-13
[`assertions_on_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants
[`assign_op_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
[`assign_ops`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_ops
[`async_yields_async`]: https://rust-lang.github.io/rust-clippy/master/index.html#async_yields_async
[`await_holding_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock
[`bad_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#bad_bit_mask
[`bind_instead_of_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#bind_instead_of_map
Expand Down
86 changes: 86 additions & 0 deletions clippy_lints/src/async_yields_async.rs
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};
// For functions, with explicitly defined types, don't warn.
// 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.
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,
);
},
);
}
}
}
}
}
}
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ mod arithmetic;
mod as_conversions;
mod assertions_on_constants;
mod assign_ops;
mod async_yields_async;
mod atomic_ordering;
mod attrs;
mod await_holding_lock;
Expand Down Expand Up @@ -483,6 +484,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&assertions_on_constants::ASSERTIONS_ON_CONSTANTS,
&assign_ops::ASSIGN_OP_PATTERN,
&assign_ops::MISREFACTORED_ASSIGN_OP,
&async_yields_async::ASYNC_YIELDS_ASYNC,
&atomic_ordering::INVALID_ATOMIC_ORDERING,
&attrs::BLANKET_CLIPPY_RESTRICTION_LINTS,
&attrs::DEPRECATED_CFG_ATTR,
Expand Down Expand Up @@ -1099,6 +1101,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box unwrap_in_result::UnwrapInResult);
store.register_late_pass(|| box self_assignment::SelfAssignment);
store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs);
store.register_late_pass(|| box async_yields_async::AsyncYieldsAsync);

store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
Expand Down Expand Up @@ -1232,6 +1235,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&assertions_on_constants::ASSERTIONS_ON_CONSTANTS),
LintId::of(&assign_ops::ASSIGN_OP_PATTERN),
LintId::of(&assign_ops::MISREFACTORED_ASSIGN_OP),
LintId::of(&async_yields_async::ASYNC_YIELDS_ASYNC),
LintId::of(&atomic_ordering::INVALID_ATOMIC_ORDERING),
LintId::of(&attrs::BLANKET_CLIPPY_RESTRICTION_LINTS),
LintId::of(&attrs::DEPRECATED_CFG_ATTR),
Expand Down Expand Up @@ -1675,6 +1679,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:

store.register_group(true, "clippy::correctness", Some("clippy_correctness"), vec![
LintId::of(&approx_const::APPROX_CONSTANT),
LintId::of(&async_yields_async::ASYNC_YIELDS_ASYNC),
LintId::of(&atomic_ordering::INVALID_ATOMIC_ORDERING),
LintId::of(&attrs::DEPRECATED_SEMVER),
LintId::of(&attrs::MISMATCHED_TARGET_OS),
Expand Down
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "assign_ops",
},
Lint {
name: "async_yields_async",
group: "correctness",
desc: "async blocks that return a type that can be awaited",
deprecation: None,
module: "async_yields_async",
},
Lint {
name: "await_holding_lock",
group: "pedantic",
Expand Down
68 changes: 68 additions & 0 deletions tests/ui/async_yields_async.fixed
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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 return_expr_span :(

There's no case for these so they don't warn.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, do you mean the XXXhuey bit, as in, this doesn't resolve to either a Block or a Path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right

Copy link
Member

Choose a reason for hiding this comment

The 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...

}
68 changes: 68 additions & 0 deletions tests/ui/async_yields_async.rs
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();
}
96 changes: 96 additions & 0 deletions tests/ui/async_yields_async.stderr
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