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

RPITIT "captures lifetime that does not appear in bounds" #128752

Open
tmandry opened this issue Aug 6, 2024 · 8 comments
Open

RPITIT "captures lifetime that does not appear in bounds" #128752

tmandry opened this issue Aug 6, 2024 · 8 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. A-trait-objects Area: trait objects, vtable layout A-traits Area: Trait system C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Aug 6, 2024

I tried this code: playground

trait MyTrait {
    fn foo<'a, 'b>(&'a self, x: &'b i32) -> impl Future<Output = i32>;
}

trait ErasedMyTrait {
    fn foo<'life0, 'life1, 'dynosaur>(&'life0 self, x: &'life1 i32)
    -> Pin<Box<dyn Future<Output = i32> + 'dynosaur>>
    where
    'life0: 'dynosaur,
    'life1: 'dynosaur;
}

struct DynMyTrait<T: ErasedMyTrait> {
    ptr: T,
}

impl<T: ErasedMyTrait> MyTrait for DynMyTrait<T> {
    fn foo<'a, 'b>(&'a self, x: &'b i32) -> impl Future<Output = i32> {
        self.ptr.foo(x)
    }
}

Diagnostics

I got a nonsensical error message:

error[E0700]: hidden type for `impl Future<Output = i32>` captures lifetime that does not appear in bounds
  --> src/lib.rs:22:9
   |
21 |     fn foo<'a, 'b>(&'a self, x: &'b i32) -> impl Future<Output = i32> {
   |                --                           ------------------------- opaque type defined here
   |                |
   |                hidden type `Pin<Box<(dyn Future<Output = i32> + 'b)>>` captures the lifetime `'b` as defined here
22 |         self.ptr.foo(x)
   |         ^^^^^^^^^^^^^^^
   |
help: to declare that `impl Future<Output = i32>` captures `'b`, you can add an explicit `'b` lifetime bound
   |
21 |     fn foo<'a, 'b>(&'a self, x: &'b i32) -> impl Future<Output = i32> + 'b {
   |                                                                       ++++

This is an RPITIT so the lifetime should not have to appear in the bounds to be captured. As you might expect, adding the + 'b leads to a different error, and using the Captures trick does not work either (in fact, it is quite a mess of surprising errors: playground).

Should this compile?

Diagnostics aside, it's understandable that this wouldn't compile, because the compiler doesn't know what to do with the extra 'dynosaur lifetime in ErasedMyTrait::foo. It would have to generate an intersection lifetime to get the full return type of that method (the 'dynosaur in Pin<Box<dyn Future<Output = i32> + 'dynosaur>>.

Adding a lifetime that corresponds to 'dynosaur to the original trait does the trick:

trait MyTrait {
    fn foo<'a, 'b, 'd>(&'a self, x: &'b i32) -> impl Future<Output = i32>
    where 'a: 'd, 'b: 'd;
}

As I alluded to above, I don't think have we to know which lifetime 'dynosaur corresponds to to know that the original program is sound. Representing it as the intersection of 'a and 'b should be enough to see that our Pin<Box<dyn Future>> satisfies -> impl Future in the signature of the method we're implementing.

Through some magic it isn't necessary if we use async fn directly; this impl compiles fine:

impl<T: ErasedMyTrait> MyTrait for DynMyTrait<T> {
    async fn foo<'a, 'b>(&'a self, x: &'b i32) -> i32 {
        self.ptr.foo(x).await
    }
}

I'm pretty much expecting to hear that this is too hard to support now, but the async fn chicanery gives me hope that it can be supported sooner.

Meta

rustc version:

1.82.0-nightly
(2024-08-05 e57f3090aec33cdbf660)
@tmandry tmandry added the C-bug Category: This is a bug. label Aug 6, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 6, 2024
@tmandry tmandry added A-diagnostics Area: Messages for errors, warnings, and lints A-traits Area: Trait system A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` A-trait-objects Area: trait objects, vtable layout T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue. and removed F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 6, 2024
@tmandry
Copy link
Member Author

tmandry commented Aug 6, 2024

cc @spastorino @compiler-errors

@compiler-errors compiler-errors self-assigned this Aug 6, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Aug 6, 2024

Yeah, you're right that this is not possible to fix. Opaques can't capture intersection lifetimes, since they're local to the body of the function and not represented by any of the params on the function that defines the RPITIT -- this is what the lifetime 'dynosaur is in this example is, on the caller side.

Sorry that the diagnostic does not make this clear this is the problem -- it shouldn't be saying "captures the lifetime 'b as defined here" because if we were trying to capture the lifetime 'b, then we wouldn't have a problem here. 'b is indeed captured by opaque by default since RPITITs capture all in scope lifetimes.

The fact that the async example works is because that async {} block (coming out of the async desugaring) doesn't actually have anything to do with object type. All it does is capture upvars (namely, &'a self and x: &'b i32, which are OK because 'a and 'b are captured by the RPITIT) -- the object type that we end up awaiting is interior to the async block, so we don't care about it for the purposes of outlives.

I'll look into making the diagnostic less misleading, though.

@compiler-errors
Copy link
Member

compiler-errors commented Aug 6, 2024

#128753 suppresses the part that says "captures the lifetime 'b" and "add + 'b", since that's just inaccurate. After that PR, it says "captures '_", which is vague, but at least it's not literally lying about what region is being captured.

@tmandry
Copy link
Member Author

tmandry commented Aug 6, 2024

Yeah, you're right that this is not possible to fix. Opaques can't capture intersection lifetimes, since they're local to the body of the function and not represented by any of the params on the function that defines the RPITIT

That's an implementation choice though, isn't it? At least in my conceptual framework, an intersection lifetime is purely a function of those input lifetimes. In other words, this refinement should be possible.

trait MyTrait {
    fn foo<'a, 'b>(&'a self, x: &'b i32) -> impl Future<Output = i32>;
}

impl MyTrait for Foo {
    fn foo<'a, 'b>(&'a self, x: &'b i32) -> Pin<Box<dyn Future<Output = i32> + intersection<'a, 'b>>> { todo!() }
}

Happy to be told either that my conceptual framework is wrong or that it is theoretically sound but we aren't ready to represent this anytime soon.. just trying to understand what is meant by "impossible" here :)

The fact that the async example works is because that async {} block (coming out of the async desugaring) doesn't actually have anything to do with object type. All it does is capture upvars (namely, &'a self and x: &'b i32, which are OK because 'a and 'b are captured by the RPITIT) -- the object type that we end up awaiting is interior to the async block, so we don't care about it for the purposes of outlives.

Thanks for that explanation. I bet we can emulate this in the proc macro @spastorino and I have been working on.

I'll look into making the diagnostic less misleading, though.

Thank you!

@compiler-errors
Copy link
Member

compiler-errors commented Aug 6, 2024

At least in my conceptual framework, an intersection lifetime is purely a function of those input lifetimes. In other words, this refinement should be possible.

Well, the real issue here is that the opaque type inference algorithm relies on being able to map a hidden type of an opaque back onto the lifetime parameters that the opaque captures. When we have some:

type RPIT<'a, 'b> = Pin<Box<dyn Future<Output = i32> + '1>>
//        ^^^^^^ These are the captures of the opaque

...and all we know from the environment is that 'a: '1 and 'b: '1, that does not give us sufficient information to actually map '1 to one of those regions (since 'a and 'b are totally unrelated regions), which we must for opaque types to work in Rust.

As an aside, you can observe that this is fixed if we added 'a: 'b or 'b: 'a to our method signature (even though I totally agree that's an arbitrary restriction to have), since saying one region outlives the other gives us an obvious choice of a least-upper-bound.

I don't think calling this an implementation choice is particularly useful here, since it overlooks the things that makes it hard in the implementation. I could call anything that Rust does an implementation detail, but the fact here is that Rust does not have first-class intersection regions in its type system (like, notice that you have to go out of your way to invent a new syntax to represent a region to put into the dyn Future).

I bet we can emulate this in the proc macro spastorino and I have been working on.

I don't believe this can be emulated in general, for the same reasons that futures cannot always be lowered into state machines because they can feature existential inner regions that can't be named. Specifically, if you wrote some:

enum ManuallyDesugaredStateMachine<'a, 'b> {
    State1 {
        self_: &'a Foo,
        x: &'b i32,
    },
    State2 {
        fut: Pin<Box<dyn Future<Output = i32> + /* what do we put here? */>>,
    },
    Done,
}

Then you'd either need to introduce some interesection region directly to the enum like 'd where 'a: 'd, 'b: 'd which brings us exactly back to where we started, or we would need an anonymous existential lifetime which does not exist right now in the type system.

@tmandry
Copy link
Member Author

tmandry commented Aug 6, 2024

I could call anything that Rust does an implementation detail, but the fact here is that Rust does not have first-class intersection regions in its type system (like, notice that you have to go out of your way to invent a new syntax to represent a region to put into the dyn Future).

Yeah this matches my understanding. I don't think "implementation detail" was a good choice of words. What I meant was that there's nothing fundamentally unsound about this pattern and a language like Rust could support it. But I get that I am totally glossing over its interaction with other features in the type system.

I don't believe this can be emulated in general, for the same reasons that futures cannot always be lowered into state machines because they can feature existential inner regions that can't be named.

I meant with unsafe code. In this case I would stick a 'static there, restrict visibility, and list all input lifetimes as parameters of the owning struct/enum to prevent use after free. I think this works? https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7f276c43d93fdc257576c8dd60c0ab06

@compiler-errors
Copy link
Member

'static may work, but you may get mysterious borrowck errors.

@compiler-errors
Copy link
Member

compiler-errors commented Aug 6, 2024

What I meant was that there's nothing fundamentally unsound about this pattern.

I don't believe that there's anything fundamentally unsound about this.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Aug 7, 2024
…=spastorino

Don't arbitrarily choose one upper bound for hidden captured region error message

You could argue that the error message is objectively worse, even though it's more accurate. I guess we could also add a note explaining like "cannot capture the intersection of two regions" or something, though I'm not sure if that is confusing due to being totally technical jargon.

This addresses the fact that rust-lang#128752 says "add `+ 'b`" even though it does nothing to fix the issue. It doesn't fix the issue's root cause, though.

r? `@spastorino`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 7, 2024
Rollup merge of rust-lang#128753 - compiler-errors:arbitrary-upper, r=spastorino

Don't arbitrarily choose one upper bound for hidden captured region error message

You could argue that the error message is objectively worse, even though it's more accurate. I guess we could also add a note explaining like "cannot capture the intersection of two regions" or something, though I'm not sure if that is confusing due to being totally technical jargon.

This addresses the fact that rust-lang#128752 says "add `+ 'b`" even though it does nothing to fix the issue. It doesn't fix the issue's root cause, though.

r? `@spastorino`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. A-trait-objects Area: trait objects, vtable layout A-traits Area: Trait system C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants