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

Properly support LUB for ReErased #102943

Closed

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Oct 12, 2022

This is needed to properly check obligations on opaque types, since instead of returning ReEmpty, we now return ReErased for unconstrained regions in MIR borrowck.

This shouldn't be unsafe for now, but it does kinda risk future bugs if we accidentally allow ReEmpty to leak into other inference contexts, since LUB would cause an ICE that is very obvious to see otherwise. I'm not really certain how to fix this other than reintroducing a ReRootEmpty region for opaque type inference that properly represents an unconstrained opaque lifetime..

cc @oli-obk and @jackh726
not sure which of y'all to request review from, so I flipped a coin and it landed on heads. therefore r? @oli-obk, feel free to reassign though.

Fixes #102649
Fixes #102510

Originally caused by #98559

This is needed to properly check obligations on opaque types, since
instead of returning ReEmpty, we now return ReErased for unconstrained
regions in MIR borrowck.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 12, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2022
@compiler-errors

This comment was marked as duplicate.

@jackh726
Copy link
Member

Yeah this is probably the easiest way out of this.

I don't think it's a "proper" fix though, but I'm not sure what that would look like. As you said, this almost merits a ReRootEmpty on its own. I'm guessing returning a ReVar here doesn't work, probably because we complain about unconstrained variables?

@compiler-errors
Copy link
Member Author

I'm guessing returning a ReVar here doesn't work

Yeah, we can't do that, since this needs to be a lifetime that we can return in the type_of query, so it can't be associated to a specific inference context, for example.

Other choices like using ReStatic for these unconstrained regions causes outlives errors, and nothing else seems to be obviously correct. It would take a lot more modifications, but we could return a binder or something, but that would be a much larger scale change.

@compiler-errors compiler-errors 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 12, 2022
Copy link
Member

@jackh726 jackh726 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 a FIXME comment added referencing the issue. I think its worth merging this now, but it's not the right long-term fix. I can also see a case to actually escalate a bit more and open an issue to actually track a "proper" fix, but I'm not sure.

@aliemjay
Copy link
Member

aliemjay commented Oct 12, 2022

I'm not really comfortable with this. I believe a more acceptable short-term fix is to instantiate ReErased with fresh region variables. This way the hack would be more localized.

For the long-term fix I imagine wrapping the hidden type with an existential binder and adding sufficient assertions that the existential variables only appear in closure/generator parent substs.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 13, 2022

Yea, I'd also prefer to limit this to just opaque types, so instantiating the ReErased to a new inference var after getting the hidden type in

let hidden_type = tcx.bound_type_of(def_id.to_def_id()).subst(tcx, substs);

@compiler-errors compiler-errors deleted the reempty-lub branch October 13, 2022 16:15
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 13, 2022
…li-obk

replace ReErased with fresh region vars in opaque types

See inline comments.

Prior art rust-lang#102943. cc `@compiler-errors` `@oli-obk`

Fixes rust-lang#100267
Fixes rust-lang#101940
Fixes rust-lang#102649
Fixes rust-lang#102510
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 13, 2022
…li-obk

replace ReErased with fresh region vars in opaque types

See inline comments.

Prior art rust-lang#102943. cc ``@compiler-errors`` ``@oli-obk``

Fixes rust-lang#100267
Fixes rust-lang#101940
Fixes rust-lang#102649
Fixes rust-lang#102510
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
6 participants