Add proof of concept serde support with PathBuf

[jamesmunns]

This is a proof of concept for supporting types from async_std to be used with Serde. For now I have only implemented support for PathBuf, but if you like this, we can start rolling it out to all of the wrapped types.

// src/lib.rs:
use async_std;
use serde::{self, Serialize, Deserialize};

#[derive(Serialize, Deserialize)]
struct Hmmm {
    ex: async_std::path::PathBuf,
}

fn main() {
    println!("Hello, world!");
}

# Cargo.toml
[package]
name = "serde-ex"
version = "0.1.0"
authors = ["James Munns <[email protected]>"]
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies.async-std]
version = "1.0"
path = "../async-std"

[dependencies.serde]
features = ["derive"]
version = "1.0"

[yoshuawuyts]

@jamesmunns would it be possible to use serde-derive here instead so that we can call the feature serde?

[jamesmunns]

@yoshuawuyts I think so! It's a habit of mine to just use serde as a dependency, but it's likely not needed here, just serde-derive.

[jamesmunns]

Things that need to be decided now:

1. What should the feature name be? just serde if we can depend on serde-derive? Something else?
2. Should this be behind the unstable feature? e.g. require serde-support AND unstable be set for now?
3. Are there any cases where a simple flatten won't work? Maybe wrapper types with more than one field?

Once we decide and correct those, we can probably land this with just PathBuf supported, then start incrementally covering more data types. Or we can hit them all at once in this PR.

[yoshuawuyts]

Another thing we may want to consider is adding an entry to our "features" section in lib.rs: https://docs.rs/async-std/1.0.0/async_std/#features. This seems like something people may want to be aware of.

[jamesmunns]

@yoshuawuyts ah, no, I think we need a serde dependency as well. Otherwise I get:

    Checking async-std v1.0.0 (/tmp/async-std)
error[E0463]: can't find crate for `serde`
  --> /tmp/async-std/src/path/pathbuf.rs:22:49
   |
22 | #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
   |                                                 ^^^^^^^^^^^ can't find crate

[ghost]

Should we put serde support behind a new feature flag, perhaps named serde1?

See also: rust-lang/api-guidelines#180

[jamesmunns]

Hmm, this needs a deeper look. My first approach won't work.

use async_std;
use serde_json::to_string;
use serde::{self, Serialize, Deserialize};

#[derive(Serialize, Deserialize)]
struct Hmmm {
    ex: async_std::path::PathBuf,
}

fn main() {
    dbg!(to_string(&Hmmm {
        ex: async_std::path::PathBuf::new(),
    }).unwrap());
}

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error("can only flatten structs and maps (got a string)", line: 0, column: 0)', src/libcore/result.rs:1165:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

There's likely still a way, just probably a bit less elegant. Merits further investigation.

[jamesmunns]

But it does work for newtypes! c70f00e switches PathBuf to be a newtype, rather than a struct containing an inner field.

[src/main.rs:11] to_string(&Hmmm{ex: async_std::path::PathBuf::new(),}).unwrap() = "{\"ex\":\"\"}"

I don't think this needs to be a breaking change? And for fields where a newtype doesn't work, we can always manually impl Serialize/Deserialize

[yoshuawuyts]

I don't think this needs to be a breaking change?

We've never provided guarantees about internals, so indeed don't think it's a breaking change.

I think this PR looks great; but probably want to give @stjepang a chance to sign off on it too before we merge.

[jamesmunns]

@stjepang just let me know if you'd prefer serde or serde1 as the feature flag, then I think we can land this, and start adding serde support to other structures.

[ghost]

@yoshuawuyts and I discussed this during triage.

I'm 👍 with the idea of making more and more async-std types implement Serialize and Deserialize.

I don't think those implementations should also go behind the unstable flag, that would a bit too cautious to the point where stabilizations becomes an nuisance with little benefit.

Approving. Thanks for submitting the PR! :)

[ghost]

@dtolnay Do you think we should name the feature flag serde or serde1?

[dtolnay]

I recommend sticking with "serde".

Two thoughts:

• I am skeptical of making this a default feature. What fraction of users need this enabled? Or is it default only because you already depend on something that depends on serde?
• I am skeptical of using serde_derive. A pair of Serialize/Deserialize impls for a wrapper type that forwards to the inner PathBuf's impls are ~15 lines to write. Is that a better tradeoff?

[yoshuawuyts]

I am skeptical of making this a default feature. (...)

Agreed; this should only be available when the serde feature is enabled. We currently don't depend on serde anywhere else, so it's indeed a new dep.

I am skeptical of using serde_derive. (...)

Oh, yeah a manual impl def sounds better here. Thanks for suggesting!

[maisiliym]

Status?
Is anyone working on a PR to convert all wrapper types to newtypes, with serde feature?

Edit: That approach wont work in every case, as quick experiment with Path showed.
Indeed, manual impls will be needed.
The case has been brought to serde: serde-rs/serde#1913

[hoijui]

How is the state of this?
As I understood from the conversation here, it is ready to be merged.. no?

( personally need serde support exactly for PathBuf)