Auto merge of rust-lang#11200 - y21:issue9695, r=Jarcho

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

Fixes rust-lang#9695
Fixes rust-lang#9359

Clippy shouldn't lint unused `async` if there are paths referencing them if that path isn't the receiver of a function call, because that means that the function might be passed to some other function:
```rs
async fn f() {} // No await statements, so unused at this point

fn requires_fn_future<F: Future<Output = ()>>(_: fn() -> F) {}
requires_fn_future(f); // `f`'s asyncness is actually not unused.
```
(This isn't limited to just passing the function as a parameter to another function, it could also first be stored in a variable and later passed to another function as an argument)

This requires delaying the linting until post-crate and collecting path references to local async functions along the way.

changelog: [`unused_async`]: don't lint if paths reference async fn that require asyncness

Author: bors

clippy_lints/src/lib.rs

@@ -913,7 +913,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();

clippy_lints/src/unused_async.rs

@@ -1,11 +1,12 @@
-use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::diagnostics::span_lint_hir_and_then;
 use clippy_utils::is_def_id_trait_method;
+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! {
@@ -38,7 +39,24 @@ 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: Vec<UnusedAsyncFn>,
+}
+
+#[derive(Copy, Clone)]
+struct UnusedAsyncFn {
+    def_id: LocalDefId,
+    fn_span: Span,
+    await_in_async_block: Option<Span>,
+}
+
+impl_lint_pass!(UnusedAsync => [UNUSED_ASYNC]);
 
 struct AsyncFnVisitor<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
@@ -101,24 +119,70 @@ 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.push(UnusedAsyncFn {
+                    await_in_async_block: visitor.await_in_async_block,
+                    fn_span: span,
+                    def_id,
+                });
             }
         }
     }
+
+    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.
+        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(|UnusedAsyncFn { def_id, .. }| (!self.async_fns_as_value.contains(def_id)));
+
+        for fun in iter {
+            span_lint_hir_and_then(
+                cx,
+                UNUSED_ASYNC,
+                cx.tcx.local_def_id_to_hir_id(fun.def_id),
+                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`",
+                        );
+                    }
+                },
+            );
+        }
+    }
 }

tests/ui/unused_async.rs

@@ -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
 }

tests/ui/unused_async.stderr

@@ -17,7 +17,15 @@ 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
+  --> $DIR/unused_async.rs:45:5
+   |
+LL |     async fn f3() {}
+   |     ^^^^^^^^^^^^^^^^
+   |
+   = help: consider removing the `async` from this function
+
+error: unused `async` for function with no await statements
+  --> $DIR/unused_async.rs:57:1
    |
 LL | / async fn foo() -> i32 {
 LL | |     4
@@ -27,7 +35,7 @@ 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
@@ -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