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 is allowed to name any in-scope lifetime parameter, unlike inherent RPIT methods #112194

Closed
tmandry opened this issue Jun 2, 2023 · 2 comments · Fixed by #114489
Closed
Labels
F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` requires-nightly This issue requires a nightly compiler in some way. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Jun 2, 2023

I noticed (while responding to this comment by @aliemjay) that there's an inconsistency in how we handle returning Self if it captures lifetimes. For both inherent methods and RPITIT, if you write -> impl Trait and then return self on an impl for a type that containing a lifetime, we give you an error:

struct Foo<'a>(&'a str);

// ERROR: hidden type for `impl Sized` captures lifetime that does not appear in bounds
impl<'a> Foo<'a> {
    fn foo(self) -> impl Sized { self }
}

trait Trait<'a> {
    fn bar(self) -> impl Sized;
}

// ERROR: hidden type for `impl Sized` captures lifetime that does not appear in bounds
impl<'a> Trait<'a> for &'a i32 {
    fn bar(self) -> impl Sized { self }
}

However, if you write -> Self on an implementation of a trait method that's written with -> impl Trait, there is no error (though this would require #[refine] with RFC 3245):

// OK?
impl<'a> Trait<'a> for &'a u32 {
    fn bar(self) -> Self { self }
}

playground

First, this inconsistency is weird from a user perspective because it seems like the capture rules say what lifetimes your hidden type is allowed to reference, and such a property can only be strengthened by an implementation (by referencing fewer, or longer-lived, lifetimes than the trait allows), never weakened. But here, the implementation specifies it a concrete type which allows it to name additional lifetimes.

If we could I we would say we should probably accept all of these examples and consider Self to be a type parameter in how we interpret RFC 1951. That said, I don't think it's possible to change today given that you can depend on the return type not referencing the lifetime.

Given that, we should probably apply the same restriction to RPITIT for the sake of consistency and not allow it to name any lifetime parameters as we assume it can today (including through Self). Though this might create other issues I'm not thinking of.

cc @compiler-errors

@tmandry tmandry added T-lang Relevant to the language team, which will review and decide on the PR/issue. requires-nightly This issue requires a nightly compiler in some way. F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` labels Jun 2, 2023
tmandry added a commit to tmandry/rfcs that referenced this issue Jun 2, 2023
@tmandry tmandry added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 15, 2023
@compiler-errors
Copy link
Member

This is even more problematic when considering you can capture early-bound lifetimes not named by the opaque:

#![feature(async_fn_in_trait)]
#![feature(return_position_impl_trait_in_trait)]

use std::future::Future;

pub trait Trait {
    fn foo<'a: 'a>(&'a self) -> impl Future<Output = ()>;
}

pub struct S;

impl Trait for S {
    async fn foo<'s: 's>(&'s self) -> () {}
}

fn main() {}

but not late-bound ones1:

#![feature(async_fn_in_trait)]
#![feature(return_position_impl_trait_in_trait)]
use std::future::Future;

pub trait Trait {
    fn foo<'a>(&'a self) -> impl Future<Output = ()>;
}

pub struct S;

impl Trait for S {
    async fn foo<'s>(&'s self) -> () {}
    //~^ ERROR `impl` item signature doesn't match `trait` item signature
}

fn main() {}

Footnotes

  1. Which for technical reason we should never be able to capture, since we literally don't have a way of desugaring that as a GAT.

@compiler-errors
Copy link
Member

compiler-errors commented Jun 26, 2023

Perhaps we should consider example (1.) from my previous comment to be a refinement since it references a function lifetime parameter that isn't mentioned by the opaque, and deny it.

What to do with self types is a bit more delicate, as you noted above, and I have no idea what the best choice is there.

TaKO8Ki added a commit to TaKO8Ki/rust that referenced this issue Jul 1, 2023
…ures, r=oli-obk

Error when RPITITs' hidden types capture more lifetimes than their trait definitions

This implements a stricter set of captures rules for RPITITs. They now may only capture:
1. Lifetimes from the impl header (both the self type and any trait substs -- we may want to restrict just to the self type's lifetimes, but the PR makes that easy to do, too)
2. Lifetimes mentioned by the `impl Trait` in the trait method's definition.

Namely, they may not mention lifetimes from the method (early or late) that are not mentioned in the `impl Trait`.

cc rust-lang#105258 which I think was trying to do this too, though I'm not super familiar with what exactly differs from that or why that one was broken.
cc rust-lang#112194 (doesn't fix this issue per se, because it's still an open question, but I think this is objectively better than the status quo, and gets us closer to resolving that issue.)

Technically is a fix for the ICE in rust-lang#108580, but it turns that issue into an error now. We can decide separately whether or not nested RPITITs should capture lifetimes from their parents.

r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 1, 2023
…ures, r=oli-obk

Error when RPITITs' hidden types capture more lifetimes than their trait definitions

This implements a stricter set of captures rules for RPITITs. They now may only capture:
1. Lifetimes from the impl header (both the self type and any trait substs -- we may want to restrict just to the self type's lifetimes, but the PR makes that easy to do, too)
2. Lifetimes mentioned by the `impl Trait` in the trait method's definition.

Namely, they may not mention lifetimes from the method (early or late) that are not mentioned in the `impl Trait`.

cc rust-lang#105258 which I think was trying to do this too, though I'm not super familiar with what exactly differs from that or why that one was broken.
cc rust-lang#112194 (doesn't fix this issue per se, because it's still an open question, but I think this is objectively better than the status quo, and gets us closer to resolving that issue.)

Technically is a fix for the ICE in rust-lang#108580, but it turns that issue into an error now. We can decide separately whether or not nested RPITITs should capture lifetimes from their parents.

r? ``@oli-obk``
@joshtriplett joshtriplett removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 8, 2023
@bors bors closed this as completed in 93dd620 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` requires-nightly This issue requires a nightly compiler in some way. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Development

Successfully merging a pull request may close this issue.

3 participants