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

TAIT regression: requires new lifetime bounds #95922

Closed
aliemjay opened this issue Apr 11, 2022 · 8 comments · Fixed by #106038
Closed

TAIT regression: requires new lifetime bounds #95922

aliemjay opened this issue Apr 11, 2022 · 8 comments · Fixed by #106038
Assignees
Labels
C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` requires-nightly This issue requires a nightly compiler in some way. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@aliemjay
Copy link
Member

aliemjay commented Apr 11, 2022

Edit: The original example was indeed unsound so it's thankfully rejected now. See this comment for a sound example that gets rejected.

Original unsound code

#![feature(type_alias_impl_trait)]
use core::future::Future;

trait Service<'a, Req> {
    type Future: Future;
    fn call(req: &'a Req) -> Self::Future;
}

impl<'a, Req> Service<'a, Req> for u8 {
    type Future = impl Future;
    fn call(req: &'a Req) -> Self::Future {
        async move { let x = req; }
    }
}

(Playground)

This code used to compile fine prior to #95519. It now requires an explicit lifetime bound Req: 'a to the impl.

Unfortunately, adding a lifetime bound may not be an option when used with HRTBS due to limitations like #95921. This caused a major breakage on my part.

Meta

regressed in the nightly version 1.62.0-nightly (f4a7ce997 2022-04-08)

Error Output

   Compiling rust-ci v0.1.0 (/home/runner/work/rust-ci/rust-ci)
error[E0309]: the parameter type `Req` may not live long enough
Error:   --> src/lib.rs:10:19
   |
10 |     type Future = impl Future;
   |                   ^^^^^^^^^^^
   |
   = help: consider adding an explicit lifetime bound `Req: 'a`...
   = note: ...so that the reference type `&'a Req` does not outlive the data it points at

For more information about this error, try `rustc --explain E0309`.
error: could not compile `rust-ci` due to previous error

@aliemjay aliemjay added the C-bug Category: This is a bug. label Apr 11, 2022
@aliemjay aliemjay changed the title TAIT regression: requires unnecessary explicit lifetime bound TAIT regression: requires new lifetime bounds Apr 11, 2022
@aliemjay
Copy link
Member Author

I thought this breakage is an unnecessary side effect to the PR, but this maybe the very unsound case that the PR tries to fix 😅 .

I'd appreciate a comment from @oli-obk @compiler-errors.

If it is unsound, wouldn't that make the following unsound as well?

fn call_service<'a, Req>(req: &'a Req) -> impl Future {
  async move { let x = req; }
}

@oli-obk
Copy link
Contributor

oli-obk commented Apr 11, 2022

The problem is a bit roundabout.

Basically there is an implicit Req: 'a on your method, so

impl<'a, Req> Service<'a, Req> for u8 {
    type Future = impl Future;
    fn call(req: &'a Req) -> Self::Future {
        async move { let x = req; }
    }
}

is in fact

impl<'a, Req> Service<'a, Req> for u8 {
    type Future = impl Future;
    fn call(req: &'a Req) -> Self::Future where Req: 'a {
        async move { let x = req; }
    }
}

(which is implied from &'a Req).

Now, TAIT desugars to putting a &'a Req into the associated type Future, but unlike the method, this type does not have this bound. So you get an error telling you to add the bound to the trait impl.

Ideally this would just be a where bound on the associated type. With GATs you can move the lifetime to the associated items, removing them from the trait, but I'm not sure if the where bound situation has been resolved already, I guess try adding where Req: 'a to the assoc type in your trait definition and the impl as well.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 11, 2022

If it is unsound, wouldn't that make the following unsound as well?

Nope, that case is easier, because the implicit Req: 'a (implied from &'a Req) exists for the desugared type alias, too.

@aliemjay
Copy link
Member Author

Thanks. So to conclude, this use case is indeed unsound and I'm happy it's now rejected.

One problem though I faced while redesigning the trait, TAIT now seems at a disadvantage to writing the full type:

Given the following example:

trait Service<Req> {
    type Output;
    fn call(req: Req) -> Self::Output;
}

impl<'a, Req> Service<&'a Req> for u8 {
    type Output= impl Copy;
    fn call(req: &'a Req) -> Self::Output {
        req
    }
}

This still fails and requires the explicit bound Req: 'a despite that, without TAIT, It is accepted and, I believe, is correct since the bound is implied by the trait parameter:

impl<'a, Req> Service<&'a Req> for u8 {
    type Output= &'a Req;
    fn call(req: &'a Req) -> Self::Output {
        req
    }
}

This is really a deal-breaker for using TAIT because of #95921

Do you think this is an easy problem and we can expect a fix in the near future?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 11, 2022

Yea, so since your service trait now has &'a Req and not 'a, Req as parameters, this should indeed pass, as that should imply Req: 'a. (This is also the sound case I noticed in the PR that now needed more bounds).

I'll fix this, but I'm going on vacation end of the week, so you may be in for a bit of a wait

@oli-obk oli-obk self-assigned this Apr 11, 2022
@oli-obk oli-obk added F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` fixed-by-chalk labels Apr 11, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Apr 11, 2022

While digging into this I noticed that -Zchalk will immediately fix this, because it does exactly what I wanted to do. But... just enabling this unconditionally (and not just in chalk), causes libcore to stop compiling 😱

Pretty sure I know why, too. This is gonna be a rabbit hole

@oli-obk
Copy link
Contributor

oli-obk commented Apr 11, 2022

Side note:

#![feature(type_alias_impl_trait)]
#![feature(generic_associated_types)]

trait Service<Req> {
    type Output<'x> where Req: 'x;
    fn call<'y>(req: &'y Req) -> Self::Output<'y>;
}

impl<Req> Service<Req> for u8 {
    type Output<'b> = impl Copy where Req: 'b;
    fn call<'c>(req: &'c Req) -> Self::Output<'c> {
        req
    }
}

may be more what you are looking for, but without knowing more about your use case, I can't be sure. (This issue requires solving regardless, but using a different scheme may unblock you)

@aliemjay
Copy link
Member Author

Unfortuanltely, I hit another blocker when using GATs as recommended:

#![feature(generic_associated_types)]
#![feature(type_alias_impl_trait)]
use core::ops::Deref;

trait Service3<Req> {
    type Output<'a> where Req: 'a;
    fn call(req: &mut Req) -> Self::Output<'_>;
}

struct WebReq<'s, S>(&'s S);

impl<'s, S> Service3<WebReq<'s, S>> for u8 {
    type Output<'a> = impl Deref where WebReq<'s, S>: 'a;
    fn call<'a>(req: &'a mut WebReq<'s, S>) -> Self::Output<'a> {
        req
    }
}

This requires the bound S: 's, but when added to the associated type rustc complains:

`impl` associated type signature for `Output` doesn't match `trait` associated type signature

So again adding the bound to the impl seems to be the only option.

@inquisitivecrystal inquisitivecrystal added requires-nightly This issue requires a nightly compiler in some way. T-types Relevant to the types team, which will review and decide on the PR/issue. labels Jun 4, 2022
@oli-obk oli-obk moved this from Todo to Can do after stabilization in type alias impl trait stabilization Oct 5, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue May 11, 2023
use implied bounds when checking opaque types

During opaque type inference, we check for the well-formedness of the hidden type in the opaque type's own environment, not the one of the defining site, which are different in the case of TAIT.

However in the case of associated-type-impl-trait, we don't use implied bounds from the impl header. This caused us to reject the following:
```rust
trait Service<Req> {
    type Output;
    fn call(req: Req) -> Self::Output;
}

impl<'a, Req> Service<&'a Req> for u8 {
    type Output= impl Sized; // we can't prove WF of hidden type  `WF(&'a Req)` although it's implied by the impl
    //~^ ERROR type parameter Req doesn't live long enough
    fn call(req: &'a Req) -> Self::Output {
        req
    }
}
```

although adding an explicit bound would make it pass:
```diff
- impl<'a, Req> Service<&'a Req> for u8 {
+ impl<'a, Req> Service<&'a Req> for u8  where Req: 'a, {
```

I believe it should pass as we already allow the concrete type to be used:
```diff
impl<'a, Req> Service<&'a Req> for u8 {
-    type Output= impl Sized;
+    type Output= &'a Req;
```

Fixes rust-lang#95922

Builds on rust-lang#105982

cc `@lcnr` (because implied bounds)

r? `@oli-obk`
@bors bors closed this as completed in 341d6df May 12, 2023
@github-project-automation github-project-automation bot moved this from Can do after stabilization to Done in type alias impl trait stabilization May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` requires-nightly This issue requires a nightly compiler in some way. T-types Relevant to the types team, which will review and decide on the PR/issue.
3 participants