Gilnaa / memoffset

offsetof for Rust
MIT License
224 stars 29 forks source link

Can't be used with unions #66

Closed asomers closed 2 years ago

asomers commented 2 years ago

memoffset does not work with unions. Would it be possible to add this feature? For example, this code:

use memoffset::offset_of;

pub union Foo {
    foo32: i32,
    foo64: i64
}

#[test]
fn alignment() {
    assert_eq!(offset_of!(Foo, foo32), offset_of!(Foo, foo64));
}

Produces this result:

error: `..` cannot be used in union patterns
  --> test/memoffset.rs:10:16
   |
10 |     assert_eq!(offset_of!(Foo, foo32), offset_of!(Foo, foo64));
   |                ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in the macro `_memoffset__field_check` (in Nightly builds, run with -Z macro-backtrace for more info)

error: `..` cannot be used in union patterns
  --> test/memoffset.rs:10:40
   |
10 |     assert_eq!(offset_of!(Foo, foo32), offset_of!(Foo, foo64));
   |                                        ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in the macro `_memoffset__field_check` (in Nightly builds, run with -Z macro-backtrace for more info)
RalfJung commented 2 years ago

Seems reasonable to support, if we can find some way to do the deref coercion protection. We already have offset_of_tuple, so having offset_of_union in a similar vein would make sense.

Notice that the assertion in your code might well fail in a future version of Rust -- without repr(C), there is no guarantee that all union fields are at the same offset.

asomers commented 2 years ago

Seems reasonable to support, if we can find some way to do the deref coercion protection. We already have offset_of_tuple, so having offset_of_union in a similar vein would make sense.

Notice that the assertion in your code might well fail in a future version of Rust -- without repr(C), there is no guarantee that all union fields are at the same offset.

Exactly! I want to use memoffset in my unit tests to detect if any future Rust version breaks that assumption.

RalfJung commented 2 years ago

In that case I would recommend you add repr(C) to actually tell the compiler that you are relying on this assumption. :) That's exactly what that attribute is for.

asomers commented 2 years ago

Well yes I did that too. I just like to double check.

RalfJung commented 2 years ago

Fair. :D

So this would probably need a _memoffset__field_check_union, and I assume that can be implemented something like this:

macro_rules! _memoffset__field_check_union {
    ($type:path, $field:tt) => {
        // Make sure the field actually exists. This line ensures that a
        // compile-time error is generated if $field is accessed through a
        // Deref impl.
        let $type { $field: _ };
    };
}

The rest would then follow the structure of the tuple macro.

Gilnaa commented 2 years ago

@RalfJung it looks like this check macro won't work even though it looks like it's trivially the same as writing the code manually:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=5eb6107b4f9860fa24d6d1a71a56eec0

   Compiling playground v0.0.1 (/playground)
error: expected pattern, found `Foo`
  --> src/main.rs:10:13
   |
10 |         let $type { $field: _ };
   |             ^^^^^ expected pattern
...
15 |     _memoffset__field_check_union!(Foo, foo32);
   |     ------------------------------------------ in this macro invocation
   |
   = note: this error originates in the macro `_memoffset__field_check_union` (in Nightly builds, run with -Z macro-backtrace for more info)

Do you understand what's the difference between the two?


edit: I apologize, I botched the field-check to use the wrong macro_rules matcher.