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]

It looks good to me now, but you might want more eyes on it from Boxy or others.

View changes since this review

[adwinwhite]

Suggested change

	/// We never late bound regions from a parent while computing implied bounds for the current item.
	/// We never ??? late bound regions from a parent while computing implied bounds for the current item.

[lcnr]

what do you not understand!

WE... NEVER... LATE!

[craterbot]

🚧 Experiment pr-160491 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

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

[lcnr]

copied this over to #161047 with a final commit which reverts all changes for the old solver

[rust-bors]

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

[craterbot]

🎉 Experiment pr-160491 is completed!
📊 0 regressed and 1 fixed (1063150 total)
📊 5545 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-160491/retry-regressed-list.txt ↩

[lcnr]

waddup T-types, see PR description, not rebasing as we've already merged this PR only for the new solver, so rebasing it on top of that would hide the actual impl change. See the PR description for a complete summary, the only user visible change is that implied bounds computation doesn't incorrectly drop implied bounds by equating NLL vars.

Also worth it to review the actual change itself as while it doesn't matter for the current solver rn, the underlying concept is already significant and this will very much matter when stabilizing the new solver.

@rfcbot fcp merge tpyes

[rust-rfcbot]

@lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.