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 register lifetime constraints from opaque types from canonical queries called from nll #123035

Closed

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 25, 2024

Unblocks #122077

in #122077 we had the fun situation where a Opaque<'1, '2> got a hidden type Opaque<'3, '4> registered (before that situation just didn't occur). Since query_response_instantiation_guess always equated opaque types with their hidden type (to let the type relation logic reregister the hidden type if applicable), during #122077 that started registering lifetime relations in the InferCtxt. Nll needs to take them out and tell borrowck about them, but that never happened, so borrowck started ICEing.

My next step was to stop equating opaque types to their hidden type, as we now always know that we can just register the hidden type in the query caller's InferCtxt. But then the opaque type's lifetimes never got resolved, even though the hidden type's lifetimes got resolved. That then errors out because there's an opaque type whose generic params do not refer to generic params of the item currently being borrowcked.

So my solution for now is to explicitly register lifetime relations in nll.

@rustbot
Copy link
Collaborator

rustbot commented Mar 25, 2024

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 25, 2024
Comment on lines +287 to +294
_ => {
if a != b {
span_bug!(
cause.span(),
"self-referential opaque type can only differ in lifetime parameters, but got {a} != {b}"
)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is super hacky, but at least it's a high signal ICE instead of some hard-to-debug issues. We also can't work on the FIXME before we get rid of DefiningAnchor, so I think we should just roll with this for now and address it later.

Comment on lines +417 to +427
for (key, hidden) in opaques {
obligations.extend(
self.at(cause, param_env)
.eq(
DefineOpaqueTypes::Yes,
Ty::new_opaque(self.tcx, key.def_id.into(), key.args),
hidden,
)?
.obligations,
)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all non-nll query callers still need their opaque types registered. After #122077 this will become a simple register_hidden_type call

}
} else {
obligations.extend(
self.handle_opaque_type(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After #122077 this will become a register_hidden_type

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 25, 2024

hmm... I am only now realizing that register_hidden_type equates the current hidden type with any previously registered ones. Which can then again cause new lifetime obligations to show up.

So maybe this isn't the way to go after all. But it also feels very hacky to just wrap this in another scrape_region_constraints. That just seemed super hacky, too. As I'm scraping regions from a call to instantiate_nll_query_response_and_region_obligations, which also writes to a QueryRegionConstraints.

@oli-obk oli-obk changed the title Opaques from canonical queries Correctly register lifetime constraints from opaque types from canonical queries called from nll Mar 25, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 25, 2024

r? @compiler-errors

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 25, 2024

Yea no, this doesn't fix everything and is horrible on top of it. I found a simpler solution that I'll do directly in #122077

@oli-obk oli-obk closed this Mar 25, 2024
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-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.

4 participants