Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regressions caused by changed of bounds on Pin::set #129601

Closed
ogoffart opened this issue Aug 26, 2024 · 2 comments · Fixed by #129668
Closed

Regressions caused by changed of bounds on Pin::set #129601

ogoffart opened this issue Aug 26, 2024 · 2 comments · Fixed by #129668
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ogoffart
Copy link
Contributor

Our CI started failing with the nightly compiler, for a day.

Code

I tried this code:

struct Foo {}
impl Foo {
    fn set(&self, _: bool) {}
}

fn main() {
    let foo = Foo {};
    let foo : core::pin::Pin<&Foo> = core::pin::Pin::new(&foo);
    foo.set(true);
}

It compiles in stable, but not with the nightly compiler:

error[E0277]: the trait bound `&Foo: DerefMut` is not satisfied
    --> src/main.rs:27:9
     |
27   |     foo.set(true);
     |         ^^^ the trait `DerefMut` is not implemented for `&Foo`
     |
note: required by a bound in `Pin::<Ptr>::set`
    --> /home/olivier/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/pin.rs:1478:14
     |
1476 |     pub fn set(&mut self, value: Ptr::Target)
     |            --- required by a bound in this associated function
1477 |     where
1478 |         Ptr: DerefMut,
     |              ^^^^^^^^ required by this bound in `Pin::<Ptr>::set`
help: consider mutably borrowing here
     |
27   |     (&mut foo).set(true);
     |     +++++    +

error[E0308]: mismatched types
    --> src/main.rs:27:13
     |
27   |     foo.set(true);
     |         --- ^^^^ expected `Foo`, found `bool`
     |         |
     |         arguments to this method are incorrect
     |
note: method defined here
    --> /home/olivier/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/pin.rs:1476:12
     |
1476 |     pub fn set(&mut self, value: Ptr::Target)
     |            ^^^

Version it worked on

It works in beta (1.81), it only started failing with nightly very recently (one or two day)

Version with regression

rustc 1.82.0-nightly (c6db1ca3c 2024-08-25)
binary: rustc
commit-hash: c6db1ca3c93ad69692a4c4b5542f26fda4bf3aec
commit-date: 2024-08-25
host: x86_64-unknown-linux-gnu
release: 1.82.0-nightly
LLVM version: 19.1.0

Looking at the recent history, I believe the regression is caused by b968b26 (#129449)

@ogoffart ogoffart added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Aug 26, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 26, 2024
ogoffart added a commit to slint-ui/slint that referenced this issue Aug 26, 2024
@jieyouxu jieyouxu added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 26, 2024
@Skgland
Copy link
Contributor

Skgland commented Aug 26, 2024

It appears that having a looser bound on the impl block containing Pin::set causes
Pin::set to be not eliminated early as a candidate and ends up preferred over implicitly dereferencing the Pin and then <Ptr as Deref>::Target::set.

As similar result can be seen in this example with Test::test_old compared to Test::test_new
uncommenting the use of the later causes the example to no longer compile.

use core::ops::DerefMut;
use core::ops::Deref;


struct A {}

impl Deref for A {
    type Target = Q;
    
    fn deref(&self) -> &Self::Target {
        &Q
    }
}

struct Q;

impl Q {
    fn test_old(&self, b: bool) {
        println!("test_old in Q: {b}")
    }
    fn test_new(&self, b: bool) {
        println!("test_new in Q: {b}")
    }
}

struct Test<T>(T);

impl<T: Deref> Deref for Test<T> {
    type Target = T::Target;
    fn deref(&self) -> &T::Target {
        self.0.deref()
    }
}


impl<T: DerefMut> Test<T> {
    fn test_old(&mut self, _t: T::Target) where T::Target: Sized  {
        println!("test_old in Test<T>");
    }
}


impl<T: Deref> Test<T> {
    fn test_new(&mut self, _t: T::Target) where T: DerefMut, T::Target: Sized {
        println!("test_new in Test<T>");
    }
}

fn main() {
    Test(A{}).test_old(true);
    //Test(A{}).test_new(true);
}

playground

@compiler-errors
Copy link
Member

For the record, this could be fixed in the type system, but likely not until we've landed the new trait solver (which will not happen in the next year or longer, in my estimation).

There's no (type-theoretical) reason that method probing cannot look at the where clauses on the method in addition to the where clauses on the impl block itself, skipping over the Pin::set method properly.

@m-ou-se m-ou-se added P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 28, 2024
@bors bors closed this as completed in 27d7fb0 Aug 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 28, 2024
Rollup merge of rust-lang#129668 - coolreader18:fix-pin-set-regr, r=dtolnay

Fix Pin::set bounds regression

Fixes rust-lang#129601

Fixes the regression from rust-lang#129449, where changing the bounds of the impl block containing `Pin::set` changed the method resolution behavior.

```rust
struct A;
impl A {
    fn set(&self) {}
}

let a: Pin<&A>;
a.set();
// before:
// - checks <impl<Ptr: DerefMut> Pin<Ptr>>::set(): `&A` doesn't impl `DerefMut`
// - autorefs -> &A: resolves to A::set()
// now:
// - checks <impl<Ptr: Deref> Pin<Ptr>>::set(): `&A` impls `Deref`! resolves to Pin::set()
// - check method bounds: `&A` doesn't impl DerefMut: error
```

r? `@dtolnay`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@m-ou-se @ogoffart @compiler-errors @Skgland @jieyouxu @rustbot and others