feat!: introduce borsh::io with either items of std:io or private borsh::nostd_io module reexported (std or no_std)
#212
Conversation
|
The crux of the change is in borsh/src/lib.rs. The rest is just |
There was a problem hiding this comment.
I do not support this change. We don't want to turn borsh-rs into maybestd provider. If you absolutely don't want to define your own maybestd, or implement some library that extends borsh capabilities, consider using use borsh::__private::maybestd; - it is still public, but from a borsh crate point of view, it is a private implementation that we don't want to provide backward compatibility guaranties.
I mean, borsh devs should have thought about that when they decided to use io::Read, io::Write and io::Error as part of borsh’s public API. For comparison, serde does offer StdError which switches between std::io::Error and its own trait depending on Cargo features. If you don’t want to offer maybestd you have an option of switching the interface to not use std. Not offering maybestd is not only hostile to users of borsh but also to all users of no_std-compatible libraries which use borsh.
But you are providing compatibility guarantees to |
That scope is small and self-contained in a single file, and that is indeed part of the public interface, as you stated.
no_std-compatible libraries have two valid options:
We optimize borsh for application developers, and we don't want them to abuse |
Just like what this PR proposes: pub mod maybestd {
pub mod io {
#[cfg(feature = "std")]
pub use std::io::{Error, ErrorKind, Read, Result, Write};
#[cfg(not(feature = "std"))]
pub use crate::nostd_io::*;
}
}Literally five symbols which are part of borsh’s public API. If you don’t like the name #[cfg(feature = "std")]
pub mod io {
pub use std::io::{Error, ErrorKind, Read, Result, Write};
}
#[cfg(not(feature = "std"))]
pub use crate::nostd_io as io;That would offer types
What’s the point of [package]
name = "pascal-str"
version = "0.1.0"
edition = "2021"
[dependencies]
borsh = { version = "1.0.0-alpha.4", default-features = false }#![no_std]
extern crate alloc;
struct PascalString(alloc::vec::Vec<u8>);
impl borsh::BorshSerialize for PascalString {
fn serialize<W: borsh::nostd_io::Write>(&self, writer: &mut W) -> borsh::nostd_io::Result<()> {
writer.write_all(&[self.0.len() as u8])?;
writer.write_all(&self.0)
}
}It compiles and works fine. But let’s now say someone uses it in std application like so: [package]
name = "foo"
version = "0.1.0"
edition = "2021"
[dependencies]
borsh = "1.0.0-alpha.4"
pascal-str = { path = "../pascal-str" }And suddenly the code breaks: Enabling a cargo feature shouldn’t break builds.
Except you don’t. Because now author of
But as shown, it’s not just a matter of a simple
If they do, tell them to GTFO. However, if something is part of borsh’s public API that should be provided by borsh in a way which doesn’t depend on whether |
Well,
This is a fair point. @mina86 @dj8yfo @matklad I would like to consider having |
Agree.
Using this recipe borsh-rs/borsh/tests/test_ser_de_with.rs Lines 22 to 26 in 09c9295 Doing the dispatch on the client lib side is more explicit about what's happening and i like it more.
This might result in people mindlessly using Here's an example https://github.com/near/nearcore/pull/9432/files#diff-5610719ce9d54675a9153a04627f8988705312f7dec97375fa702283ed796c1aL4 and there're a few more. This pr has most likely protected from cases when items unrelated to Supposedly, the recipe above addresses this too. |
The fact that Unless there’s some new convincing argument, I must conclude that borsh 1.x is in state of perpetual brokenness poorly suited for no_std environments and that I should stick to borsh 0.10. |
|
Well, i've found an example pascal_string crate/repo. It doesn't have Can you post a link to the problematic repo? Your example #![no_std]
extern crate alloc;
struct PascalString(alloc::vec::Vec<u8>);
impl borsh::BorshSerialize for PascalString {
fn serialize<W: borsh::nostd_io::Write>(&self, writer: &mut W) -> borsh::nostd_io::Result<()> {
writer.write_all(&[self.0.len() as u8])?;
writer.write_all(&self.0)
}
}attempts to assert, that the
Is it supposed to still be using If so, it can be helped by removing just this line Line 100 in 09c9295 borsh will still use either std::io or self::nostd_io depending on flag, but provide borsh::nostd_io outside in both situations.But then, what is the reason to support this, if pascal-str won't be compatible with other types from borsh in an std application?
|
|
Imo, this is what we need (haven't checked this specific code, might need some adjustments): #[cfg(feature = "std")] use std::io as io_impl;
#[cfg(not(feature = "std"))] mod nostd_io;
#[cfg(not(feature = "std"))] use nostd_io as io_impl;
pub mod io {
pub use crate::io_impl::{Error, ErrorKind, Read, Result, Write};
}That is:
In terms of diff for the current PR, rename&flatten |
mina86
changed the title
mina86
force-pushed
the
a
branch
2 times, most recently
from
3e04ae9 to
ffa6c3f
Compare
When built with `std` feature, borsh uses a handful of std::io types
in its public API (e.g. std::io::Write is used in BorshSerialize
trait). When `std` feature is disabled, borsh switches to its own
types which mimics behaviour of standard types offered through
`borsh::nostd_io`.
Problem is that this approach results in no consistent way to name
`Write`, `Read` etc. symbols used in the public API. This creates
a problem for authors and users of no_std libraries. A no_std library
might have code such as:
impl borsh::BorshSerialize for Foo {
fn serialize<W: borsh::nostd_io::Write>(
&self, writer: &mut W,
) -> borsh::maybestd::io::Result<()> {
todo!()
}
}
So long as borsh is built with default features disabled it will work
correctly. However, if author of a std application enables borsh’s
std feature, the aforementioned example crate will fail to build since
borsh::nostd_io will no longer be offered.
Address this by introducing `borsh::io` module which exports Error,
ErrorKind, Read, Result and Write symbols, i.e. the types which are
used in borsh’s public API.
With this change, author of a no_std library may write:
impl borsh::BorshSerialize for Foo {
fn serialize<W: borsh::io::Write>(
&self, writer: &mut W,
) -> borsh::maybestd::io::Result<()> {
todo!()
}
}
and their code will work without problems when used in std
applications.
| /// the exported types are custom borsh types which try to mimic behaviour of | ||
| /// corresponding standard types usually offering subset of features. | ||
| pub mod io { | ||
| pub use super::io_impl::{Error, ErrorKind, Read, Result, Write}; |
There was a problem hiding this comment.
rust allows to do this without much confusion )))
Haircut with fine nozzle
dj8yfo
changed the title
borsh::io with either items of std:io or private borsh::nostd_io module reexported (std or no_std)
Because it wasn’t a cornerstone reason. It was a quick example I came up with on the spot. The cornerstone reason was that there was no consistent way to name types used in the API which in turn resulted in build breaking when I don’t care if it’s |
|
@mina86 well, alternatively, u can use 4 lines to dispatch on an #[cfg(feature = "std")]
use std::io;
#[cfg(not(feature = "std"))]
use borsh::nostd_io as io; It's explicit and prevents you from importing 5 lines in client with explicit stating of what's used when and what's not vs 500 lines change in the subj repo is a good tradeoff imo. |
No, it’s not. I’m writing a no_std library. I don’t care about std. I wrote code which works with borsh with default features disabled. And then, when someone uses my library, the code breaks because borsh’s std feature was enabled. That a bug in borsh.
Yes, depending on whether Imagine borsh had a
I can make this a ‘5 line’ change in borsh if you prefer: +#[cfg(feature = "std")]
+pub mod io {
+ pub use std::io::{Read, Write, Error, ErrorKind, Result};
+}
+
+#[cfg(not(feature = "std"))]
+pub use nostd_io as io;I made it 500 to simplify other things in the code. This isn’t a valid argument. Not to mention that even if this is a 5 lines in client library, it’s 5 lines in every library and additional changes in every crate that transitively depends on it. |
When built with
stdfeature, borsh uses a handful of std::io typesin its public API (e.g. std::io::Write is used in BorshSerialize
trait). When
stdfeature is disabled, borsh switches to its owntypes which mimics behaviour of standard types offered through
borsh::nostd_io.Problem is that this approach results in no consistent way to name
Write,Readetc. symbols used in the public API. This createsa problem for authors and users of no_std libraries. A no_std library
might have code such as:
So long as borsh is built with default features disabled it will work
correctly. However, if author of a std application enables borsh’s
std feature, the aforementioned example crate will fail to build since
borsh::nostd_io will no longer be offered.
Address this by introducing
borsh::iomodule which exports Error,ErrorKind, Read, Result and Write symbols, i.e. the types which are
used in borsh’s public API.
With this change, author of a no_std library may write:
and their code will work without problems when used in std
applications.