Auto merge of #5423 - rkuhn:add_futures_not_send, r=flip1995

bors · bors · commit f1fb815603e0 · 2020-04-17T16:04:14.000Z
add lint futures_not_send changelog: add lint futures_not_send fixes #5379 ~Remark: one thing that can (should?) still be improved is to directly include the error message from the `Send` check so that the programmer stays in the flow. Currently, getting the actual error message requires a restructuring of the code to make the `Send` constraint explicit.~ It now shows all unmet constraints for allowing the Future to be Send.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1281,6 +1281,7 @@ Released 2018-09-13
 [`for_loop_over_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loop_over_result
 [`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy
 [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
+[`future_not_send`]: https://rust-lang.github.io/rust-clippy/master/index.html#future_not_send
 [`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len
 [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap
 [`identity_conversion`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion
diff --git a/clippy_lints/src/future_not_send.rs b/clippy_lints/src/future_not_send.rs
@@ -0,0 +1,113 @@
+use crate::utils;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{Body, FnDecl, HirId};
+use rustc_infer::infer::TyCtxtInferExt;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::{Opaque, Predicate::Trait, ToPolyTraitRef};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::{sym, Span};
+use rustc_trait_selection::traits::error_reporting::suggestions::InferCtxtExt;
+use rustc_trait_selection::traits::{self, FulfillmentError, TraitEngine};
+
+declare_clippy_lint! {
+    /// **What it does:** This lint requires Future implementations returned from
+    /// functions and methods to implement the `Send` marker trait. It is mostly
+    /// used by library authors (public and internal) that target an audience where
+    /// multithreaded executors are likely to be used for running these Futures.
+    ///
+    /// **Why is this bad?** A Future implementation captures some state that it
+    /// needs to eventually produce its final value. When targeting a multithreaded
+    /// executor (which is the norm on non-embedded devices) this means that this
+    /// state may need to be transported to other threads, in other words the
+    /// whole Future needs to implement the `Send` marker trait. If it does not,
+    /// then the resulting Future cannot be submitted to a thread pool in the
+    /// end user’s code.
+    ///
+    /// Especially for generic functions it can be confusing to leave the
+    /// discovery of this problem to the end user: the reported error location
+    /// will be far from its cause and can in many cases not even be fixed without
+    /// modifying the library where the offending Future implementation is
+    /// produced.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// async fn not_send(bytes: std::rc::Rc<[u8]>) {}
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// async fn is_send(bytes: std::sync::Arc<[u8]>) {}
+    /// ```
+    pub FUTURE_NOT_SEND,
+    nursery,
+    "public Futures must be Send"
+}
+
+declare_lint_pass!(FutureNotSend => [FUTURE_NOT_SEND]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for FutureNotSend {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'a, 'tcx>,
+        kind: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'tcx>,
+        _: &'tcx Body<'tcx>,
+        _: Span,
+        hir_id: HirId,
+    ) {
+        if let FnKind::Closure(_) = kind {
+            return;
+        }
+        let def_id = cx.tcx.hir().local_def_id(hir_id);
+        let fn_sig = cx.tcx.fn_sig(def_id);
+        let fn_sig = cx.tcx.erase_late_bound_regions(&fn_sig);
+        let ret_ty = fn_sig.output();
+        if let Opaque(id, subst) = ret_ty.kind {
+            let preds = cx.tcx.predicates_of(id).instantiate(cx.tcx, subst);
+            let mut is_future = false;
+            for p in preds.predicates {
+                if let Some(trait_ref) = p.to_opt_poly_trait_ref() {
+                    if Some(trait_ref.def_id()) == cx.tcx.lang_items().future_trait() {
+                        is_future = true;
+                        break;
+                    }
+                }
+            }
+            if is_future {
+                let send_trait = cx.tcx.get_diagnostic_item(sym::send_trait).unwrap();
+                let span = decl.output.span();
+                let send_result = cx.tcx.infer_ctxt().enter(|infcx| {
+                    let cause = traits::ObligationCause::misc(span, hir_id);
+                    let mut fulfillment_cx = traits::FulfillmentContext::new();
+                    fulfillment_cx.register_bound(&infcx, cx.param_env, ret_ty, send_trait, cause);
+                    fulfillment_cx.select_all_or_error(&infcx)
+                });
+                if let Err(send_errors) = send_result {
+                    utils::span_lint_and_then(
+                        cx,
+                        FUTURE_NOT_SEND,
+                        span,
+                        "future cannot be sent between threads safely",
+                        |db| {
+                            cx.tcx.infer_ctxt().enter(|infcx| {
+                                for FulfillmentError { obligation, .. } in send_errors {
+                                    infcx.maybe_note_obligation_cause_for_async_await(db, &obligation);
+                                    if let Trait(trait_pred, _) = obligation.predicate {
+                                        let trait_ref = trait_pred.to_poly_trait_ref();
+                                        db.note(&*format!(
+                                            "`{}` doesn't implement `{}`",
+                                            trait_ref.self_ty(),
+                                            trait_ref.print_only_trait_path(),
+                                        ));
+                                    }
+                                }
+                            })
+                        },
+                    );
+                }
+            }
+        }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -218,6 +218,7 @@ mod floating_point_arithmetic;
 mod format;
 mod formatting;
 mod functions;
+mod future_not_send;
 mod get_last_with_len;
 mod identity_conversion;
 mod identity_op;
@@ -566,6 +567,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &functions::NOT_UNSAFE_PTR_ARG_DEREF,
         &functions::TOO_MANY_ARGUMENTS,
         &functions::TOO_MANY_LINES,
+        &future_not_send::FUTURE_NOT_SEND,
         &get_last_with_len::GET_LAST_WITH_LEN,
         &identity_conversion::IDENTITY_CONVERSION,
         &identity_op::IDENTITY_OP,
@@ -1045,6 +1047,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default());
     store.register_late_pass(|| box unnamed_address::UnnamedAddress);
     store.register_late_pass(|| box dereference::Dereferencing);
+    store.register_late_pass(|| box future_not_send::FutureNotSend);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1689,6 +1692,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&fallible_impl_from::FALLIBLE_IMPL_FROM),
         LintId::of(&floating_point_arithmetic::IMPRECISE_FLOPS),
         LintId::of(&floating_point_arithmetic::SUBOPTIMAL_FLOPS),
+        LintId::of(&future_not_send::FUTURE_NOT_SEND),
         LintId::of(&missing_const_for_fn::MISSING_CONST_FOR_FN),
         LintId::of(&mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL),
         LintId::of(&mutex_atomic::MUTEX_INTEGER),
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
@@ -696,6 +696,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "drop_forget_ref",
     },
+    Lint {
+        name: "future_not_send",
+        group: "nursery",
+        desc: "public Futures must be Send",
+        deprecation: None,
+        module: "future_not_send",
+    },
     Lint {
         name: "get_last_with_len",
         group: "complexity",
diff --git a/tests/ui/future_not_send.rs b/tests/ui/future_not_send.rs
@@ -0,0 +1,79 @@
+// edition:2018
+#![warn(clippy::future_not_send)]
+
+use std::cell::Cell;
+use std::rc::Rc;
+use std::sync::Arc;
+
+async fn private_future(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
+    async { true }.await
+}
+
+pub async fn public_future(rc: Rc<[u8]>) {
+    async { true }.await;
+}
+
+pub async fn public_send(arc: Arc<[u8]>) -> bool {
+    async { false }.await
+}
+
+async fn private_future2(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
+    true
+}
+
+pub async fn public_future2(rc: Rc<[u8]>) {}
+
+pub async fn public_send2(arc: Arc<[u8]>) -> bool {
+    false
+}
+
+struct Dummy {
+    rc: Rc<[u8]>,
+}
+
+impl Dummy {
+    async fn private_future(&self) -> usize {
+        async { true }.await;
+        self.rc.len()
+    }
+
+    pub async fn public_future(&self) {
+        self.private_future().await;
+    }
+
+    pub fn public_send(&self) -> impl std::future::Future<Output = bool> {
+        async { false }
+    }
+}
+
+async fn generic_future<T>(t: T) -> T
+where
+    T: Send,
+{
+    let rt = &t;
+    async { true }.await;
+    t
+}
+
+async fn generic_future_send<T>(t: T)
+where
+    T: Send,
+{
+    async { true }.await;
+}
+
+async fn unclear_future<T>(t: T) {}
+
+fn main() {
+    let rc = Rc::new([1, 2, 3]);
+    private_future(rc.clone(), &Cell::new(42));
+    public_future(rc.clone());
+    let arc = Arc::new([4, 5, 6]);
+    public_send(arc);
+    generic_future(42);
+    generic_future_send(42);
+
+    let dummy = Dummy { rc };
+    dummy.public_future();
+    dummy.public_send();
+}
diff --git a/tests/ui/future_not_send.stderr b/tests/ui/future_not_send.stderr
@@ -0,0 +1,125 @@
+error: future cannot be sent between threads safely
+  --> $DIR/future_not_send.rs:8:62
+   |
+LL | async fn private_future(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
+   |                                                              ^^^^ future returned by `private_future` is not `Send`
+   |
+   = note: `-D clippy::future-not-send` implied by `-D warnings`
+note: future is not `Send` as this value is used across an await
+  --> $DIR/future_not_send.rs:9:5
+   |
+LL | async fn private_future(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
+   |                         -- has type `std::rc::Rc<[u8]>` which is not `Send`
+LL |     async { true }.await
+   |     ^^^^^^^^^^^^^^^^^^^^ await occurs here, with `rc` maybe used later
+LL | }
+   | - `rc` is later dropped here
+   = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Send`
+note: future is not `Send` as this value is used across an await
+  --> $DIR/future_not_send.rs:9:5
+   |
+LL | async fn private_future(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
+   |                                       ---- has type `&std::cell::Cell<usize>` which is not `Send`
+LL |     async { true }.await
+   |     ^^^^^^^^^^^^^^^^^^^^ await occurs here, with `cell` maybe used later
+LL | }
+   | - `cell` is later dropped here
+   = note: `std::cell::Cell<usize>` doesn't implement `std::marker::Sync`
+
+error: future cannot be sent between threads safely
+  --> $DIR/future_not_send.rs:12:42
+   |
+LL | pub async fn public_future(rc: Rc<[u8]>) {
+   |                                          ^ future returned by `public_future` is not `Send`
+   |
+note: future is not `Send` as this value is used across an await
+  --> $DIR/future_not_send.rs:13:5
+   |
+LL | pub async fn public_future(rc: Rc<[u8]>) {
+   |                            -- has type `std::rc::Rc<[u8]>` which is not `Send`
+LL |     async { true }.await;
+   |     ^^^^^^^^^^^^^^^^^^^^ await occurs here, with `rc` maybe used later
+LL | }
+   | - `rc` is later dropped here
+   = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Send`
+
+error: future cannot be sent between threads safely
+  --> $DIR/future_not_send.rs:20:63
+   |
+LL | async fn private_future2(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
+   |                                                               ^^^^
+   |
+   = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Send`
+   = note: `std::cell::Cell<usize>` doesn't implement `std::marker::Sync`
+
+error: future cannot be sent between threads safely
+  --> $DIR/future_not_send.rs:24:43
+   |
+LL | pub async fn public_future2(rc: Rc<[u8]>) {}
+   |                                           ^
+   |
+   = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Send`
+
+error: future cannot be sent between threads safely
+  --> $DIR/future_not_send.rs:35:39
+   |
+LL |     async fn private_future(&self) -> usize {
+   |                                       ^^^^^ future returned by `private_future` is not `Send`
+   |
+note: future is not `Send` as this value is used across an await
+  --> $DIR/future_not_send.rs:36:9
+   |
+LL |     async fn private_future(&self) -> usize {
+   |                             ----- has type `&Dummy` which is not `Send`
+LL |         async { true }.await;
+   |         ^^^^^^^^^^^^^^^^^^^^ await occurs here, with `&self` maybe used later
+LL |         self.rc.len()
+LL |     }
+   |     - `&self` is later dropped here
+   = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Sync`
+
+error: future cannot be sent between threads safely
+  --> $DIR/future_not_send.rs:40:39
+   |
+LL |     pub async fn public_future(&self) {
+   |                                       ^ future returned by `public_future` is not `Send`
+   |
+note: future is not `Send` as this value is used across an await
+  --> $DIR/future_not_send.rs:41:9
+   |
+LL |     pub async fn public_future(&self) {
+   |                                ----- has type `&Dummy` which is not `Send`
+LL |         self.private_future().await;
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ await occurs here, with `&self` maybe used later
+LL |     }
+   |     - `&self` is later dropped here
+   = 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
+   |
+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
+   |
+LL |     let rt = &t;
+   |         -- has type `&T` which is not `Send`
+LL |     async { true }.await;
+   |     ^^^^^^^^^^^^^^^^^^^^ await occurs here, with `rt` maybe used later
+LL |     t
+LL | }
+   | - `rt` is later dropped here
+   = note: `T` doesn't implement `std::marker::Sync`
+
+error: future cannot be sent between threads safely
+  --> $DIR/future_not_send.rs:65:34
+   |
+LL | async fn unclear_future<T>(t: T) {}
+   |                                  ^
+   |
+   = note: `T` doesn't implement `std::marker::Send`
+
+error: aborting due to 8 previous errors
+
