traits: Fix rigid alias liveness matching

[Dnreikronos]

View all comments

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.

[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.

[Dnreikronos]

Wassup, ty both for your time invested!

Two things I hit while working through @adwinwhite's plan that I want to confirm first, because I don't think it and @jackh726's "no hack" constraint fully agree yet.

The filtering I'll just do. The clause that ICEs comes from impl Foo<'x, Out: 'static>, which the AssocItemConstraintKind::Bound arm lowers to <Opaque<'x0> as Foo<'x0>>::Out: 'static. Its self ty is a different alias than the one we're computing for, so filtering on clause_alias.kind == kind drops it before it reaches extract_verify_if_eq, and we lose nothing: the relate would have failed on the def ids anyway.

Question 1: making alias_ty rigid isn't enough by itself. Item bounds are lowered non-rigid unconditionally, both in associated_type_bounds and in opaque_type_bounds. Today alias_ty is non-rigid too, so for a plain fn f<'a>() -> impl Sized + 'a the outlives.0 == alias_ty fast path hits and we never call the matcher. If I flip only alias_ty to yes_if_next_solver, that equality fails, we fall into extract_verify_if_eq, and the assert fires on a case that works today. So the filtered clause's self ty has to be rigidified as well. After the def-id filter that's provably just the top-level bit, since item-bound self types are always the alias at its identity args, so there are no nested aliases and I can assert that. Is that acceptable, or is it the hack you don't want?

Question 2: the assert in FreeRegionsVisitor and the instantiated output pull against each other. EarlyBinder::bind sets everything non-rigid, so the instantiate in the visitor hands non-rigid aliases back in and trips the new assert. Patching that with set_aliases_to_rigid would add the first caller of a function that currently has none, right next to the param-env one that's already commented as a hack.

The alternative I'd rather write: everything the query and args_known_to_outlive_alias_params return is an element of GenericArgs::identity_for_item, so both can return arg indices instead of EarlyBinder<GenericArg>, and the visitor does args[idx].visit_with(self). That drops the binder and the instantiate entirely, so nothing needs a rigidness fold, and the args are rigid because the alias we're visiting is. It also handles the BitSet FIXME. The cost is a changed query signature and rmeta table, and for RPITITs it makes the "opaque params line up positionally with the assoc type params" assumption explicit instead of leaving it implicit in instantiate.

imo the index version is worth the churn even though it's the bigger diff. Every time we launder rigidness through an EarlyBinder we're writing down something we don't actually know, and this module is only going to grow. I'd rather pay for it now than add a second set_aliases_to_rigid caller that someone has to reason about in six months. It does touch the query signature though, so @jackh726 is that the shape you have in mind? lmk if it's more than you want in this PR and I'll do the narrow version instead.

One more, @jackh726, on <X as Trait>::Assoc: 'a with impl<T> Trait for T { type Assoc = Struct; }. That clause reaches liveness through live_args_for_outlives_clause, which is the param-env path rather than the item_bounds path that ICEs. My read is that the rigid assert rules it out, because an alias that normalizes to Struct wouldn't still be rigid by the time borrowck sees it, so we'd never take the restricting branch. Does that match how you're thinking about it, or is there a case where a normalizable alias shows up rigid there?

[lcnr]

see discussion on zulip https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/rigid.20aliases.20in.20region.20handling/near/615557841

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[Dnreikronos]

@lcnr update: I pulled the bitset work out into #160936 so this PR stays focused on the behavioral / extract_verify_if_eq side.

Zulip context still applies: https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/rigid.20aliases.20in.20region.20handling/near/615557841

imo the bitset was the right first step, but mixing it here made the review surface noisier than it needed to be. the open question on this PR is still the matcher: do we need proper matching/normalization in extract_verify_if_eq, or is underapproximating fine for liveness (only exact Self: 'a / structural eq)? idk which way we want yet.

fyi I left the ignore-rigidness matcher commit here for now. lmk if you'd rather I drop it / swap to the exact-Self: 'a filter asap.

[lcnr]

can you split out the bitset change from the behaviorial changes in this PR? ideally open a separate PR for just the bitset change

[Dnreikronos]

can you split out the bitset change from the behaviorial changes in this PR? ideally open a separate PR for just the bitset change

Ofc!
I will do it asap

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Dnreikronos]

@lcnr split's done.

Bitset landed in #160936. This PR is back to just the behavioral / extract_verify_if_eq stuff.

imo that's cleaner for review. the bitset is pure churn on query shape, and mixing it with the ICE fix was making the diff harder to reason about than it needed to be. lmk if you'd rather I stack #160212 on top of that one instead of keeping them independent.