instantiate higher ranked goals in candidate selection again #127568

lcnr · 2024-07-10T12:23:20Z

This reverts #119820 as that PR has a significant impact and breaks code which feels like it should work. The impact ended up being larger than we expected during the FCP and we've ended up with some ideas for how we can work around this issue in the next solver. This has been discussed in the previous high bandwidth t-types meeting: https://rust-lang.zulipchat.com/#narrow/stream/326132-t-types.2Fmeetings/topic/2024-07-09.20high.20bandwidth.20meeting.

We'll therefore keep this inconsistency between the two solvers for now and will have to deal with it before stabilizating the use of the new solver outside of coherence: rust-lang/trait-system-refactor-initiative#120.

fixes #125194 after a beta-backport.

The pattern which is more widely used than expected and feels like it should work, especially without deep knowledge of the type system is

trait Trait<'a> {}
impl<'a, T> Trait<'a> for T {}

fn trait_bound<T: for<'a> Trait<'a>>() {}

// A function with a where-bound which is more restrictive than the impl.
fn function1<T: Trait<'static>>() {
    // stable: ok
    // with #119820: error as we prefer the where-bound over the impl
    // with this PR: back to ok
    trait_bound::<T>();
}

r? @rust-lang/types

reverts rust-lang#119820

oli-obk · 2024-07-10T13:08:40Z

@bors r+

bors · 2024-07-10T13:08:43Z

📌 Commit f77394f has been approved by oli-obk

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#126476 (Fix running bootstrap tests with a local Rust toolchain as the stage0) - rust-lang#127094 (E0191 suggestion correction, inserts turbofish) - rust-lang#127554 ( do not run test where it cannot run) - rust-lang#127564 (Temporarily remove me from review rotation.) - rust-lang#127568 (instantiate higher ranked goals in candidate selection again) - rust-lang#127569 (Fix local download of Docker caches from CI) - rust-lang#127570 ( small normalization improvement) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#127568 - lcnr:undo-leakcheck, r=oli-obk instantiate higher ranked goals in candidate selection again This reverts rust-lang#119820 as that PR has a significant impact and breaks code which *feels like it should work*. The impact ended up being larger than we expected during the FCP and we've ended up with some ideas for how we can work around this issue in the next solver. This has been discussed in the previous high bandwidth t-types meeting: https://rust-lang.zulipchat.com/#narrow/stream/326132-t-types.2Fmeetings/topic/2024-07-09.20high.20bandwidth.20meeting. We'll therefore keep this inconsistency between the two solvers for now and will have to deal with it before stabilizating the use of the new solver outside of coherence: rust-lang/trait-system-refactor-initiative#120. fixes rust-lang#125194 after a beta-backport. The pattern which is more widely used than expected and feels like it should work, especially without deep knowledge of the type system is ```rust trait Trait<'a> {} impl<'a, T> Trait<'a> for T {} fn trait_bound<T: for<'a> Trait<'a>>() {} // A function with a where-bound which is more restrictive than the impl. fn function1<T: Trait<'static>>() { // stable: ok // with rust-lang#119820: error as we prefer the where-bound over the impl // with this PR: back to ok trait_bound::<T>(); } ``` r? `@rust-lang/types`

apiraino · 2024-07-11T14:12:21Z

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted

[beta] backports - Update LLVM submodule rust-lang#127364 - instantiate higher ranked goals in candidate selection again rust-lang#127568 r? cuviper

`-Znext-solver` instantiate predicate binder without recanonicalizing goal This strengthens the leak check to match the old trait solver. The new trait solver now also instantiates higher ranked goals in the same scope as candidate selection, so the leak check in each candidate detects placeholder errors involving this higher ranked goal. E.g. let's look at tests/ui/higher-ranked/leak-check/leak-check-in-selection-2.rs ```rust trait Trait<T, U> {} impl<'a> Trait<&'a str, &'a str> for () {} impl<'a> Trait<&'a str, String> for () {} fn impls_trait<T: for<'a> Trait<&'a str, U>, U>() {} fn main() { impls_trait::<(), _>(); } ``` Here proving `(): for<'a> Trait<&'a str, ?u>` via `impl<'a> Trait<&'a str, &'a str> for ()` equates `?u` with `&'!a str` which results in a leak check error as `?u` cannot name `'a`. If this leak check error happens while considering candidates we drop the first impl and infer `?u` to `String`. If not, this remains ambiguous. This behavior is a bit iffy, see the FCP proposal in #119820 for more details on why this current behavior is somewhat undesirable. However, considering placeholders from higher-ranked goals for candidate selection does allow more code to compile and a lot of the code *feels like it should compile*. **This caused us to revert the change of #119820 in #127568.** I originally expected that we can avoid breakage with the new solver differently here, e.g. by considering OR-region constraints. However, doing so is a significant change and I don't have a great idea for how that should work. Matching the old solver behavior for now should not make this cleaner approach any more difficult in the future, so let's just go with what actually allows us to stabilize the new solver for now. This PR changing the new solver to match the behavior of the old one wrt the leak check. As the new solver is already used by default in coherence, this allows more code to compile, see `tests/ui/higher-ranked/leak-check/leak-check-in-selection-7-coherence.rs`: ```rust struct W<T, U>(T, U); trait Trait<T> {} // using this impl results in a higher-ranked region error. impl<'a> Trait<W<&'a str, &'a str>> for () {} impl<'a> Trait<W<&'a str, String>> for () {} trait NotString {} impl NotString for &str {} impl NotString for u32 {} trait Overlap {} impl<T: for<'a> Trait<W<&'a str, U>>, U> Overlap for T {} impl<U: NotString> Overlap for () {} fn main() {} ``` This behavior is quite arbitrary and not something I expect users to rely on in practice, however, it should still go through an FCP imo. r? `@BoxyUwU` originally implemented by `@compiler-errors` in #136997. Closes rust-lang/trait-system-refactor-initiative#120.

`-Znext-solver` instantiate predicate binder without recanonicalizing goal This strengthens the leak check to match the old trait solver. The new trait solver now also instantiates higher ranked goals in the same scope as candidate selection, so the leak check in each candidate detects placeholder errors involving this higher ranked goal. E.g. let's look at tests/ui/higher-ranked/leak-check/leak-check-in-selection-2.rs ```rust trait Trait<T, U> {} impl<'a> Trait<&'a str, &'a str> for () {} impl<'a> Trait<&'a str, String> for () {} fn impls_trait<T: for<'a> Trait<&'a str, U>, U>() {} fn main() { impls_trait::<(), _>(); } ``` Here proving `(): for<'a> Trait<&'a str, ?u>` via `impl<'a> Trait<&'a str, &'a str> for ()` equates `?u` with `&'!a str` which results in a leak check error as `?u` cannot name `'a`. If this leak check error happens while considering candidates we drop the first impl and infer `?u` to `String`. If not, this remains ambiguous. This behavior is a bit iffy, see the FCP proposal in rust-lang/rust#119820 for more details on why this current behavior is somewhat undesirable. However, considering placeholders from higher-ranked goals for candidate selection does allow more code to compile and a lot of the code *feels like it should compile*. **This caused us to revert the change of rust-lang/rust#119820 in rust-lang/rust#127568.** I originally expected that we can avoid breakage with the new solver differently here, e.g. by considering OR-region constraints. However, doing so is a significant change and I don't have a great idea for how that should work. Matching the old solver behavior for now should not make this cleaner approach any more difficult in the future, so let's just go with what actually allows us to stabilize the new solver for now. This PR changing the new solver to match the behavior of the old one wrt the leak check. As the new solver is already used by default in coherence, this allows more code to compile, see `tests/ui/higher-ranked/leak-check/leak-check-in-selection-7-coherence.rs`: ```rust struct W<T, U>(T, U); trait Trait<T> {} // using this impl results in a higher-ranked region error. impl<'a> Trait<W<&'a str, &'a str>> for () {} impl<'a> Trait<W<&'a str, String>> for () {} trait NotString {} impl NotString for &str {} impl NotString for u32 {} trait Overlap {} impl<T: for<'a> Trait<W<&'a str, U>>, U> Overlap for T {} impl<U: NotString> Overlap for () {} fn main() {} ``` This behavior is quite arbitrary and not something I expect users to rely on in practice, however, it should still go through an FCP imo. r? `@BoxyUwU` originally implemented by `@compiler-errors` in rust-lang/rust#136997. Closes rust-lang/trait-system-refactor-initiative#120.

instantiate higher ranked goals in candidate selection

f77394f

reverts rust-lang#119820

lcnr added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 10, 2024

rustbot assigned BoxyUwU Jul 10, 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 Jul 10, 2024

This was referenced Jul 10, 2024

higher-ranked goals in trait goal candidate selection rust-lang/trait-system-refactor-initiative#120

Closed

instantiate higher ranked goals outside of candidate selection #119820

Merged

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 Jul 10, 2024

oli-obk assigned oli-obk and unassigned BoxyUwU Jul 10, 2024

lcnr mentioned this pull request Jul 10, 2024

remove unnecessary trait bound renato145/docker_queue#1

Closed

matthiaskrgr mentioned this pull request Jul 10, 2024

Rollup of 7 pull requests #127580

Merged

bors merged commit 7faef5d into rust-lang:master Jul 10, 2024

rustbot added this to the 1.81.0 milestone Jul 10, 2024

lcnr deleted the undo-leakcheck branch July 11, 2024 07:40

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 11, 2024

cuviper mentioned this pull request Jul 11, 2024

[beta] backports #127618

Merged

cuviper modified the milestones: 1.81.0, 1.80.0 Jul 11, 2024

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 11, 2024

apiraino mentioned this pull request Jul 18, 2024

regression: trait bound is not satisfied Cow/Borrow<str> #127339

Closed

lcnr mentioned this pull request Sep 18, 2025

-Znext-solver instantiate predicate binder without recanonicalizing goal #146725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

instantiate higher ranked goals in candidate selection again #127568

instantiate higher ranked goals in candidate selection again #127568

Uh oh!

lcnr commented Jul 10, 2024

Uh oh!

oli-obk commented Jul 10, 2024

Uh oh!

bors commented Jul 10, 2024

Uh oh!

apiraino commented Jul 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

instantiate higher ranked goals in candidate selection again #127568

instantiate higher ranked goals in candidate selection again #127568

Uh oh!

Conversation

lcnr commented Jul 10, 2024

Uh oh!

oli-obk commented Jul 10, 2024

Uh oh!

bors commented Jul 10, 2024

Uh oh!

apiraino commented Jul 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants