Auto merge of #5859 - ebroto:5765_manual_async_fn_fp, r=yaahc

manual_async_fn: take input lifetimes into account The anonymous future returned from an `async fn` captures all input lifetimes. This was not being taken into account. See https://github.com/rust-lang/rfcs/blob/master/text/2394-async_await.md#lifetime-capture-in-the-anonymous-future changelog: Take input lifetimes into account in [`manual_async_fn`]. Fixes #5765
rust-lang · Aug 5, 2020 · 2eab060 · 2eab060
2 parents 3d7e3fd + e336fe8
commit 2eab060
Show file tree

Hide file tree

Showing 6 changed files with 146 additions and 40 deletions.
diff --git a/clippy_lints/src/manual_async_fn.rs b/clippy_lints/src/manual_async_fn.rs
@@ -4,8 +4,8 @@ use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{
-    AsyncGeneratorKind, Block, Body, Expr, ExprKind, FnDecl, FnRetTy, GeneratorKind, GenericBound, HirId, IsAsync,
-    ItemKind, TraitRef, Ty, TyKind, TypeBindingKind,
+    AsyncGeneratorKind, Block, Body, Expr, ExprKind, FnDecl, FnRetTy, GeneratorKind, GenericArg, GenericBound, HirId,
+    IsAsync, ItemKind, LifetimeName, TraitRef, Ty, TyKind, TypeBindingKind,
 };
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -27,8 +27,6 @@ declare_clippy_lint! {
     /// ```
     /// Use instead:
     /// ```rust
-    /// use std::future::Future;
-    ///
     /// async fn foo() -> i32 { 42 }
     /// ```
     pub MANUAL_ASYNC_FN,
@@ -53,8 +51,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn {
             if let IsAsync::NotAsync = header.asyncness;
             // Check that this function returns `impl Future`
             if let FnRetTy::Return(ret_ty) = decl.output;
-            if let Some(trait_ref) = future_trait_ref(cx, ret_ty);
+            if let Some((trait_ref, output_lifetimes)) = future_trait_ref(cx, ret_ty);
             if let Some(output) = future_output_ty(trait_ref);
+            if captures_all_lifetimes(decl.inputs, &output_lifetimes);
             // Check that the body of the function consists of one async block
             if let ExprKind::Block(block, _) = body.value.kind;
             if block.stmts.is_empty();
@@ -97,16 +96,35 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn {
     }
 }
 
-fn future_trait_ref<'tcx>(cx: &LateContext<'tcx>, ty: &'tcx Ty<'tcx>) -> Option<&'tcx TraitRef<'tcx>> {
+fn future_trait_ref<'tcx>(
+    cx: &LateContext<'tcx>,
+    ty: &'tcx Ty<'tcx>,
+) -> Option<(&'tcx TraitRef<'tcx>, Vec<LifetimeName>)> {
     if_chain! {
-        if let TyKind::OpaqueDef(item_id, _) = ty.kind;
+        if let TyKind::OpaqueDef(item_id, bounds) = ty.kind;
         let item = cx.tcx.hir().item(item_id.id);
         if let ItemKind::OpaqueTy(opaque) = &item.kind;
-        if opaque.bounds.len() == 1;
-        if let GenericBound::Trait(poly, _) = &opaque.bounds[0];
-        if poly.trait_ref.trait_def_id() == cx.tcx.lang_items().future_trait();
+        if let Some(trait_ref) = opaque.bounds.iter().find_map(|bound| {
+            if let GenericBound::Trait(poly, _) = bound {
+                Some(&poly.trait_ref)
+            } else {
+                None
+            }
+        });
+        if trait_ref.trait_def_id() == cx.tcx.lang_items().future_trait();
         then {
-            return Some(&poly.trait_ref);
+            let output_lifetimes = bounds
+                .iter()
+                .filter_map(|bound| {
+                    if let GenericArg::Lifetime(lt) = bound {
+                        Some(lt.name)
+                    } else {
+                        None
+                    }
+                })
+                .collect();
+
+            return Some((trait_ref, output_lifetimes));
         }
     }
 
@@ -129,6 +147,29 @@ fn future_output_ty<'tcx>(trait_ref: &'tcx TraitRef<'tcx>) -> Option<&'tcx Ty<'t
     None
 }
 
+fn captures_all_lifetimes(inputs: &[Ty<'_>], output_lifetimes: &[LifetimeName]) -> bool {
+    let input_lifetimes: Vec<LifetimeName> = inputs
+        .iter()
+        .filter_map(|ty| {
+            if let TyKind::Rptr(lt, _) = ty.kind {
+                Some(lt.name)
+            } else {
+                None
+            }
+        })
+        .collect();
+
+    // The lint should trigger in one of these cases:
+    // - There are no input lifetimes
+    // - There's only one output lifetime bound using `+ '_`
+    // - All input lifetimes are explicitly bound to the output
+    input_lifetimes.is_empty()
+        || (output_lifetimes.len() == 1 && matches!(output_lifetimes[0], LifetimeName::Underscore))
+        || input_lifetimes
+            .iter()
+            .all(|in_lt| output_lifetimes.iter().any(|out_lt| in_lt == out_lt))
+}
+
 fn desugared_async_block<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) -> Option<&'tcx Body<'tcx>> {
     if_chain! {
         if let Some(block_expr) = block.expr;

diff --git a/tests/ui/await_holding_lock.rs b/tests/ui/await_holding_lock.rs
@@ -47,6 +47,7 @@ async fn not_good(x: &Mutex<u32>) -> u32 {
     first + second + third
 }
 
+#[allow(clippy::manual_async_fn)]
 fn block_bad(x: &Mutex<u32>) -> impl std::future::Future<Output = u32> + '_ {
     async move {
         let guard = x.lock().unwrap();

diff --git a/tests/ui/await_holding_lock.stderr b/tests/ui/await_holding_lock.stderr
@@ -46,13 +46,13 @@ LL | |     };
    | |_____^
 
 error: this MutexGuard is held across an 'await' point. Consider using an async-aware Mutex type or ensuring the MutexGuard is dropped before calling await.
-  --> $DIR/await_holding_lock.rs:52:13
+  --> $DIR/await_holding_lock.rs:53:13
    |
 LL |         let guard = x.lock().unwrap();
    |             ^^^^^
    |
 note: these are all the await points this lock is held through
-  --> $DIR/await_holding_lock.rs:52:9
+  --> $DIR/await_holding_lock.rs:53:9
    |
 LL | /         let guard = x.lock().unwrap();
 LL | |         baz().await

diff --git a/tests/ui/manual_async_fn.fixed b/tests/ui/manual_async_fn.fixed
@@ -43,10 +43,6 @@ impl S {
         42
     }
 
-    async fn meth_fut(&self) -> i32 { 42 }
-
-    async fn empty_fut(&self)  {}
-
     // should be ignored
     fn not_fut(&self) -> i32 {
         42
@@ -64,4 +60,40 @@ impl S {
     }
 }
 
+// Tests related to lifetime capture
+
+async fn elided(_: &i32) -> i32 { 42 }
+
+// should be ignored
+fn elided_not_bound(_: &i32) -> impl Future<Output = i32> {
+    async { 42 }
+}
+
+async fn explicit<'a, 'b>(_: &'a i32, _: &'b i32) -> i32 { 42 }
+
+// should be ignored
+#[allow(clippy::needless_lifetimes)]
+fn explicit_not_bound<'a, 'b>(_: &'a i32, _: &'b i32) -> impl Future<Output = i32> {
+    async { 42 }
+}
+
+// should be ignored
+mod issue_5765 {
+    use std::future::Future;
+
+    struct A;
+    impl A {
+        fn f(&self) -> impl Future<Output = ()> {
+            async {}
+        }
+    }
+
+    fn test() {
+        let _future = {
+            let a = A;
+            a.f()
+        };
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/manual_async_fn.rs b/tests/ui/manual_async_fn.rs
@@ -51,14 +51,6 @@ impl S {
         }
     }
 
-    fn meth_fut(&self) -> impl Future<Output = i32> {
-        async { 42 }
-    }
-
-    fn empty_fut(&self) -> impl Future<Output = ()> {
-        async {}
-    }
-
     // should be ignored
     fn not_fut(&self) -> i32 {
         42
@@ -76,4 +68,44 @@ impl S {
     }
 }
 
+// Tests related to lifetime capture
+
+fn elided(_: &i32) -> impl Future<Output = i32> + '_ {
+    async { 42 }
+}
+
+// should be ignored
+fn elided_not_bound(_: &i32) -> impl Future<Output = i32> {
+    async { 42 }
+}
+
+fn explicit<'a, 'b>(_: &'a i32, _: &'b i32) -> impl Future<Output = i32> + 'a + 'b {
+    async { 42 }
+}
+
+// should be ignored
+#[allow(clippy::needless_lifetimes)]
+fn explicit_not_bound<'a, 'b>(_: &'a i32, _: &'b i32) -> impl Future<Output = i32> {
+    async { 42 }
+}
+
+// should be ignored
+mod issue_5765 {
+    use std::future::Future;
+
+    struct A;
+    impl A {
+        fn f(&self) -> impl Future<Output = ()> {
+            async {}
+        }
+    }
+
+    fn test() {
+        let _future = {
+            let a = A;
+            a.f()
+        };
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/manual_async_fn.stderr b/tests/ui/manual_async_fn.stderr
@@ -65,34 +65,34 @@ LL |             let c = 21;
  ...
 
 error: this function can be simplified using the `async fn` syntax
-  --> $DIR/manual_async_fn.rs:54:5
+  --> $DIR/manual_async_fn.rs:73:1
    |
-LL |     fn meth_fut(&self) -> impl Future<Output = i32> {
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | fn elided(_: &i32) -> impl Future<Output = i32> + '_ {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
 help: make the function `async` and return the output of the future directly
    |
-LL |     async fn meth_fut(&self) -> i32 {
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | async fn elided(_: &i32) -> i32 {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 help: move the body of the async block to the enclosing function
    |
-LL |     fn meth_fut(&self) -> impl Future<Output = i32> { 42 }
-   |                                                     ^^^^^^
+LL | fn elided(_: &i32) -> impl Future<Output = i32> + '_ { 42 }
+   |                                                      ^^^^^^
 
 error: this function can be simplified using the `async fn` syntax
-  --> $DIR/manual_async_fn.rs:58:5
+  --> $DIR/manual_async_fn.rs:82:1
    |
-LL |     fn empty_fut(&self) -> impl Future<Output = ()> {
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | fn explicit<'a, 'b>(_: &'a i32, _: &'b i32) -> impl Future<Output = i32> + 'a + 'b {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-help: make the function `async` and remove the return type
+help: make the function `async` and return the output of the future directly
    |
-LL |     async fn empty_fut(&self)  {
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | async fn explicit<'a, 'b>(_: &'a i32, _: &'b i32) -> i32 {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 help: move the body of the async block to the enclosing function
    |
-LL |     fn empty_fut(&self) -> impl Future<Output = ()> {}
-   |                                                     ^^
+LL | fn explicit<'a, 'b>(_: &'a i32, _: &'b i32) -> impl Future<Output = i32> + 'a + 'b { 42 }
+   |                                                                                    ^^^^^^
 
 error: aborting due to 6 previous errors