Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods #129449

coolreader18 · 2024-08-23T05:24:07Z

Tracking issue: #86918

Based on the suggestion in #86918 (comment)

Some advantages:

Synergy with the existing as_ref and as_mut signatures (stable since Rust 1.33)

Lifetime elision reduces noise in the signature

Turbofish less verbose: Pin::<&mut T>::as_deref_mut vs Pin::<&mut Pin<&mut T>>::as_deref_mut

The comment seemed to imply that Pin::as_ref and Pin::as_mut already share an impl block, which they don't. So, I rearranged it so that they do, and we can see which looks better in the docs.

Docs screenshots

Current nightly:

Pin::as_deref_mut moved into the same block as as_mut:

Pin::as_ref, as_mut, and as_deref_mut all in the same block:

I think I like the last one the most; obviously I'm biased since I'm the one who rearranged it, but it doesn't make sense to me to have as_ref methods split up by an into_inner method.

r? dtolnay

dtolnay

Thank you!

library/core/src/pin.rs

coolreader18 · 2024-08-23T18:05:47Z

I guess the other option is to put into_inner_unchecked at the end of the block - it's certainly less used than the other methods would be, but then that messes up the synchrony with the new() & into_inner() methods being next to each other for Unpin types. Though that is how it is currently anyway. I'm gonna try that.

coolreader18 · 2024-08-23T18:08:32Z

Pin::new_unchecked followed directly by the as_ref methods:

dtolnay · 2024-08-23T18:25:13Z

@bors r+

bors · 2024-08-23T18:25:17Z

📌 Commit c65ef3d has been approved by dtolnay

It is now in the queue for this repository.

…ature, r=dtolnay Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods Tracking issue: rust-lang#86918 Based on the suggestion in rust-lang#86918 (comment) > Some advantages: > > * Synergy with the existing `as_ref` and `as_mut` signatures (stable since Rust 1.33) > > * Lifetime elision reduces noise in the signature > > * Turbofish less verbose: `Pin::<&mut T>::as_deref_mut` vs `Pin::<&mut Pin<&mut T>>::as_deref_mut` The comment seemed to imply that `Pin::as_ref` and `Pin::as_mut` already share an impl block, which they don't. So, I rearranged it so that they do, and we can see which looks better in the docs. <details><summary>Docs screenshots</summary> Current nightly: ![image](https://github.com/user-attachments/assets/b432cb82-8f4b-48ae-bafc-2fe49d0ad48c) `Pin::as_deref_mut` moved into the same block as `as_mut`: ![image](https://github.com/user-attachments/assets/f9b35722-6a88-4465-ad1c-28d8e91902ac) `Pin::as_ref`, `as_mut`, and `as_deref_mut` all in the same block: ![image](https://github.com/user-attachments/assets/9a1b2bf0-70a6-4751-b13f-390f1d575244) </details> I think I like the last one the most; obviously I'm biased since I'm the one who rearranged it, but it doesn't make sense to me to have `as_ref` methods split up by an `into_inner` method. r? dtolnay

…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#127021 (Add target support for RTEMS Arm) - rust-lang#128467 (Detect `*` operator on `!Sized` expression) - rust-lang#128735 (Add a special case for `CStr`/`CString` in the `improper_ctypes` lint) - rust-lang#129416 (library: Move unstable API of new_uninit to new features) - rust-lang#129418 (rustc: Simplify getting sysroot library directory) - rust-lang#129429 (Print the generic parameter along with the variance in dumps.) - rust-lang#129430 (rustdoc: show exact case-sensitive matches first) - rust-lang#129449 (Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods) - rust-lang#129481 (Update `compiler_builtins` to `0.1.121`) - rust-lang#129482 (Add myself to the review rotation for libs) r? `@ghost` `@rustbot` modify labels: rollup

…ature, r=dtolnay Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods Tracking issue: rust-lang#86918 Based on the suggestion in rust-lang#86918 (comment) > Some advantages: > > * Synergy with the existing `as_ref` and `as_mut` signatures (stable since Rust 1.33) > > * Lifetime elision reduces noise in the signature > > * Turbofish less verbose: `Pin::<&mut T>::as_deref_mut` vs `Pin::<&mut Pin<&mut T>>::as_deref_mut` The comment seemed to imply that `Pin::as_ref` and `Pin::as_mut` already share an impl block, which they don't. So, I rearranged it so that they do, and we can see which looks better in the docs. <details><summary>Docs screenshots</summary> Current nightly: ![image](https://github.com/user-attachments/assets/b432cb82-8f4b-48ae-bafc-2fe49d0ad48c) `Pin::as_deref_mut` moved into the same block as `as_mut`: ![image](https://github.com/user-attachments/assets/f9b35722-6a88-4465-ad1c-28d8e91902ac) `Pin::as_ref`, `as_mut`, and `as_deref_mut` all in the same block: ![image](https://github.com/user-attachments/assets/9a1b2bf0-70a6-4751-b13f-390f1d575244) </details> I think I like the last one the most; obviously I'm biased since I'm the one who rearranged it, but it doesn't make sense to me to have `as_ref` methods split up by an `into_inner` method. r? dtolnay

…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#127021 (Add target support for RTEMS Arm) - rust-lang#128467 (Detect `*` operator on `!Sized` expression) - rust-lang#128524 (Don't suggest turning crate-level attributes into outer style) - rust-lang#128735 (Add a special case for `CStr`/`CString` in the `improper_ctypes` lint) - rust-lang#129418 (rustc: Simplify getting sysroot library directory) - rust-lang#129429 (Print the generic parameter along with the variance in dumps.) - rust-lang#129430 (rustdoc: show exact case-sensitive matches first) - rust-lang#129449 (Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods) - rust-lang#129481 (Update `compiler_builtins` to `0.1.121`) - rust-lang#129482 (Add myself to the review rotation for libs) - rust-lang#129492 (make text more easy to read) r? `@ghost` `@rustbot` modify labels: rollup

…ature, r=dtolnay Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods Tracking issue: rust-lang#86918 Based on the suggestion in rust-lang#86918 (comment) > Some advantages: > > * Synergy with the existing `as_ref` and `as_mut` signatures (stable since Rust 1.33) > > * Lifetime elision reduces noise in the signature > > * Turbofish less verbose: `Pin::<&mut T>::as_deref_mut` vs `Pin::<&mut Pin<&mut T>>::as_deref_mut` The comment seemed to imply that `Pin::as_ref` and `Pin::as_mut` already share an impl block, which they don't. So, I rearranged it so that they do, and we can see which looks better in the docs. <details><summary>Docs screenshots</summary> Current nightly: ![image](https://github.com/user-attachments/assets/b432cb82-8f4b-48ae-bafc-2fe49d0ad48c) `Pin::as_deref_mut` moved into the same block as `as_mut`: ![image](https://github.com/user-attachments/assets/f9b35722-6a88-4465-ad1c-28d8e91902ac) `Pin::as_ref`, `as_mut`, and `as_deref_mut` all in the same block: ![image](https://github.com/user-attachments/assets/9a1b2bf0-70a6-4751-b13f-390f1d575244) </details> I think I like the last one the most; obviously I'm biased since I'm the one who rearranged it, but it doesn't make sense to me to have `as_ref` methods split up by an `into_inner` method. r? dtolnay

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#127021 (Add target support for RTEMS Arm) - rust-lang#128467 (Detect `*` operator on `!Sized` expression) - rust-lang#128524 (Don't suggest turning crate-level attributes into outer style) - rust-lang#128735 (Add a special case for `CStr`/`CString` in the `improper_ctypes` lint) - rust-lang#129429 (Print the generic parameter along with the variance in dumps.) - rust-lang#129430 (rustdoc: show exact case-sensitive matches first) - rust-lang#129449 (Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods) - rust-lang#129481 (Update `compiler_builtins` to `0.1.121`) - rust-lang#129482 (Add myself to the review rotation for libs) - rust-lang#129492 (make text more easy to read) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 10 pull requests Successful merges: - rust-lang#128467 (Detect `*` operator on `!Sized` expression) - rust-lang#128524 (Don't suggest turning crate-level attributes into outer style) - rust-lang#128735 (Add a special case for `CStr`/`CString` in the `improper_ctypes` lint) - rust-lang#129429 (Print the generic parameter along with the variance in dumps.) - rust-lang#129430 (rustdoc: show exact case-sensitive matches first) - rust-lang#129449 (Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods) - rust-lang#129481 (Update `compiler_builtins` to `0.1.121`) - rust-lang#129482 (Add myself to the review rotation for libs) - rust-lang#129492 (Make wasm32 platform support docs easier to read) - rust-lang#129512 (update the doc comment on lintchecker b/c it parses html now) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129449 - coolreader18:pin-as_deref_mut-signature, r=dtolnay Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods Tracking issue: rust-lang#86918 Based on the suggestion in rust-lang#86918 (comment) > Some advantages: > > * Synergy with the existing `as_ref` and `as_mut` signatures (stable since Rust 1.33) > > * Lifetime elision reduces noise in the signature > > * Turbofish less verbose: `Pin::<&mut T>::as_deref_mut` vs `Pin::<&mut Pin<&mut T>>::as_deref_mut` The comment seemed to imply that `Pin::as_ref` and `Pin::as_mut` already share an impl block, which they don't. So, I rearranged it so that they do, and we can see which looks better in the docs. <details><summary>Docs screenshots</summary> Current nightly: ![image](https://github.com/user-attachments/assets/b432cb82-8f4b-48ae-bafc-2fe49d0ad48c) `Pin::as_deref_mut` moved into the same block as `as_mut`: ![image](https://github.com/user-attachments/assets/f9b35722-6a88-4465-ad1c-28d8e91902ac) `Pin::as_ref`, `as_mut`, and `as_deref_mut` all in the same block: ![image](https://github.com/user-attachments/assets/9a1b2bf0-70a6-4751-b13f-390f1d575244) </details> I think I like the last one the most; obviously I'm biased since I'm the one who rearranged it, but it doesn't make sense to me to have `as_ref` methods split up by an `into_inner` method. r? dtolnay

ogoffart · 2024-08-27T20:02:56Z

Btw, the changes of bounds in Pin::set from this PR most likely causes regression described in #129601

coolreader18 · 2024-08-27T20:54:18Z

Ah, shoot, that makes sense.

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

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`

rustbot assigned dtolnay Aug 23, 2024

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 23, 2024

dtolnay reviewed Aug 23, 2024

View reviewed changes

library/core/src/pin.rs Outdated Show resolved Hide resolved

Put Pin::as_deref_mut in impl Pin<Ptr>

b968b26

coolreader18 force-pushed the pin-as_deref_mut-signature branch from 249e2da to b968b26 Compare August 23, 2024 17:45

Move into_inner_unchecked back to the bottom of the impl block

c65ef3d

dtolnay approved these changes Aug 23, 2024

View reviewed changes

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 23, 2024

workingjubilee mentioned this pull request Aug 24, 2024

Rollup of 10 pull requests #129495

Closed

workingjubilee mentioned this pull request Aug 24, 2024

Rollup of 10 pull requests #129496

Closed

workingjubilee mentioned this pull request Aug 24, 2024

Rollup of 10 pull requests #129497

Closed

matthiaskrgr mentioned this pull request Aug 24, 2024

Rollup of 11 pull requests #129502

Closed

matthiaskrgr mentioned this pull request Aug 24, 2024

Rollup of 10 pull requests #129506

Closed

tgross35 mentioned this pull request Aug 25, 2024

Rollup of 10 pull requests #129540

Merged

bors merged commit e96faab into rust-lang:master Aug 25, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 25, 2024

coolreader18 deleted the pin-as_deref_mut-signature branch August 25, 2024 18:04

ogoffart mentioned this pull request Aug 26, 2024

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

Closed

dtolnay mentioned this pull request Aug 27, 2024

Tracking Issue for pin_deref_mut #86918

Closed

3 tasks

coolreader18 mentioned this pull request Aug 27, 2024

Fix Pin::set bounds regression #129668

Merged

compiler-errors mentioned this pull request Aug 27, 2024

Method probe should consider where clauses on method #129669

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods #129449

Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods #129449

coolreader18 commented Aug 23, 2024 •

edited

Loading

dtolnay left a comment

coolreader18 commented Aug 23, 2024

coolreader18 commented Aug 23, 2024

dtolnay commented Aug 23, 2024

bors commented Aug 23, 2024

ogoffart commented Aug 27, 2024

coolreader18 commented Aug 27, 2024

Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods #129449

Put Pin::as_deref_mut in impl Pin<Ptr> / rearrange Pin methods #129449

Conversation

coolreader18 commented Aug 23, 2024 • edited Loading

dtolnay left a comment

Choose a reason for hiding this comment

coolreader18 commented Aug 23, 2024

coolreader18 commented Aug 23, 2024

dtolnay commented Aug 23, 2024

bors commented Aug 23, 2024

ogoffart commented Aug 27, 2024

coolreader18 commented Aug 27, 2024

coolreader18 commented Aug 23, 2024 •

edited

Loading