Enforce predicates which link bivariant opaque lifetimes - #122311
Enforce predicates which link bivariant opaque lifetimes#122311compiler-errors wants to merge 2 commits into
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.
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.
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'whateverunconstrained
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.
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_typeto 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?