Skip to content

Fix Pin::set bounds regression #129668

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

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Conversation

coolreader18
Copy link
Contributor

@coolreader18 coolreader18 commented Aug 27, 2024

Fixes #129601

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

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 27, 2024
@coolreader18
Copy link
Contributor Author

This is kinda subtle - I guess theoretically, this could be deemed an acceptable breaking change, but I don't think it's worth it given it's already demonstrated breakage.

@coolreader18
Copy link
Contributor Author

Theoretically the same could happen with as_mut(), though it's much less likely that there would be a method named as_mut that takes &self

@coolreader18
Copy link
Contributor Author

Moved all methods back to their original block, just to be safe. I still kept the order of the methods, which means that new_unchecked and into_inner_unchecked are in separate impl blocks that both have Ptr: Deref bounds.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Aug 27, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 27, 2024

📌 Commit 0d6c915 has been approved by dtolnay

It is now in the queue for this repository.

@bors 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 Aug 27, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#129421 (add repr to the allowlist for naked functions)
 - rust-lang#129480 (docs: correct panic conditions for rem_euclid and similar functions)
 - rust-lang#129551 (ub_checks intrinsics: fall back to cfg(ub_checks))
 - rust-lang#129608 (const-eval: do not make UbChecks behavior depend on current crate's flags)
 - rust-lang#129613 (interpret: do not make const-eval query result depend on tcx.sess)
 - rust-lang#129641 (rustdoc: fix missing resource suffix on `crates.js`)
 - rust-lang#129657 (Rename `BikeshedIntrinsicFrom` to `TransmuteFrom`)
 - rust-lang#129666 (interpret: add missing alignment check in raw_eq)
 - rust-lang#129667 (Rustc driver cleanup)
 - rust-lang#129668 (Fix Pin::set bounds regression)
 - rust-lang#129686 (coverage: Rename `CodeRegion` to `SourceRegion`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#129421 (add repr to the allowlist for naked functions)
 - rust-lang#129480 (docs: correct panic conditions for rem_euclid and similar functions)
 - rust-lang#129551 (ub_checks intrinsics: fall back to cfg(ub_checks))
 - rust-lang#129608 (const-eval: do not make UbChecks behavior depend on current crate's flags)
 - rust-lang#129613 (interpret: do not make const-eval query result depend on tcx.sess)
 - rust-lang#129641 (rustdoc: fix missing resource suffix on `crates.js`)
 - rust-lang#129657 (Rename `BikeshedIntrinsicFrom` to `TransmuteFrom`)
 - rust-lang#129666 (interpret: add missing alignment check in raw_eq)
 - rust-lang#129667 (Rustc driver cleanup)
 - rust-lang#129668 (Fix Pin::set bounds regression)
 - rust-lang#129686 (coverage: Rename `CodeRegion` to `SourceRegion`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 27d7fb0 into rust-lang:master Aug 28, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request 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`
@coolreader18 coolreader18 deleted the fix-pin-set-regr branch August 29, 2024 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regressions caused by changed of bounds on Pin::set
4 participants