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

Add cast_{arc,rc} (and slice and try), and {wrap,peel}_{arc,rc}. #132

Merged
merged 4 commits into from Sep 1, 2022

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Sep 1, 2022

Add allocation::{try_,}cast_{arc,rc} (and corresponding slice functions), and allocation::TransparentWrapperAlloc::{wrap,peel}_{arc,rc}.

May fix #131 .

Mostly a copy-paste of the *_box functions, with relevant safety comments/changes (and using *const instead of *mut).

Added tests for {wrap,peel}_{arc,rc} where the existing tests for {wrap,peel}_box are. I didn't add tests for the other functions (yet).

Possible future work:

  • Equivalent functions for rc::Weak and sync::Weak;
    • Bikeshedding the names (cast_weak_rc vs cast_rc_weak)
    • For {cast,wrap,peel}_weak_{arc,rc}, these would be mostly copy-pastes of *_{arc,rc} etc.
    • For cast_weak_slice, these would need manual handling of length, since you can't use input.len()
  • zeroed_{arc,rc} and zeroed_slice_{arc,rc}. These cannot be implemented manually like zeroed_box was, and would need to be behind an additional nightly-only feature, since {Rc,Arc}::new_zeroed{,_slice} are in feature(new_uninit)
  • try_zeroed_{arc,rc}. These also cannot be implemented manually, and would need to be behind an additional nightly-only feature(s?), since {Rc,Arc}::new_zeroed_slice are in feature(allocator_api) (and/or feature(new_uninit), depending on which gets stabilized first).
  • try_zeroed_slice_{arc,rc}. The corresponding functions currently do not currently exist in the stdlib.
  • Note: the *_zeroed_* could be implemented (less efficiently) in terms of their _box counterparts, using impl<T: ?Sized> From<Box<T>> for Rc<T>/Arc<T>. These impls are from 1.21.0, so it would not require a nightly-oly feature nor raise the MSRV of using the extern_crate_alloc feature (Sidenote: Does this crate keep MSRVs for specific features?).

@zachs18 zachs18 changed the title Add {try_,}cast_{arc,rc}, and {wrap,peel}_{arc,rc}. Add cast_{arc,rc} (etc.), and {wrap,peel}_{arc,rc}. Sep 1, 2022
@zachs18 zachs18 changed the title Add cast_{arc,rc} (etc.), and {wrap,peel}_{arc,rc}. Add cast_{arc,rc} (and slice and try), and {wrap,peel}_{arc,rc}. Sep 1, 2022
@zachs18 zachs18 marked this pull request as ready for review September 1, 2022 08:03
@zachs18
Copy link
Contributor Author

zachs18 commented Sep 1, 2022

Note: In safe code, there is no way for a user to observe modifications to a B as modifications to an A, but with Rc::get_mut_unchecked this could cause problems.

#![feature(get_mut_unchecked)]
use std::rc::Rc;
fn main() {
    let a: Rc<bool> = Rc::new(false);
    let mut b: Rc<u8> = bytemuck::allocation::cast_rc(a.clone());
    unsafe {
        let b = Rc::get_mut_unchecked(&mut b);
        *b = 2;
    }
    dbg!(a); // UB: reading 0x02 as bool
}

I'll add a note about this to the doc-comment of cast_rc and similar. I think this is probaly fine to have, since the only way to mutate the inner value is with additionaly unsafe code.

Alternately, the bounds for cast_rc could be changed from <A: NoUninit, B: AnyBitPattern> (like cast_ref) to be <A: NoUninit + AnyBitPattern, B: NoUninit + AnyBitPattern> (like cast_mut) but that might be overkill, since the problem is only a problem with other unsafe code.

(This is not a problem with `Rc::get_mut` or `Rc::make_mut`)
use std::rc::Rc;
fn main() {
    let a: Rc<bool> = Rc::new(false);
    let mut b: Rc<u8> = bytemuck::allocation::cast_rc(a.clone());
    drop(a);
    *Rc::get_mut(&mut b).unwrap() = 2;
    // There is no way to observe 0x02 as bool, becasuse `a` was dropped,
    // otherwise get_mut would have returned None

    let c: Rc<bool> = Rc::new(false);
    let mut d: Rc<u8> = bytemuck::allocation::cast_rc(c.clone());
    *Rc::make_mut(&mut d) = 2;
    // There is no way to observe 0x02 as bool, becasuse the value in `d`
    // was cloned.
    assert_eq!(*c, false);
}

@Lokathor
Copy link
Owner

Lokathor commented Sep 1, 2022

We need the stronger bounds. Because an Rc puts a value "in two places", we have to basically act like the cast is going in both directions at once.

@zachs18 zachs18 force-pushed the alloc_rc_arc branch 2 times, most recently from 3c50442 to ca54869 Compare September 1, 2022 19:57
@zachs18
Copy link
Contributor Author

zachs18 commented Sep 1, 2022

Okay, I changed the bounds to<A: NoUninit + AnyBitPattern, B: NoUninit + AnyBitPattern>. I suppose if Rc::get_unchecked_mut is definitively determined to be unsound and removed (it is currently unstable, with some soundness issues involving variance on its tracking issue), the bounds can be relaxed later.

@Lokathor
Copy link
Owner

Lokathor commented Sep 1, 2022

I completely thought that Rc let you mutate the value with some sort of set method, but i guess you have to manually layer in Cell too for that to happen.

@notgull
Copy link
Contributor

notgull commented Sep 1, 2022

My main concern with this is that the API is very quickly becoming very large. You have cast, wrap, peel and try_cast, the owned versions, ref and mut, box and vec, not to mention the traits involved here. In addition, as the OP admits, many of these functions are copy-pasted versions of eachother.

I imagine that there is a more concise trait-based solution hidden inside all of this code duplication.

@Lokathor
Copy link
Owner

Lokathor commented Sep 1, 2022

I do share the concern about duplication.

I'm going to approve and merge this now, because I don't like to have people's PRs get stuck. However, I won't publish it to crates.io immediately. If someone hatches an idea for the duplication problem we can do that before a publish, and if people want to use this right away in the mean time they can use it straight out of the git repo temporarily.

@Lokathor Lokathor merged commit 09dd2ff into Lokathor:main Sep 1, 2022
leod pushed a commit to leod/bytemuck that referenced this pull request Jun 3, 2023
…okathor#132)

* Add `allocation::{try_,}cast_{arc,rc}`, and add `{wrap,peel}_{arc,rc}` to `TransparentWrapperAlloc`.

* Avoid intermediate slice reference in `try_cast_slice_{arc,rc}`.

* remove `unsafe` block; run `cargo +nightly fmt` (ignoring files I didn't modify)

* Make `cast_rc` (etc) have the same bounds as `cast_mut`, due to the existence of `Rc::get_mut_unchecked`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional TransparentWrapper methods
3 participants