Auto merge of #5576 - ebroto:manual_async_fn, r=flip1995

Add the manual_async_fn lint

changelog: Added the `manual_async_fn` lint to warn on functions that can be simplified using async syntax

Closes #5503

Author: bors

CHANGELOG.md

@@ -1422,6 +1422,7 @@ Released 2018-09-13
 [`lossy_float_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#lossy_float_literal
 [`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports
 [`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion
+[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
 [`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
 [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
 [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic

clippy_lints/src/lib.rs

@@ -247,6 +247,7 @@ mod literal_representation;
 mod loops;
 mod macro_use;
 mod main_recursion;
+mod manual_async_fn;
 mod manual_non_exhaustive;
 mod map_clone;
 mod map_unit_fn;
@@ -629,6 +630,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &loops::WHILE_LET_ON_ITERATOR,
         &macro_use::MACRO_USE_IMPORTS,
         &main_recursion::MAIN_RECURSION,
+        &manual_async_fn::MANUAL_ASYNC_FN,
         &manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
         &map_clone::MAP_CLONE,
         &map_unit_fn::OPTION_MAP_UNIT_FN,
@@ -1067,6 +1069,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box if_let_mutex::IfLetMutex);
     store.register_late_pass(|| box match_on_vec_items::MatchOnVecItems);
     store.register_early_pass(|| box manual_non_exhaustive::ManualNonExhaustive);
+    store.register_late_pass(|| box manual_async_fn::ManualAsyncFn);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1284,6 +1287,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::WHILE_LET_LOOP),
         LintId::of(&loops::WHILE_LET_ON_ITERATOR),
         LintId::of(&main_recursion::MAIN_RECURSION),
+        LintId::of(&manual_async_fn::MANUAL_ASYNC_FN),
         LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
         LintId::of(&map_clone::MAP_CLONE),
         LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN),
@@ -1478,6 +1482,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::NEEDLESS_RANGE_LOOP),
         LintId::of(&loops::WHILE_LET_ON_ITERATOR),
         LintId::of(&main_recursion::MAIN_RECURSION),
+        LintId::of(&manual_async_fn::MANUAL_ASYNC_FN),
         LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
         LintId::of(&map_clone::MAP_CLONE),
         LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH),

clippy_lints/src/manual_async_fn.rs

@@ -0,0 +1,159 @@
+use crate::utils::paths::FUTURE_FROM_GENERATOR;
+use crate::utils::{match_function_call, snippet_block, snippet_opt, span_lint_and_then};
+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,
+};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::Span;
+
+declare_clippy_lint! {
+    /// **What it does:** It checks for manual implementations of `async` functions.
+    ///
+    /// **Why is this bad?** It's more idiomatic to use the dedicated syntax.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// use std::future::Future;
+    ///
+    /// fn foo() -> impl Future<Output = i32> { async { 42 } }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// use std::future::Future;
+    ///
+    /// async fn foo() -> i32 { 42 }
+    /// ```
+    pub MANUAL_ASYNC_FN,
+    style,
+    "manual implementations of `async` functions can be simplified using the dedicated syntax"
+}
+
+declare_lint_pass!(ManualAsyncFn => [MANUAL_ASYNC_FN]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ManualAsyncFn {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'a, 'tcx>,
+        kind: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'_>,
+        body: &'tcx Body<'_>,
+        span: Span,
+        _: HirId,
+    ) {
+        if_chain! {
+            if let Some(header) = kind.header();
+            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(output) = future_output_ty(trait_ref);
+            // 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();
+            if let Some(closure_body) = desugared_async_block(cx, block);
+            then {
+                let header_span = span.with_hi(ret_ty.span.hi());
+
+                span_lint_and_then(
+                    cx,
+                    MANUAL_ASYNC_FN,
+                    header_span,
+                    "this function can be simplified using the `async fn` syntax",
+                    |diag| {
+                        if_chain! {
+                            if let Some(header_snip) = snippet_opt(cx, header_span);
+                            if let Some(ret_pos) = header_snip.rfind("->");
+                            if let Some((ret_sugg, ret_snip)) = suggested_ret(cx, output);
+                            then {
+                                let help = format!("make the function `async` and {}", ret_sugg);
+                                diag.span_suggestion(
+                                    header_span,
+                                    &help,
+                                    format!("async {}{}", &header_snip[..ret_pos], ret_snip),
+                                    Applicability::MachineApplicable
+                                );
+
+                                let body_snip = snippet_block(cx, closure_body.value.span, "..", Some(block.span));
+                                diag.span_suggestion(
+                                    block.span,
+                                    "move the body of the async block to the enclosing function",
+                                    body_snip.to_string(),
+                                    Applicability::MachineApplicable
+                                );
+                            }
+                        }
+                    },
+                );
+            }
+        }
+    }
+}
+
+fn future_trait_ref<'tcx>(cx: &LateContext<'_, 'tcx>, ty: &'tcx Ty<'tcx>) -> Option<&'tcx TraitRef<'tcx>> {
+    if_chain! {
+        if let TyKind::Def(item_id, _) = 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();
+        then {
+            return Some(&poly.trait_ref);
+        }
+    }
+
+    None
+}
+
+fn future_output_ty<'tcx>(trait_ref: &'tcx TraitRef<'tcx>) -> Option<&'tcx Ty<'tcx>> {
+    if_chain! {
+        if let Some(segment) = trait_ref.path.segments.last();
+        if let Some(args) = segment.args;
+        if args.bindings.len() == 1;
+        let binding = &args.bindings[0];
+        if binding.ident.as_str() == "Output";
+        if let TypeBindingKind::Equality{ty: output} = binding.kind;
+        then {
+            return Some(output)
+        }
+    }
+
+    None
+}
+
+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;
+        if let Some(args) = match_function_call(cx, block_expr, &FUTURE_FROM_GENERATOR);
+        if args.len() == 1;
+        if let Expr{kind: ExprKind::Closure(_, _, body_id, ..), ..} = args[0];
+        let closure_body = cx.tcx.hir().body(body_id);
+        if let Some(GeneratorKind::Async(AsyncGeneratorKind::Block)) = closure_body.generator_kind;
+        then {
+            return Some(closure_body);
+        }
+    }
+
+    None
+}
+
+fn suggested_ret(cx: &LateContext<'_, '_>, output: &Ty<'_>) -> Option<(&'static str, String)> {
+    match output.kind {
+        TyKind::Tup(tys) if tys.is_empty() => {
+            let sugg = "remove the return type";
+            Some((sugg, "".into()))
+        },
+        _ => {
+            let sugg = "return the output of the future directly";
+            snippet_opt(cx, output.span).map(|snip| (sugg, format!("-> {}", snip)))
+        },
+    }
+}

clippy_lints/src/utils/paths.rs

@@ -42,6 +42,7 @@ pub const FMT_ARGUMENTS_NEW_V1_FORMATTED: [&str; 4] = ["core", "fmt", "Arguments
 pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"];
 pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"];
 pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"];
+pub const FUTURE_FROM_GENERATOR: [&str; 3] = ["core", "future", "from_generator"];
 pub const HASH: [&str; 2] = ["hash", "Hash"];
 pub const HASHMAP: [&str; 5] = ["std", "collections", "hash", "map", "HashMap"];
 pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entry"];

src/lintlist/mod.rs

@@ -1081,6 +1081,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "main_recursion",
     },
+    Lint {
+        name: "manual_async_fn",
+        group: "style",
+        desc: "manual implementations of `async` functions can be simplified using the dedicated syntax",
+        deprecation: None,
+        module: "manual_async_fn",
+    },
     Lint {
         name: "manual_memcpy",
         group: "perf",

tests/ui/future_not_send.rs

@@ -41,6 +41,7 @@ impl Dummy {
         self.private_future().await;
     }
 
+    #[allow(clippy::manual_async_fn)]
     pub fn public_send(&self) -> impl std::future::Future<Output = bool> {
         async { false }
     }

tests/ui/future_not_send.stderr

@@ -96,13 +96,13 @@ LL |     }
    = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Sync`
 
 error: future cannot be sent between threads safely
-  --> $DIR/future_not_send.rs:49:37
+  --> $DIR/future_not_send.rs:50:37
    |
 LL | async fn generic_future<T>(t: T) -> T
    |                                     ^ future returned by `generic_future` is not `Send`
    |
 note: future is not `Send` as this value is used across an await
-  --> $DIR/future_not_send.rs:54:5
+  --> $DIR/future_not_send.rs:55:5
    |
 LL |     let rt = &t;
    |         -- has type `&T` which is not `Send`
@@ -114,7 +114,7 @@ LL | }
    = note: `T` doesn't implement `std::marker::Sync`
 
 error: future cannot be sent between threads safely
-  --> $DIR/future_not_send.rs:65:34
+  --> $DIR/future_not_send.rs:66:34
    |
 LL | async fn unclear_future<T>(t: T) {}
    |                                  ^

tests/ui/manual_async_fn.fixed

@@ -0,0 +1,67 @@
+// run-rustfix
+// edition:2018
+#![warn(clippy::manual_async_fn)]
+#![allow(unused)]
+
+use std::future::Future;
+
+async fn fut() -> i32 { 42 }
+
+async fn empty_fut()  {}
+
+async fn core_fut() -> i32 { 42 }
+
+// should be ignored
+fn has_other_stmts() -> impl core::future::Future<Output = i32> {
+    let _ = 42;
+    async move { 42 }
+}
+
+// should be ignored
+fn not_fut() -> i32 {
+    42
+}
+
+// should be ignored
+async fn already_async() -> impl Future<Output = i32> {
+    async { 42 }
+}
+
+struct S {}
+impl S {
+    async fn inh_fut() -> i32 {
+        // NOTE: this code is here just to check that the identation is correct in the suggested fix
+        let a = 42;
+        let b = 21;
+        if a < b {
+            let c = 21;
+            let d = 42;
+            if c < d {
+                let _ = 42;
+            }
+        }
+        42
+    }
+
+    async fn meth_fut(&self) -> i32 { 42 }
+
+    async fn empty_fut(&self)  {}
+
+    // should be ignored
+    fn not_fut(&self) -> i32 {
+        42
+    }
+
+    // should be ignored
+    fn has_other_stmts() -> impl core::future::Future<Output = i32> {
+        let _ = 42;
+        async move { 42 }
+    }
+
+    // should be ignored
+    async fn already_async(&self) -> impl Future<Output = i32> {
+        async { 42 }
+    }
+}
+
+fn main() {}