Add a no_std/alloc feature #606

Xanewok · 2020-01-20T21:52:01Z

This builds up on #588 (thanks @Freax13!).

With this, we support no_std with alloc crate, starting from Rust 1.36 (this was done so that we can use it in WebAssembly environment but any such environment will work).

To minimize the noise, I tried to use the facade approach already used in Serde to provide some consistency between the two and improved on the io facade to work around it missing in core.

Thanks in advance for taking your time to review this and let me know if I can improve anything to help this land!

Closes #588. Closes #516.

dtolnay

Thanks! This is so much easier to review than the earlier no_std attempts. I intend to dedicate time toward getting this reviewed and merged.

I have not looked at most of this yet but here are some initial comments:

dtolnay · 2020-01-21T07:16:16Z

Cargo.toml

+
+# Provide integration for heap-allocated collections without depending on the
+# rest of the Rust standard library.
+# NOTE: Disabling both `std` *and* `alloc` features is not supported yet.


Please find a way to produce a reasonable error message for this case. The current error if you do serde_json = { default-features = false } is >1500 lines which is insane.

https://github.com/dtolnay/syn/blob/1.0.14/tests/features/mod.rs + https://github.com/dtolnay/syn/blob/1.0.14/tests/features/error.rs shows one good way.

Oh, that's neat! I wanted to do so but couldn't think of a reliable way to only show one compilation error.

~~As written here, maybe it'd be good to just support std and no-std (implying using alloc)? This would mean we wouldn't have to care about detecting no std and alloc anymore.~~

EDIT: Nevermind, we still need the Cargo alloc feature for serde/alloc and thus we still must check for either std or alloc to be enabled.

Done in 7852d2f.

Is there a way to run trybuild with custom feature flags, like compiletest? It'd be quite handy to have a UI test for --no-default-features so that we can catch whenever the error message regresses.

The current error if you do serde_json = { default-features = false } [...]

Shouldn't this new error be considered a breaking change?

dtolnay · 2020-01-21T07:19:12Z

src/lib.rs

+    pub use self::core::marker::{self, PhantomData};
+    pub use self::core::result::{self, Result};
+
+    #[cfg(all(feature = "alloc", not(feature = "std")))]


This can be simplified to not(feature = "std") because we know alloc must be enabled in that case. Same thing below.

In general feature = "alloc" should not need to appear anywhere.

I did that to introduce distinction between std, alloc and no feature at all, but maybe it'd be good only introduce two: default std and no-std (which implies alloc and is supported from 1.36 onwards)...

I'll adapt the code accordingly, thanks!

dtolnay · 2020-01-21T07:30:28Z

src/lib.rs

@@ -365,6 +427,7 @@ pub mod map;
 pub mod ser;


From cargo doc --no-default-features --features alloc:

This seems bad because the following would be accepted in alloc mode and break if a different part of the dependency graph enables std mode.

struct S; impl serde_json::ser::Formatter for S { fn write_null<W: ?Sized>(&mut self, _: &mut W) -> Result<(), &'static str> { Err("") } }

Either Formatter needs to be removed from the public API in alloc mode or the error type needs to be changed to an opaque type with no more than a subset of io::Error's API.

Oops, I didn't notice that, thanks!

~~Since std::io::Result is part of the default, public API I don't think we can be backwards compatible (methods are one thing but sth may depend on it being explicitly this type from std IIUC).~~

~~Because of this, I think the only thing we can do now, until std::io somehow lands in core, is to hide this from public API in no-std case.~~

EDIT: Again, sorry for the noise. You're right, that's a concern about code written for core not being compatible with std (not being a subset of real io) rather than the concrete public API... I'm on it.

Reimplemented a small subset of std::io::Error and friends so that accidentally opting into std should not break stuff

Xanewok · 2020-01-21T20:29:20Z

I addressed the feedback and thus also managed to unify the io facade as well (which now lives in io/mod.rs, while the small no_std-specific mplementation lives in io/core.rs). With this, the only place which specifies std-related conditional compilation is:

lib.rs - for the prelude/facade used across the library
io module - to reexport either std::io or the no_std reimplemented bits
features_check - to provide user-friendly compilation error for missing both std/alloc features

Overall, I'm pretty happy with how the io improvements turned out to be and with the overall patch. Thanks for reviewing this and let me know if I should change anything else!

Inlining this simple, already `core`-compatible function is better than noisily repeating the same definition that does exactly the same, albeit hidden behind a fn call.

So that when implementing a no-`std` logic we don't break the build when some other dependency opts into `std` (causing API mismatch).

dtolnay

Thanks, this looks great to me. I am making some tweaks in later commits but this is ready to land.

Question: for your use case do you require to_writer and/or Serializer, or only to_string? I am removing to_writer and Serializer from the public API in no-std mode for now. These rely on the caller to pass a Write impl and the only impl this PR provides in serde_json is for Vec<u8>. It is going to require more design work to figure out how best to expose that. With the io facade as implemented here, the caller is unable to write Write impls, and I also don't want to be in the business of copying many more Write impls from std into serde_json.

Xanewok · 2020-01-22T23:16:32Z

Awesome, thank you!

for your use case do you require to_writer and/or Serializer, or only to_string?

I think to_string suffices for now, yeah.

(...) and I also don't want to be in the business of copying many more Write impls from std into serde_json.

That's completely understandable! It's worth noting that for the second iteration of io I designed it so that we don't need to do any std-specific conditional compilation in files other than the io or lib facades (but couldn't make it proper pub(crate) due to io::Result leaking in the public API).

I am making some tweaks in later commits (...)

Thanks for not blocking on this but it'd be completely fine to ask me to follow up with other tweaks related to this PR!

dtolnay · 2020-01-23T00:08:18Z

Published in 1.0.45.

Relevant Serde PR: serde-rs/serde#1620 To support both no-/std builds without using somewhat noisy conditional compilation directives, we implement the re-exported `serde::de::StdError` trait in serde-rs#606. However, this was only introduced in >= 1.0.100, so we need to bump the version requirement of serde. On the off chance of someone pulling in incompatible 1.0.4{5,6} versions of serde_json, I believe it'd be good to yank those and cut a new release with this patch. Sorry for the omission in the original PR. Fixes serde-rs#621.

Relevant Serde PR: serde-rs/serde#1620 To support both no-/std builds without using somewhat noisy conditional compilation directives, we implement the re-exported `serde::de::StdError` trait in serde-rs#606. However, this was only introduced in >= 1.0.100, so we need to bump the version requirement of serde. On the off chance of someone pulling in incompatible 1.0.4{5,6} versions of serde_json, I believe it'd be good to yank those and cut a new release with this patch. Sorry for the omission in the original PR. Fixes serde-rs#612.

dtolnay requested changes Jan 21, 2020

View reviewed changes

Freax13 added 2 commits January 21, 2020 15:52

add alloc/no_std support

4600a9a

Fix std feature dependency for itoa

6533670

Xanewok force-pushed the no-std-patch branch from 1f586cd to f0f69a8 Compare January 21, 2020 18:41

Xanewok force-pushed the no-std-patch branch from f0f69a8 to 245ab69 Compare January 21, 2020 20:39

Xanewok and others added 9 commits January 21, 2020 21:43

Add facade around std/alloc/core like serde does

9ecf915

Reorganize std::io facade

15bfaf6

Mention availability of alloc feature

b47d2d1

travis: Test no_std/alloc on 1.36.0

d7f7a51

Inline the itoa::write calls

176ff71

Inlining this simple, already `core`-compatible function is better than noisily repeating the same definition that does exactly the same, albeit hidden behind a fn call.

Add user-friendly error when no alloc or std feature are enabled

7852d2f

Imply using alloc crate when std is not enabled

bac77ed

Make io a subset of std::io

3b03040

So that when implementing a no-`std` logic we don't break the build when some other dependency opts into `std` (causing API mismatch).

Move the core::io implementation to separate module

8e8058a

Xanewok force-pushed the no-std-patch branch from 245ab69 to 8e8058a Compare January 21, 2020 20:45

dtolnay approved these changes Jan 22, 2020

View reviewed changes

dtolnay merged commit 91f791b into serde-rs:master Jan 22, 2020

cbeck88 mentioned this pull request Jan 22, 2020

No std support? oli-obk/cargo_metadata#98

Closed

Xanewok deleted the no-std-patch branch January 22, 2020 23:16

This was referenced Jan 22, 2020

Support no_std #362

Closed

Add support for alloc::* types rust-embedded-community/serde-json-core#4

Closed

Correct no-std support in README.md #609

Merged

Xanewok mentioned this pull request Jan 23, 2020

Support no_std paritytech/jsonrpsee#1

Closed

Xanewok mentioned this pull request Feb 7, 2020

Require serde >= 1.0.100 due to no_std-related Error re-export #617

Merged

JoshOrndorff mentioned this pull request Mar 31, 2021

Upgrade Recipes to Substrate 3.0 JoshOrndorff/substrate-recipes#419

Merged

7 tasks

rjsberry mentioned this pull request Jul 14, 2023

Supporting no-std and no-alloc #1040

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a no_std/alloc feature #606

Add a no_std/alloc feature #606

Xanewok commented Jan 20, 2020 •

edited by dtolnay

Loading

dtolnay left a comment

dtolnay Jan 21, 2020

Xanewok Jan 21, 2020 •

edited

Loading

Xanewok Jan 21, 2020 •

edited

Loading

cuviper Feb 6, 2020

dtolnay Jan 21, 2020

Xanewok Jan 21, 2020

dtolnay Jan 21, 2020

Xanewok Jan 21, 2020 •

edited

Loading

Xanewok Jan 21, 2020

Xanewok commented Jan 21, 2020

dtolnay left a comment

Xanewok commented Jan 22, 2020

dtolnay commented Jan 23, 2020

Add a no_std/alloc feature #606

Add a no_std/alloc feature #606

Conversation

Xanewok commented Jan 20, 2020 • edited by dtolnay Loading

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay Jan 21, 2020

Choose a reason for hiding this comment

Xanewok Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Xanewok Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

cuviper Feb 6, 2020

Choose a reason for hiding this comment

dtolnay Jan 21, 2020

Choose a reason for hiding this comment

Xanewok Jan 21, 2020

Choose a reason for hiding this comment

dtolnay Jan 21, 2020

Choose a reason for hiding this comment

Xanewok Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Xanewok Jan 21, 2020

Choose a reason for hiding this comment

Xanewok commented Jan 21, 2020

dtolnay left a comment

Choose a reason for hiding this comment

Xanewok commented Jan 22, 2020

dtolnay commented Jan 23, 2020

Xanewok commented Jan 20, 2020 •

edited by dtolnay

Loading

Xanewok Jan 21, 2020 •

edited

Loading

Xanewok Jan 21, 2020 •

edited

Loading

Xanewok Jan 21, 2020 •

edited

Loading