instantiate higher ranked goals outside of candidate selection

[lcnr]

edit: this PR has been reverted in #127568, keeping the current inconsistent behavior. Supporting this behavior in the new trait solver is tracked in rust-lang/trait-system-refactor-initiative#120

---

This PR modifies evaluate to more eagerly instantiate higher-ranked goals, preventing the leak_check during candidate selection from detecting placeholder errors involving that binder.

For a general background regarding higher-ranked region solving and the leak check, see https://hackmd.io/qd9Wp03cQVy06yOLnro2Kg.

The first is something called the leak check. You can think of it as a "quick and dirty" approximation for the region check, which will come later. The leak check detects some kinds of errors early, essentially deciding between "this set of outlives constraints are guaranteed to result in an error eventually" or "this set of outlives constraints may be solvable".

The ideal future

We would like to end up with the following idealized design to handle universal binders:

fn enter_forall<'tcx, T, R>(
    forall: Binder<'tcx, T>,
    f: impl FnOnce(T) -> R,
) -> R {
    let new_universe = infcx.increment_universe_index();
    let value = instantiate_binder_with_placeholders_in(new_universe, forall);
    
    let result = f(value);
    
    eagerly_handle_higher_ranked_region_constraints_in(new_universe);
    infcx.decrement_universe_index();
    
    assert!(!result.has_placeholders_in_or_above(new_universe));
    result
}

That is, when universally instantiating a binder, anything using the placeholders has to happen inside of a limited scope (the closure f). After this closure has completed, all constraints involving placeholders are known.

We then handle any external constraints which name these placeholders. We destructure TypeOutlives constraints involving placeholders and eagerly handle any region constraints involving these placeholders. We do not return anything mentioning the placeholders created inside of this function to the caller.

Being able to eagerly handle all region constraints involving placeholders will be difficult due to complex TypeOutlives constraints, involving inference variables or alias types, and higher ranked implied bounds. The exact issues and possible solutions are out of scope of this FCP.

How does the leak check fit into this

The leak_check is an underapproximation of eagerly_handle_higher_ranked_region_constraints_in. It detects some kinds of errors involving placeholders from new_universe, but not all of them.

It only looks at region outlives constraints, ignoring TypeOutlives, and checks whether one of the following two conditions are met for placeholders in or above new_universe, in which case it results in an error:

• '!p1: '!p2 a placeholder '!p2 outlives a different placeholder '!p1
• '!p1: '?2 an inference variable '?2 outlives a placeholder '!p1 which it cannot name

It does not handle all higher ranked region constraints, so we still return constraints involving placeholders from new_universe which are then (re)checked by lexical_region_resolve or MIR borrowck.

As we check higher ranked constraints in the full regionck anyways, the leak_check is not soundness critical. It's current only purpose is to move some higher ranked region errors earlier, enabling it to guide type inference and trait solving. Adding additional uses of the leak_check in the future would only strengthen inference and is therefore not breaking.

Where do we use currently use the leak check

The leak_check is currently used in two places:

Coherence does not use a proper regionck, only relying on the leak_check called at the end of the implicit negative overlap check. During coherence all parameters are instantiated with inference variables, so the only possible region errors are higher-ranked. We currently also sometimes make guesses when destructuring TypeOutlives constraints which can theoretically result in incorrect errors. This could result in overlapping impls.

We also use the leak_check at the end of fn evaluation_probe. This function is used during candidate assembly for Trait goals. Most notably we use inside of evaluate_candidate during winnowing. Conceptionally, it is as if we compute each candidate in a separate enter_forall.

The current use in fn evaluation_probe is undesirable

Because we only instantiate a higher-ranked goal once inside of fn evaluation_probe, errors involving placeholders from that binder can impact selection. This results in inconsistent behavior (playground):

trait Leak<'a> {}
impl Leak<'_>      for Box<u32> {}
impl Leak<'static> for Box<u16> {}

fn impls_leak<T: for<'a> Leak<'a>>() {}

trait IndirectLeak<'a> {}
impl<'a, T: Leak<'a>> IndirectLeak<'a> for T {}
fn impls_indirect_leak<T: for<'a> IndirectLeak<'a>>() {}

fn main() {
    // ok
    //
    // The `Box<u16>` impls fails the leak check,
    // meaning that we apply the `Box<u32>` impl.
    impls_leak::<Box<_>>();
    
    // error: type annotations needed
    //
    // While the `Box<u16>` impl would fail the leak check
    // we have already instantiated the binder while applying
    // the generic `IndirectLeak` impl, so during candidate
    // selection of `Leak` we do not detect the placeholder error.
    // Evaluation of `Box<_>: Leak<'!a>` is therefore ambiguous,
    // resulting in `for<'a> Box<_>: Leak<'a>` also being ambiguous.
    impls_indirect_leak::<Box<_>>();
}

We generally prefer where-bounds over implementations during candidate selection, both for trait goals and during normalization. However, we currently do not use the leak_check during candidate assembly in normalizing. This can result in inconsistent behavior:

trait Trait<'a> {
    type Assoc;
}
impl<'a, T> Trait<'a> for T {
    type Assoc = usize;
}

fn trait_bound<T: for<'a> Trait<'a>>() {}
fn projection_bound<T: for<'a> Trait<'a, Assoc = usize>>() {}

// A function with a trivial where-bound which is more
// restrictive than the impl.
fn function<T: Trait<'static, Assoc = usize>>() {
    // ok
    //
    // Proving `for<'a> T: Trait<'a>` using the where-bound results
    // in a leak check failure, so we use the more general impl,
    // causing this to succeed.
    trait_bound::<T>();
    
    // error
    //
    // Proving the `Projection` goal `for<'a> T: Trait<'a, Assoc = usize>`
    // does not use the leak check when trying the where-bound, causing us
    // to prefer it over the impl, resulting in a placeholder error.
    projection_bound::<T>();
    
    // error
    //
    // Trying to normalize the type `for<'a> fn(<T as Trait<'a>>::Assoc)`
    // only gets to `<T as Trait<'a>>::Assoc` once `'a` has been already
    // instantiated, causing us to prefer the where-bound over the impl
    // resulting in a placeholder error. Even if were were to also use the
    // leak check during candidate selection for normalization, this
    // case would still not compile.
    let _higher_ranked_norm: for<'a> fn(<T as Trait<'a>>::Assoc) = |_| ();
}

This is also likely to be more performant. It enables more caching in the new trait solver by simply recursively calling the canonical query after instantiating the higher-ranked goal.

It is also unclear how to add the leak check to normalization in the new solver. To handle rust-lang/trait-system-refactor-initiative#1 Projection goals are implemented via AliasRelate. This again means that we instantiate the binder before ever normalizing any alias. Even if we were to avoid this, we lose the ability to cache normalization by itself, ignoring the expected term. We cannot replace the term with an inference variable before instantiating the binder, as otherwise for<'a> T: Trait<Assoc<'a> = &'a ()> breaks. If we only replace the term after instantiating the binder, we cannot easily evaluate the goal in a separate context, as we'd then lose the information necessary for the leak check. Adding this information to the canonical input also seems non-trivial.

Proposed solution

I propose to instantiate the binder outside of candidate assembly, causing placeholders from higher-ranked goals to get ignored while selecting their candidate. This mostly¹ matches the current behavior of the new solver. The impact of this change is therefore as follows:

trait Leak<'a> {}
impl Leak<'_>      for Box<u32> {}
impl Leak<'static> for Box<u16> {}

fn impls_leak<T: for<'a> Leak<'a>>() {}

trait IndirectLeak<'a> {}
impl<'a, T: Leak<'a>> IndirectLeak<'a> for T {}
fn impls_indirect_leak<T: for<'a> IndirectLeak<'a>>() {}

fn guide_selection() {
    // ok -> ambiguous
    impls_leak::<Box<_>>();
    
    // ambiguous
    impls_indirect_leak::<Box<_>>();
}

trait Trait<'a> {
    type Assoc;
}
impl<'a, T> Trait<'a> for T {
    type Assoc = usize;
}

fn trait_bound<T: for<'a> Trait<'a>>() {}
fn projection_bound<T: for<'a> Trait<'a, Assoc = usize>>() {}

// A function which a trivial where-bound which is more
// restrictive than the impl.
fn function<T: Trait<'static, Assoc = usize>>() {
    // ok -> error
    trait_bound::<T>();
    
    // error
    projection_bound::<T>();
    
    // error
    let _higher_ranked_norm: for<'a> fn(<T as Trait<'a>>::Assoc) = |_| ();
}

This does not change the behavior if candidates have higher ranked nested goals, as in this case the leak_check causes the nested goal to result in an error (playground):

trait LeakCheckFailure<'a> {}
impl LeakCheckFailure<'static> for () {}

trait Trait<T> {}
impl Trait<u32> for () where for<'a> (): LeakCheckFailure<'a> {}
impl Trait<u16> for () {}
fn impls_trait<T: Trait<U>, U>() {}
fn main() {
    // ok
    //
    // It does not matter whether candidate assembly
    // considers the placeholders from higher-ranked goal.
    // 
    // Either `for<'a> (): LeakCheckFailure<'a>` has no 
    // applicable candidate or it has a single applicable candidate
    // when then later results in an error. This allows us to
    // infer `U` to `u16`.
    impls_trait::<(), _>()
}

Impact on existing crates

This is a breaking change. A crater run found 17 regressed crates with 7 root causes.

For a full analysis of all affected crates, see https://gist.github.com/lcnr/7c1c652f30567048ea240554a36ed95c.

---

I believe this breakage to be acceptable and would merge this change. I am confident that the new position of the leak check matches our idealized future and cannot envision any other consistent alternative. Where possible, I intend to open PRs fixing/avoiding the regressions before landing this PR.

I originally intended to remove the coherence_leak_check lint in the same PR. However, while I am confident in the position of the leak check, deciding on its exact behavior is left as future work, cc #112999. This PR therefore only moves the leak check while keeping the lint when relying on it in coherence.

r? @nikomatsakis

Footnotes

1. the new solver has a separate cause of inconsistent behavior rn https://github.com/rust-lang/trait-system-refactor-initiative/issues/53#issuecomment-1914310171 ↩

[rustbot]

Type relation code was changed

cc @compiler-errors, @lcnr

[lcnr]

@bors try @rust-timer queue

(perf and need crater for rust-lang/trait-system-refactor-initiative#34)

[bors]

⌛ Trying commit e49e69b with merge b69bfee...

[bors]

☀️ Try build successful - checks-actions
Build commit: b69bfee (b69bfeecc49bf5661cf54497638430035ff61aa4)

[rust-timer]

Finished benchmarking commit (b69bfee): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 0.8%]	8
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.2%, 0.8%]	8

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.6%, 1.2%]	3
Regressions ❌ (secondary)	1.7%	[1.3%, 2.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	1.0%	[0.6%, 1.2%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	6.1%	[6.1%, 6.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 666.26s -> 666.778s (0.08%)
Artifact size: 308.39 MiB -> 308.42 MiB (0.01%)

[lcnr]

@craterbot check

[craterbot]

👌 Experiment pr-119820 created and queued.
🤖 Automatically detected try build b69bfee
🔍 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

[craterbot]

🚧 Experiment pr-119820 is now running

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

[craterbot]

🎉 Experiment pr-119820 is completed!
📊 61 regressed and 5 fixed (406907 total)
📰 Open the full report.

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

[lcnr]

looking at https://crates.io/crates/rene, I think the error pattern is the following:

trait Trait {}

impl<T: Trait> Trait for &T {}
impl Trait for u32 {}


fn hr_bound<T>() 
where
    for<'a> &'a T: Trait,
{}

fn foo<T>()
where
    T: Trait,
    for<'a> &'a &'a T: Trait,
{
    hr_bound::<&T>();
    // We get a universe error when using the `param_env` candidate
    // but are able to successfully use the impl candidate. Without
    // the leak check both candidates may apply and we prefer the
    // `param_env` candidate in winnowing.
}

fn main() {}

[lcnr]

@craterbot check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-119820/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-119820-1 created and queued.
🤖 Automatically detected try build b69bfee
🔍 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

[craterbot]

🚧 Experiment pr-119820-1 is now running

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

[BoxyUwU]

@bors rollup=never

[bors]

⌛ Testing commit f090de8 with merge 43f4f2a...

[bors]

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 43f4f2a to master...

[rust-timer]

Finished benchmarking commit (43f4f2a): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.3%, 0.8%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.3%, 0.8%]	8

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.1%, 1.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.1%, 1.1%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.2%, -1.8%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.809s -> 667.94s (-0.28%)
Artifact size: 318.07 MiB -> 318.08 MiB (0.00%)

[Kobzol]

Expected small perf. regression.

@rustbot label: +perf-regression-triaged