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

Allow specifying serde path and avoid injecting extern crate serde as _serde. #1487

Closed
carllerche opened this issue Feb 20, 2019 · 4 comments

Comments

@carllerche
Copy link

tower-web provides a #[derive(Response)] macro. This macro is a shim on top of serde's derive(Serialize). It works by catching HTTP specific attributes and leaving any serde attributes. It then generates a "shadow" version of the struct that has the actual derive(Serialize).

The problem is that the user must add serde to their Cargo.toml even if they do not interact with any serde APIs directly.

tower-web re-exports serde in the crate. I propose to have a serde attribute that allows specifying the path to serde. If this attribute is set, the extern crate serde as _serde is omitted.

This gist illustrates the problem. hello.rs is very basic. I also included the expanded code.

In short, I would like this line omitted and all references to _serde replaced with __tw::codegen::serde.

One way to expose the API is as an attribute:

#[derive(Serialize)]
#[serde(serde_path = "__tw::codegen::serde")]
struct Routes {
// ....
}

If you like this strategy, I could attempt a PR.

@dtolnay
Copy link
Member

dtolnay commented Feb 20, 2019

Seems reasonable. Please send a PR.

You will need to decide whether the given path is a use path or a relative path i.e. whether the expanded code is:

use __tw::codegen::serde;
impl serde::Serialize for T

or this:

impl __tw::codegen::serde::Serialize for T

Those would have different behavior in 2015 edition.

@carllerche
Copy link
Author

I'm pretty sure use doesn't work in the anon const scope thing. Since tower-web generates in a const scope, it cannot do option 1.

My thought is that whatever is set in #[serde(serde_path = "__tw::codegen::serde")] is used verbatim in the generated code. i.e. option 2.

Do you have thoughts on this? Also, do you have thoughts on what the attr should be?

@carllerche
Copy link
Author

Gentle ping :)

When you are +1 on the path, I can try to put together a PR (since it will probably be non-trivial, I'm hoping to get approval on the direction first).

@dtolnay
Copy link
Member

dtolnay commented Feb 26, 2019

Thanks for the reminder. I would accept a PR for option 2 using:

#[serde(crate = "__tw::codegen::serde")]

as the attribute.

sgrif added a commit to sgrif/serde that referenced this issue Mar 18, 2019
This is intended to be used by other crates which provide their own proc
macros and use serde internally. Today there's no consistent way to put
`#[derive(Deserialize)]` on a struct that consistently works, since
crates may be using either `features = ["derive"]` or relying on
`serde_derive` separately.

Even if we assume that everyone is using `features = ["derive"]`,
without this commit, any crate which generates
`#[derive(serde::Deserialize)]` forces its consumers to put `serde` in
their `Cargo.toml`, even if they aren't otherwise using serde for
anything.

Examples of crates which suffer from this in the real world are
tower-web and swirl.

With this feature, it's expected that these crates would have `pub
extern crate serde;` in some accessible path, and add
`#[serde(serde_path = "that_crate::wherever::serde")]` anywhere they
place serde's derives. Those crates would also have to derive
`that_crate::whatever::serde::Deserialize`, or `use` the macros
explicitly beforehand.

The test for this is a little funky, as it's testing this in a way that
is not the intended use case, or even one we want to support. It has its
own module which re-exports all of serde, but defines its own
`Serialize` and `Deserialize` traits. We then test that we generated
impls for those traits, instead of serde's. The only other way to test
this would be to create a new test crate which does not depend on serde,
but instead depends on `serde_derive` and a third crate which publicly
re-exports serde. This feels like way too much overhead for a single
test case, hence the funky test given.

I didn't see anywhere in this repo to document this attribute, so I
assume the docs will have to be done as a separate PR to a separate
repo.

Fixes serde-rs#1487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants