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

Async fn does not compile if lifetime does not appear in bounds (sometimes) #63033

Open
dtolnay opened this issue Jul 27, 2019 · 45 comments · May be fixed by #89056
Open

Async fn does not compile if lifetime does not appear in bounds (sometimes) #63033

dtolnay opened this issue Jul 27, 2019 · 45 comments · May be fixed by #89056

Comments

@dtolnay
Copy link
Member

@dtolnay dtolnay commented Jul 27, 2019

#![feature(async_await)]

use std::marker::PhantomData;

trait Trait {}

async fn f<'a>(reference: &(), marker: PhantomData<dyn Trait>) {}

This fails with:

error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds
 --> src/main.rs:7:64
  |
7 | async fn f<'a>(reference: &(), marker: PhantomData<dyn Trait>) {}
  |                                                                ^
  |
note: hidden type `impl std::future::Future` captures the scope of call-site for function at 7:64
 --> src/main.rs:7:64
  |
7 | async fn f<'a>(reference: &(), marker: PhantomData<dyn Trait>) {}
  |                                                                ^^

I believe this should compile because:

  1. The non-async function with the same signature does compile;

    fn f<'a>(reference: &(), marker: PhantomData<dyn Trait>) {}
  2. Seemingly insignificant tweaks, like adding a wrapper struct, make it compile.

    struct Wrapper(PhantomData<dyn Trait>);
    async fn f<'a>(reference: &(), marker: Wrapper) {}

Mentioning @cramertj who fixed a similar error message in #59001.
Mentioning @nikomatsakis who touched this error message in #49041 and #61775.

rustc 1.38.0-nightly (c43753f 2019-07-26)

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 13, 2019

Check-in from WG-async-await meeting: I agree this is a bug -- it doesn't necessarily block stabilization (no fwd compat risk) but would definitely be a good one to fix. Marking as blocking + unclear for the moment and will return to it.

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 14, 2019

I did a bit of investigation. The error results because we have an inference variable ('_#3r) that seems to have no constraints, but must be equal to one of the captured regions (e.g., 'a). I haven't 100% figured out where this variable comes from, I imagine it's related to 'a though? In any case, the resolver basically winds up in a situation where it could pick any of the lifetimes but has no reason to prefer one over the other, and hence it reports an error.

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 14, 2019

OK, I think I see what's going on. It doesn't actually have much to do with the unused lifetime per se. What happens is this:

  • The type of the generated result is something like Foo<&'2 (), PhantomData<dyn Trait + '3>> -- i.e., it includes the types of the parameters, though with region variables in place of the known regions. Those types must be supertypes of the actual parameter types.
  • We capture the argument marker of type dyn Trait + 'static -- per the subtyping rules, we create the type dyn Trait + '3 for the type of this captured value, with the requirement that 'static: '3 -- as it happens, this is always true, so there is effectively no constraint on '3.
  • We do impose the constraint that '2 and '3 must both be equal to one of the free regions -- in this case, 'a or the anonymous lifetime of reference (which I'll call 'b).

So thus the region solvers get in a state where '3 must be equal to 'a, 'b, or 'static -- but any one of them would be a reasonable choice.

Removing the unused lifetime ('a) makes this an easy choice -- it can pick 'b, which is smaller than 'static. (Per the "least choice" rules we usually use.)

Removing the type and putting it into a struct (Wrapper) removes the lifetime variable altogether, as then we have Foo<&'2 (), Wrapper>, and there are no lifetime parameters involved.

I'm not sure what I think is the best fix. It would be nice for the solver to pick 'static, though it complicates the algorithm. You could certainly imagine a tweak that picks 'static if the value is legal and otherwise looks for a least choice. That might indeed be superior to the current approach, though I'd want to think about the full implications -- in particular, when impl Trait is used in places outside of the fn return type (e.g., in a let statement), picking 'static might lead to unnecessary borrow check errors. (The alternative would be to only look for 'static when there is no least choice; that seems safer.)

(Interestingly, at some point at least I thought that a polonius-like approach sidesteps a lot of the challenges here-- if we had confidence that we would transition that might ease some of my concerns.)

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 14, 2019

Now that I understand better what is happening, I don't feel this has to block stabilization -- in particular, I don't think that there is a forward compatibility risk from us improving the region inference algorithm. I don't think we would choose an algorithm that makes existing code illegal. =)

As such, I'm going to mark this as Deferred.

Loading

@sfackler
Copy link
Member

@sfackler sfackler commented Oct 10, 2019

Here's another example of the same problem that doesn't have an unused lifetime (AFAIKT):

struct Foo;

impl Foo {
    async fn wat(&self, _: &'static str, _: Bar<'_>) {}
}

struct Bar<'a>(Box<dyn std::fmt::Debug + 'a>);
error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds
 --> src/lib.rs:4:54
  |
4 |     async fn wat(&self, _: &'static str, _: Bar<'_>) {}
  |                                                      ^
  |
note: hidden type `impl std::future::Future` captures the scope of call-site for function at 4:54
 --> src/lib.rs:4:54
  |
4 |     async fn wat(&self, _: &'static str, _: Bar<'_>) {}
  |                                                      ^^

error: aborting due to previous error

Removing any of the three arguments to the function allows it to compile.

Loading

@lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Dec 14, 2019

I am also affected by this bug, where having both a reference and a box as parameters to a function causes the cryptic "hidden type for impl Trait captures lifetime that does not appear in bounds" :

trait Trait {}
async fn f<'a>(_a: &(), _b: Box<dyn Trait>) {}

(playground)

Is there a resolution in sight ?

Loading

@sfackler
Copy link
Member

@sfackler sfackler commented Dec 14, 2019

'a is unused in that function - if you remove the <'a> it compiles.

Loading

@lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Dec 14, 2019

Ok, let me change it to something closer to what I had in my original code:

trait T {}
struct S;
impl S {
    async fn f(_a: &S, _b:&S, _c: Box<dyn T>) {}
}

(playground)

Loading

MindFlavor added a commit to MindFlavor/AzureSDKForRust that referenced this issue Dec 23, 2019
@netvl
Copy link
Contributor

@netvl netvl commented Dec 25, 2019

I think I found another example of this:

trait T {}

struct L<A = Box<dyn T>> {
    value: A,
}

async fn test1(l: L, s1: &str, s2: &str) {
}

(playground)

This example more or less represents what I met in my code (when I tried to pass a structure with a type parameter to an async function with the default value for this type parameter being a trait object), but it can be simplified by removing the default parameter assignment and passing the Box<dyn T> argument to L explicitly:

trait T {}

struct L<A> {
    value: A,
}

async fn test1(l: L<Box<dyn T>>, s1: &str, s2: &str) {
}

(playground)

Curiously, if you replace async fn ... { ... } with fn ... -> impl Future { async { ... } }, the error goes away:

use std::future::Future;

trait T {}

struct L<A> {
    value: A,
}

fn test1(l: L<Box<dyn T>>, s1: &str, s2: &str) -> impl Future<Output=()> {
    async {
    }
}

(playground)

It is important to note that the test1 function has two additional arguments. If you remove one or both of them, it starts compiling again:

trait T {}

struct L<A> {
    value: A,
}

async fn test2(l: L<Box<dyn T>>, s1: &str) {
}

async fn test3(l: L<Box<dyn T>>) {
}

(playground)

Loading

@skeet70
Copy link

@skeet70 skeet70 commented Jan 22, 2020

@netvl 's comment is a minimal reproduction of what I ran into. I had a function async f(a: &CacheA, b: &CacheB, c: &CacheC, d: Arc<dyn Client>). If I remove two of the cache arguments it compiles. As soon as I add one back in it gives me the title error.

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 26, 2020

Hmm, it is probably worth revisiting this problem now that stabilization has happened.

Loading

facebook-github-bot added a commit to facebookarchive/mononoke that referenced this issue Jan 29, 2020
Summary:
walker: migrate to new futures (0.3.1).

* walk.rs is not fully migrated, due to function call to `bounded_traversal_stream`, which uses old futures.

* `scrub::scrub_objects`, `sizing::compression_benefit` and `validate::validate` returns `BoxFuture` instead of being a regular `async` function, due to limitation of rust issue [#63303](rust-lang/rust#63033).

Reviewed By: farnz

Differential Revision: D19536696

fbshipit-source-id: a0df337b86d7b067a44bf3b18834193d3f63f5dc
@swilcox3
Copy link

@swilcox3 swilcox3 commented Apr 11, 2020

Any word on this? I'm getting this error as well.

pub async fn publish(
    conn: &mut StateConnection,
    file: &FileID,
    user: &UserID,
    latest: ChangeID,
    mut subscribers: Vec<Box<dyn ChangeSubscriber>>,
) -> Result<(), ObjError> {
    let to_publish = gather_changes(conn, file, latest).await?;
    for sub in subscribers {
        sub.get_changes(file, user, latest, &to_publish).await?;
    }
    Ok(())
}

Changing mut subscribers: Vec<Box<dyn ChangeSubscriber>> to subscribers: &mut Vec<Box<dyn ChangeSubscriber>> fixes the issue. Only problem is that I actually want to move that vector in because I want to tokio::spawn each call to get_changes.

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Apr 13, 2020

No progress or major updates. The description here is still accurate as to the root cause, but I think we don't yet know the best fix.

Loading

@danieleades
Copy link

@danieleades danieleades commented May 30, 2020

this is going to become a problem now that #72459 is merged.

It makes it impossible to implement IntoFuture for types with lifetimes, when the future that's returned is unnameable.

consider

pub struct Request<'a> {
    ...
}

impl<'a> Request<'a> {
    async fn send(self) -> Result<(), Error> {
        ...
    }
}

// does not compile
// "cannot infer an appropriate lifetime for lifetime parameter `'a` due to conflicting requirements"
impl<'a> IntoFuture for Request<'a> {
    type Output = Result<(), Error>;
    type Future = impl Future<Output = Self::Output>;

    fn into_future(self) -> Self::Future {
        self.send()
    }
}

// does not compile
// "hidden type for `impl Trait` captures lifetime that does not appear in bounds"
impl<'a> IntoFuture for Request<'a> {
    type Output = Result<(), Error>;
    type Future = impl Future<Output = Self::Output> + 'a;

    fn into_future(self) -> Self::Future {
        self.send()
    }
}

Loading

@danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented May 18, 2021

FWIW, in the meantime, and also to help solve the issue with manual -> impl Trait capturing a potentially bigger invariant lifetime, I have published a helper proc-macro-attribute crate that automatically appends the necessary Captures<'_> hacks: https://crates.io/crates/fix-hidden-lifetime-bug

Loading

@tubzby
Copy link

@tubzby tubzby commented Jul 1, 2021

Coming to this too, a little google drives me here.

async fn read_loop(
    &self,
    st: &mut State,
    stream: Box<dyn std::io::Read>
 ) -> Result<(), Error> {
.....
}

This thread is almost 2 years old, is there any solution or plan to fix this?

Loading

@nazar-pc
Copy link

@nazar-pc nazar-pc commented Jul 1, 2021

@tubzby explicit lifetimes on references should fix the issue for you until compiler is smart enough to figure that out on its own

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 2, 2021

@rustbot release-assignment

I haven't had time to write up any mentoring instructions here, I'm afraid. :(

Loading

@tmandry tmandry moved this from Claimed to On deck in wg-async-foundations work Jul 2, 2021
@wigy-opensource-developer

This comment was marked as spam.

@tubzby
Copy link

@tubzby tubzby commented Jul 5, 2021

@nazar-pc I want to ensure that, is it because the read_loop function might execute after the scope of self reference?
If so, how can I explicitly set the lifetime of read_loop is greater than the reference?

Loading

@danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Jul 5, 2021

@tubzby note that your code can be automatically fixed by the tool I mentioned in my previous comment:

image

A reliable fix that doesn't change the semantics of the function 🙂

How to manually fix this without changing the semantics of the function

You can even query that attribute with the extra showme parameter fed to the attribute, and the eponymous Cargo feature, to get:

Screen Shot 2021-07-05 at 14 36 38

Fixing the Box<dyn Trait…> case in practice

For the specific case of Box<dyn Trait…> usage, you can try changing the signature of the function to one using Box<dyn Trait… + 'static>, that is, with the + 'static bound on the trait object explicitly spelled out:

  async fn read_loop(
      &self,
      st: &mut State,
-     stream: Box<dyn std::io::Read>,
+     stream: Box<dyn std::io::Read + 'static>,
  ) -> Result<(), Error> {
      todo!("FUNCTION BODY")
  }

This leads to a simpler signature regarding async fn / -> impl … / TAIT unsugaring, and circumvents the issue of this thread.


@nikomatsakis I'd like to experiment with -> impl … unsugared's TAIT being automatically generic over all the lifetime parameters in scope of the return type. This is what #[fix_hidden_lifetime_bug] achieves by using the + Capture<'lt> hack for each and every such lifetime, and I've never encountered cases where this would be an error.

If that approach seems fine to you, I can try to implement this new unsugaring on the compiler myself, although I may not be very fast since I'll need to experiment a bit to get acquainted with the related compiler internals 😅

Loading

@tubzby
Copy link

@tubzby tubzby commented Jul 6, 2021

@danielhenrymantilla thanks, I will try that.

Loading

@abc-mikey
Copy link

@abc-mikey abc-mikey commented Jul 11, 2021

I just got bitten by this and I'm a beginner. @danielhenrymantilla 's workarround worked but a proper fix would be desirable.

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 11, 2021

@danielhenrymantilla are you saying you want to change how -> impl Trait works for all cases? That would certainly be a breaking change, so I don't think we can do that. However, it would be good to talk about a better fix than the Captures<'tcx> hack. One thing we talked about in the very beginning was having a syntax like impl<'a, T> to be very explicit about what things are captured and what are not. But that would, I think, require a lang-team proposal.

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 16, 2021

@rustbot claim

Loading

@eholk eholk moved this from On deck to In progress (current sprint) in wg-async-foundations work Sep 16, 2021
@danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Sep 16, 2021

That would certainly be a breaking change

When I thought about this I struggled to come up with an instance of this, so I'm genuinely curious about it. In other words: could annotating a function with #[fix_hidden_lifetime_bug] / add Captures<'…> lead to breakage somewhere along the way? If so, such an example could help us evaluate the different options at our disposal 🙂

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 17, 2021

OK, I've read up on this issue again. The basic diagnosis I had is correct. The fact that some of the lifetimes are unused isn't really especially relevant.

For example, in the example from this comment, the problem is simply that there are two lifetimes (from _a and _b) along with the Box<dyn T>. This still gets us into the situation where the inferencer must pick between _a, _b, or 'static.

Let me run this through the compiler and take a look at the constraints. I also want to think about how polonius affects this.

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 17, 2021

I just read over @danielhenrymantilla's comment. The reason that this "desugared" version works is because it introduces a new lifetime, '_async_fut, and this lifetime has a relationship to every other lifetime. So when we have to pick between 'a, 'b, and 'static, we now also haev the choice of '_async_fut, which is strictly less than all of those choices. So we pick that.

Now I realize that the reason that this whole situation is a non-issue in polonius is basically that the polonius representation permits lifetimes that are the intersection of a set of lifetimes. So you could infer the result to be 'a ^ 'b, which is effectively the same as '_async_fut. But in the current implementation, we don't have anything like that.

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 17, 2021

I'd like to experiment with -> impl … unsugared's TAIT being automatically generic over all the lifetime parameters in scope of the return type.

I'm not sure if I understand exactly what you were proposing here, @danielhenrymantilla. I thought at first that it meant:

fn foo(&self) -> impl Debug

would be able to capture self (because it would capture that lifetime). That would be a breaking change, but is that what you meant?

Loading

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 17, 2021

Fix in #89056

Loading

@danielhenrymantilla
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla commented Sep 17, 2021

The reason that this "desugared" version works is because it introduces a new lifetime, '_async_fut, and this lifetime has a relationship to every other lifetime

, effectively defining it, for all intents and purposes, as the intersection lifetime! (which indeed meets the bounds) Ah yes, I see now how there are definitely two distinct issues at hand, here, and that my fix for one just happened to fix the other as a byproduct 😅


So, while your PR will fix the async fn one, I'm personally still concerned about the case of the existential capturing both 'long_invariant : 'short, and it's to tackle that one that I was suggesting that -> impl … existentials were, internally, allowed to be generic over all the lifetime parameters in scope.

That is, what I meant, or what I'm basically qualifying as a bug, is the fact that impl … and dyn … don't have the same semantics, lifetime-wise (so, to clarify, I am definitely not talking about changing the rules for lifetime elision in function signatures):

  1. In the same fashion that fn foo(&self) -> Box<dyn Debug> stands for Box<dyn Debug + 'static>, and can thus not capture self since it is '_-bound,

  2. then fn foo(&self) -> Box<impl Debug> stands for -> Box<impl Debug + 'static> and can thus not capture self yadda yadda. And in the impl case, the Box is not needed, so it can be skipped if the implementation doesn't need it.

    • But the key change I am suggesting is the definition of that impl Debug + 'static existential. Currently, it would be defined as something like type Ret = impl Debug + 'static;, and then unified. What I'm suggesting is that it be defined as type Ret<'self_> = impl Debug + 'static;, much like what adding a + Captures<'self_> would achieve.
  3. This would be meaningful, for instance, in all the cases with a captured invariant lifetime which is not equal to the lower bound.
    Take Self = &'long (), and fn bar<'s>(&'s mut self) -> Box<impl Debug + 's> { Box::new(self) }.
    In that case, if instead of impl we had dyn, the signature would work. But in the impl case, we have a type Ret<'s> = impl Debug + 's;, and such a type definition cannot be unified with the actual type of the implementor, &'s mut (&'long ()), since there is no 'long in scope of that definition. My observation is that I believe it cannot hurt to have too many lifetime parameters "in scope" / reachable for an existential's type definition.

    • In that regard, the + Captures<'lt> hack, be it manual or automated through the #[fix…] attribute, is just a way to test my theory: the attribute currently adds + Captures<'lt> bounds for every lifetime it meets, without asking further questions, and that suffices to make the existential type definition be generic over all of them, and this whole thing has, empirically, solved the compilation problems without any problems on its own, thus allowing for impl and dyn to remain quite interchangeable (lifetime-wise, barring obvious dyn safety caveats).

    • (Slight aside: I've noticed that this type Ret seems to currently be always generic over all the type parameters in scope, so it's really just the lifetime parameters that it's being (overly) picky around, it seems)

Loading

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 20, 2021
make member constraints pick static if no upper bounds

The current member constraint algorithm has a failure mode when it encounters a
variable `'0` and the only constraint is that `':static: '0`. In that case,
there are no upper bounds, or at least no non-trivial upper bounds.  As a
result, we are not able to rule out any of the choices, so if you have a
constraint like `'0 member ['a, 'b, 'static]`, where `'a` and `'b` are
unrelated, then the algorithm gets stuck as there is no 'least choice' from
that set.

The tweak in this commit changes the algorithm so that *if* there are no upper
bounds (and hence `'0` can get as large as desired without creating a region
check error), it will just pick `'static`. This should only occur in cases
where the data is flowing out from a `'static` value.

This change is probably *not* right for impl Trait in let bindings, but those
are challenging with member constraints anyway, and not currently supported.
Furthermore, this change is not needed in a polonius-like formulation, which
effectively permits "ad-hoc intersections" of lifetimes as the value for a
region, and hence could give a value like `'a ^ 'b` as the resulting lifetime.
Therefore I think there isn't forwards compat danger here. (famous last words?)

r? `@lqd`
cc `@tmandry` `@eholk` `@jackh726` `@pnkfelix`

I was thinking it might be nice to schedule a recorded zoom session to talk over this PR and how this bit of the code works, as I imagine it has a ... low bus factor. =)

Fixes rust-lang#63033
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 23, 2021
make member constraints pick static if no upper bounds

The current member constraint algorithm has a failure mode when it encounters a
variable `'0` and the only constraint is that `':static: '0`. In that case,
there are no upper bounds, or at least no non-trivial upper bounds.  As a
result, we are not able to rule out any of the choices, so if you have a
constraint like `'0 member ['a, 'b, 'static]`, where `'a` and `'b` are
unrelated, then the algorithm gets stuck as there is no 'least choice' from
that set.

The tweak in this commit changes the algorithm so that *if* there are no upper
bounds (and hence `'0` can get as large as desired without creating a region
check error), it will just pick `'static`. This should only occur in cases
where the data is flowing out from a `'static` value.

This change is probably *not* right for impl Trait in let bindings, but those
are challenging with member constraints anyway, and not currently supported.
Furthermore, this change is not needed in a polonius-like formulation, which
effectively permits "ad-hoc intersections" of lifetimes as the value for a
region, and hence could give a value like `'a ^ 'b` as the resulting lifetime.
Therefore I think there isn't forwards compat danger here. (famous last words?)

r? `@lqd`
cc `@tmandry` `@eholk` `@jackh726` `@pnkfelix`

I was thinking it might be nice to schedule a recorded zoom session to talk over this PR and how this bit of the code works, as I imagine it has a ... low bus factor. =)

Fixes rust-lang#63033
@mingmwang

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
wg-async-foundations work
In progress (current sprint)
Linked pull requests

Successfully merging a pull request may close this issue.