Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow conditional Send futures in future_not_send #13590

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow conditional Send futures in future_not_send #13590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 57 additions & 10 deletions clippy_lints/src/future_not_send.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
}
}
}
7 changes: 0 additions & 7 deletions tests/ui/crashes/ice-10645.rs
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ICE path isn't really reachable anymore for this test since the function is generic so I'm not sure it's still useful to have, and if I understand the original ICE correctly it would have happened for all the regular tests anyway (anything that led to the .err_ctxt() call)?

This file was deleted.

17 changes: 0 additions & 17 deletions tests/ui/crashes/ice-10645.stderr
View file
Open in desktop

This file was deleted.

18 changes: 17 additions & 1 deletion tests/ui/future_not_send.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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,
Expand All @@ -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]);
Expand Down
51 changes: 27 additions & 24 deletions tests/ui/future_not_send.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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`
Expand All @@ -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`
Expand All @@ -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`
Expand All @@ -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`
Expand All @@ -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 | |
Expand All @@ -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`
Expand All @@ -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