cleanup `candidate_should_be_dropped_in_favor_of` #124592

lcnr · 2024-05-01T18:29:57Z

and make sure it's actually a partial ordering :< the previous ordering was:

non-global where-bounds are preferred over impls
impls are preferred over global where-bound
non-global where-bounds and global where-bounds are considered equal

The new solver currently does not implement the lowered priority of global ParamEnv candidates, so the divergence between it and the old solver in the added tests will be resolved in favor of the old solver behavior after this PR.

r? @compiler-errors

lcnr · 2024-05-01T18:54:40Z

@bors try

cleaning `candidate_should_be_dropped_in_favor_of` and make sure it's actually a partial ordering :< - non-global where-bounds are preferred over impls - impls are preferred over global where-bound - non-global where-bounds and global where-bounds are considered equal The new solver currently does not implement thelowered priority of global `ParamEnv` candidates, so the divergence between it and the old solver in the added tests will be resolved in favor of the old solver behavior after this PR. r? `@compiler-errors`

bors · 2024-05-01T18:55:06Z

⌛ Trying commit a0359c8 with merge 2466b92...

bors · 2024-05-01T20:32:50Z

☀️ Try build successful - checks-actions
Build commit: 2466b92 (2466b92ed5feb282a98bb984f45d1e6d39f8c1aa)

compiler-errors · 2024-05-01T20:54:40Z

@craterbot check

craterbot · 2024-05-01T20:54:50Z

👌 Experiment pr-124592 created and queued.
🤖 Automatically detected try build 2466b92
🔍 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-05-04T08:05:33Z

🚧 Experiment pr-124592 is now running

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

craterbot · 2024-05-06T01:32:50Z

🎉 Experiment pr-124592 is completed!
📊 51 regressed and 4 fixed (444840 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

fmease · 2024-05-10T12:41:19Z

📊 51 regressed and 4 fixed (444840 total)

Minimizations:

tc-collection
- Minimized (playground)
- Error message of MIN differs slightly cmp'ed to ORIG but that's not substantial
  - We still have a u64 / T mismatch
Andlon/ice_testcase
- Already tracked in tests/ui/associated-item/issue-105449.rs
droundy/sad-monte-carlo
- Minimized (playground)
imgui
- Minimized (playground)
47 “unknown” build failures
- TODO(fmease)

fmease · 2024-05-17T15:11:43Z

@lcnr, I just went through the crater reports of all 47 “unknown” regressions. Some of them were spurious (no space left on device), all of the the remaining ones are type mismatches of the expected kind as well as secondary regressions only (except for redst4r/bustools_cli-rs / bustools_cli-0.2.1) with the primary regression being imgui in most cases and tc-collection otherwise (downstream crates are owned by the same person (monorepo) in the latter case).

As discussed, this is ready for an FCP. I will open “incoming breakage” PRs for the relevant crates if/once this PR gets accepted.

lcnr · 2024-05-18T19:31:23Z

This PR changes the rule to candidate winnowing during selection. We prefer candidates over others, both to guide inference and to avoid ambiguity in case there are multipe applicable candidates.

The current preference rules aren't a proper order, making candidate selection order-dependent. The issue arrises for "global ParamEnv candidates": where-bounds which are not higher-ranked and do not mention any generic parameters.

This current preference rules are as follows

non-global ParamEnv > impl
impl > global ParamEnv
global ParamEnv == non-global ParamEnv

I propose to change these rules to a proper ordering:

non-global ParamEnv > impl > global ParamEnv

This avoids order-dependence and is easier to support in the next-generation trait solver. The current behavior is also unintuitive.

This change adds additional incomplete inference guidance in cases with

at least one applicable global ParamEnv candidate
exactly one applicable non-global ParamEnv candidate
less than 2 or 3 (depending of the order of where-clauses) impl candidates

With this setup, we previously did not use the impls to drop the global ParamEnv candidate as the impls were first dropped via the non-global candidate.

If there were more than 2 or 3 impl candidates, we ended dropping the global ParamEnv candidates via an impl candidate before all impls were dropped due to the non-global ParamEnv candidate. This subtle behavior is caused by using swap_remove when dropping impl candidates:

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

Lines 498 to 508 in cf2baaa

    
           let should_drop_i = (0..candidates.len()).filter(|&j| i != j).any(|j| { 
        
               self.candidate_should_be_dropped_in_favor_of( 
        
                   &candidates[i], 
        
                   &candidates[j], 
        
                   has_non_region_infer, 
        
               ) == DropVictim::Yes 
        
           }); 
        
           if should_drop_i { 
        
               debug!(candidate = ?candidates[i], "Dropping candidate #{}/{}", i, candidates.len()); 
        
               candidates.swap_remove(i); 
        
           } else {

This causes the following patterns to change behavior

while keeping the behavior of the patterns in

The new solver currently does not implement the lowered priority of global ParamEnv candidates, so the divergence between it and the old solver in the added tests will be resolved in favor of the old solver behavior after this PR.

There are multiple existing crates which break due to inference changes, see #124592 (comment). All breakage should be fixable by adding explicit type annotations and we intend to open PRs doing so once this is in FCP. Thank you @fmease for minimizing them.

@rfcbot fcp merge

rfcbot · 2024-05-18T19:31:25Z

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

Concerns:

could we instead prioritize trivial where-bounds over impls without preventing impls in normalization somehow? (cleanup candidate_should_be_dropped_in_favor_of #124592 (comment))

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.

BoxyUwU · 2024-05-22T19:01:46Z

is imgui not a relatively significant crate to be breaking? I would expect that to be being used by a lot of projects 🤔

lcnr · 2024-05-22T19:44:49Z

I still think we should go ahead with it. The fix to it will be local. I don't mind if we have to keep this regression on nightly for 1 or 2 versions

lcnr · 2024-06-14T09:21:23Z

I still think we should go ahead with it. The fix to it will be local. I don't mind if we have to keep this regression on nightly for 1 or 2 versions

@rfcbot concern could we instead prioritize trivial where-bounds over impls without preventing impls in normalization somehow?

nikomatsakis · 2024-06-17T13:21:12Z

As I've said a few times, I hope that in the new solver we can convert this to more of a "guidance" setup, but regardless we will need some kind of ordering to give that guidance, and in that vein...

@rfcbot reviewed

...I think this ordering makes intuitive sense. It would be good to work with the regressions (especially imgui) to ensure they update their where-clauses up front, if that is needed, and/or issue point releases.

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`

lcnr · 2024-11-14T18:03:51Z

closing in favor of #132325

rework winnowing to sensibly handle global where-bounds 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[^1] - always prefer trivial builtin impls[^6] - then prefer non-global[^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[^2] and builtin trait object candidates[^3][^2] - merge everything ignoring global where-bounds - if there are no other candidates, try using global where-bounds[^5] **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.** [^1]: see the source for more details [^2]: [we arbitrary select a single object and alias-bound candidate in case multiple apply and they don't impact inference](https://github.com/rust-lang/rust/blob/a4cedecc9ec76b46dcbb954750068c832cf2dd43/compiler/rustc_trait_selection/src/traits/select/mod.rs#L1906-L1911). This should be unnecessary in the new solver. [^3]: Necessary for `dyn Any` and rust-lang#57893 [^global]: a where-bound is global if it is not higher-ranked and doesn't contain any generic parameters, `'static` is ok [^5]: global where-bounds are only used if they are unsatisfiable, i.e. no impl candidate exists [^6]: they don't constrain inference and don't add any lifetime constraints ## Why this behavior? ### inference guidance via where-bounds and alias-bounds #### where-bounds ```rust 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 ```rust 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 ```rust 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: ```rust 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](rust-lang#124592 (comment)). ## 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 rust-lang#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 rust-lang#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`. https://github.com/rust-lang/rust/blob/a4cedecc9ec76b46dcbb954750068c832cf2dd43/compiler/rustc_infer/src/infer/canonical/canonicalizer.rs#L49-L64 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](rust-lang/trait-system-refactor-initiative#30). An alternative would be to eagerly detect trivial where-bounds when constructing the `ParamEnv`. We can't entirely drop them [as explained above](https://hackmd.io/qoesqyzVTe2v9cOgFXd2SQ#Consider-true-global-where-bounds-at-all), 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`

lcnr added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-types Relevant to the types team, which will review and decide on the PR/issue. labels May 1, 2024

rustbot assigned compiler-errors May 1, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2024

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the winnowing-xd branch from 5b84662 to 2634b4b Compare May 1, 2024 18:47

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 1, 2024

lcnr removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label May 1, 2024

rust-cloud-vms bot force-pushed the winnowing-xd branch from 2634b4b to 7891306 Compare May 1, 2024 18:53

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label May 1, 2024

This comment was marked as outdated.

Sign in to view

lcnr added 4 commits May 1, 2024 18:54

add winnowing tests

2017214

remove unreachable code

1395937

refactor winnowing

b2ac4e5

small coherence cleanup

a0359c8

rust-cloud-vms bot force-pushed the winnowing-xd branch from 7891306 to a0359c8 Compare May 1, 2024 18:54

lcnr removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label May 1, 2024

lcnr changed the title ~~cleaning candidate_should_be_dropped_in_favor_of~~ cleanup candidate_should_be_dropped_in_favor_of May 1, 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 May 1, 2024

craterbot removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 6, 2024

craterbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2024

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 18, 2024

compiler-errors added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2024

Dylan-DPC added the S-waiting-on-concerns Status: Awaiting concerns to be addressed by the author label Oct 24, 2024

lcnr mentioned this pull request Oct 29, 2024

rework winnowing to sensibly handle global where-bounds #132325

Merged

lcnr closed this Nov 14, 2024

rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup `candidate_should_be_dropped_in_favor_of` #124592

cleanup `candidate_should_be_dropped_in_favor_of` #124592

lcnr commented May 1, 2024 •

edited

Loading

This comment has been minimized.

This comment was marked as outdated.

lcnr commented May 1, 2024

bors commented May 1, 2024

bors commented May 1, 2024

compiler-errors commented May 1, 2024

craterbot commented May 1, 2024

craterbot commented May 4, 2024

craterbot commented May 6, 2024

fmease commented May 10, 2024 •

edited

Loading

fmease commented May 17, 2024 •

edited

Loading

lcnr commented May 18, 2024

rfcbot commented May 18, 2024 •

edited by jackh726

Loading

BoxyUwU commented May 22, 2024

lcnr commented May 22, 2024

lcnr commented Jun 14, 2024

nikomatsakis commented Jun 17, 2024

lcnr commented Nov 14, 2024

cleanup candidate_should_be_dropped_in_favor_of #124592

cleanup candidate_should_be_dropped_in_favor_of #124592

Conversation

lcnr commented May 1, 2024 • edited Loading

This comment has been minimized.

This comment was marked as outdated.

lcnr commented May 1, 2024

bors commented May 1, 2024

bors commented May 1, 2024

compiler-errors commented May 1, 2024

craterbot commented May 1, 2024

craterbot commented May 4, 2024

craterbot commented May 6, 2024

fmease commented May 10, 2024 • edited Loading

fmease commented May 17, 2024 • edited Loading

lcnr commented May 18, 2024

rfcbot commented May 18, 2024 • edited by jackh726 Loading

BoxyUwU commented May 22, 2024

lcnr commented May 22, 2024

lcnr commented Jun 14, 2024

nikomatsakis commented Jun 17, 2024

lcnr commented Nov 14, 2024

cleanup `candidate_should_be_dropped_in_favor_of` #124592

cleanup `candidate_should_be_dropped_in_favor_of` #124592

lcnr commented May 1, 2024 •

edited

Loading

fmease commented May 10, 2024 •

edited

Loading

fmease commented May 17, 2024 •

edited

Loading

rfcbot commented May 18, 2024 •

edited by jackh726

Loading