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
3 changes: 3 additions & 0 deletions .travis.yml
Expand Up @@ -19,6 +19,9 @@ matrix:
- rust: stable
- rust: beta
- rust: 1.31.0
- rust: 1.36.0
script:
- cargo build --no-default-features --features alloc

- rust: nightly
name: Clippy
Expand Down
14 changes: 11 additions & 3 deletions Cargo.toml
Expand Up @@ -16,9 +16,9 @@ travis-ci = { repository = "serde-rs/json" }
appveyor = { repository = "serde-rs/json" }

[dependencies]
serde = "1.0.60"
serde = { version = "1.0.60", default-features = false }
indexmap = { version = "1.2", optional = true }
itoa = "0.4.3"
itoa = { version = "0.4.3", default-features = false }
ryu = "1.0"

[dev-dependencies]
Expand All @@ -42,7 +42,15 @@ features = ["raw_value"]
### FEATURES #################################################################

[features]
default = []
default = ["std"]

std = ["serde/std", "itoa/std"]

# 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

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

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?

# Available on Rust 1.36+.
alloc = ["serde/alloc"]

# Use a different representation for the map type of serde_json::Value.
# This allows data to be read into a Value and written back to a JSON string
Expand Down
9 changes: 4 additions & 5 deletions src/de.rs
@@ -1,10 +1,9 @@
//! Deserialize JSON data to a Rust data structure.

use std::io;
use std::marker::PhantomData;
use std::result;
use std::str::FromStr;
use std::{i32, u64};
use lib::str::FromStr;
use lib::*;

use io;

use serde::de::{self, Expected, Unexpected};

Expand Down
13 changes: 6 additions & 7 deletions src/error.rs
@@ -1,10 +1,9 @@
//! When serializing or deserializing JSON goes wrong.

use std::error;
use std::fmt::{self, Debug, Display};
use std::io;
use std::result;
use std::str::FromStr;
use lib::str::FromStr;
use lib::*;

use io;

use serde::de;
use serde::ser;
Expand Down Expand Up @@ -333,8 +332,8 @@ impl Display for ErrorCode {
}
}

impl error::Error for Error {
fn source(&self) -> Option<&(error::Error + 'static)> {
impl serde::de::StdError for Error {
fn source(&self) -> Option<&(serde::de::StdError + 'static)> {
match self.err.code {
ErrorCode::Io(ref err) => Some(err),
_ => None,
Expand Down
1 change: 1 addition & 0 deletions src/features_check/error.rs
@@ -0,0 +1 @@
"serde_json doesn't work without `alloc` or default `std` feature yet"
13 changes: 13 additions & 0 deletions src/features_check/mod.rs
@@ -0,0 +1,13 @@
//! Shows a user-friendly compiler error on incompatible selected features.

#[allow(unused_macros)]
macro_rules! hide_from_rustfmt {
($mod:item) => {
$mod
};
}

#[cfg(all(not(feature = "std"), not(feature = "alloc")))]
hide_from_rustfmt! {
mod error;
}
158 changes: 158 additions & 0 deletions src/io/core.rs
@@ -0,0 +1,158 @@
//! Reimplements core logic and types from `std::io` in an `alloc`-friendly
//! fashion.
#![cfg(not(feature = "std"))]

use lib::*;

pub enum ErrorKind {
InvalidData,
WriteZero,
Other,
UnexpectedEof,
}

impl ErrorKind {
#[inline]
fn as_str(&self) -> &'static str {
match self {
ErrorKind::InvalidData => "invalid data",
ErrorKind::WriteZero => "write zero",
ErrorKind::Other => "other os error",
ErrorKind::UnexpectedEof => "unexpected end of file",
}
}
}

pub struct Error {
repr: Repr,
}

enum Repr {
Simple(ErrorKind),
Custom(ErrorKind, Box<dyn serde::de::StdError + Send + Sync>),
}

impl Display for Error {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
match &self.repr {
Repr::Custom(_, msg) => write!(fmt, "{}", msg),
Repr::Simple(kind) => write!(fmt, "{}", kind.as_str()),
}
}
}

impl Debug for Error {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt(self, fmt)
}
}

impl serde::de::StdError for Error {}

impl From<ErrorKind> for Error {
#[inline]
fn from(kind: ErrorKind) -> Error {
Error {
repr: Repr::Simple(kind),
}
}
}

impl Error {
#[inline]
pub fn new<E>(kind: ErrorKind, error: E) -> Error
where
E: Into<Box<dyn serde::de::StdError + Send + Sync>>,
{
Error {
repr: Repr::Custom(kind, error.into()),
}
}
}

pub type Result<T> = result::Result<T, Error>;

pub trait Write {
fn write(&mut self, buf: &[u8]) -> Result<usize>;

fn write_all(&mut self, mut buf: &[u8]) -> Result<()> {
while !buf.is_empty() {
match self.write(buf) {
Ok(0) => {
return Err(Error::new(
ErrorKind::WriteZero,
"failed to write whole buffer",
))
}
Ok(n) => buf = &buf[n..],
Err(e) => return Err(e),
}
}
Ok(())
}

fn flush(&mut self) -> Result<()>;
}

impl<W: Write> Write for &mut W {
#[inline]
fn write(&mut self, buf: &[u8]) -> Result<usize> {
(*self).write(buf)
}

#[inline]
fn write_all(&mut self, buf: &[u8]) -> Result<()> {
(*self).write_all(buf)
}

#[inline]
fn flush(&mut self) -> Result<()> {
(*self).flush()
}
}

impl Write for Vec<u8> {
#[inline]
fn write(&mut self, buf: &[u8]) -> Result<usize> {
self.extend_from_slice(buf);
Ok(buf.len())
}

#[inline]
fn write_all(&mut self, buf: &[u8]) -> Result<()> {
self.extend_from_slice(buf);
Ok(())
}

#[inline]
fn flush(&mut self) -> Result<()> {
Ok(())
}
}

pub trait Read {
fn read(&mut self, buf: &mut [u8]) -> Result<usize>;
fn bytes(self) -> Bytes<Self>
where
Self: Sized,
{
Bytes { inner: self }
}
}

pub struct Bytes<R> {
inner: R,
}

impl<R: Read> Iterator for Bytes<R> {
type Item = Result<u8>;

fn next(&mut self) -> Option<Result<u8>> {
let mut byte = 0;
match self.inner.read(slice::from_mut(&mut byte)) {
Ok(0) => None,
Ok(..) => Some(Ok(byte)),
Err(e) => Some(Err(e)),
}
}
}
19 changes: 19 additions & 0 deletions src/io/mod.rs
@@ -0,0 +1,19 @@
//! A tiny, `no_std`-friendly facade around `std::io`.
//! Reexports types from `std` when available; otherwise reimplements and
//! provides some of the core logic.
//!
//! The main reason that `std::io` hasn't found itself reexported as part of
//! the `core` crate is the `std::io::{Read, Write}` traits' reliance on
//! `std::io::Error`, which may contain internally a heap-allocated `Box<Error>`
//! and/or now relying on OS-specific `std::backtrace::Backtrace`.

pub use self::imp::{Bytes, Error, ErrorKind, Read, Result, Write};

mod core;

mod imp {
#[cfg(not(feature = "std"))]
pub use super::core::*;
#[cfg(feature = "std")]
pub use std::io::*;
}
2 changes: 1 addition & 1 deletion src/iter.rs
@@ -1,4 +1,4 @@
use std::io;
use io;

pub struct LineColIterator<I> {
iter: I,
Expand Down
69 changes: 67 additions & 2 deletions src/lib.rs
Expand Up @@ -331,6 +331,7 @@
must_use_candidate,
))]
#![deny(missing_docs)]
#![cfg_attr(not(feature = "std"), no_std)]

#[macro_use]
extern crate serde;
Expand All @@ -339,6 +340,67 @@ extern crate indexmap;
extern crate itoa;
extern crate ryu;

////////////////////////////////////////////////////////////////////////////////

#[cfg(not(feature = "std"))]
extern crate alloc;

/// A facade around all the types we need from the `std`, `core`, and `alloc`
/// crates. This avoids elaborate import wrangling having to happen in every
/// module.
mod lib {
mod core {
#[cfg(not(feature = "std"))]
pub use core::*;
#[cfg(feature = "std")]
pub use std::*;
}

pub use self::core::{char, str};
pub use self::core::{cmp, mem, num, slice};

pub use self::core::{borrow, iter, ops};

pub use self::core::cell::{Cell, RefCell};
pub use self::core::clone::{self, Clone};
pub use self::core::convert::{self, From, Into};
pub use self::core::default::{self, Default};
pub use self::core::fmt::{self, Debug, Display};
pub use self::core::hash::{self, Hash};
pub use self::core::marker::{self, PhantomData};
pub use self::core::result::{self, Result};

#[cfg(not(feature = "std"))]
pub use alloc::borrow::{Cow, ToOwned};
#[cfg(feature = "std")]
pub use std::borrow::{Cow, ToOwned};

#[cfg(not(feature = "std"))]
pub use alloc::string::{String, ToString};
#[cfg(feature = "std")]
pub use std::string::{String, ToString};

#[cfg(not(feature = "std"))]
pub use alloc::vec::{self, Vec};
#[cfg(feature = "std")]
pub use std::vec::{self, Vec};

#[cfg(not(feature = "std"))]
pub use alloc::boxed::Box;
#[cfg(feature = "std")]
pub use std::boxed::Box;

#[cfg(not(feature = "std"))]
pub use alloc::collections::{btree_map, BTreeMap};
#[cfg(feature = "std")]
pub use std::collections::{btree_map, BTreeMap};

#[cfg(feature = "std")]
pub use std::error;
}

////////////////////////////////////////////////////////////////////////////////

#[doc(inline)]
pub use self::de::{from_reader, from_slice, from_str, Deserializer, StreamDeserializer};
#[doc(inline)]
Expand All @@ -355,8 +417,8 @@ pub use self::value::{from_value, to_value, Map, Number, Value};
macro_rules! try {
($e:expr) => {
match $e {
::std::result::Result::Ok(val) => val,
::std::result::Result::Err(err) => return ::std::result::Result::Err(err),
::lib::Result::Ok(val) => val,
::lib::Result::Err(err) => return ::lib::Result::Err(err),
}
};
}
Expand All @@ -370,6 +432,9 @@ pub mod map;
pub mod ser;
Copy link
Member

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

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

pub mod value;

mod features_check;

mod io;
mod iter;
mod number;
mod read;
Expand Down