move implied bounds computation out of borrowck

[lcnr]

View all comments

@tiif did the initial implementation work in #152051. This ended up being more involved than I originally expected, so I ended up finishing this PR after spending a few days on it myself.

Computing implied bounds now happens in a new query mir_borrowck_implied_outlives_bounds which does two things differently from MIR borrowck:

• we use param and placeholder regions instead of NLL vars
• if we're a typeck root we don't reveal any opaque types

Using param and placeholder regions instead of NLL vars

This fixes #106569. We previously computed the implied bounds using ty::ReVar even for universal variables, which meant that resolving them can drop constraints in borrowck.

As explained in #106569 (comment)

what's happening here is that normalizing <Equal<'a, 'b> as Trait>::Ty requires 'a and 'b to be equal. We represent 'a and 'b as region inference variables right now.

This means the 'a == 'b constraint does not get returned as region outlives constraints, but we instead just unify the two existentials. The implied bound 'a == 'b then ends up as 'a == 'a which means in the caller, we can't reconstruct that it's actually about 'a and 'b.

Computing implied bounds now uses universal variables instead of ReVar, fixing this issue.

Do not reveal the hidden type of opaques for typeck roots

This fixes rust-lang/trait-system-refactor-initiative#159 with the new trait solver.

trait Extend<'a, 'b> {
    fn extend(self, _: &'a str) -> &'b str;
}
impl<'a, 'b> Extend<'a, 'b> for Option<&'b &'a ()> {
    fn extend(self, s: &'a str) -> &'b str {
        s
    }
}

fn boom<'a, 'b>() -> impl Extend<'a, 'b> {
    None::<&'b &'a ()>
}

fn main() {
    let y = boom().extend(&String::from("temporary"));
    println!("{}", y);
}

Computing the implied bounds for boom previously revealed the hidden type of impl Extend<'a, 'b> giving us a 'a: 'b implied bound. Calling boom cannot reveal the opaque type as it's outside of the defining scope, so the caller never has to prove that outlives requirement.

We do still reveal opaque types when computing the implied bounds for nested bodies! This is subtle and I nearly missed this. For nested bodies, they are only ever used inside of their parent function, which is able to define the same opaque types. We never check that e.g. a closure is well-formed outside of the parent body.

This means trying to compute implied bounds for closures without defining opaque types can result in incorrect errors, see tests/ui/traits/next-solver/opaques/implied-bounds-opaque-hidden-in-closure-sig.rs:

trait Trait {
    type Assoc;
}
impl Trait for () {
    type Assoc = ();
}

trait Func {
    type Output;
}
impl<F: FnOnce() -> R, R> Func for F {
    type Output = R;
}

struct RequiresWf<F>(F)
where
    F: Func,
    F::Output: Trait,
    <F::Output as Trait>::Assoc: 'static;

fn opaque() -> impl Sized {
    // Closure signature is `fn(RequiresWf<fn_def>)` and we need to
    // normalize the RPIT for the where-clauses of the argument to hold.
    (|_| ())(RequiresWf(opaque));
}

Implementation details and nuances

var_values

Returning implied bounds and canonicalization. Figuring out how to do so was quite challenging. The main question is how to link regions from the query to the correct regions in MIR borrowck. The way to do so is via var_values.

As we're using old style canonicalizing we keep early and late bound parameters around, so these don't have to be part of the var_values. We do need to link regions from the closure signature in the query to the regions in the signature used in MIR borrowck. We do this by going over the signature and collecting all regions we find in the var_values. The query uses placeholders for these while MIR borrowck uses external NLL vars for them.

Normalizing the signature and unconstrained region vars

Normalizing a function signature can result in unconstrained existential regions due to #136547. Types involving these regions can be relevant for implied bounds. Using such type outlives bounds relies on structural equality. If we separately normalize the signature two times, once in borrowck and once in the implied bounds query, we get different unconstrained region vars, breaking the gluon_salsa test.

To avoid this, mir_borrowck_implied_outlives_bounds normalizes the signature without revealing opaque types and returns its result to MIR borrowck. MIR borrowck now renormalizes this signature to also correctly normalize opaque types.

The bevy implied bounds hack

This PR keeps the current behavior of #119956 while somewhat changing the actual implementation.

We continue to consider constraints from computing implied bounds as implied bounds only for arguments whose type mentions bevy_ecs::ParamSet.

[lcnr]

@bors try @rust-timer queue

[lcnr]

@bors cancel

@bors try @rust-timer queue

[rust-bors]

❗ There is currently no auto build in progress on this PR.

Hint: There is a pending try build on this PR. Maybe you meant to cancel it? You can do that using @bors try cancel.

[lcnr]

@bors try @rust-timer queue

[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 75 candidates
• Random selection from 20 candidates

[adwinwhite]

Finally get to understand the several related issues and the fix now (mostly) 😄

View changes since this review

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