traits: Fix rigid alias liveness matching

[Dnreikronos]

Fixes #160206

The ICE happens while borrowck computes liveness for an opaque return type with an associated type bound. In the repro, the opaque has a bound like <impl Foo<'x> as Foo<'x>>::Out: 'static. live_args_for_alias_from_outlives_bounds tries to use that bound to decide which opaque args may still be live, but it does that through extract_verify_if_eq, which is just a syntactic matcher.

With the next solver, aliases that cannot normalize further are represented as rigid. The type reaching liveness is already in that shape, but this query rebuilds the opaque identity alias as non-rigid and reads item bounds that can still contain non-rigid aliases. So the matcher gets two different representations of the same kind of alias and hits the debug assert before it can give the conservative answer.

This puts the identity alias and the outlives clauses into the next-solver alias form before matching. imo that is the right layer for this fix: liveness still does the cheap syntactic check it already did, and idk that adding normalization in borrowck liveness would be a good tradeoff here.

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 74 candidates
• Random selection from 17 candidates

[jackh726]

I'm not sure this is correct. Or, at least, I'm not sure this is the right fix. It certainly isn't principled.

I don't have time to fully dig in right now, but generally I would not expect us to arbitrarily be setting things as rigid. I'm not sure the correct fix, but the may even be in the use or definition of extract_verify_if_eq (or may not be).

View changes since this review

[Dnreikronos]

Yeah, that makes sense. I reworked this so liveness does not set aliases rigid anymore.

The ICE was in the path you pointed at, or at least right next to it: extract_verify_if_eq uses its local syntactic matcher, then falls through to structural relation when the two types are not pointer-equal. That shared structural relation asserts that alias rigidness matches. Here the alias shape was the same, but one side had next-solver rigidness state and the other did not.

So imo the better fix is to make MatchAgainstHigherRankedOutlives handle aliases itself. It relates the alias kind and args, but ignores only the IsRigid bit. idk if we want to push this further, but this boundary feels right to me: liveness stays dumb and syntactic, and we do not pre-fold clauses just to satisfy the matcher.

btw I kept the liveness-side identity alias as IsRigid::No and removed the set_aliases_to_rigid calls.

[adwinwhite]

We previously have a set_aliases_to_non_rigid hack in extract_verify_if_eq. I removed it and added the asserts in 7c1c571 without noticing this new outlives_for_liveness module.

[jackh726]

@adwinwhite can you review here? You're much more familiar with what might be the correct fix here than I am.

[adwinwhite]

r? me
I'll look at it in the next few days.

[jackh726]

Thanks! Ping me if there's a question here for me or something otherwise needed from me!

[adwinwhite]

I think we shouldn't weaken the checks in extract_verify_if_eq unless we have no other choice. We want aliases to be rigid in region handling in general. And they probably are.

The reasons we have non-rigid aliases here are

• inside live_args_for_alias_from_outlives_bounds, we operates on generic aliases.
• item_bounds returns non-rigid predicates.

It's difficult to normalize here. So we have to live with a hack, hopefully less bad :/
Assuming the aliases we visit in FreeRegionVisitor are rigid (should assert this), we can treat the input and instantiated output of live_args_for_alias_from_outlives_bounds as rigid. So we can have Ty::new_alias(.., yes_if_next_solver) in it. The method being a query now is for caching reason, I guess?
As for type-outlives predicates from item_bounds, we only care about those whose self ty is of the same kind as our alias. We can do some filtering similar to the beginning of live_args_for_outlives_clause.

[rust-bors]

☔ The latest upstream changes (presumably #160559) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

[jackh726]

I would very much not like to land a hack here; I'd like to stay principled in this space, because the soundness around opaque type liveness involves accurately identifying regions that could be within an alias.

This is a query because:

1. Staying principled: it's much easier to reason about what most hold for all alias instantiation than for any given one
2. Performance/caching: we don't want to have to, recalculate this for every alias use
3. Cross-crate metadata: this maybe isn't needed, since everything should be able to be calculated from the definition, which is available across crates

The only thing that I worry about is something like <X as Trait>::Assoc: 'a with an impl impl<T> Trait for T { type Assoc = Struct; }. I think although we could normalize that to Struct: 'a, and then see that trivially holds (because Struct contains no lifetimes), I am concerned about the non-local reasoning required for that.