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

the type of ProjectionElem::Field is not correct wrt subtyping #96514

Closed
lcnr opened this issue Apr 28, 2022 · 7 comments · Fixed by #103880 or #107969
Closed

the type of ProjectionElem::Field is not correct wrt subtyping #96514

lcnr opened this issue Apr 28, 2022 · 7 comments · Fixed by #103880 or #107969
Assignees
Labels
A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Apr 28, 2022

struct Foo<T>(T); // `T` is covariant.

fn foo<'b>(x: Foo<for<'a> fn(&'a ())>) {
    let Foo(y): Foo<fn(&'b ())> = x;
}

fn main() {}

should compile but errors with

error[E0308]: mismatched types
 --> src/main.rs:4:13
  |
4 |     let Foo(y): Foo<fn(&'b ())> = x;
  |             ^ one type is more general than the other
  |
  = note: expected fn pointer `for<'a> fn(&'a ())`
             found fn pointer `fn(&())`

looking at the mir of foo it becomes clear why that happens:

fn foo(_1: Foo<for<'a> fn(&'a ())>) -> () {
    debug x => _1;                       // in scope 0 at src/main.rs:3:12: 3:13
    let mut _0: ();                      // return place in scope 0 at src/main.rs:3:40: 3:40
    let _2: fn(&()) as UserTypeProjection { base: UserType(0), projs: [Field(field[0], ())] }; // in scope 0 at src/main.rs:4:13: 4:14
    scope 1 {
        debug y => _2;                   // in scope 1 at src/main.rs:4:13: 4:14
    }

    bb0: {
        AscribeUserType(_1, +, UserTypeProjection { base: UserType(1), projs: [] }); // scope 0 at src/main.rs:4:17: 4:32
        StorageLive(_2);                 // scope 0 at src/main.rs:4:13: 4:14
        _2 = (_1.0: fn(&()));            // scope 0 at src/main.rs:4:13: 4:14
        _0 = const ();                   // scope 0 at src/main.rs:3:40: 5:2
        StorageDead(_2);                 // scope 0 at src/main.rs:5:1: 5:2
        return;                          // scope 0 at src/main.rs:5:2: 5:2
    }
}

notice that in _2 = (_1.0: fn(&())); the type of _1.0 is actually for<'a> fn(&'a ()).

Possible ways to fix this are:

  • correctly deal with subtyping when sanitizing field projections, note that they cannot just always be covariant.
  • change the type of the field to be the actual one during mir building, so that subtyping actually happens at the assignment
  • always lazily recompute the type of the field instead of storing it in the mir
@lcnr lcnr added the C-bug Category: This is a bug. label Apr 28, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Apr 28, 2022

@rustbot claim

intend to work on this myself

@lcnr
Copy link
Contributor Author

lcnr commented Apr 29, 2022

also add a test for

struct Foo<T>(T); // `T` is covariant.

fn foo<'b>(x: Foo<Foo<for<'a> fn(&'a ())>>) {
    let Foo(Foo(y)): Foo<Foo<fn(&'b ())>> = x;
}

bors added a commit to rust-lang-ci/rust that referenced this issue May 21, 2022
correctly deal with user type ascriptions in pat

supersedes rust-lang#93856

`thir::PatKind::AscribeUserType` previously resulted in `CanonicalUserTypeAnnotations` where the inferred type already had a subtyping relation according to `variance` to the `user_ty`.

The bug can pretty much be summarized as follows:

- during mir building
  - `user_ty -> inferred_ty`: considers variance
  - `StatementKind::AscribeUserType`: `inferred_ty` is the type of the place, so no variance needed
- during mir borrowck
  - `user_ty -> inferred_ty`: does not consider variance
  - `StatementKind::AscribeUserType`: applies variance

This mostly worked fine. The lifetimes in `inferred_ty` were only bound by its relation to `user_ty` and to the `place` of `StatementKind::AscribeUserType`, so it doesn't matter where exactly the subtyping happens.

It does however matter when having higher ranked subtying. At this point the place where the subtyping happens is forced, causing this mismatch between building and borrowck to result in unintended errors.

cc rust-lang#96514 which is pretty much the same issue

r? `@nikomatsakis`
@lcnr lcnr removed their assignment Oct 21, 2022
@lcnr lcnr added E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) T-types Relevant to the types team, which will review and decide on the PR/issue. labels Oct 21, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Oct 21, 2022

change the type of the field to be the actual one during mir building, so that subtyping actually happens at the assignment

That's imo the correct solution. Open to mentor someone here, ideally with some experience with working with ty::Ty and mir building.

If you're interested and need help, contact me on zulip and we can either chat about it there or on zoom.

@b-naber
Copy link
Contributor

b-naber commented Oct 21, 2022

@rustbot claim

@bors bors closed this as completed in 03770f0 Dec 16, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 19, 2022
Revert rust-lang#103880 "Use non-ascribed type as field's type in mir"

This PR prepares a revert for rust-lang#103880 to fix rust-lang#105809, rust-lang#105881, rust-lang#105886 and others (like the duplicates of the first one), in case an actual fix can't get done today.

I've also added the MCVE from rust-lang#105809. There is no MCVE for the rust-lang#105881 and rust-lang#105886 ICEs yet however, so there are no tests for them here, although we'll need one before relanding the original changes.

Were this PR to land, it would also reopen rust-lang#96514 as it was fixed by the original PR.

Opening as draft to allow time for a possible fix.

r? `@jackh726`
@trevyn
Copy link
Contributor

trevyn commented Dec 20, 2022

This needs a reopen since 696563e landed.

@lqd
Copy link
Member

lqd commented Dec 24, 2022

Reopening, as I don't think @RalfJung expected to close this with the rust-lang/miri#2738 miri rustup.

@lqd lqd reopened this Dec 24, 2022
@RalfJung
Copy link
Member

RalfJung commented Dec 24, 2022

Yeah that was some rustc commit with a "Fixes #96514" in it being mirrored. This might happen a few more times, since github will re-execute these commit commands when commits move around repositories.

@bors bors closed this as completed in e7eaed2 Feb 20, 2023
RalfJung pushed a commit to RalfJung/miri that referenced this issue Feb 21, 2023
Use covariance on type relations of field projection types if possible

It's fine to use covariance here unless we're in a mutating context.

Fixes rust-lang/rust#96514

Supersedes rust-lang/rust#105958

r? `@lcnr`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
Use non-ascribed type as field's type in mir

Fixes rust-lang/rust#96514

r? `@lcnr`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
Use covariance on type relations of field projection types if possible

It's fine to use covariance here unless we're in a mutating context.

Fixes rust-lang/rust#96514

Supersedes rust-lang/rust#105958

r? `@lcnr`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Use non-ascribed type as field's type in mir

Fixes rust-lang/rust#96514

r? `@lcnr`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Use covariance on type relations of field projection types if possible

It's fine to use covariance here unless we're in a mutating context.

Fixes rust-lang/rust#96514

Supersedes rust-lang/rust#105958

r? `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
5 participants