Skip to content
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

[unused_async]: don't lint if paths reference async fn without immediate call #11200

Merged
merged 2 commits into from
Jul 22, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(move |_| Box::new(if_then_some_else_none::IfThenSomeElseNone::new(msrv())));
store.register_late_pass(|_| Box::new(bool_assert_comparison::BoolAssertComparison));
store.register_early_pass(move || Box::new(module_style::ModStyle));
store.register_late_pass(|_| Box::new(unused_async::UnusedAsync));
store.register_late_pass(|_| Box::<unused_async::UnusedAsync>::default());
let disallowed_types = conf.disallowed_types.clone();
store.register_late_pass(move |_| Box::new(disallowed_types::DisallowedTypes::new(disallowed_types.clone())));
let import_renames = conf.enforced_import_renames.clone();
Expand Down
103 changes: 84 additions & 19 deletions clippy_lints/src/unused_async.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::is_def_id_trait_method;
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::def::DefKind;
use rustc_hir::intravisit::{walk_body, walk_expr, walk_fn, FnKind, Visitor};
use rustc_hir::{Body, Expr, ExprKind, FnDecl, YieldSource};
use rustc_hir::{Body, Expr, ExprKind, FnDecl, Node, YieldSource};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::hir::nested_filter;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::def_id::LocalDefId;
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::def_id::{LocalDefId, LocalDefIdSet};
use rustc_span::Span;

declare_clippy_lint! {
Expand Down Expand Up @@ -38,7 +40,23 @@ declare_clippy_lint! {
"finds async functions with no await statements"
}

declare_lint_pass!(UnusedAsync => [UNUSED_ASYNC]);
#[derive(Default)]
pub struct UnusedAsync {
/// Keeps track of async functions used as values (i.e. path expressions to async functions that
/// are not immediately called)
async_fns_as_value: LocalDefIdSet,
/// Functions with unused `async`, linted post-crate after we've found all uses of local async
/// functions
unused_async_fns: FxHashMap<LocalDefId, UnusedAsyncFn>,
y21 marked this conversation as resolved.
Show resolved Hide resolved
}

#[derive(Copy, Clone)]
struct UnusedAsyncFn {
fn_span: Span,
await_in_async_block: Option<Span>,
}

impl_lint_pass!(UnusedAsync => [UNUSED_ASYNC]);

struct AsyncFnVisitor<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
Expand Down Expand Up @@ -101,24 +119,71 @@ impl<'tcx> LateLintPass<'tcx> for UnusedAsync {
};
walk_fn(&mut visitor, fn_kind, fn_decl, body.id(), def_id);
if !visitor.found_await {
span_lint_and_then(
cx,
UNUSED_ASYNC,
span,
"unused `async` for function with no await statements",
|diag| {
diag.help("consider removing the `async` from this function");

if let Some(span) = visitor.await_in_async_block {
diag.span_note(
span,
"`await` used in an async block, which does not require \
the enclosing function to be `async`",
);
}
// Don't lint just yet, but store the necessary information for later.
// The actual linting happens in `check_crate_post`, once we've found all
// uses of local async functions that do require asyncness to pass typeck
self.unused_async_fns.insert(
def_id,
UnusedAsyncFn {
await_in_async_block: visitor.await_in_async_block,
fn_span: span,
},
);
}
}
}

fn check_path(&mut self, cx: &LateContext<'tcx>, path: &rustc_hir::Path<'tcx>, hir_id: rustc_hir::HirId) {
fn is_node_func_call(node: Node<'_>, expected_receiver: Span) -> bool {
matches!(
node,
Node::Expr(Expr {
kind: ExprKind::Call(Expr { span, .. }, _) | ExprKind::MethodCall(_, Expr { span, .. }, ..),
..
}) if *span == expected_receiver
)
}

// Find paths to local async functions that aren't immediately called.
// E.g. `async fn f() {}; let x = f;`
// Depending on how `x` is used, f's asyncness might be required despite not having any `await`
// statements, so don't lint at all if there are any such paths.
Comment on lines +147 to +148
Copy link
Member

Choose a reason for hiding this comment

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

I believe this can use #11166 once that's merged, that way some cases are still linted

Copy link
Contributor

Choose a reason for hiding this comment

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

This is unlikely to be the case. The signatures of the two functions are incompatible.

if let Some(def_id) = path.res.opt_def_id()
&& let Some(local_def_id) = def_id.as_local()
&& let Some(DefKind::Fn) = cx.tcx.opt_def_kind(def_id)
&& cx.tcx.asyncness(def_id).is_async()
&& !is_node_func_call(cx.tcx.hir().get_parent(hir_id), path.span)
{
self.async_fns_as_value.insert(local_def_id);
}
}

// After collecting all unused `async` and problematic paths to such functions,
// lint those unused ones that didn't have any path expressions to them.
fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
let iter = self
.unused_async_fns
.iter()
.filter_map(|(did, item)| (!self.async_fns_as_value.contains(did)).then_some(item));

for fun in iter {
span_lint_and_then(
y21 marked this conversation as resolved.
Show resolved Hide resolved
cx,
UNUSED_ASYNC,
fun.fn_span,
"unused `async` for function with no await statements",
|diag| {
diag.help("consider removing the `async` from this function");

if let Some(span) = fun.await_in_async_block {
diag.span_note(
span,
"`await` used in an async block, which does not require \
the enclosing function to be `async`",
);
}
},
);
}
}
}
17 changes: 17 additions & 0 deletions tests/ui/unused_async.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,23 @@ mod issue10459 {
}
}

mod issue9695 {
use std::future::Future;

async fn f() {}
async fn f2() {}
async fn f3() {}

fn needs_async_fn<F: Future<Output = ()>>(_: fn() -> F) {}

fn test() {
let x = f;
needs_async_fn(x); // async needed in f
needs_async_fn(f2); // async needed in f2
f3(); // async not needed in f3
}
}

async fn foo() -> i32 {
4
}
Expand Down
34 changes: 21 additions & 13 deletions tests/ui/unused_async.stderr
Original file line number Diff line number Diff line change
@@ -1,3 +1,22 @@
error: unused `async` for function with no await statements
--> $DIR/unused_async.rs:45:5
|
LL | async fn f3() {}
| ^^^^^^^^^^^^^^^^
|
= help: consider removing the `async` from this function
= note: `-D clippy::unused-async` implied by `-D warnings`

error: unused `async` for function with no await statements
--> $DIR/unused_async.rs:57:1
|
LL | / async fn foo() -> i32 {
LL | | 4
LL | | }
| |_^
|
= help: consider removing the `async` from this function

error: unused `async` for function with no await statements
--> $DIR/unused_async.rs:13:5
|
Expand All @@ -14,20 +33,9 @@ note: `await` used in an async block, which does not require the enclosing funct
|
LL | ready(()).await;
| ^^^^^
= note: `-D clippy::unused-async` implied by `-D warnings`

error: unused `async` for function with no await statements
--> $DIR/unused_async.rs:40:1
|
LL | / async fn foo() -> i32 {
LL | | 4
LL | | }
| |_^
|
= help: consider removing the `async` from this function

error: unused `async` for function with no await statements
--> $DIR/unused_async.rs:51:5
--> $DIR/unused_async.rs:68:5
|
LL | / async fn unused(&self) -> i32 {
LL | | 1
Expand All @@ -36,5 +44,5 @@ LL | | }
|
= help: consider removing the `async` from this function

error: aborting due to 3 previous errors
error: aborting due to 4 previous errors