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

RPIT allows defining use with invalid args #111935

Closed
aliemjay opened this issue May 25, 2023 · 3 comments · Fixed by #116891
Closed

RPIT allows defining use with invalid args #111935

aliemjay opened this issue May 25, 2023 · 3 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. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@aliemjay
Copy link
Member

aliemjay commented May 25, 2023

Note: All examples in this post was fixed in #112842. However the issue persists for lifetime arguments. See this comment for more.


The following compiles since v1.62:

fn foo<T>() -> impl Sized {
    let _: () = foo::<u8>();
}

While the equivalent TAIT version doesn't compile because Opaque<u8> == u8 is not a valid defining use for Opaque (because it is ambiguous whether Opaque<T> = T or Opaque<T> = u8):

#![feature(type_alias_impl_trait)]

type Opaque<T> = impl Sized;
fn foo<T>() -> Opaque<T> {
    let _: () = foo::<u8>();
    //~^ ERROR expected generic type parameter, found `u8`
}

I believe that TAIT behavior is the correct one because it's maximally compatible with future changes. Keeping this behavior for RPIT can also result in some surprising errors:

fn foo<T>(val: T) -> impl Sized {
    let _: u8 = foo(0u8);
    val
    //~^ ERROR concrete type differs from previous defining opaque type use
}

This was stabilized (most likely unintentionally) in #94081. Cc @oli-obk.

See related code:

fn check_opaque_type_parameter_valid(

@aliemjay aliemjay added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. C-bug Category: This is a bug. labels May 25, 2023
@lcnr lcnr added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Jun 15, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 15, 2023
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 15, 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.
        (|| ())()

    }
}
```
@aliemjay aliemjay added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 19, 2023
@aliemjay aliemjay changed the title RPIT allows defining use with invalid substs RPIT allows defining use with invalid args Oct 19, 2023
@aliemjay
Copy link
Member Author

aliemjay commented Oct 19, 2023

The PR #112842 fixed this issue for type arguments. The same issue persists for lifetime arguments.

The following snippets are examples of invalid defining uses that should not compile:

// Lt indirection is necessary to make the lifetime of the function late-bound,
// in order to bypass some other bugs.
type Lt<'lt> = Option<*mut &'lt ()>;

// PASS!
#[cfg(case1)]
fn foo<'a>(_: Lt<'a>) -> impl Sized + 'a {
    let _: () = foo(Lt::<'static>::None);
    // invalid defining use: Opaque<'static> := ()
}

// ICE!
#[cfg(case2)]
fn foo<'a>(_: Lt<'a>) -> impl Sized + 'a {
    let _: () = foo(Lt::<'_>::None);
    // invalid defining use: Opaque<'_> := ()
}

// PASS!
#[cfg(case3)]
fn foo<'a, 'b>(_: Lt<'a>, _: Lt<'b>) -> impl Sized + 'a + 'b {
    let _: () = foo(Lt::<'a>::None, Lt::<'a>::None);
    // invalid defining use: Opaque<'a, 'a> := ()
    // because of the use of equal lifetimes in args
}

@lcnr
Copy link
Contributor

lcnr commented Oct 19, 2023

cc #113916

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. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants