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

Cannot derive BorshDeserialize for a struct containing Box<[T]> #323

Open
Tracked by #279
SmnTin opened this issue Nov 26, 2024 · 2 comments
Open
Tracked by #279

Cannot derive BorshDeserialize for a struct containing Box<[T]> #323

SmnTin opened this issue Nov 26, 2024 · 2 comments

Comments

@SmnTin
Copy link

SmnTin commented Nov 26, 2024

The implementation of BorshDeserialize for Box<T> has the following bounds, which I find quite strange:

impl<T, U> BorshDeserialize for Box<T>
where
    U: Into<Box<T>> + Borrow<T>,
    T: ToOwned<Owned = U> + ?Sized,
    T::Owned: BorshDeserialize,
{
    // ...
}

I guess this has been done with the intention to reuse the impl of BorshDeserialize for String when deserializing Box<str>.

Unfortunately, for Box<[T]>, these bounds require [T]: ToOwned and, thus, T: Clone.

First, this differs from the impl for Vec<T>, which only requires T: BorshDeserialize without the additional bound of T: Clone. Second, this has a nasty interaction with deriving and generic parameters.

So, while the following code compiles:

#[derive(BorshDeserialize)]
pub struct Items<T> {
    pub items: Vec<T>,
}

The version with Box<[T]> fails to compile:

#[derive(BorshDeserialize)]
pub struct Items<T> {
    pub items: Box<[T]>,
}

...with the following error:

error[E0277]: the trait bound `[T]: ToOwned` is not satisfied
   --> borsh-bug/src/lib.rs:3:10
    |
  3 | #[derive(BorshDeserialize)]
    |          ^^^^^^^^^^^^^^^^ the trait `ToOwned` is not implemented for `[T]`, which is required by `Box<[T]>: BorshDeserialize`
    |
    = note: required for `Box<[T]>` to implement `BorshDeserialize`
    = note: this error originates in the derive macro `BorshDeserialize` (in Nightly builds, run with -Z macro-backtrace for more info)

Semantically, however, deserializing Box<[T]> is the same as Vec<T>. Therefore, there should be no differences in bounds.

I believe it would have been much better if there were just separate implementations for sized and unsized types:

impl<T: BorshDeserialize> BorshDeserialize for Box<T> { ... }
impl<T: BorshDeserialize> BorshDeserialize for Box<[T]> { ... }
impl BorshDeserialize for Box<str> { ... }
impl BorshDeserialize for Box<CStr> { ... }
impl BorshDeserialize for Box<OsStr> { ... }

But this will be a breaking change if it is changed to this now. Maybe it can at least be special cased in borsh-derive.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Nov 28, 2024

@SmnTin you can try patch this deficiency of borsh with

use borsh::BorshDeserialize;

mod de_for_box_of_slice {
    use borsh::{
        io::{Read, Result},
        BorshDeserialize,
    };

    pub fn deserialize_reader<R: Read, T: BorshDeserialize>(reader: &mut R) -> Result<Box<[T]>> {
        let vecy: Vec<T> = <Vec<T> as BorshDeserialize>::deserialize_reader(reader)?;

        Ok(vecy.into())
    }
}

#[derive(BorshDeserialize)]
pub struct Items<T> {
    #[borsh(deserialize_with = "de_for_box_of_slice::deserialize_reader")]
    pub items: Box<[T]>,
}
fn main() {
    println!("Hello, world!");
}

moreover, it's the same with Box<T> - T: Clone is required there too.

mod de_for_boxy {
    use borsh::{
        io::{Read, Result},
        BorshDeserialize,
    };

    pub fn deserialize_reader<R: Read, T: BorshDeserialize>(reader: &mut R) -> Result<Box<T>> {
        let val: T = <T as BorshDeserialize>::deserialize_reader(reader)?;

        Ok(Box::new(val))
    }
}

#[derive(BorshDeserialize)]
pub struct Items2<T> {
    #[borsh(deserialize_with = "de_for_boxy::deserialize_reader")]
    pub items: Box<T>,
}

while your suggestion to do one implementation per one type from std, which implements ToOwned
image

and replacing generic implementation with these individual ones may sound like a good idea, it'll
still be a breaking change for anyone, who uses the generic one for his/her custom types outside of std, which implement ToOwned. I'm unsure if any such cases already exist or not.
More likely not, than yes. But nope, there's at least one example outside of std (thus there may be others):

use ascii::AsciiStr;
use borsh::{BorshDeserialize, BorshSerialize};

fn main() {

    let var = ascii::AsciiString::from_ascii("little string").unwrap();
    let mut vecy = vec![];
    let slice = var.serialize(&mut vecy).unwrap();; 
    let mut reader = &mut vecy.as_slice();
    let another_box_of_borrowed: Box<AsciiStr> = BorshDeserialize::deserialize_reader(&mut reader).unwrap();
    println!("{:#?}", another_box_of_borrowed);
}

And specialization isn't stable in rust.

ToOwned is used in this case to put bounds on U, thus selecting a single U which is Borrow<T> + Into<Box<T>,
and the extra bounds from ToOwned aren't needed, as they're not used during deserialization.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Nov 28, 2024

the following compiles and runs

use std::{ffi::{CStr, CString, OsStr, OsString}, io::Read, path::{Path, PathBuf}};

pub trait ToOwnedSEMANTICALLY {
    type OwnedSEMANTICALLY;
}

impl<T> ToOwnedSEMANTICALLY for T {
    type OwnedSEMANTICALLY = T;
}


impl<T> ToOwnedSEMANTICALLY for [T] {
    type OwnedSEMANTICALLY = Vec<T>;
}

impl ToOwnedSEMANTICALLY for str {
    type OwnedSEMANTICALLY = String;
}

impl ToOwnedSEMANTICALLY for CStr {
    type OwnedSEMANTICALLY = CString;
}

impl ToOwnedSEMANTICALLY for OsStr {
    type OwnedSEMANTICALLY = OsString;
}

impl ToOwnedSEMANTICALLY for Path {
    type OwnedSEMANTICALLY = PathBuf;
}


pub trait BorshDeserialize2: Sized {
    fn deserialize_reader<R: Read>(reader: &mut R) -> std::io::Result<Self>;
}


impl<T, U> BorshDeserialize2 for Box<T>
where
    U: Into<Box<T>> + std::borrow::Borrow<T>,
    T: ToOwnedSEMANTICALLY<OwnedSEMANTICALLY = U> + ?Sized,
    T::OwnedSEMANTICALLY: BorshDeserialize2,
{
    fn deserialize_reader<R: Read>(reader: &mut R) -> std::io::Result<Self> {
        Ok(T::OwnedSEMANTICALLY::deserialize_reader(reader)?.into())
    }
}

#[derive(Debug)]
struct Item {
    val: String,
}

impl BorshDeserialize2 for Item {
    fn deserialize_reader<R: Read>(reader: &mut R) -> std::io::Result<Self> {
        Ok(Item {
            val: "30".into(),
        })
    }
    
} 

impl<T: BorshDeserialize2> BorshDeserialize2 for Vec<T> {
    fn deserialize_reader<R: Read>(reader: &mut R) -> std::io::Result<Self> {
        let little_item = T::deserialize_reader(reader)?;
        let little_item2 = T::deserialize_reader(reader)?;
        let little_item3 = T::deserialize_reader(reader)?;
        Ok(vec![little_item, little_item2, little_item3])
    }
    
} 

fn main() {
    let slice = vec![0u8, 20, 30 , 40]; 
    let mut reader = &mut slice.as_slice();
    let another_box_of_slice: Box<[Item]> = BorshDeserialize2::deserialize_reader(&mut reader).unwrap();
    println!("{:#?}", another_box_of_slice);

    let item0 = Item { val: "is_it_clonable?".into() };
    // let item1 = item0.clone();

}

@dj8yfo dj8yfo mentioned this issue Nov 28, 2024
4 tasks
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

No branches or pull requests

2 participants