rework winnowing to sensibly handle global where-bounds #132325

lcnr · 2024-10-29T16:43:33Z

There may be multiple ways to prove a given trait-bound. In case there are multiple such applicable candidates we need to somehow merge them or bail with ambiguity. When merging candidates we prefer some over others for multiple reasons:

we want to guide inference during typeck, even if not strictly necessary
to avoid ambiguity if there if there are at most lifetime differences
- old solver needs exactly one candidate
- new solver only needs to handle lifetime differences
we disable normalization via impls if the goal is proven by using a where-bound

The approach in this PR¹

always prefer trivial builtin impls²
then prefer non-global³ where-bounds
- if there exists exactly one where-bound, guide inference
- if there are multiple where-bounds even if some of them are global, ambig
then prefer alias bounds⁴ and builtin trait object candidates⁵⁴
merge everything ignoring global where-bounds
if there are no other candidates, try using global where-bounds⁶

We disable normalization via impls when using non-global where-bounds or alias-bounds, even if we're unable to normalize by using the where-bound.

Why this behavior?

inference guidance via where-bounds and alias-bounds

where-bounds

fn method_selection<T: Into<u64>>(x: T) -> Option<u32> {
    x.into().try_into().ok()
    // prove `T: Into<?0>` and then select a method `?0`,
    // needs eager inference.
}

While the above pattern exists in the wild, I think that most inference guidance due to where-bounds is actually unintended. I believe we may want to restrict inference guidance in the future, e.g. limit it to where-bounds whose self-type is a param.

alias-bounds

pub trait Dyn {
    type Word: Into<u64>;
    fn d_tag(&self) -> Self::Word;
    fn tag32(&self) -> Option<u32> {
        self.d_tag().into().try_into().ok()
        // prove `Self::Word: Into<?0>` and then select a method
        // on `?0`, needs eager inference.
    }
}

Disable normalization via impls when using where-bounds

cc rust-lang/trait-system-refactor-initiative#125

trait Trait<'a> {
    type Assoc;
}

impl<T> Trait<'static> for T {
    type Assoc = ();
}

// normalizing requires `'a == 'static`, the trait bound does not.
fn foo<'a, T: Trait<'a>>(_: T::Assoc) {}

If an impl adds constraints not required by a where-bound, using the impl may cause compilation failure avoided by treating the associated type as rigid.

This is also why we can always use trivial builtin impls, even for normalization. They are guaranteed to never add any requirements.

Lower priority for global where-bounds

A where-bound is considered global if it does not refer to any generic parameters and is not higher-ranked. It may refer to 'static.

This means global where-bounds are either always fully implied by an impl or unsatisfiable. We don't really care about the inference behavior of unsatisfiable where-bounds :3

If a where-bound is fully implied then using an applicable impl for normalization cannot result in additional constraints. As this is the - afaict only - reason why we disable normalization via impls in the first place, we don't have to disable normalization via impls when encountering global where-bounds.

Consider global where-bounds at all

Given that we just use impls even if there exists a global where-bounds, you may ask why we don't just ignore these global where-bounds entirely: we use them to weaken the inference guidance from non-global where-bounds.

Without a global where-bound, we currently prefer non-global where bounds even though there would be an applicable impl as well. By adding a non-global where-bound, this unnecessary inference guidance is disabled, allowing the following to compile:

fn check<Color>(color: Color)
where
    Vec: Into<Color> + Into<f32>,
{
    let _: f32 = Vec.into();
    // Without the global `Vec: Into<f32>`  bound we'd
    // eagerly use the non-global `Vec: Into<Color>` bound
    // here, causing this to fail.
}

struct Vec;
impl From<Vec> for f32 {
    fn from(_: Vec) -> Self {
        loop {}
    }
}

There exist multiple crates which rely on this behavior.

Design considerations

We would like to be able to normalize via impls as much as possible. Disabling normalization simply because there exists a where-bound is undesirable.

For the sake of backwards compatability I intend to mostly mirror the current inference guidance rules and then explore possible improvements once the new solver is done. I do believe that removing unnecessary inference guidance where possible is desirable however.

Whether a where-bound is global depends on whether used lifetimes are 'static. The where-bound u32: Trait<'static> is either entirely implied by an impl, meaning that it does not have to disable normalization via impls, while u32: Trait<'a> needs to disable normalization via impls as the impl may only hold for 'static. Considering all where-bounds to be non-global once they contain any region is unfortunately a breaking change.

How does this differ from stable

The currently stable approach is order dependent:

it prefers impls over global where-bounds: impl > global
it prefers non-global where-bounds over impls: non-global > impl
it treats all where-bounds equally: global = non-global

This means that whether we bail with ambiguity or simply use the non-global where bound depending on the order of where-clauses and number of applicable impl candidates. See the tests added in the first commit for more details. With this PR we now always bail with ambiguity.

I've previously tried to always use the non-global candidate, causing unnecessary inference guidance and undesirable breakage. This already went through an FCP in #124592. However, I consider the new approach to be preferable as it exclusively removes incompleteness. It also doesn't cause any crater breakage.

How to support this in the new solver :o

This is separately implemented in #133643 and not part of this FCP!

To implement the global vs non-global where-bound distinction, we have to either keep 'static in the param_env when canonicalizing, or eagerly distinguish global from non-global where-bounds and provide that information to the canonical query.

The old solver currently keeps 'static only the param_env, replacing it with an inference variable in the value.

rust/compiler/rustc_infer/src/infer/canonical/canonicalizer.rs

Lines 49 to 64 in a4cedec

    
           let canonical_param_env = self.tcx.canonical_param_env_cache.get_or_insert( 
        
               self.tcx, 
        
               param_env, 
        
               query_state, 
        
               |tcx, param_env, query_state| { 
        
                   // FIXME(#118965): We don't canonicalize the static lifetimes that appear in the 
        
                   // `param_env` because they are treated differently by trait selection. 
        
                   Canonicalizer::canonicalize( 
        
                       param_env, 
        
                       None, 
        
                       tcx, 
        
                       &CanonicalizeFreeRegionsOtherThanStatic, 
        
                       query_state, 
        
                   ) 
        
               }, 
        
           );

I dislike that based on vibes and it may end up being a problem once we extend the environment inside of the solver as we must not rely on 'static in the predicate as it would get erased in MIR typeck.

An alternative would be to eagerly detect trivial where-bounds when constructing the ParamEnv. We can't entirely drop them as explained above, so we'd instead replace them with a new clause kind TraitImpliedByImpl which gets entirely ignored except when checking whether we should eagerly guide inference via a where-bound. This approach can be extended to where-bounds which are currently not considered global to stop disabling normalization for them as well.

Keeping 'static in the param_env is the simpler solution here and we should be able to move to the second approach without any breakage. I therefore propose to keep 'static in the environment for now.

r? @compiler-errors

see the source for more details ↩
they don't constrain inference and don't add any lifetime constraints ↩
a where-bound is global if it is not higher-ranked and doesn't contain any generic parameters, 'static is ok ↩
we arbitrary select a single object and alias-bound candidate in case multiple apply and they don't impact inference. This should be unnecessary in the new solver. ↩ ↩²
Necessary for dyn Any and https://github.com/rust-lang/rust/issues/57893 ↩
global where-bounds are only used if they are unsatisfiable, i.e. no impl candidate exists ↩

lcnr · 2024-10-29T16:43:43Z

@bors try @rust-timer queue

rework winnowing to sensibly handle global where-bounds this is somewhat weird, but it at least allows us to mirror this behavior in the new solver: - trivial builtin-impls - non-global where-bounds, bailing with ambiguity if at least one global where-bound exists - object ☠️ + alias-bound candidates - merge candidates ignoring global where-bounds r? `@compiler-errors`

bors · 2024-10-29T16:44:56Z

⌛ Trying commit dc50c64 with merge 6d565b7...

compiler/rustc_trait_selection/src/traits/select/mod.rs

bors · 2024-10-29T18:41:53Z

☀️ Try build successful - checks-actions
Build commit: 6d565b7 (6d565b71a096078f59574161a2a382ec4a1eb751)

rust-timer · 2024-10-29T20:00:54Z

Finished benchmarking commit (6d565b7): comparison URL.

Overall result: ✅ improvements - no 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.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-1.2%, -0.1%]	69
Improvements ✅ (secondary)	-1.2%	[-2.2%, -0.7%]	3
All ❌✅ (primary)	-0.4%	[-1.2%, -0.1%]	69

Max RSS (memory usage)

Results (primary -2.2%, secondary 0.6%)

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)	3.4%	[1.7%, 5.6%]	4
Improvements ✅ (primary)	-2.2%	[-3.2%, -1.2%]	4
Improvements ✅ (secondary)	-3.1%	[-4.4%, -1.8%]	3
All ❌✅ (primary)	-2.2%	[-3.2%, -1.2%]	4

Cycles

Results (primary 2.3%)

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)	2.3%	[1.8%, 2.7%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[1.8%, 2.7%]	2

Binary size

Results (primary -0.1%)

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.1%	[-0.1%, -0.0%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.1%, -0.0%]	8

Bootstrap: 784.701s -> 783.947s (-0.10%)
Artifact size: 333.58 MiB -> 333.58 MiB (0.00%)

lcnr · 2024-10-29T21:45:17Z

@craterbot check

craterbot · 2024-10-29T21:45:29Z

👌 Experiment pr-132325 created and queued.
🤖 Automatically detected try build 6d565b7
🔍 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 · 2024-11-03T22:14:20Z

🚧 Experiment pr-132325 is now running

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

craterbot · 2024-11-06T12:50:15Z

🎉 Experiment pr-132325 is completed!
📊 403 regressed and 4 fixed (532439 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 · 2024-11-07T17:37:59Z

@bors try

for later: check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-132325/retry-regressed-list.txt

bors · 2024-11-07T17:39:10Z

⌛ Trying commit 2090bf2 with merge 5be2110...

rework winnowing to sensibly handle global where-bounds this is somewhat weird, but it at least allows us to mirror this behavior in the new solver: - trivial builtin-impls - non-global where-bounds, bailing with ambiguity if at least one global where-bound exists - object ☠️ + alias-bound candidates - merge candidates ignoring global where-bounds This is a different approach from rust-lang#124592 which maintains the "if there are global where-bounds, don't guide type inference using non-global where-bounds" behavior, hopefully avoiding the breakage and means we use guidance from non-global where-bounds in fewer, instead of in more cases. r? `@compiler-errors`

bors · 2024-11-07T19:29:53Z

☀️ Try build successful - checks-actions
Build commit: 5be2110 (5be211015c49a4eb5c973cc5ebe17db5c6e7ffb8)

compiler-errors · 2024-11-07T20:12:29Z

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

craterbot · 2024-11-07T20:12:33Z

👌 Experiment pr-132325-1 created and queued.
🤖 Automatically detected try build 5be2110
🔍 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 · 2024-12-05T02:50:09Z

🚧 Experiment pr-132325-2 is now running

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

craterbot · 2024-12-05T05:42:51Z

🎉 Experiment pr-132325-2 is completed!
📊 0 regressed and 0 fixed (987 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 · 2024-12-05T08:54:25Z

it looks like the result are unknown yet again 🤔 what's going on there

cc @rust-lang/infra

rfcbot · 2024-12-14T14:19:26Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

lcnr · 2024-12-17T08:44:17Z

@bors r=oli-obk rollup=never

bors · 2024-12-17T08:44:19Z

📌 Commit 3350b9f has been approved by oli-obk

It is now in the queue for this repository.

bors · 2024-12-17T09:18:21Z

⌛ Testing commit 3350b9f with merge 604d669...

bors · 2024-12-17T12:06:23Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 604d669 to master...

rust-timer · 2024-12-17T13:22:58Z

Finished benchmarking commit (604d669): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-1.0%, -0.2%]	64
Improvements ✅ (secondary)	-1.6%	[-2.1%, -0.7%]	9
All ❌✅ (primary)	-0.4%	[-1.0%, -0.2%]	64

Max RSS (memory usage)

Results (primary 0.9%, secondary -0.3%)

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.7%	[0.9%, 2.5%]	2
Regressions ❌ (secondary)	2.8%	[2.3%, 3.9%]	3
Improvements ✅ (primary)	-0.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-2.7%	[-3.1%, -1.9%]	4
All ❌✅ (primary)	0.9%	[-0.9%, 2.5%]	3

Cycles

Results (primary -0.3%, secondary -4.1%)

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)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.5%	[-1.5%, -1.4%]	2
Improvements ✅ (secondary)	-4.1%	[-9.8%, -2.3%]	5
All ❌✅ (primary)	-0.3%	[-1.5%, 2.1%]	3

Binary size

Results (primary -0.1%)

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.1%	[-0.1%, -0.0%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.1%, -0.0%]	7

Bootstrap: 770.381s -> 768.397s (-0.26%)
Artifact size: 330.98 MiB -> 330.36 MiB (-0.19%)

…errors -Znext-solver: modify candidate preference rules This implements the design proposed in the FCP in rust-lang#132325 and matches the old solver behavior. I hope the inline comments are all sufficiently clear, I personally think this is a fairly clear improvement over the existing approach using `fn discard_impls_shadowed_by_env`. This fixes rust-lang/trait-system-refactor-initiative#96. This also fixes rust-lang#133639 which encounters an ICE in negative coherence when evaluating the where-clause. Given the features required to trigger this ICE 🤷 r? `@compiler-errors`

…errors -Znext-solver: modify candidate preference rules This implements the design proposed in the FCP in rust-lang#132325 and matches the old solver behavior. I hope the inline comments are all sufficiently clear, I personally think this is a fairly clear improvement over the existing approach using `fn discard_impls_shadowed_by_env`. This fixes rust-lang/trait-system-refactor-initiative#96. This also fixes rust-lang#133639 which encounters an ICE in negative coherence when evaluating the where-clause. Given the features required to trigger this ICE 🤷 r? ``@compiler-errors``

Rollup merge of rust-lang#133643 - lcnr:merge-candidates, r=compiler-errors -Znext-solver: modify candidate preference rules This implements the design proposed in the FCP in rust-lang#132325 and matches the old solver behavior. I hope the inline comments are all sufficiently clear, I personally think this is a fairly clear improvement over the existing approach using `fn discard_impls_shadowed_by_env`. This fixes rust-lang/trait-system-refactor-initiative#96. This also fixes rust-lang#133639 which encounters an ICE in negative coherence when evaluating the where-clause. Given the features required to trigger this ICE 🤷 r? ``@compiler-errors``

rustbot assigned compiler-errors Oct 29, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 29, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 29, 2024

lcnr commented Oct 29, 2024

View reviewed changes

compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 29, 2024

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 29, 2024

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 6, 2024

lcnr force-pushed the damn-son branch from dc50c64 to 2090bf2 Compare November 7, 2024 17:36

bors mentioned this pull request Nov 7, 2024

[StableMIR] A few fixes to pretty printing #132161

Merged

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2024

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. labels Dec 4, 2024

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 5, 2024

rustbot mentioned this pull request Dec 14, 2024

Tracking issue for release notes of #132325: rework winnowing to sensibly handle global where-bounds #134309

Open

3 tasks

lcnr added 2 commits December 17, 2024 08:50

add tests

5f54889

consistently handle global where-bounds

3350b9f

lcnr force-pushed the damn-son branch from f168e79 to 3350b9f Compare December 17, 2024 07:56

oli-obk approved these changes Dec 17, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 17, 2024

bors merged commit 604d669 into rust-lang:master Dec 17, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Dec 17, 2024

lcnr deleted the damn-son branch December 17, 2024 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework winnowing to sensibly handle global where-bounds #132325

rework winnowing to sensibly handle global where-bounds #132325

lcnr commented Oct 29, 2024 •

edited

Loading

lcnr commented Oct 29, 2024

This comment has been minimized.

bors commented Oct 29, 2024

bors commented Oct 29, 2024

rust-timer commented Oct 29, 2024

lcnr commented Oct 29, 2024

craterbot commented Oct 29, 2024

craterbot commented Nov 3, 2024

craterbot commented Nov 6, 2024

lcnr commented Nov 7, 2024

bors commented Nov 7, 2024

bors commented Nov 7, 2024

compiler-errors commented Nov 7, 2024

craterbot commented Nov 7, 2024

craterbot commented Dec 5, 2024

craterbot commented Dec 5, 2024

lcnr commented Dec 5, 2024

rfcbot commented Dec 14, 2024

lcnr commented Dec 17, 2024

bors commented Dec 17, 2024

bors commented Dec 17, 2024

bors commented Dec 17, 2024

rust-timer commented Dec 17, 2024

	let canonical_param_env = self.tcx.canonical_param_env_cache.get_or_insert(
	self.tcx,
	param_env,
	query_state,
	\|tcx, param_env, query_state\| {
	// FIXME(#118965): We don't canonicalize the static lifetimes that appear in the
	// `param_env` because they are treated differently by trait selection.
	Canonicalizer::canonicalize(
	param_env,
	None,
	tcx,
	&CanonicalizeFreeRegionsOtherThanStatic,
	query_state,
	)
	},
	);

rework winnowing to sensibly handle global where-bounds #132325

rework winnowing to sensibly handle global where-bounds #132325

Conversation

lcnr commented Oct 29, 2024 • edited Loading

The approach in this PR1

Why this behavior?

inference guidance via where-bounds and alias-bounds

where-bounds

alias-bounds

Disable normalization via impls when using where-bounds

Lower priority for global where-bounds

Consider global where-bounds at all

Design considerations

How does this differ from stable

How to support this in the new solver :o

Footnotes

lcnr commented Oct 29, 2024

This comment has been minimized.

bors commented Oct 29, 2024

bors commented Oct 29, 2024

rust-timer commented Oct 29, 2024

Overall result: ✅ improvements - no action needed

lcnr commented Oct 29, 2024

craterbot commented Oct 29, 2024

craterbot commented Nov 3, 2024

craterbot commented Nov 6, 2024

lcnr commented Nov 7, 2024

bors commented Nov 7, 2024

bors commented Nov 7, 2024

compiler-errors commented Nov 7, 2024

craterbot commented Nov 7, 2024

craterbot commented Dec 5, 2024

craterbot commented Dec 5, 2024

lcnr commented Dec 5, 2024

rfcbot commented Dec 14, 2024

lcnr commented Dec 17, 2024

bors commented Dec 17, 2024

bors commented Dec 17, 2024

bors commented Dec 17, 2024

rust-timer commented Dec 17, 2024

Overall result: ✅ improvements - no action needed

lcnr commented Oct 29, 2024 •

edited

Loading

The approach in this PR¹