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

fix let _: ty = expr subtyping error #93856

Closed
wants to merge 7 commits into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Feb 10, 2022

fn uwu<'b>(func: for<'a> fn(&'a ())) {
    let _: fn(&'b ()) = func;
}

Placeholder patterns _ end up getting converted to AscribeUserType(_1, UserTypeProjection { base: UserType(1), projs: [] }) in the mir. This type ascription is covariant.

In fn check_user_type_annotations we previously checked the type annotations without considering variance, breaking the above example. As lifetimes in the inferred_type get erased they don't have any relation to other lifetimes in the body, so

struct Foo<'a>(&'a ());
fn uwu<'a>(f: Foo<'static>) {
    let _: Foo<'a> = f;
}

does not error because 'static in the inferred_ty gets erased and then replaced with an unbound inference variable.
Note that the following example wouldn't have any issues here either way as there is an implicit reborrow before the assignment.

fn uwu<'a>(v: &'static i32) {
    let _: &'a i32 = v;
}

With this PR we now eagerly consider variance in check_user_type_annotations and then always use invariance when encountering user type annotation statements in the mir.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 10, 2022
@rust-highfive
Copy link
Collaborator

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2022
@lcnr lcnr added the A-NLL Area: Non-lexical lifetimes (NLL) label Feb 10, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Feb 10, 2022

r? @rust-lang/wg-compiler-nll

@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr force-pushed the type-annotations-variance branch from c928e55 to e2f3bb5 Compare February 10, 2022 12:57
@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr marked this pull request as draft February 14, 2022 07:20
@bors
Copy link
Contributor

bors commented Feb 15, 2022

☔ The latest upstream changes (presumably #93148) made this pull request unmergeable. Please resolve the merge conflicts.

src/test/ui/hr-subtype/placeholder-pattern.rs Show resolved Hide resolved
compiler/rustc_middle/src/ty/context.rs Show resolved Hide resolved
ty,
variance.xform(ty::Variance::Contravariant),
inferred_ty,
Locations::All(span),
ConstraintCategory::BoringNoLocation,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to remember why we chose to use "type equality" here. I have a vague memory that there was a reason, but it seems there aren't any failing tests, at least.

@nikomatsakis
Copy link
Contributor

r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors rollup=never

@lcnr lcnr marked this pull request as ready for review February 18, 2022 12:44
@lcnr lcnr force-pushed the type-annotations-variance branch from b8d1a3f to 5ee4525 Compare February 18, 2022 12:44
@lcnr
Copy link
Contributor Author

lcnr commented Feb 18, 2022

@nikomatsakis this is now ready for review.

I think we always eq the inferred_ty with some other type while typechecking the mir, and that other type should be checked to be wf. So I don't think that we also have to emit a wf obligation for inferred_ty here.

@lcnr lcnr changed the title Fix let _: ty = expr subtyping error fix let _: ty = expr subtyping error Feb 18, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr force-pushed the type-annotations-variance branch from 8fe1b2c to c3a91a8 Compare February 18, 2022 14:48
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with fresh comments + tests

/// If this is covariant, then `inferred_ty` has to be a subtype of `user_ty`.
///
/// As the variance is already applied from `user_ty` to `inferred_ty`, all
/// other uses of `inferred_ty` should be invariant.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have a comment explaining:

let x: T = ...; // invariant
let _: T = ...; // covariant

src/test/ui/hr-subtype/placeholder-pattern.rs Show resolved Hide resolved
@lcnr
Copy link
Contributor Author

lcnr commented Mar 7, 2022

the following is still broken:

struct Foo<T>(T); // `T` is covariant.
fn foo<'b>(x: Foo<for<'a> fn(&'a ())>) {
    let Foo(y): Foo<fn(&'b ())> = x;
}

going to take some time to think about the bigger picture here

@bors
Copy link
Contributor

bors commented Mar 10, 2022

☔ The latest upstream changes (presumably #94059) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2022
@Dylan-DPC
Copy link
Member

@lcnr whats the status on this?

@lcnr
Copy link
Contributor Author

lcnr commented Apr 25, 2022

this needs a bigger change to the mir, intend to continue this work at some point in the near future. Can close this PR for now

@lcnr lcnr closed this Apr 25, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request 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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants