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 transmute_ref! macro #183

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Add transmute_ref! macro #183

merged 1 commit into from
Oct 4, 2023

Conversation

joshlf
Copy link
Member

@joshlf joshlf commented May 25, 2023

This macro is like the existing transmute!, but it transmutes immutable references rather than values.

Issue #159

@joshlf joshlf force-pushed the transmute-ref branch 8 times, most recently from a629a67 to f104c55 Compare May 25, 2023 23:46
@jswrenn jswrenn self-assigned this May 26, 2023
@joshlf joshlf force-pushed the transmute-ref branch 11 times, most recently from ad117dd to f463170 Compare May 26, 2023 20:35
@joshlf joshlf requested a review from jswrenn May 26, 2023 20:35
@joshlf
Copy link
Member Author

joshlf commented May 26, 2023

@djkoloski Pinging you in case you have opinions about this.

($e:expr) => {{
// NOTE: This must be a macro (rather than a function with trait bounds)
// because there's no way, in a generic context, to enforce that two
// types have the same size or alignment.
Copy link

@gootorov gootorov May 28, 2023

Choose a reason for hiding this comment

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

There's one "trick" with traits and associated constants you can do:

use std::mem;
use std::marker::PhantomData;

struct AlignChecker<T, U>(PhantomData<T>, PhantomData<U>);

trait AlignCheck {
    const ALIGN_OF_T: usize;
    const ALIGN_OF_U: usize;
    const ALIGN_EQ_PROOF: () =
        assert!(Self::ALIGN_OF_T == Self::ALIGN_OF_U, "T and U have different alignment");
    
    fn noop();
}

impl<T, U> AlignCheck for AlignChecker<T, U> {
    const ALIGN_OF_T: usize = mem::align_of::<T>();
    const ALIGN_OF_U: usize = mem::align_of::<U>();
    
    #[inline(always)]
    fn noop() {
        // NOTE: This does not compile to anything.
        // However, this `let` must be present to force the compiler to const evaluate our checks.
        let _consteval_proof = Self::ALIGN_EQ_PROOF;
    }
}

Then, noop as-is is zero-cost*

// does not compile to anything*
AlignChecker::<u8, u8>::noop();

// but this produces a compilation error
AlignChecker::<u8, u16>::noop();

So, you can modify noop to contain your generic code.

  • About zero-cost. So, noop is compiled to just a single ret. Calling noop produces a call instruction, unless it is inlined. https://godbolt.org/z/YnaMvxYqh

Sorry if this isn't useful/already known. Also, not sure if this approach has some other limitations. Sharing just in case, because I do not see this technique used often :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a fantastic idea, thanks! We'll definitely consider it. Just want to confirm first that the check is guaranteed to catch any unsoundness: rust-lang/unsafe-code-guidelines#409

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's another thread about it: rust-lang/rust#112090

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's another thread about it: rust-lang/rust#112090

Okay, based on the discussion there, it seems that there is such a guarantee, but we'd need to be careful about how we structure things to make sure that we actually write code that benefits from that guarantee.

Copy link

@gootorov gootorov May 31, 2023

Choose a reason for hiding this comment

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

These threads are quite interesting and insightful, thanks!

I got a little more curious, and decided to try to implement something that matches std::mem::transmute as close as possible. It seems it works, but I'm not sure if I haven't missed anything.

use std::mem;
use std::mem::ManuallyDrop;

use zerocopy::AsBytes;
use zerocopy::FromBytes;

// === Part of private API ===

trait SafeTransmute<T: AsBytes, U: FromBytes> {
    const SAFE_TO_TRANSMUTE: () = assert!(mem::size_of::<T>() == mem::size_of::<U>());

    fn transmute(t: T) -> U {
        let _ = Self::SAFE_TO_TRANSMUTE;

        unsafe { transmute_anything(t) }
    }
}

trait SafeTransmuteRef<T: Sized + AsBytes, U: Sized + FromBytes> {
    const SAFE_TO_TRANSMUTE_REF: () = assert!(
        mem::size_of::<T>() == mem::size_of::<U>() && mem::align_of::<T>() >= mem::align_of::<U>(),
    );

    fn transmute_ref<'b, 'a: 'b>(t: &'a T) -> &'b U {
        let _ = Self::SAFE_TO_TRANSMUTE_REF;

        unsafe { transmute_anything(t) }
    }
}

const unsafe fn transmute_anything<T, U>(t: T) -> U {
    union UnsafeTransmuter<T, U> {
        t: ManuallyDrop<T>,
        u: ManuallyDrop<U>,
    }

    ManuallyDrop::into_inner(unsafe {
        UnsafeTransmuter {
            t: ManuallyDrop::new(t),
        }
        .u
    })
}

struct Transmuter;

impl<T: AsBytes, U: FromBytes> SafeTransmute<T, U> for Transmuter {}
impl<T: Sized + AsBytes, U: Sized + FromBytes> SafeTransmuteRef<T, U> for Transmuter {}

// === Part of public API ===

pub fn transmute<T: AsBytes, U: FromBytes>(t: T) -> U {
    Transmuter::transmute(t)
}

pub fn transmute_ref<'b, 'a: 'b, T: Sized + AsBytes, U: Sized + FromBytes>(t: &'a T) -> &'b U {
    Transmuter::transmute_ref(t)
}

fn main() {
    let array_of_u8s = [0u8, 1, 2, 3, 4, 5, 6, 7];
    let array_of_arrays = [[0, 1], [2, 3], [4, 5], [6, 7]];
    let x: &[[u8; 2]; 4] = transmute_ref(&array_of_u8s);
    assert_eq!(*x, array_of_arrays);
    let x: &[u8; 8] = transmute_ref(&array_of_arrays);
    assert_eq!(*x, array_of_u8s);

    let u = 0u64;
    let array = [0, 0, 0, 0, 0, 0, 0, 0];
    let x: &[u8; 8] = transmute_ref(&u);
    assert_eq!(*x, array);

    // does not compile (as expected)
    //
    // let increase_alignment: &u16 = transmute_ref::<[u8; 2], u16>(&[0; 2]);
    // let decrease_size: &u8 = transmute_ref::<[u8; 2], u8>(&[0; 2]);
    // let increase_size: &u16 = transmute_ref::<u8, u16>(&0);
    // let int_to_bool: bool = transmute::<u8, bool>(42);
    // let int_ref_to_bool_ref: &bool = transmute_ref::<u8, bool>(&42);
}

I wish we had const fn in traits, then transmute/transmute_ref could've been const as well :)

edit: The original code was a bit too strict w.r.t. transmute, updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah something like this would be awesome! I'm not gonna get a chance to work on this much for the next few weeks, but feel free to leave more comments here, and I'll get to them when I'm working on this again!

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that rust-lang/rust#112301 will likely make this technique a bit annoying for users (although the error messages are way better than the existing technique, and the implementation is way simpler, so IMO it's still worth it on balance).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've made a branch experimenting with this approach: #190

Choose a reason for hiding this comment

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

Exciting news! With inline_const being stabilized, the following check will soon be possible on stable compiler:

fn check_align<T, U>() {
    const {
        assert!(
            mem::size_of::<T>() == mem::size_of::<U>() 
                && mem::align_of::<T>() >= mem::align_of::<U>()
        );
    }
}

Link to nightly playground:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=24d5ca56b2834fdfde8ec7681359e729

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately our MSRV is currently 1.56, so we won't be able to use that for a while, but I'm sure we'll make use of it once our MSRV is high enough.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@joshlf joshlf force-pushed the transmute-ref branch 2 times, most recently from a5b5927 to 7995055 Compare October 4, 2023 18:57
Copy link
Collaborator

Choose a reason for hiding this comment

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

The triple-reporting of the same error is unfortunate. I think we can punt on the issue for now (let's land something that works, then we can hone error messages), but we should revisit this — especially if we manage to re-implement this macro atop const fns.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

tests/ui-nightly/transmute-ref-alignment-increase.stderr Outdated Show resolved Hide resolved
@joshlf joshlf force-pushed the transmute-ref branch 5 times, most recently from 8f12a5b to 76c2c43 Compare October 4, 2023 20:52
jswrenn
jswrenn previously approved these changes Oct 4, 2023
This macro is like the existing `transmute!`, but it transmutes
immutable references rather than values.

Release 0.7.8.

Issue #159
@joshlf joshlf added this pull request to the merge queue Oct 4, 2023
Merged via the queue into main with commit 646fe3b Oct 4, 2023
144 checks passed
@joshlf joshlf deleted the transmute-ref branch October 4, 2023 22:28
samuelselleck pushed a commit to samuelselleck/zerocopy that referenced this pull request Oct 13, 2023
This macro is like the existing `transmute!`, but it transmutes
immutable references rather than values.

Release 0.7.8.

Issue google#159
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.

3 participants