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

Enforce predicates which link bivariant opaque lifetimes #122311

Conversation

compiler-errors
Copy link
Member

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.

fn test<'a: 'a>() -> impl Sized {
    let _: () = test::<'a>();
}

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?

@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 10, 2024
@compiler-errors compiler-errors changed the title Opaque linking outlives Enforce predicates which link bivariant opaque lifetimes Mar 10, 2024
@aliemjay
Copy link
Member

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.

@aliemjay
Copy link
Member

I guess f72f5d8 is unnecessary, because it's still possible to ICE with RPITs and uncaptured lifetimes, e.g.

This ICE should be fixed by #116891.

Comment on lines +615 to +620
obligations.push(traits::Obligation::new(
tcx,
cause.clone(),
param_env,
ty::ClauseKind::WellFormed(Ty::new_opaque(self.tcx, def_id, args).into()),
));
Copy link
Contributor

@lcnr lcnr Mar 11, 2024

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.

Copy link
Contributor

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>() returns Opaque<'a, 'a>, we then assign this to a local of type () which uses subtyping.
  • generalizing Opaque<'a, 'a> results in Opaque<'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 🤔

Comment on lines +795 to +796
let obligations = self.nominal_obligations(def_id, args);
self.out.extend(obligations);
Copy link
Contributor

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?

@compiler-errors
Copy link
Member Author

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.

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