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

Cloning DSTs by bytes #2275

Open
kupiakos opened this issue Jan 29, 2025 · 2 comments
Open

Cloning DSTs by bytes #2275

kupiakos opened this issue Jan 29, 2025 · 2 comments
Labels
customer-request Documents customer requests.

Comments

@kupiakos
Copy link
Contributor

kupiakos commented Jan 29, 2025

Working with DSTs is much improved but is still challenging - especially owned DSTs. Being able to simply perform a bytewise clone into a Box would be very helpful. I propose it be derived via ByteClone.

While you can impl Clone for Box<LocalType> due to it being #[fundamental] and derive(ByteClone) probably should, there's no built-in trait for &self -> Box<Self>. So, this should also define a local ByteClone trait or something similar to generically define this.

Sample:

// In zerocopy:
pub trait ByteClone: KnownLayout + IntoBytes + FromBytes {
    fn byte_clone(&self) -> Self where Self: Sized;
    fn byte_clone_box(&self) -> Box<Self>;
}

// In local crate:
#[derive(ByteClone, IntoBytes, FromBytes, Immutable, KnownLayout)]
#[repr(C)]
struct Foo([u8]);

// Generates:

// This `for<'a>` trick is useful for macros to conditionally implement functionality
// on a concrete type dependent on a maybe-implemented trait.
impl Clone for Foo where for<'a> Foo: Sized {
    fn clone(&self) -> Self { self.byte_clone() }
}

impl ByteClone for Foo {
    fn byte_clone(&self) -> Self where for<'a> Self: Sized {
        Self::read_from_bytes(self.as_bytes()).unwrap()
    }
    
    fn byte_clone_box(&self) -> Box<Self> {
        Foo::read_box_from_bytes(self.as_bytes()).unwrap()
    }
}

impl Clone for Box<Foo> {
    fn clone(&self) -> Self {
        <Foo as ByteClone>::byte_clone_box(self.as_bytes())
    }
}
  • This requires Conversion between Box<T: Unalign> and Box<[u8]> #2258 and the ability to construct a Box from existing bytes for ?Sized.
  • Like in derive(ByteEq) #2274, this bytewise clone can also optimize better than field-wise derived clone for Sized + !Copy types.
  • Naming: fn byte_clone or fn clone_bytes? I like the readability of verb-noun, especially with clone_bytes_to_box instead of byte_clone_box. If clone_bytes is preferred, then should the trait be named CloneBytes? Then derive(ByteEq) #2274 (comment) should apply for consistency. ByteHash is already implemented.
@kupiakos kupiakos added the customer-request Documents customer requests. label Jan 29, 2025
@jswrenn
Copy link
Collaborator

jswrenn commented Jan 29, 2025

Could we just define:

pub trait FromBytes {
    fn clone(self: Box<Self>) -> Box<Self>
    where
        Self: KnownLayout
    {
        /* ... */
    }
}

Relatedly, we'd probably benefit from a more general read_from_box constructor on FromBytes:

pub trait FromBytes {
    fn read_from_box<Src>(src: Box<Src>) -> Result<Box<Self>, SizeError<Box<Src>, Self>>
    where
        Src: ?Sized + IntoBytes + KnownLayout,
        Self: KnownLayout
    {
        ...
    }
}

@kupiakos
Copy link
Contributor Author

kupiakos commented Jan 29, 2025

@jswrenn I definitely prefer avoiding another trait, though I think that needs some adjustment:

  • It doesn't really make sense to consume a Box<Self> and output a Box<Self> - that's a no-op. Did you mean &Box<Self> or &Self?
  • The key operation is &Self -> Box<Self>. It's unidiomatic to pass around &Box<T> and it Derefs, so cloning a DST shouldn't require having one.
  • I want to avoid naming it clone as that'd be frustrating to use with a Clone implementation - what do you think of fn clone_bytes_to_box(&self) -> Box<Self> where Self: KnownLayout and fn clone_bytes(&self) -> Self where Self: Sized? Or maybe _bytewise_ to imply it's not converting to [u8]?
  • derive(ByteClone) could still be useful as a derive to implement Clone for Self and Box<Self> but we don't have a specific need for a macro to do that. It'd be nice to have for the same better-optimization reason as derive(ByteEq) #2274.

Relatedly, we'd probably benefit from a more general read_from_box constructor on FromBytes

I agree, though I do have some comments:

@kupiakos kupiakos changed the title derive(ByteClone) Cloning DSTs by bytes Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-request Documents customer requests.
Projects
None yet
Development

No branches or pull requests

2 participants