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

Make all serialization functions available for no-std #1122

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

devrandom
Copy link

@devrandom devrandom commented Apr 15, 2024

We ran into OOMs in a memory constrained no-std environment, when serializing. We noticed that to_writer was not available for no-std, even though there is nothing preventing it from working.

This PR exposes the relevant serialization functions. It also exposes the crate's no-std io shim, since types from there appears in the ser API.

related to #1040

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

This isn't appropriate to expose in this way, because it makes the "std" feature non-additive. Cargo features are required to be additive. In this PR, one crate could build fine against no-std serde_json but as soon as any other crate in someone's dependency graph enables "serde_json/std", it would cause compilation failures in the first crate, which is not supposed to happen with additive features.

Could you see if it's possible for the "std" feature to remain additive? If not, I would prefer for the no-std use case to be served by a different library. This one does not need to be the only JSON library for serde.

@devrandom
Copy link
Author

If I understand correctly, you are concerned that if someone refers to the no_std version of structs / fns in serde_json::io, then they could break if the implementation changes to std via feature unification.

While you are correct in general, I don't think that's possible in this case, because the alloc version of serde_json::io is a strict subset of std::io, so something that compiled with the alloc version cannot fail to compile once the implementation changes to std. There are no extra fns in the former that are not in the latter, etc. . I tried to write a failing example, but could not come up with one.

@devrandom
Copy link
Author

OK, I did find one incompatibility, in the construction of Error - and I think it's fixed now. Added the binary that I used to test.

@dtolnay
Copy link
Member

dtolnay commented Apr 16, 2024

Some other examples of things that compile only if std is off, and fail non-additively as soon as std is enabled anywhere:

fn main() {
    let msg = Box::new("...");
    let _ = serde_json::io::Error::new(serde_json::io::ErrorKind::Other, &msg);
}
fn main() {
    match serde_json::io::ErrorKind::Other {
        serde_json::io::ErrorKind::Other => {}
    };
}
fn main() {
    let error = serde_json::io::Error::new(serde_json::io::ErrorKind::Other, "...");
    let _ = std::panic::catch_unwind(|| error);
}
use std::fmt::{self, Debug, Display};

enum MyError {
    JsonIo(serde_json::io::Error),
}

impl Display for MyError {
    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
        match self {
            MyError::JsonIo(error) => error.fmt(formatter),
        }
    }
}
trait WriteExt: serde_json::io::Write {/* ... */}

impl serde_json::io::Write for &mut dyn WriteExt {
    fn write(&mut self, bytes: &[u8]) -> serde_json::io::Result<usize> {/* ... */}
    fn flush(&mut self) -> serde_json::io::Result<()> {/* ... */}
}

Not sure what can be done about this one:

struct Buffer {/* ... */}

impl serde_json::io::Write for Buffer {
    fn write(&mut self, bytes: &[u8]) -> serde_json::io::Result<usize> {/* ... */}
    fn flush(&mut self) -> serde_json::io::Result<()> {/* ... */}
}

impl core2::io::Write for Buffer {
    fn write(&mut self, bytes: &[u8]) -> core2::io::Result<usize> {/* ... */}
    fn flush(&mut self) -> core2::io::Result<()> {/* ... */}
}

@devrandom
Copy link
Author

OK, good points.

I pushed a commit that changes the approach - the no-std io module is now always exported.

However, this is still non-additive, because Serializer uses different Write depending on the feature.

I'm not sure there's a complete solution without duplicating Serializer for the two cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants