Allow conditional Send futures in future_not_send (#13590)

Closes #6947

This changes the lint to allow futures which are not `Send` as a result
of a generic type parameter not having a `Send` bound and only lint
futures that are always `!Send` for any type, which I believe is the
more useful behavior (like the comments in the linked issue explain).

This is still only a heuristic (I'm not sure if there's a more general
way to do this), but it should cover the common cases I could think of
(including the code examples in the linked issue)

changelog: [`future_not_send`]: allow conditional `Send` futures

clippy_lints/src/future_not_send.rs

@@ -1,11 +1,15 @@
+use std::ops::ControlFlow;
+
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::return_ty;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{Body, FnDecl};
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::print::PrintTraitRefExt;
-use rustc_middle::ty::{self, AliasTy, ClauseKind, PredicateKind};
+use rustc_middle::ty::{
+    self, AliasTy, Binder, ClauseKind, PredicateKind, Ty, TyCtxt, TypeVisitable, TypeVisitableExt, TypeVisitor,
+};
 use rustc_session::declare_lint_pass;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::{Span, sym};
@@ -15,9 +19,16 @@ use rustc_trait_selection::traits::{self, FulfillmentError, ObligationCtxt};
 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.
+    /// functions and methods to implement the `Send` marker trait,
+    /// ignoring type parameters.
+    ///
+    /// If a function is generic and its Future conditionally implements `Send`
+    /// based on a generic parameter then it is considered `Send` and no warning is emitted.
+    ///
+    /// This can be used by library authors (public and internal) to ensure
+    /// their functions are compatible with both multi-threaded runtimes that require `Send` futures,
+    /// as well as single-threaded runtimes where callers may choose `!Send` types
+    /// for generic parameters.
     ///
     /// ### Why is this bad?
     /// A Future implementation captures some state that it
@@ -64,22 +75,46 @@ impl<'tcx> LateLintPass<'tcx> for FutureNotSend {
             return;
         }
         let ret_ty = return_ty(cx, cx.tcx.local_def_id_to_hir_id(fn_def_id).expect_owner());
-        if let ty::Alias(ty::Opaque, AliasTy { def_id, args, .. }) = *ret_ty.kind() {
+        if let ty::Alias(ty::Opaque, AliasTy { def_id, args, .. }) = *ret_ty.kind()
+            && let Some(future_trait) = cx.tcx.lang_items().future_trait()
+            && let Some(send_trait) = cx.tcx.get_diagnostic_item(sym::Send)
+        {
             let preds = cx.tcx.explicit_item_super_predicates(def_id);
             let is_future = preds.iter_instantiated_copied(cx.tcx, args).any(|(p, _)| {
-                p.as_trait_clause().is_some_and(|trait_pred| {
-                    Some(trait_pred.skip_binder().trait_ref.def_id) == cx.tcx.lang_items().future_trait()
-                })
+                p.as_trait_clause()
+                    .is_some_and(|trait_pred| trait_pred.skip_binder().trait_ref.def_id == future_trait)
             });
             if is_future {
-                let send_trait = cx.tcx.get_diagnostic_item(sym::Send).unwrap();
                 let span = decl.output.span();
                 let infcx = cx.tcx.infer_ctxt().build(cx.typing_mode());
                 let ocx = ObligationCtxt::new_with_diagnostics(&infcx);
                 let cause = traits::ObligationCause::misc(span, fn_def_id);
                 ocx.register_bound(cause, cx.param_env, ret_ty, send_trait);
                 let send_errors = ocx.select_all_or_error();
-                if !send_errors.is_empty() {
+
+                // Allow errors that try to prove `Send` for types that "mention" a generic parameter at the "top
+                // level".
+                // For example, allow errors that `T: Send` can't be proven, but reject `Rc<T>: Send` errors,
+                // which is always unconditionally `!Send` for any possible type `T`.
+                //
+                // We also allow associated type projections if the self type is either itself a projection or a
+                // type parameter.
+                // This is to prevent emitting warnings for e.g. holding a `<Fut as Future>::Output` across await
+                // points, where `Fut` is a type parameter.
+
+                let is_send = send_errors.iter().all(|err| {
+                    err.obligation
+                        .predicate
+                        .as_trait_clause()
+                        .map(Binder::skip_binder)
+                        .is_some_and(|pred| {
+                            pred.def_id() == send_trait
+                                && pred.self_ty().has_param()
+                                && TyParamAtTopLevelVisitor.visit_ty(pred.self_ty()) == ControlFlow::Break(true)
+                        })
+                });
+
+                if !is_send {
                     span_lint_and_then(
                         cx,
                         FUTURE_NOT_SEND,
@@ -107,3 +142,15 @@ impl<'tcx> LateLintPass<'tcx> for FutureNotSend {
         }
     }
 }
+
+struct TyParamAtTopLevelVisitor;
+impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for TyParamAtTopLevelVisitor {
+    type Result = ControlFlow<bool>;
+    fn visit_ty(&mut self, ty: Ty<'tcx>) -> Self::Result {
+        match ty.kind() {
+            ty::Param(_) => ControlFlow::Break(true),
+            ty::Alias(ty::AliasTyKind::Projection, ty) => ty.visit_with(self),
+            _ => ControlFlow::Break(false),
+        }
+    }
+}

tests/ui/future_not_send.rs

@@ -1,6 +1,7 @@
 #![warn(clippy::future_not_send)]
 
 use std::cell::Cell;
+use std::future::Future;
 use std::rc::Rc;
 use std::sync::Arc;
 
@@ -63,6 +64,22 @@ where
     t
 }
 
+async fn maybe_send_generic_future<T>(t: T) -> T {
+    async { true }.await;
+    t
+}
+
+async fn maybe_send_generic_future2<F: Fn() -> Fut, Fut: Future>(f: F) {
+    async { true }.await;
+    let res = f();
+    async { true }.await;
+}
+
+async fn generic_future_always_unsend<T>(_: Rc<T>) {
+    //~^ ERROR: future cannot be sent between threads safely
+    async { true }.await;
+}
+
 async fn generic_future_send<T>(t: T)
 where
     T: Send,
@@ -71,7 +88,6 @@ where
 }
 
 async fn unclear_future<T>(t: T) {}
-//~^ ERROR: future cannot be sent between threads safely
 
 fn main() {
     let rc = Rc::new([1, 2, 3]);

tests/ui/future_not_send.stderr

@@ -1,11 +1,11 @@
 error: future cannot be sent between threads safely
-  --> tests/ui/future_not_send.rs:7:1
+  --> tests/ui/future_not_send.rs:8:1
    |
 LL | async fn private_future(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `private_future` is not `Send`
    |
 note: future is not `Send` as this value is used across an await
-  --> tests/ui/future_not_send.rs:9:20
+  --> tests/ui/future_not_send.rs:10:20
    |
 LL | async fn private_future(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
    |                         -- has type `std::rc::Rc<[u8]>` which is not `Send`
@@ -14,7 +14,7 @@ LL |     async { true }.await
    |                    ^^^^^ await occurs here, with `rc` maybe used later
    = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Send`
 note: captured value is not `Send` because `&` references cannot be sent unless their referent is `Sync`
-  --> tests/ui/future_not_send.rs:7:39
+  --> tests/ui/future_not_send.rs:8:39
    |
 LL | async fn private_future(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
    |                                       ^^^^ has type `&std::cell::Cell<usize>` which is not `Send`, because `std::cell::Cell<usize>` is not `Sync`
@@ -23,13 +23,13 @@ LL | async fn private_future(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
    = help: to override `-D warnings` add `#[allow(clippy::future_not_send)]`
 
 error: future cannot be sent between threads safely
-  --> tests/ui/future_not_send.rs:12:1
+  --> tests/ui/future_not_send.rs:13:1
    |
 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
-  --> tests/ui/future_not_send.rs:14:20
+  --> tests/ui/future_not_send.rs:15:20
    |
 LL | pub async fn public_future(rc: Rc<[u8]>) {
    |                            -- has type `std::rc::Rc<[u8]>` which is not `Send`
@@ -39,45 +39,45 @@ LL |     async { true }.await;
    = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Send`
 
 error: future cannot be sent between threads safely
-  --> tests/ui/future_not_send.rs:21:1
+  --> tests/ui/future_not_send.rs:22:1
    |
 LL | async fn private_future2(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `private_future2` is not `Send`
    |
 note: captured value is not `Send`
-  --> tests/ui/future_not_send.rs:21:26
+  --> tests/ui/future_not_send.rs:22:26
    |
 LL | async fn private_future2(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
    |                          ^^ has type `std::rc::Rc<[u8]>` which is not `Send`
    = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Send`
 note: captured value is not `Send` because `&` references cannot be sent unless their referent is `Sync`
-  --> tests/ui/future_not_send.rs:21:40
+  --> tests/ui/future_not_send.rs:22:40
    |
 LL | async fn private_future2(rc: Rc<[u8]>, cell: &Cell<usize>) -> bool {
    |                                        ^^^^ has type `&std::cell::Cell<usize>` which is not `Send`, because `std::cell::Cell<usize>` is not `Sync`
    = note: `std::cell::Cell<usize>` doesn't implement `std::marker::Sync`
 
 error: future cannot be sent between threads safely
-  --> tests/ui/future_not_send.rs:26:1
+  --> tests/ui/future_not_send.rs:27:1
    |
 LL | pub async fn public_future2(rc: Rc<[u8]>) {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `public_future2` is not `Send`
    |
 note: captured value is not `Send`
-  --> tests/ui/future_not_send.rs:26:29
+  --> tests/ui/future_not_send.rs:27:29
    |
 LL | pub async fn public_future2(rc: Rc<[u8]>) {}
    |                             ^^ has type `std::rc::Rc<[u8]>` which is not `Send`
    = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Send`
 
 error: future cannot be sent between threads safely
-  --> tests/ui/future_not_send.rs:38:5
+  --> tests/ui/future_not_send.rs:39:5
    |
 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
-  --> tests/ui/future_not_send.rs:40:24
+  --> tests/ui/future_not_send.rs:41:24
    |
 LL |     async fn private_future(&self) -> usize {
    |                             ----- has type `&Dummy` which is not `Send`
@@ -87,20 +87,20 @@ LL |         async { true }.await;
    = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Sync`
 
 error: future cannot be sent between threads safely
-  --> tests/ui/future_not_send.rs:44:5
+  --> tests/ui/future_not_send.rs:45:5
    |
 LL |     pub async fn public_future(&self) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `public_future` is not `Send`
    |
 note: captured value is not `Send` because `&` references cannot be sent unless their referent is `Sync`
-  --> tests/ui/future_not_send.rs:44:32
+  --> tests/ui/future_not_send.rs:45:32
    |
 LL |     pub async fn public_future(&self) {
    |                                ^^^^^ has type `&Dummy` which is not `Send`, because `Dummy` is not `Sync`
    = note: `std::rc::Rc<[u8]>` doesn't implement `std::marker::Sync`
 
 error: future cannot be sent between threads safely
-  --> tests/ui/future_not_send.rs:55:1
+  --> tests/ui/future_not_send.rs:56:1
    |
 LL | / async fn generic_future<T>(t: T) -> T
 LL | |
@@ -109,7 +109,7 @@ LL | |     T: Send,
    | |____________^ future returned by `generic_future` is not `Send`
    |
 note: future is not `Send` as this value is used across an await
-  --> tests/ui/future_not_send.rs:61:20
+  --> tests/ui/future_not_send.rs:62:20
    |
 LL |     let rt = &t;
    |         -- has type `&T` which is not `Send`
@@ -118,17 +118,20 @@ LL |     async { true }.await;
    = note: `T` doesn't implement `std::marker::Sync`
 
 error: future cannot be sent between threads safely
-  --> tests/ui/future_not_send.rs:73:1
+  --> tests/ui/future_not_send.rs:78:1
    |
-LL | async fn unclear_future<T>(t: T) {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `unclear_future` is not `Send`
+LL | async fn generic_future_always_unsend<T>(_: Rc<T>) {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `generic_future_always_unsend` is not `Send`
    |
-note: captured value is not `Send`
-  --> tests/ui/future_not_send.rs:73:28
+note: future is not `Send` as this value is used across an await
+  --> tests/ui/future_not_send.rs:80:20
    |
-LL | async fn unclear_future<T>(t: T) {}
-   |                            ^ has type `T` which is not `Send`
-   = note: `T` doesn't implement `std::marker::Send`
+LL | async fn generic_future_always_unsend<T>(_: Rc<T>) {
+   |                                          - has type `std::rc::Rc<T>` which is not `Send`
+LL |
+LL |     async { true }.await;
+   |                    ^^^^^ await occurs here, with `_` maybe used later
+   = note: `std::rc::Rc<T>` doesn't implement `std::marker::Send`
 
 error: aborting due to 8 previous errors