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

Add a no_std/alloc feature #606

Merged
merged 11 commits into from Jan 22, 2020
Merged

Add a no_std/alloc feature #606

merged 11 commits into from Jan 22, 2020

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Jan 20, 2020

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.

Copy link
Member

@dtolnay dtolnay left a comment

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:


# 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.
Copy link
Member

@dtolnay dtolnay Jan 21, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Xanewok Xanewok Jan 21, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Xanewok Xanewok Jan 21, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cuviper cuviper Feb 6, 2020

Choose a reason for hiding this comment

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

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

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

src/lib.rs Outdated
pub use self::core::marker::{self, PhantomData};
pub use self::core::result::{self, Result};

#[cfg(all(feature = "alloc", not(feature = "std")))]
Copy link
Member

@dtolnay dtolnay Jan 21, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Xanewok Xanewok Jan 21, 2020

Choose a reason for hiding this comment

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

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!

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

@dtolnay dtolnay Jan 21, 2020

Choose a reason for hiding this comment

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

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

Screenshot from 2020-01-20 23-26-49


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.

Copy link
Contributor Author

@Xanewok Xanewok Jan 21, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Xanewok Xanewok Jan 21, 2020

Choose a reason for hiding this comment

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

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

@Xanewok
Copy link
Contributor Author

Xanewok commented Jan 21, 2020

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:

  1. lib.rs - for the prelude/facade used across the library
  2. io module - to reexport either std::io or the no_std reimplemented bits
  3. 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!

Xanewok and others added 9 commits Jan 21, 2020
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).
Copy link
Member

@dtolnay dtolnay left a comment

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.

@dtolnay dtolnay merged commit 91f791b into serde-rs:master Jan 22, 2020
1 check passed
@Xanewok
Copy link
Contributor Author

Xanewok commented Jan 22, 2020

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
Copy link
Member

dtolnay commented Jan 23, 2020

Published in 1.0.45.

Xanewok added a commit to Xanewok/json that referenced this issue Feb 7, 2020
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:🇩🇪: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.
Xanewok added a commit to Xanewok/json that referenced this issue Feb 7, 2020
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:🇩🇪: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.
Xanewok added a commit to Xanewok/json that referenced this issue Feb 7, 2020
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:🇩🇪: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.
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

5 participants