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.

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

Lifetime bounds in auto trait impls prevent trait from being implemented on generators #64552

Open
leo60228 opened this issue Sep 17, 2019 · 19 comments · May be fixed by #92449
Open

Lifetime bounds in auto trait impls prevent trait from being implemented on generators #64552

leo60228 opened this issue Sep 17, 2019 · 19 comments · May be fixed by #92449

Comments

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Sep 17, 2019

use std::collections::{BTreeMap, HashMap};
use std::sync::Arc;

fn needs_send<T: Send>(_val: T) {}
async fn async_fn_a(_num: u32) {}
async fn async_fn_b(map: Arc<BTreeMap<u32, &'static u32>>) {
    for (_i, v) in &*map {
        async_fn_a(**v).await;
    }
}
async fn async_fn_c(map: Arc<HashMap<u32, &'static u32>>) {
    for (_i, v) in &*map {
        async_fn_a(**v).await;
    }
}

fn main() {
    // this works...
    let map: Arc<HashMap<u32, &'static u32>> = Arc::new(HashMap::new());
    needs_send(async_fn_c(map.clone()));
    
    // but this doesn't
    let map: Arc<BTreeMap<u32, &'static u32>> = Arc::new(BTreeMap::new());
    needs_send(async_fn_b(map.clone()));
}

(playground)

error: implementation of `std::marker::Send` is not general enough
  --> src/main.rs:24:5
   |
24 |     needs_send(async_fn_b(map.clone()));
   |     ^^^^^^^^^^
   |
   = note: `std::marker::Send` would have to be implemented for the type `alloc::collections::btree::node::NodeRef<alloc::collections::btree::node::marker::Immut<'0>, u32, &'1 u32, alloc::collections::btree::node::marker::Leaf>`, for any two lifetimes `'0` and `'1`
   = note: but `std::marker::Send` is actually implemented for the type `alloc::collections::btree::node::NodeRef<alloc::collections::btree::node::marker::Immut<'2>, u32, &u32, alloc::collections::btree::node::marker::Leaf>`, for some specific lifetime `'2`
@Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Sep 18, 2019

Minimized:

fn needs_send<T: Send>(_val: T) {}
async fn use_async<T>(_val: T) {}

struct MyStruct<'a, T: 'a> {
    val: &'a T
}

unsafe impl<'a, T: 'a> Send for MyStruct<'a, T> {} 

async fn use_my_struct(val: MyStruct<'static, &'static u8>) {
    use_async(val).await;
}

fn main() {
    let first_struct: MyStruct<'static, &'static u8> = MyStruct { val: &&26 };
    let second_struct: MyStruct<'static, &'static u8> = MyStruct { val: &&27 };
    
    needs_send(first_struct);
    needs_send(use_my_struct(second_struct)); // ERROR
}

It seems as though the manual Send impl is confusing some async-related code. The code compiles if you remove unsafe impl<'a, T: 'a> Send for MyStruct<'a, T> {} , even though it's not actually adding any additional region constraints.

@Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Sep 19, 2019

I think the bug occurs here:

// The types in the generator interior contain lifetimes local to the generator itself,
// which should not be exposed outside of the generator. Therefore, we replace these
// lifetimes with existentially-bound lifetimes, which reflect the exact value of the
// lifetimes not being known by users.
//
// These lifetimes are used in auto trait impl checking (for example,
// if a Sync generator contains an &'α T, we need to check whether &'α T: Sync),
// so knowledge of the exact relationships between them isn't particularly important.
debug!("types in generator {:?}, span = {:?}", type_list, body.value.span);
// Replace all regions inside the generator interior with late bound regions
// Note that each region slot in the types gets a new fresh late bound region,
// which means that none of the regions inside relate to any other, even if
// typeck had previously found constraints that would cause them to be related.
let mut counter = 0;
let type_list = fcx.tcx.fold_regions(&type_list, &mut false, |_, current_depth| {
counter += 1;
fcx.tcx.mk_region(ty::ReLateBound(current_depth, ty::BrAnon(counter)))
});

I believe that 'knowledge of the exact relationships between them isn't particularly important' is incorrect. If an auto trait impl imposes region constraints (as with MyStruct), then the relationship between regions does matter.

For this particular issue, leaving 'static unmodified (i.e. not replacing it with a region variable) should be sufficient to allow the code to compile.

In the general case, I think a full solution would require some notion of 'higher-ranked' region constraints. For example, consider the function:

async fn use_my_struct<'a, 'b: 'a>(val: MyStruct<'a, &'b u8>) {
    use_async(val).await;
}

The returned generator should implement Send, because MyStruct<'a, &'b u8> implements Send. However, proving this requires us to know that 'b: 'a holds when we're inspecting the generator witness.

@leo60228 leo60228 changed the title Strange error when iterating over BTreeMap in async fn Lifetime bounds in auto trait impls prevent trait from being implemented on generators Sep 19, 2019
@Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Sep 19, 2019

Concretely, I think we could implement this as follows:

  1. Add a List<RegionOutlivesPredicate> to GeneratorWitness. This would store a list of predicates involving any 'internal' regions variables. This list would be created here by recording all known relationships between interior region variables.
  2. Modify collect_predicates_for_types to take into account the original type we're determining the auto impl for. If it's a GeneratorWitness, we extend the ParamEnv for the newly created sub-obligations with all of the RegionOutlivesPredicate from our generator witness. Effectively, a GeneratorWitness causes us to learn something new - however, this knowledge only applies to to the interior region variables.

This should only affect auto-trait resolution for generators. Users cannot implement any traits on the underlying generator type (in fact, they cannot even name it). This should ensure that these RegionOutlivesPredicate cannot affect any code external to the generator, except to the extent that they allow us to prove that an auto-trait impl applies. It should be impossible user code to observe these regions in any way, since the generator can only be interacted with via Generator::Resume and the .await syntax.

cc @nikomatsakis @tmandry

@dtolnay
Copy link
Member

@dtolnay dtolnay commented Oct 23, 2019

I haven't seen a "me too" yet so I thought I'd mention that I hit this as well, to help assess how frequent this bug is.

use futures::future;
use futures::stream::{self, StreamExt};

struct Thing;

async fn genfoo(t: &[Thing]) {
    let _ = stream::iter(t)
        .map(future::ready)
        .buffer_unordered(10)
        .collect::<Vec<_>>()
        .await;
}

fn foo<'a>(t: &'a [Thing]) -> impl Send + 'a {
    genfoo(t)
}
error: implementation of `std::iter::Iterator` is not general enough
    --> src/main.rs:14:31
     |
14   |   fn foo<'a>(t: &'a [Thing]) -> impl Send + 'a {
     |                                 ^^^^^^^^^^^^^^ implementation of `std::iter::Iterator` is not general enough
     |
     = note: `std::iter::Iterator` would have to be implemented for the type `std::slice::Iter<'0, Thing>`, for any lifetime `'0`...
     = note: ...but `std::iter::Iterator` is actually implemented for the type `std::slice::Iter<'1, Thing>`, for some specific lifetime `'1`

@withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Aug 10, 2020

We looked at this in lang triage and retagged it as T-compiler; since its not a soundness bug and seems like strictly an implementation issue, we don't think it needs lang input.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 17, 2020

I wasn't at the lang team triage meeting, but I do think that's largely right. It's basically a question of implementation doing "the best it can"

hdevalence added a commit to ZcashFoundation/zebra that referenced this issue Sep 17, 2020
> Added a test that the handshake's version message matches specified fields, but the test does not compile, because rustc doesn't believe that the Box<dyn std::error::Error + Send + Sync + 'static> is 'static, and therefore isn't a Box<dyn std::error::Error + Send + Sync + 'static>. This manifests as being unable to spawn the connect_isolated task. From digging through Tokio issues I believe that this is an instance of rust-lang/rust#64552 .

Co-authored-by: Jane Lusby <jlusby42@gmail.com>
hdevalence added a commit to ZcashFoundation/zebra that referenced this issue Sep 17, 2020
> Added a test that the handshake's version message matches specified fields, but the test does not compile, because rustc doesn't believe that the Box<dyn std::error::Error + Send + Sync + 'static> is 'static, and therefore isn't a Box<dyn std::error::Error + Send + Sync + 'static>. This manifests as being unable to spawn the connect_isolated task. From digging through Tokio issues I believe that this is an instance of rust-lang/rust#64552 .

Co-authored-by: Jane Lusby <jlusby42@gmail.com>
hdevalence added a commit to ZcashFoundation/zebra that referenced this issue Sep 18, 2020
hdevalence added a commit to ZcashFoundation/zebra that referenced this issue Sep 18, 2020
hdevalence added a commit to ZcashFoundation/zebra that referenced this issue Sep 18, 2020
hdevalence added a commit to ZcashFoundation/zebra that referenced this issue Sep 18, 2020
The original version of this commit ran into

rust-lang/rust#64552

again.  Thanks to @yaahc for suggesting a workaround (using futures combinators
to avoid writing an async block).
hdevalence added a commit to ZcashFoundation/zebra that referenced this issue Sep 18, 2020
The original version of this commit ran into

rust-lang/rust#64552

again.  Thanks to @yaahc for suggesting a workaround (using futures combinators
to avoid writing an async block).
hdevalence added a commit to ZcashFoundation/zebra that referenced this issue Sep 19, 2020
The original version of this commit ran into

rust-lang/rust#64552

again.  Thanks to @yaahc for suggesting a workaround (using futures combinators
to avoid writing an async block).
@tmandry
Copy link
Contributor

@tmandry tmandry commented Sep 22, 2020

@rustbot assign @csmoe

@tmandry tmandry moved this from On deck to Claimed in wg-async-foundations work Sep 22, 2020
@tmandry
Copy link
Contributor

@tmandry tmandry commented Dec 3, 2020

@csmoe Are you still planning to work on this?

@leo60228
Copy link
Contributor Author

@leo60228 leo60228 commented Jan 12, 2021

Since it seems like fixing this is a somewhat large task, maybe having the diagnostic link to this issue would be a workable stopgap solution?

@dtolnay
Copy link
Member

@dtolnay dtolnay commented Jan 27, 2021

Here is a minimization from xacrimon/dashmap#131 that reveals something interesting (possibly generalizable as a workaround): there appears to be a difference from hiding the affected type behind a newtype wrapper. The buggy lifetime error in the code below is resolved by replacing one Box<dyn Send + Sync + 'static> by a newtype wrapper around Box<dyn Send + Sync + 'static>, which ordinarily I would expect to have zero bearing on lifetime errors.

use std::marker::PhantomData;

pub struct One<T>(PhantomData<T>);
pub struct Two<T, U>(PhantomData<(T, U)>);

unsafe impl<T, U> Send for Two<T, U> where U: IsOne<T> {}

pub trait IsOne<T> {}
impl<T> IsOne<T> for One<T> {}

fn main() {
    fn assert_send(_: impl Send) {}
    assert_send(async {
        //struct T(Box<dyn Send + Sync + 'static>);   // <-- WORKS
        type T = Box<dyn Send + Sync + 'static>;      // <-- FAILS
        let _value = Two::<T, One<T>>(PhantomData);
        async {}.await;
    });
}
error: implementation of `IsOne` is not general enough
  --> src/main.rs:13:5
   |
8  | pub trait IsOne<T> {}
   | --------------------- trait `IsOne` defined here
...
13 |     assert_send(async {
   |     ^^^^^^^^^^^ implementation of `IsOne` is not general enough
   |
   = note: `One<Box<(dyn Send + Sync + '0)>>` must implement `IsOne<Box<(dyn Send + Sync + '1)>>`, for any two lifetimes `'0` and `'1`...
   = note: ...but `IsOne<Box<dyn Send + Sync>>` is actually implemented for the type `One<Box<dyn Send + Sync>>`

@tmandry
Copy link
Contributor

@tmandry tmandry commented Jan 28, 2021

@rustbot remove-assignment

@csmoe feel free to claim again if you're still working on this.

@tmandry tmandry moved this from Claimed to On deck in wg-async-foundations work Jan 28, 2021
hawkw added a commit to linkerd/linkerd2-proxy that referenced this issue Feb 17, 2021
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to linkerd/linkerd2-proxy that referenced this issue Feb 17, 2021
…922)

The proxy currently has its own implementation of a `tower` `Service`
that makes an inner service `Clone`able by driving it in a spawned task
and buffering requests on a channel. This also exists upstream, as
`tower::buffer`.

We implemented our own version for a couple of reasons: to avoid an
upstream issue where memory was leaked when a buffered request was
cancelled, and to implement an idle timeout when the buffered service
has been unready for too long. However, it's no longer necessary to
reimplement our own buffer service for these reasons: the upstream bug
was fixed in `tower` 0.4 (see tower-rs/tower#476, tower-rs/tower#480,
and tower-rs/tower#556); and we no longer actually use the buffer idle
timeout (instead, we idle out unresponsive services with the separate
`Failfast` middleware, note that `push_spawn_buffer_with_idle_timeout`
is never actually used).

Therefore, we can remove our _sui generis_ implementation in favour of
`tower::buffer` from upstream. This eliminates dead code for the idle
timeout, which we never actually use, and reduces duplication (since
`tonic` uses `tower::buffer` internally, its code is already compiled
into the proxy). It also reduces the amount of code I'm personally
responsible for maintaining in two separate places ;)

Since the `linkerd-buffer` crate erases the type of the buffered
service, while `tower::buffer` does not, I've changed the
`push_spawn_buffer`/`spawn_buffer` helpers to also include a
`BoxService` layer. This required adding a `BoxServiceLayer` type, since
`BoxService::layer` returns a `LayerFn` with an unnameable type.

Also, this change ran into issues due to a compiler bug where generators
(async blocks) sometimes forget concrete lifetimes,
rust-lang/rust#64552. In order to resolve this, I had to remove the
outermost `async` blocks from the OpenCensus and identity daemon tasks.
These async blocks were used only for emitting a tracing event when the
task is started, so it wasn't a big deal to remove them; I moved the
trace events into the actual daemon task functions, and used a `tracing`
span to propagate the remote addresses which aren't known inside the
daemon task functions.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
rudib added a commit to hashmismatch/mininet.rs that referenced this issue Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.