-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Enforce predicates which link bivariant opaque lifetimes #122311
Enforce predicates which link bivariant opaque lifetimes #122311
Conversation
If the parent lifetimes are relevant in any way then why they are bivariant in the first place? I do not expect the WF of an opaque type to constrain its bivariant lifetimes. A bit off-topic but the current state does not look right to me. |
obligations.push(traits::Obligation::new( | ||
tcx, | ||
cause.clone(), | ||
param_env, | ||
ty::ClauseKind::WellFormed(Ty::new_opaque(self.tcx, def_id, args).into()), | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this necessary? I'd expect that we require the opaque to be well-formed in the location which introduces it. We have to check the hidden type, as the structural recursion in wf.rs
does not check it, so there are types in "relevant positions" which are not wf.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing for me 🤔 I think I now get what's going on:
- calling
test::<'a>()
returnsOpaque<'a, 'a>
, we then assign this to a local of type()
which uses subtyping. - generalizing
Opaque<'a, 'a>
results inOpaque<'whatever, 'a>
because the first lifetime param is bivariant (would be the same if it were co/contravariant) - we then never check that
Opaque<'whatever, 'a>
is wf, leaving'whatever
unconstrained
That's also not quite right, as even changing Generalizer::regions
to always set self.has_unconstrained_ty_var
when creating a new region var does not fix this 🤔
let obligations = self.nominal_obligations(def_id, args); | ||
self.out.extend(obligations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very much in favor of this 😁 wanted to actually open a PR for that myself soon.
Can you merge the behavior of all ty::Alias
here?
This PR is unnecessary since (afaict) this bug is fixed by #116891, or at least it was fixed when I rebased and tested that one. Another fix (one with less fallout) would also be to just mark all TAIT args as invariant, since they're always captured anyways. |
During
predicates_of
, we install outlives obligations which link the bivariant lifetimes of an opaque to their invariant (captured) copies. During NLL, we don't enforce well-formedness of an opaque that we are trying to define, and therefore we never actually enforce that these lifetimes are equal. This means that we have strange errors (#122307), or ICEs (#121512) from unconstrained lifetimes that should be constrained.Fix this by requiring the opaque in
add_item_bounds_for_hidden_type
to be well-formed, which will enforce its own predicates.r? @aliemjay cc @lcnr @oli-obk
You have any thoughts about this approach? I guess f72f5d8 is unnecessary, because it's still possible to ICE with RPITs and uncaptured lifetimes, e.g.
side-note: I'm feeling a déjà-vu writing this PR, like this has been attempted before... if it has, could you remind me where this was, and why this approach ultimately wasn't taken?