move implied bounds computation out of borrowck

[lcnr]

View all comments

First 3 commits don't change behavior and I moved them into #160504.

---

@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

[rust-bors]

☀️ Try build successful (CI)
Build commit: 615014f (615014f15745532730b9ce7a1d9309f8b4756f93)
Base parent: c9ff496 (c9ff496891c278ad660bc0ab85c1f0b72059464a)

[lcnr]

@craterbot check

[craterbot]

👌 Experiment pr-160491 created and queued.
🤖 Automatically detected try build 615014f
⚠️ Try build based on commit 4b5fc7f, but latest commit is 9151a02. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

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

[rust-timer]

Finished benchmarking commit (615014f): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.9%]	40
Regressions ❌ (secondary)	0.7%	[0.1%, 1.4%]	27
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.9%, -0.2%]	12
All ❌✅ (primary)	0.4%	[0.2%, 0.9%]	40

Max RSS (memory usage)

Results (primary 1.3%, secondary -0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.0%	[0.5%, 4.6%]	3
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-0.6%	[-1.1%, -0.4%]	16
All ❌✅ (primary)	1.3%	[-0.7%, 4.6%]	4

Cycles

Results (primary -0.1%, secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.5%, 1.4%]	11
Regressions ❌ (secondary)	1.1%	[0.4%, 2.3%]	8
Improvements ✅ (primary)	-2.6%	[-7.2%, -0.6%]	5
Improvements ✅ (secondary)	-1.6%	[-3.2%, -0.5%]	6
All ❌✅ (primary)	-0.1%	[-7.2%, 1.4%]	16

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.838s -> 489.061s (-0.16%)
Artifact size: 390.28 MiB -> 390.68 MiB (0.10%)

[adwinwhite]

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

View changes since this review

[adwinwhite]

why can we ignore errors here?

[adwinwhite]

Nested bodies are exempted because

• To have implied bounds from hidden types, they need to have opaques in their signatures.
• If they have opaques in the signatures, the call to them would add wf obligations for normalized inputs and output thus those implied bounds are still checked in the parent.

Is my understanding correct? 🤔

[adwinwhite]

Confused here 🤔
Placeholders are from erased regions and closure ty has erased regions in signature and upvars.
So these var_values are corresponding to local universals while we compute the original vars from external universals in compute_implied_bounds. Why so?

[adwinwhite]

ImpliedOutlivesBounds is no longer a type op. Maybe we can cleanup the file path later?

[adwinwhite]

Why don't we worry about the same implied bounds problem in wfcheck? Because mir borrowck would run first if opaques are involved? 🤔

[rust-bors]

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