-Znext-solver: modify candidate preference rules #133643

lcnr · 2024-11-29T17:48:50Z

This implements the design proposed in the FCP in #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 #133639 which encounters an ICE in negative coherence when evaluating the where-clause. Given the features required to trigger this ICE 🤷

r? @compiler-errors

[DO NOT MERGE] bootstrap with `-Znext-solver=globally` A revival of rust-lang#124812. Current status: ~~`./x.py b --stage 2` passes 🎉~~ `try` builds succeed 🎉 🎉 🎉 ### commits - rust-lang#133643 - ce66d92 is a rebased version of rust-lang#125334, unsure whether I actually want to land this PR for now * rust-lang#133559 * rust-lang#133558 r? `@ghost`

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`

bors · 2024-12-04T20:56:41Z

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

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 · 2024-12-17T13:33:23Z

ready for review

compiler/rustc_middle/src/ty/mod.rs

compiler/rustc_next_trait_solver/src/solve/trait_goals.rs

compiler-errors · 2024-12-18T15:38:03Z

r=me

lcnr · 2024-12-18T15:50:31Z

@bors r+ rollup

bors · 2024-12-18T15:50:34Z

📌 Commit 5fa4b09 has been approved by lcnr

It is now in the queue for this repository.

lcnr · 2024-12-18T15:50:38Z

@bors r=compiler-errors

bors · 2024-12-18T15:50:41Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: ci: Move dist-aarch64-linux to an aarch64 runner #133809

bors · 2024-12-18T15:50:41Z

📌 Commit 5fa4b09 has been approved by compiler-errors

It is now in the queue for this repository.

…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`

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#132056 (Stabilize `#[diagnostic::do_not_recommend]`) - rust-lang#133643 (-Znext-solver: modify candidate preference rules) - rust-lang#134418 (Advent of `tests/ui` (misc cleanups and improvements) [3/N]) - rust-lang#134432 (Fix intra doc links not generated inside footnote definitions) - rust-lang#134473 (chore: fix some typos) - rust-lang#134474 (Forbid overwriting types in typeck) - rust-lang#134477 (move lint_unused_mut into sub-fn) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 8 pull requests Successful merges: - rust-lang#132056 (Stabilize `#[diagnostic::do_not_recommend]`) - rust-lang#133643 (-Znext-solver: modify candidate preference rules) - rust-lang#134388 (Update books) - rust-lang#134418 (Advent of `tests/ui` (misc cleanups and improvements) [3/N]) - rust-lang#134473 (chore: fix some typos) - rust-lang#134481 (Point at lint name instead of whole attr for gated lints) - rust-lang#134484 (Add nnethercote to the `triagebot.toml` vacation list.) - rust-lang#134490 (Fix typo in ptr/mod.rs) r? `@ghost` `@rustbot` modify labels: rollup

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 Nov 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Nov 29, 2024

lcnr mentioned this pull request Nov 29, 2024

[DO NOT MERGE] bootstrap with -Znext-solver=globally #133502

Draft

lcnr force-pushed the merge-candidates branch from ec553cd to 1cbbc83 Compare November 29, 2024 17:52

lcnr mentioned this pull request Nov 29, 2024

rework winnowing to sensibly handle global where-bounds #132325

Merged

This comment has been minimized.

Sign in to view

lcnr mentioned this pull request Nov 30, 2024

useless_attribute incorrectly triggers for unknown tool attributes on use rust-lang/rust-clippy#13764

Closed

This comment has been minimized.

Sign in to view

lcnr force-pushed the merge-candidates branch from 256a39f to 92a2ce9 Compare December 2, 2024 10:30

lcnr force-pushed the merge-candidates branch from 92a2ce9 to 4f5e010 Compare December 17, 2024 13:31

compiler-errors reviewed Dec 17, 2024

View reviewed changes

compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_next_trait_solver/src/solve/trait_goals.rs Outdated Show resolved Hide resolved

compiler/rustc_next_trait_solver/src/solve/trait_goals.rs Show resolved Hide resolved

canonicalizer: keep 'static in the param_env

38ce731

lcnr force-pushed the merge-candidates branch from 4f5e010 to 709b0bf Compare December 18, 2024 15:32

lcnr mentioned this pull request Dec 18, 2024

discard_impls_shadowed_by_env forces unnecessary ambiguity rust-lang/trait-system-refactor-initiative#96

Closed

lcnr force-pushed the merge-candidates branch from 709b0bf to c7d0dfc Compare December 18, 2024 15:33

lcnr added 3 commits December 18, 2024 16:35

update new solver candidate assembly

d5a0c5c

add tests

f0ae48d

resolve FIXME

5fa4b09

lcnr force-pushed the merge-candidates branch from c7d0dfc to 5fa4b09 Compare December 18, 2024 15:35

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 18, 2024

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 18, 2024

matthiaskrgr mentioned this pull request Dec 18, 2024

Rollup of 7 pull requests #134480

Closed

jhpratt mentioned this pull request Dec 19, 2024

Rollup of 8 pull requests #134492

Merged

bors merged commit 4cd157e into rust-lang:master Dec 19, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 19, 2024

lcnr deleted the merge-candidates branch December 19, 2024 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

-Znext-solver: modify candidate preference rules #133643

-Znext-solver: modify candidate preference rules #133643

lcnr commented Nov 29, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

bors commented Dec 4, 2024

lcnr commented Dec 17, 2024

compiler-errors commented Dec 18, 2024

lcnr commented Dec 18, 2024

bors commented Dec 18, 2024

lcnr commented Dec 18, 2024

bors commented Dec 18, 2024

bors commented Dec 18, 2024

-Znext-solver: modify candidate preference rules #133643

-Znext-solver: modify candidate preference rules #133643

Conversation

lcnr commented Nov 29, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

bors commented Dec 4, 2024

lcnr commented Dec 17, 2024

compiler-errors commented Dec 18, 2024

lcnr commented Dec 18, 2024

bors commented Dec 18, 2024

lcnr commented Dec 18, 2024

bors commented Dec 18, 2024

bors commented Dec 18, 2024

lcnr commented Nov 29, 2024 •

edited

Loading