[Feature] extend TransparentWrapper conversion functions

[luiswirth]

This PR extends the TransparentWrapper trait by adding new conversion functions.
Namely:

These three missing wrap_{} functions.

• TransparentWrapper::wrap(s: Inner) -> Self
• TransparentWrapper::wrap_slice(s: &[Inner]) -> &[Self]
• TransparentWrapper::wrap_slice_mut(s: &mut [Inner]) -> &mut [Self]

These five unwrap_{} functions, which are the counterparts of the wrap_{} functions. They just invert the conversion.

• TransparentWrapper::unwrap(self) -> Inner
• TransparentWrapper::unwrap_ref(&self) -> &Inner
• TransparentWrapper::unwrap_mut(&mut self) -> &mut Inner
• TransparentWrapper::unwrap_slice(s: &[Self]) -> &[Inner]
• TransparentWrapper::unwrap_slice_mut(s: &mut [Self]) -> &mut [Inner]

This is useful for implementing Zeroable and Pod on types from extern crates in order to use bytemuck::{} functions like bytemuck::cast_slice using the new-ype pattern and TransparentWrapper.

The goal of this PR is to support all bytemuck::{} functions.

This is possible with this PR:

  // An external type defined in a different crate.
  #[derive(Copy, Clone, Default)]
  struct Foreign(u8);

  use bytemuck::TransparentWrapper;

  #[derive(Copy, Clone)]
  #[repr(transparent)]
  struct Wrapper(Foreign);

  unsafe impl TransparentWrapper<Foreign> for Wrapper {}

  // Traits can be implemented on crate-local wrapper.
  unsafe impl bytemuck::Zeroable for Wrapper {}
  unsafe impl bytemuck::Pod for Wrapper {}

  let _: u8 = bytemuck::cast(Wrapper::wrap(Foreign::default()));
  let _: Foreign = Wrapper::unwrap(bytemuck::cast(u8::default()));

  let _: &u8 = bytemuck::cast_ref(Wrapper::wrap_ref(&Foreign::default()));
  let _: &Foreign = Wrapper::unwrap_ref(bytemuck::cast_ref(&u8::default()));

  let _: &mut u8 = bytemuck::cast_mut(Wrapper::wrap_mut(&mut Foreign::default()));
  let _: &mut Foreign = Wrapper::unwrap_mut(bytemuck::cast_mut(&mut u8::default()));

  let _: &[u8] = bytemuck::cast_slice(Wrapper::wrap_slice(&[Foreign::default()]));
  let _: &[Foreign] = Wrapper::unwrap_slice(bytemuck::cast_slice(&[u8::default()]));

  let _: &mut [u8] = bytemuck::cast_slice_mut(Wrapper::wrap_slice_mut(&mut [Foreign::default()]));
  let _: &mut [Foreign] = Wrapper::unwrap_slice_mut(bytemuck::cast_slice_mut(&mut [u8::default()]));

  let _: &[u8] = bytemuck::bytes_of(Wrapper::wrap_ref(&Foreign::default()));
  let _: &Foreign = Wrapper::unwrap_ref(bytemuck::from_bytes(&[u8::default()]));

  let _: &mut [u8] = bytemuck::bytes_of_mut(Wrapper::wrap_mut(&mut Foreign::default()));
  let _: &mut Foreign = Wrapper::unwrap_mut(bytemuck::from_bytes_mut(&mut [u8::default()]));

  // not sure if this is the right usage
  let _ = bytemuck::pod_align_to::<_, u8>(Wrapper::wrap_slice(&[Foreign::default()]));
  // counterpart?

  // not sure if this is the right usage
  let _ = bytemuck::pod_align_to_mut::<_, u8>(Wrapper::wrap_slice_mut(&mut [Foreign::default()]));
  // counterpart?

This PR also includes a test, which uses the exact example code from above. This is for MIRI to check for any potential UB.
As you can see, there currently isn't any checking done on the output of the functions. This is something left todo.

Can somebody please check the soundness of the implementation, because I have very little expertise with unsafe code.

I've written up some docs on how this trait can be used in combination with the newtype pattern to use the bytemuck functions on foreign types from external crates.

I also adjusted rustfmt.toml in the first commit, because of the deprecation of merge_imports = true. It is being replaced by imports_granularity = "Crate". Furthermore I ran cargo +nightly fmt to format the codebase. I hope these two changes are fine.

I'm open to suggestions!

This is my first PR ever 🥳 .

[Lokathor]

This looks correct.

I'll have @thomcc take a look too, because they wrote this trait.

[thomcc]

Up on line 28 you should add this method to the list of methods that may not be overwridden.

Otherwise it seems fine.

[luiswirth]

I've decided to extend this PR some more, therefore I converted it into a draft.
I would like to add all conversions needed, to support all bytemuck functions.

[luiswirth]

I've now rewritten and extended the PR quite a bit. Please take a look at it again :).

I would also like to introduce the wrap(s: Wrapped) -> Self and unwrap(self) -> Wrapped conversions functions.
This would be nice, because then the conversion for bytemuck::cast would look the same as all other conversions.
I'm not quite sure how we could do this... Maybe a transmute?
This would require a Sized bound for Self and Wrapped on the function.
Is this desirable?

Also I would like to add some docs, mentioning the usefulness of this trait for using bytemuck with types from extern crates.

[Lokathor]

I believe that the transmute_copy trick used to wrap can also be used to unwrap.

[luiswirth]

Okay, another rather big overhaul of this PR is done 😄 .

I adjusted the docs a little to be more concise. Also I renamed Wrapped to Inner to make it visually more distinct from Wrapper.

I've also added the wrap and unwrap functions. I didn't use the transmute_copy trick because I don't have to transmute any pointers since there is a Sized bound on Self and Inner. This is needed because rust needs Sized functions arguments and return types.

I also added a test which currently just checks for UB using MIRI. Thanks to this I already catched a bug causing UB.
There isn't any actual checking done on the results of the functions.

[thomcc]

I'm not sure unwrap is the right term for these, given that that terminology is already used very commonly in Rust to mean something different...

[luiswirth]

@thomcc I agree, but I haven't come up with a better fitting terminology. It is a wrapper after all :D.
I'm open for suggestions.

[luiswirth]

I've now added some docs and a example mentioning how TransparentWrapper can be used to implement traits like Zeroable and Pod on external types.

[luiswirth]

This PR is no longer a draft! I'm pretty happy with it.

Now I'm waiting for a review and some feedback on what's left to do :).

[luiswirth]

I just noticed, that deriving TransparentWrapper, Zeroable and Pod on a struct doesn't work as expected. The derive fails, because the inner type doesn't implement Zeroable and Pod. We should lift this restriction if TransparentWrapper is also implemented (manually or derived).
I feel like this is hard to check with a proc macro. This might be a challenge for another PR.

[Lokathor]

I feel like the derive should continue to fail even then. You can still write it manually. The fact that you didn't just derive it is a good time to say "i know the base type doesn't impl the trait but i trust i can bend the rule in this case"

[luiswirth]

@Lokathor Hmm, It's just unfortunate, that in this case you can't have the derive check, if the safety guarantees hold.

Does the current proc-macro implementation check all aspects of the unsafe contract? It should, because otherwise there would be a possibility of introducing UB without having an unsafe block...

If the derive can check all aspects of the unsafe contract, then you shouldn't need to think about bending the rules, because it's guaranteed to be safe and sound :).
I'm just not sure if these new restrictions are easily checkable.

[Lokathor]

It may be possible to fix. We could probably finish off the PR noe and file an issue for the derives later.

[luiswirth]

Sounds good to me!

[Lokathor]

I don't think that we should document this idea.

It's possible, but it's kinda extra dodgy, particularly because it runs explicitly against the claimed rules of Pod and Zeroable.

[luiswirth]

Yeah, I get your point. It's getting kinda convoluted.

But I don't know how obvious this use of TransparentWrapper is. It is a pretty nice feature to have, so people should know about it.

Maybe we need to reformulate the docs some more? I'll give it a try and if it looks bad, we probably have to remove it...

What is most desirable?

• Mentioning TransparentWrapper everywhere where it's relevant (so also in Pod).
• Being subtle about it and just mentioning it in it's own docs.
• Not mention it at all.

[thomcc]

I don't think we should mention it, since it's not sound unless you pin the version exactly. If other crates haven't opted into the bytemuck requirements, they're free to break them at any time without a semver-major change.

[Lokathor]

Yeah, the reason that the docs for Pod and Zeroable require that the field type also is Pod or Zeroable is because then SemVer kicks in, specifically that it's a SemVer Break to remove a trait impl.

If you newtype a struct and declare the wrapper type Pod, then the inner type adds a &'static str field in a minor update (not unreasonable!) then suddenly you're hosed.

So I think that this should stay as a "secret thing the type system technically allows but officially you shouldn't so legally you can't complain when this blows up in your face (and it easily might)" sort of thing.

[luiswirth]

Okay, I see. That's a reasonable argument.
In this case I'm going to remove all mentions of this.

[thomcc]

I think I'm not really a fan of having this many functions on the trait. It would probably be better to move some/most of them to free functions, since at this point there's a real risk of collision with inherent or deref impls, especially with the generic naming scheme...

[luiswirth]

@thomcc
I'm not quite sure how collisions might happen?
Is it enough to change the self to Self in the arguments of unwrap, unwrap_ref and unwrap_mut?
This is what I did for now.

[luiswirth]

Anything left to do?

[Lokathor]

I haven't had time to get to this. My "weekend" is Sun/Mon, so I'll try to get this merged.

[Lokathor]

I'm going to approve and merge now, and I'll think hard for a bit about if there's a better name than unwrap to use.

I think I also want to sort out how we're going to adapt to min_const_generics being available before the next crates.io release.

[luiswirth]

Yeah, my first PR was merged 🥳. Thanks :)