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

"correctly" check opaque type lifetimes to be unique params #113916

Closed
lcnr opened this issue Jul 21, 2023 · 0 comments · Fixed by #116891
Closed

"correctly" check opaque type lifetimes to be unique params #113916

lcnr opened this issue Jul 21, 2023 · 0 comments · Fixed by #116891
Labels
A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Jul 21, 2023

see https://rust-lang.zulipchat.com/#narrow/stream/315482-t-compiler.2Fetc.2Fopaque-types/topic/non-defining.20uses.20due.20to.20equal.20lifetimes for more details. This did not get a "formal approval" yet but summarizes the previous discussion on zulip.

We currently don't check that the provided lifetimes are unique for RPIT and async function return values in fn check_opaque_type_parameter_valid. We do check them for TAIT. This currently results in somewhat inconsistent behavior:

#![feature(type_alias_impl_trait)]
trait Trait<'a, 'b> {}
impl Trait<'_, '_> for () {}

type Ok<'a, 'b> = impl Trait<'a, 'b>;
fn ok<'a: 'b, 'b: 'a>() -> Ok<'a, 'b> {
    ()
}

type Fail1<'a, 'b> = impl Trait<'a, 'b>;
fn fail1<'a: 'b, 'b: 'a>() -> Fail1<'a, 'b> {
    (|| {})() //~ ERROR non-defining opaque type use in defining scope
}

type Fail2<'a, 'b> = impl Trait<'a, 'b>;
fn fail2<'a, 'b>(x: &'a &'b &'a ()) -> Fail2<'a, 'b> {
    let _: &'a &'b &'a () = x; //~ ERROR non-defining opaque type use in defining scope
}

The issue here is that we don't have unique lifetimes to instantiate the TAITs here, as the lifetimes are required to be equal.

The reason fn ok works is because nll evaluates lifetime equality based on the constraint graph alone, ignoring the the environment bounds. This is a bug, not a feature.

In #112842 I check type and const arguments for defined RPITs and async returns to be unique params but ignore parameters. This is the case as otherwise the following example breaks

struct Type<'a>(&'a ());
impl<'a> Type<'a> {
    // `'b == 'a`
    fn do_stuff<'b: 'a>(&'b self) -> impl Trait<'a, 'b> {}
}

Because of the implied bound from the self type and the explicit 'b: 'a bound 'a and 'b are equal. This causes us to use the same region param multiple times while defining the RPIT. What we should instead do here is the following:

  • only allow duplicate lifetime params in the defining use iff the params of the opaque they instantiate are required to be equal by bounds of the opaque
  • this includes the implied WF bounds of their containing item, i.e. RPITs have the same bounds as the function defining them

This handling should allow us to check the region params for RPIT and async function return types, fixing buggy (but probably not unsound) behavior. It also allowsg the following code with TAIT:

#![feature(type_alias_impl_trait)]
trait Trait<'a, 'b> {}
impl Trait<'_, '_> for () {}

type Tait<'a: 'b, 'b: 'a> = impl Trait<'a, 'b>;
fn foo<'a>() -> Tait<'a, 'a> {}

Landing this change will require an FCP.

cc @rust-lang/initiative-impl-trait @aliemjay

related to #104477 #112841

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 21, 2023
@lcnr lcnr added F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` C-bug Category: This is a bug. labels Jul 21, 2023
@aliemjay aliemjay added A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. T-types Relevant to the types team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 21, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 14, 2023
check for non-defining uses of RPIT

This PR requires defining uses of RPIT and the async functions return type to use unique generic parameters as type and const arguments, (mostly) fixing rust-lang#111935. This changes the following snippet to an error (it compiled since 1.62):
```rust
fn foo<T>() -> impl Sized {
    let _: () = foo::<u8>(); //~ ERROR non-defining use of `impl Sized`
}
```
Since 1.62 we only checked that the generic arguments of opaque types are unique parameters for TAIT and ignored RPITs, so this PR changes the behavior here to be consistent.

For defining uses which do not have unique params as arguments it is unclear how the hidden type should map to the generic params of the opaque. In the following snippet, should the hidden type of `foo<T>::opaque` be `T` or `u32`.
```rust
fn foo<T>() -> impl Sized {
    let _: u32 = foo::<u32>();
    foo::<T>()
}
```
There are no crater regressions caused by this change.

---

The same issue exists for lifetime arguments which is not fixed by this PR, currently resulting in an ICE in mir borrowck (I wasn't able to get an example which didn't ICE, it might be possible):
```rust
fn foo<'a: 'a>() -> impl Sized {
    let _: &'static () = foo::<'static>();
    //~^ ICE opaque type with non-universal region substs
    foo::<'a>()
}
```
Fixing this for lifetimes as well is blocked on rust-lang#113916. Due to this issue, functions returning an RPIT with lifetime parameters equal in the region constraint graph would always result in an error, resulting in breakage found via crater: rust-lang#112842 (comment)
```rust
trait Trait<'a, 'b> {}
impl Trait<'_, '_> for () {}

struct Type<'a>(&'a ());
impl<'a> Type<'a> {
    // `'b == 'a`
    fn do_stuff<'b: 'a>(&'b self) -> impl Trait<'a, 'b> {
        // This fails as long there is something in the body
        // which adds the outlives constraints to the constraint graph.
        //
        // This is the case for nested closures.
        (|| ())()

    }
}
```
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 23, 2023
…2, r=<try>

rework opaque type region inference

fixes rust-lang#113971 Pass -> Error

fixes rust-lang#111906 ICE -> Pass
fixes rust-lang#110623 ==
fixes rust-lang#109059 ==

fixes rust-lang#112841 Pass -> Error

fixes rust-lang#110726 ICE->Error

fixes rust-lang#111935 Pass -> Error
fixes rust-lang#113916 ==

r? `@ghost`
@bors bors closed this as completed in 551abd6 Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. C-bug Category: This is a bug. F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` T-types Relevant to the types team, which will review and decide on the PR/issue.
Development

Successfully merging a pull request may close this issue.

3 participants