Read/Write mutable reference in `serialize` and `deserialize_reader` is unnecessary

[billythedummy]

Currently, the required method for BorshDeserialize has the following signature:

fn deserialize_reader<R: Read>(reader: &mut R) -> Result<Self>;

However, it can be changed to just:

fn deserialize_reader<R: Read>(reader: R) -> Result<Self>;

because Read has a blanket impl for mut references: https://docs.rs/borsh/latest/borsh/io/trait.Read.html#impl-Read-for-%26mut+R

Similarly, BorshSerialize can be changed from

fn serialize<W: Write>(&self, writer: &mut W) -> Result<()>;

to

fn serialize<W: Write>(&self, writer: W) -> Result<()>;

because Write has a blanket impl for mut references: https://docs.rs/borsh/latest/borsh/io/trait.Write.html#impl-Write-for-%26mut+W

This will be a breaking change

[dj8yfo]

it probably should've been

fn deserialize_reader<R: Read>(mut reader: R) -> Result<Self>;

Does this change allow any wider use cases for the traits? I believe both signatures are equivalent with respect to how the traits can be used in practice, and mutable reference argument better reflects semantics of what the traits are doing: they borrow Reader/Writer for de/ser of one object and return it intact to the caller (by-value argument drops Reader/Writer after one object).

[billythedummy]

It probably should've been fn deserialize_reader<R: Read>(mut reader: R) -> Result<Self>;

No. You do not need to specify mut in the trait definition itself, only in the implementation. See playground link below.

Does this change allow any wider use cases for the traits?

It's somewhat weird that you have to pass a mutable reference to a immutable buffer when you're deserializing. See playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=dd2f2606d714535fbba3e5e5b00208cc

mutable reference argument better reflects semantics of what the traits are doing

Yes. But if your writer is a mutable reference itself then it makes sense for the mutable reference to be dropped after one object, does it not?

For reference: serde_json also bounds most stuff by W and R instead of &mut W and &mut R https://docs.rs/serde_json/latest/serde_json/fn.to_writer.html , https://docs.rs/serde_json/latest/serde_json/fn.from_reader.html

[dj8yfo]

It's somewhat weird that you have to pass a mutable reference to a immutable buffer when you're deserializing

these are playground examples for Read and Write

https://doc.rust-lang.org/src/std/io/impls.rs.html#248

[billythedummy]

I believe Rust API guidelines recommends to pass Read and Write by value: https://rust-lang.github.io/api-guidelines/interoperability.html#generic-readerwriter-functions-take-r-read-and-w-write-by-value-c-rw-value

Regarding the playground examples, I think this is what by_ref() is for. Read example fixed, Write example fixed

[billythedummy]

More info here: https://github.com/rust-lang/api-guidelines/issues/174

[dj8yfo]

Frankly speaking this is illuminating

https://github.com/near/borsh-rs/assets/26653921/ff818879-fcc5-48ed-8ed2-706d9ef5cac8

[billythedummy]

So, do you guys intend to change this for 2.0? I also wanna add that this issue has surfaced before: https://github.com/near/borsh-rs/pull/34

[frol]

@billythedummy Thank you for raising it

So, do you guys intend to change this for 2.0?

We don't intent to release 2.0 any time soon unless there is a very good reason to. I have created a tracking issue for the ideas that would require us to bump the major version here: https://github.com/near/borsh-rs/issues/279