Implement TransparentWrapper for `[T; 1]`, `(T,)`, and `(T, ())`

Lokathor / bytemuck

A crate for mucking around with piles of bytes

https://docs.rs/bytemuck

Apache License 2.0

738 stars 80 forks source link

Implement TransparentWrapper for `[T; 1]`, `(T,)`, and `(T, ())` #264

Closed 0xAdk closed 3 months ago

0xAdk commented 3 months ago

When attempting to remove some unsafe code I came across the limitation that TransparentWrapper<T> isn't implemented for (T, ()). I added the other two since they also make sense and I don't see a downside.

0xAdk commented 3 months ago

while reading the help doc to verify (T, ()) is a transparent wrapper (I'm pretty sure it is) I noticed it mentions PhantomData as a common ZST, so I added it as well.

chorman0773 commented 3 months ago

Other than [T;1] I don't believe any of them are guaranteed to even be layout compatible. [T;1] is layout compatible by definition with T but not ABI Compatible at all.

zachs18 commented 3 months ago

IIRC tuples do not have a defined layout (they are implicitly repr(Rust) and specifically not repr(transparent)), even single-element tuples, so it would not be correct to implement TransparentWrapper<T> for (T,) or (T, Zst).

impl TransparentWrapper<T> for [T; 1] would be fine (since they layout is the same), except that TransparentWrapper's docs explicitly call out that repr(transparent) must be used, which means that [T; 1] does not qualify (repr(transparent) has implications for passing by value to a function, e.g.).

Even if the repr(transparent) restriction is lifted (which would be a breaking change IMO) and instead it is just required that the types have the same size and alignment, then [T; 1] is okay, but for the tuples, at the very least there would need to be some kind of generic const assertion that their layouts are the same before any actual use of the trait for a particular instatiation.

0xAdk commented 3 months ago

I think I'm having a hard time wrapping my head around what what repr(rust) actually guaranteed. Re-reading it again I think I get it now that it only talks about alignment not size, so (T, ()) could be any size and offset_of!((T, ()), 0) could be any offset?

except that TransparentWrapper's docs explicitly call out that repr(transparent) must be used

I think I misread that part as requiring they have the same alignment and size.

Lokathor commented 3 months ago

So, when I reviewed the exact unsafe contract for TransparentWrapper (https://github.com/Lokathor/bytemuck/blob/main/src/transparent.rs#L15-L35), it's definitely a little confusingly worded. Point (1) contains the word "either", but then only gives one way to satisfy that point (the type must be repr(transparent)).

Since all previous usage of TransparentWrapper seems to have been built around this repr(transparent) assumption, and since that's a higher bar than the basic "data layout compatible" criteria, then I think that we have to continue to require the high bar for the trait, and so I don't think we can accept any new impls that aren't repr(transparent).

chorman0773 commented 3 months ago

repr(Rust) doesn't guarantee much about alignment either (or call ABI).

The requirements are that which is required for soundness:

Each field is at an offset such that it is non-overlapping with any other field and is aligned (absent repr(packed)),
The size and alignment requirement of the entire struct is such that each field is placed wholy within the struct, the struct itself is aligned to at least the maximum accross all of its fields, and the size is a multiple of the alignment requirement.

0xAdk commented 3 months ago

and so I don't think we can accept any new impls that aren't repr(transparent)

ok I'll just close this then, looks like what I was trying to remove unsafe from might not be safe then? Is this something miri don't catch?

I changed the commit if you do want the [T; 1] impl

Lokathor commented 3 months ago

In your specific context it might be safe, depending on how much that situation was assuming.

If you've got a link to some code you can post about it in the #dark-arts channel of the rust community discord (if you use discord that is), or you could open a new issue in this repo and approximately the same people would take a look there too.

0xAdk commented 3 months ago

Point (1) contains the word "either", but then only gives one way to satisfy that point (the type must be repr(transparent)).

I think this is why I got confused about what it exactly wanted. I thought #1 was just saying "Wrapper must be a wrapper around Inner with an identical data representations", I think the either is just confusing since the other option is wrapped in parentheses