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

Allow Weak::as_ptr and friends for unsized T #74160

Merged
merged 6 commits into from
Oct 3, 2020

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Jul 8, 2020

Relaxes impl<T> Weak<T> to impl<T: ?Sized> Weak<T> for the methods rc::Weak::as_ptr, into_raw, and from_raw.

Follow-up to #73845, which did most of the impl work to make these functions work for T: ?Sized.

We still have to adjust the implementation of Weak::from_raw here, however, because I missed a use of ptr.is_null() previously. This check was necessary when into/from_raw were first implemented, as into_raw returned ptr::null() for dangling weak. However, we now just (wrapping) offset dangling weaks' pointers the same as nondangling weak, so the null check is no longer necessary (or even hit). (I can submit just 17a928f as a separate PR if desired.)

As a nice side effect, moves the fn is_dangling definition closer to Weak::new, which creates the dangling weak.

This technically stabilizes that "something like align_of_val_raw" is possible to do. However, I believe the part of the functionality required by these methods here -- specifically, getting the alignment of a pointee from a pointer where it may be dangling iff the pointee is Sized -- is uncontroversial enough to stabilize these methods without a way to implement them on stable Rust.

r? @RalfJung, who reviewed #73845.

ATTN: This changes (relaxes) the (input) generic bounds on stable fn!

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2020
src/liballoc/rc.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

I assume the plan is to do the same for Arc once we are done with review?

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 12, 2020

Yes.

... Though to be completely honest, I had convinced myself (somehow) that the sync versions already supported unsized values and didn't need to be tweaked. Obviously that's not true, so I don't know why I thought that...

@RalfJung
Copy link
Member

The code changes look good to me, but I'd like to see unit tests added that use all of the newly T: ?Sized with an actually unsized type.

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 14, 2020

Translated the existing into/from raw tests to also do the same for Weak. I still cannot get the Rust test harness to cmake properly on my machine, so I'm relying on CI to get test results here.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

👍 from a code perspective.

@rust-lang/libs I think this needs FCP for being an insta-stable change.

@RalfJung RalfJung added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 15, 2020
@Amanieu
Copy link
Member

Amanieu commented Jul 16, 2020

@rfcbot fcp merge

Weak::from_raw, Weak::into_raw, Weak::as_raw now support pointers to DSTs.

@rfcbot

This comment has been minimized.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 16, 2020
@dtolnay
Copy link
Member

dtolnay commented Jul 18, 2020

I think I am going to restart this with T-lang on here as well. :/

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Jul 18, 2020

@dtolnay proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 18, 2020
@dtolnay dtolnay added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jul 18, 2020
@dtolnay
Copy link
Member

dtolnay commented Jul 18, 2020

@rust-lang/libs: This makes the following usable with T: ?Sized:

@rust-lang/lang: Calling attention to the following callout in the PR description:

This technically stabilizes that "something like size_of_val_raw" is possible to do. However, I believe the part of the functionality required by these methods here -- specifically, getting the size of a pointee from a pointer where it may be dangling iff the pointee is Sized -- is uncontroversial enough to stabilize these methods without a way to implement them on stable Rust.

(From skimming the implementation I think they might have meant align_of_val_raw not size_of_val_raw, or maybe both? @CAD97 could you confirm how size_of_val_raw is involved in the implementation?)

Here is the code in question:

https://github.com/rust-lang/rust/blob/f244725afda8e124705356f40f75f15ea3af81d3/src/liballoc/rc.rs#L2121-L2137

Effectively this stabilization is committing to the following semantic being implementable internally to std:

unsafe fn align_of_val_raw<T: ?Sized>(val: *const T) -> usize {
    if T is Sized {
        mem::align_of::<T>()
    } else {
        mem::align_of_val::<T>(&*val)
    }
}

Right now we expose the same thing unstably in https://doc.rust-lang.org/nightly/std/mem/fn.align_of_val_raw.html which is tracked by #69835.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jul 18, 2020

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 18, 2020
@CAD97
Copy link
Contributor Author

CAD97 commented Jul 18, 2020

meant align_of_val_raw not size_of_val_raw

Ah, yes, only align_of_val_raw is actually needed here. I've chunked the two into the same "feature" in my head, thus the mixup 😅

@nikomatsakis
Copy link
Contributor

Notably, align_of_val is already possible and stable, so the only diff here is saying that it can be done from a raw pointer and not just a reference, right? This doesn't seem like much of anything at all to me.

@nikomatsakis
Copy link
Contributor

Ah, I see, the key point is that the pointee may be dangling if T: Sized (but not otherwise).

@nikomatsakis
Copy link
Contributor

Removing nominated label as this was discussed in today's @rust-lang/lang meeting. No concerns were raised.

@bors
Copy link
Contributor

bors commented Jul 28, 2020

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

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 13, 2020
@Amanieu
Copy link
Member

Amanieu commented Sep 13, 2020

@bors r-

Oops didn't notice @RalfJung requested changes.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 13, 2020
@RalfJung
Copy link
Member

RalfJung commented Oct 3, 2020

@CAD97 I'll just use GH suggestions to change "iff" to "if" and land that.

library/alloc/src/rc.rs Outdated Show resolved Hide resolved
library/alloc/src/sync.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Oct 3, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2020

📌 Commit e27ef13 has been approved by RalfJung

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 3, 2020
Allow Weak::as_ptr and friends for unsized T

Relaxes `impl<T> Weak<T>` to `impl<T: ?Sized> Weak<T>` for the methods `rc::Weak::as_ptr`, `into_raw`, and `from_raw`.

Follow-up to rust-lang#73845, which did most of the impl work to make these functions work for `T: ?Sized`.

We still have to adjust the implementation of `Weak::from_raw` here, however, because I missed a use of `ptr.is_null()` previously. This check was necessary when `into`/`from_raw` were first implemented, as `into_raw` returned `ptr::null()` for dangling weak. However, we now just (wrapping) offset dangling weaks' pointers the same as nondangling weak, so the null check is no longer necessary (or even hit). (I can submit just 17a928f as a separate PR if desired.)

As a nice side effect, moves the `fn is_dangling` definition closer to `Weak::new`, which creates the dangling weak.

This technically stabilizes that "something like `align_of_val_raw`" is possible to do. However, I believe the part of the functionality required by these methods here -- specifically, getting the alignment of a pointee from a pointer where it may be dangling iff the pointee is `Sized` -- is uncontroversial enough to stabilize these methods without a way to implement them on stable Rust.

r? @RalfJung, who reviewed rust-lang#73845.

ATTN: This changes (relaxes) the (input) generic bounds on stable fn!
@bors
Copy link
Contributor

bors commented Oct 3, 2020

⌛ Testing commit e27ef13 with merge 738d4a7...

@bors
Copy link
Contributor

bors commented Oct 3, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: RalfJung
Pushing 738d4a7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 3, 2020
@bors bors merged commit 738d4a7 into rust-lang:master Oct 3, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 3, 2020
@CAD97
Copy link
Contributor Author

CAD97 commented Oct 3, 2020

Sorry I wasn't able to make the change myself; I've "just" (August... wow time is fast) started grad school and that's eating up all my time now 😅

@CAD97 CAD97 deleted the weak-as-unsized-ptr branch October 3, 2020 17:50
@RalfJung
Copy link
Member

RalfJung commented Oct 3, 2020

No worries, and enjoy grad school. :)

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 16, 2021
…RalfJung

Re-stabilize Weak::as_ptr and friends for unsized T

As per [T-lang consensus](https://hackmd.io/7r3_is6uTz-163fsOV8Vfg), this uses a branch to handle the dangling case. The discussed optimization of only doing the branch in the T: ?Sized case is left for a followup patch, as doing so is not trivial (as it requires specialization) and not _obviously_ better (as it requires using `wrapping_offset` rather than `offset` more).

<details><summary>Basically said optimization</summary>

Specialize on `T: Sized`:

```rust
fn as_ptr(&self) -> *const T {
    if [ T is Sized ] || !is_dangling(ptr) {
        (ptr as *mut T).set_ptr_value( (ptr as *mut u8).wrapping_offset(data_offset) )
    } else {
        ptr::null()
    }
}

fn from_raw(*const T) -> Self {
    if [ T is Sized ] || !ptr.is_null() {
        let ptr = (ptr as *mut RcBox).set_ptr_value( (ptr as *mut u8).wrapping_offset(-data_offset) );
        Weak { ptr }
    } else {
        Weak::new()
    }
}
```

(but with more `set_ptr_value` to avoid `Sized` restrictions and maintain metadata.)

Written in this fashion, this is not a correctness-critical specialization (i.e. so long as `[ T is Sized ]` is false for unsized `T`, it can be `rand()` for sized `T` without breaking correctness), but it's still touchy, so I'd rather do it in another PR with separate review.

---
</details>

This effectively reverts rust-lang#80422 and re-establishes rust-lang#74160. T-libs [previously signed off](rust-lang#74160 (comment)) on this stable API change in rust-lang#74160.
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 16, 2021
…RalfJung

Re-stabilize Weak::as_ptr and friends for unsized T

As per [T-lang consensus](https://hackmd.io/7r3_is6uTz-163fsOV8Vfg), this uses a branch to handle the dangling case. The discussed optimization of only doing the branch in the T: ?Sized case is left for a followup patch, as doing so is not trivial (as it requires specialization) and not _obviously_ better (as it requires using `wrapping_offset` rather than `offset` more).

<details><summary>Basically said optimization</summary>

Specialize on `T: Sized`:

```rust
fn as_ptr(&self) -> *const T {
    if [ T is Sized ] || !is_dangling(ptr) {
        (ptr as *mut T).set_ptr_value( (ptr as *mut u8).wrapping_offset(data_offset) )
    } else {
        ptr::null()
    }
}

fn from_raw(*const T) -> Self {
    if [ T is Sized ] || !ptr.is_null() {
        let ptr = (ptr as *mut RcBox).set_ptr_value( (ptr as *mut u8).wrapping_offset(-data_offset) );
        Weak { ptr }
    } else {
        Weak::new()
    }
}
```

(but with more `set_ptr_value` to avoid `Sized` restrictions and maintain metadata.)

Written in this fashion, this is not a correctness-critical specialization (i.e. so long as `[ T is Sized ]` is false for unsized `T`, it can be `rand()` for sized `T` without breaking correctness), but it's still touchy, so I'd rather do it in another PR with separate review.

---
</details>

This effectively reverts rust-lang#80422 and re-establishes rust-lang#74160. T-libs [previously signed off](rust-lang#74160 (comment)) on this stable API change in rust-lang#74160.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language 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 this pull request may close these issues.

10 participants