-
-
Notifications
You must be signed in to change notification settings - Fork 861
feat: Extract serde_core out of serde crate #2608
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Besides the gains here, this allows serde_json and friends to depend on serde_core, preventing packages using serde w/ derive from serializing things. |
There're merge conflicts on this branch again which needs to be resolved. |
Hey, the merge conflicts on this PR are usually trivial to resolve (the PR itself doesn't change anything about the code in serde, just it's structure) and they'll pop up with each new serde version released; I am not sure if it makes sense to massage it all the time given that this PR has went almost unnoticed for 2 months now. I'd say it should be fine to resolve them once this PR gets some attention. =) |
Hmmm I thought this PR is blocked because merge conflicts and would get some attentions once it is resolved, but it turns out that it just doesn't get much attention regardless of merge conflicts. This PR would really help improve compilation time of serde with derive enabled and I was looking forward to it, it's sad that it has been blocking for so long was no review from maintainer. |
Yeah, I think so too. On the other hand, it was not asked for so no hard feelings there. But yeah, the conflicts will always be there whenever a new version of serde is released, as this PR moves half of the files into other directory (which |
Two things:
|
Hey, should we come back to this PR whenever you're available then? By that I also mean whether I should fix it up in the meantime or is it fine to just come back to it in 2 months and rebase it then? |
I think it would be fine to just not rebase it. Either @dtolnay will pick it up at whatever state it is in while I'm gone and ask you to rebase, or I will when I'm back. |
As a real-world example of how this can be beneficial, I was looking at the timings chart for ripgrep on the parallel frontend blog post. I did some analysis on it. I'm guessing this change on its own would take ripgrep build times on a 24-core machine from 9s to 6-7s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
Some things I would want to see further consideration of before deciding how to move forward with this design are:
-
the effect on compiler diagnostics seen by users, or other potential friction from this change, particularly when it comes to users who may be new to Rust
-
implications for serde 2.0 when it comes to the private helper types that serde_derive relies on
-
implications for serde 2.0 when it comes to wishlist for 2.0
(Maybe @oli-obk can work with you to flesh these out.)
Compiler diagnostics
After this change, what are the situations in which diagnostics that used to look like "the trait bound `T: serde::ser::Serialize` is not satisfied" will instead say "the trait bound `T: serde_core::ser::Serialize` is not satisfied"? — these occur often when you are using a data format library like serde_json together with a data structure library whose Serde trait impls are behind a feature.
I am not familiar with the heuristics rustc uses to choose whether to display the trait path in these errors as serde::ser::Serialize
or serde_core::ser::Serialize
or serde_core::Serialize
or just Serialize
.
For errors that refer to serde
, users have been able to unblock themselves well enough by finding that the crate's documentation mentions "serde support" behind a feature, or noticing in cargo add
it printed that there is a serde feature, or noticing that the relevant impl serde::Serialize for ...
in the crate's source code has a conditional compilation attribute.
A crate is unlikely to document that it has "serde_core support". To some users it will be obvious that a serde
trait impl is interchangeable with a serde_core
trait impl, but this isn't the case in general. For example thiserror
and thiserror_core
are unrelated in this way, serde_json
and serde_json_core
are unrelated, etc. Maybe consider whether serde_traits
, or something else, would be better naming.
I think this is probably inevitable given that moving the traits to a different crate that is a dependency of serde is most likely required for ever publishing serde_derive 2.0. (Although semver trick is another option here, which log
used successfully in the past.) So this one is a nonblocking concern. Nevertheless it would be good to understand if there is a need for any kind of followup in rustc. Serde is commonly one of the very first crates that someone new to Rust would first use, so there is a lot of leverage to scare users off of Rust with hours of frustration, likely greater than the cost of 3 seconds in a first-time build.
What mitigation options exist, and how do they compare? Is setting [lib] name = "serde"
on serde_core better or worse?
Private helper types
Currently this PR has moved all of serde::__private
into serde_core.
That may not fly. It means that someone is able to write:
serde = { package = "serde_core", version = "1" }
serde_derive = "1"
which then means all of serde_derive 1.0's helper types need to be present in serde_core in perpetuity, even all the ones serde_derive 2.0 does not use, even if serde 1.0 is not in the build graph. I have not measured recently, but these helper types account for a nontrivial part of the compile time of serde.
What alternative options exist, and how do they compare? Keep helper types in serde for now? Put helper types in a third crate somehow? Some feature voodoo? Progress on rust-lang/rust#54363 would be valuable here too.
Items from 2.0 wishlist
Relocating traits into a different crate is a one-time deal. When we do serde_derive 2.0 (in which I'd like to remove support for many attributes that are too niche, and some changes like default = $expr
should take an expression instead of a string literal) and serde 2.0 to go with it, it needs to be written against serde_core 1.0 because the cost–benefit from improving the macro is a lot better than the cost–benefit of breaking the ecosystem with incompatible traits.
That means some of what might be on the table for serde 2.0 needs to be accounted for now. In particular, this would be the time to figure out what(whether) to do about #1070. Do the Serialize and Deserialize impl for Result
need to be behind a feature that is enabled only by serde 1.0 and not 2.0? Missing that now would be a blunder because it cannot be fixed later.
Hopefully that one is the only 2.0 item needing consideration, but I am not sure.
serde_core/src/lib.rs
Outdated
@@ -0,0 +1,318 @@ | |||
//! # Serde |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the crate-level documentation to convey the intended way to use this crate, and why someone would want to. From reading this documentation, ideally there should be no remaining questions about why this crate (which looks superficially identical to the serde crate) exists, including for someone quite inexperienced with Rust, and for experienced authors of data format libraries. Having identical documentation in both is not helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I unresolved this again. The main serde_core rustdocs need something specific to serde_core
Not tested with Using a type in a
Trait not in scope. Note the suggestion of
|
My assumption for this is the helpers would be in
Maybe with |
Just to add to second example provided by @epage; it looks like rustc may sometimes pick up a different path to (De)serialize trait, based on whether user has serde in non-macro scope.
Stderr with `use serde`
Stderr with `use serde_core`
When both serde and serde_core are used, the one that's imported first is the one that's suggested in diagnostics. As for the first example, could it be that rustc always picks "local" path to a (De)serialize trait, since the failure here is related to monomorphization/typeck? |
That's fine. When people are actually directly depending on serde_core, it's fine to suggest importing from it. Almost no one will be directly depending on serde_core. The fact that the import suggestions and other diagnostics always refer to
There are possibly other diagnostics that use the full path instead of just the reduced path, especially on older compiler versions. We've since moved diagnostics to refer to imported paths, but there may be some cases left where we report the full path (and that may then refer to serde_core). That's rare and in almost all cases a compiler bug, so we can ignore that. |
So... the only remaining step is to consider putting various "fishy" This step requires going through every impl in serde_core and making a decision for whether to cargo-feature it or not. Most of them should be obviously fine (integers, atomics, string, vec, ...) |
also next time you merge, maybe rebase and squash instead? |
12b7260
to
ffe914d
Compare
c71ecab
to
7d74d54
Compare
Outside of |
I wonder, is there a clear migration path for crates such as serde_json to start using |
They could have a "never matching" |
667ea72
to
f9baf39
Compare
Using |
That benefit would largely be superseded by this PR though, because the format crates ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -0,0 +1,62 @@ | |||
[package] | |||
name = "serde_core" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've since moved diagnostics to refer to imported paths, but there may be some cases left where we report the full path (and that may then refer to serde_core). That's rare and in almost all cases a compiler bug, so we can ignore that.
I don't believe that this is necessarily rare. I expect that the following kind of thing would be pretty common in the code of people new to Rust. A lot of people use serde and serde_json in their first days learning Rust so it is important to be mindful of this cohort.
struct MyStruct {}
fn main() {
let thing = MyStruct {};
serde_json::to_string(&thing);
}
error[E0277]: the trait bound `MyStruct: serde_core::ser::Serialize` is not satisfied
--> src/main.rs:5:27
|
5 | serde_json::to_string(&thing);
| --------------------- ^^^^^^ unsatisfied trait bound
| |
| required by a bound introduced by this call
|
help: the trait `serde_core::ser::Serialize` is not implemented for `MyStruct`
--> src/main.rs:1:1
|
1 | struct MyStruct {}
| ^^^^^^^^^^^^^^^
= note: for local types consider adding `#[derive(serde::Serialize)]` to your `MyStruct` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `serde_core::ser::Serialize`:
&'a T
&'a mut T
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
(T0, T1, T2, T3, T4)
and 131 others
note: required by a bound in `serde_json::to_string`
--> $CARGO/serde_json-1.0.143/src/ser.rs:2247:17
|
2245 | pub fn to_string<T>(value: &T) -> Result<String>
| --------- required by a bound in this function
2246 | where
2247 | T: ?Sized + Serialize,
| ^^^^^^^^^ required by this bound in `to_string`
A diagnostic::on_unimplemented
that rewrites the message from serde_core::ser::Serialize
to serde::Serialize
would be beneficial I think.
- [An overview of Serde](https://serde.rs/) | ||
- [Data formats supported by Serde](https://serde.rs/#data-formats) | ||
- [Examples](https://serde.rs/examples.html) | ||
- [Release notes](https://github.com/serde-rs/serde_core/releases) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken link.
- [Release notes](https://github.com/serde-rs/serde_core/releases) | |
- [Release notes](https://github.com/serde-rs/serde/releases) |
//////////////////////////////////////////////////////////////////////////////// | ||
|
||
// Serde types in rustdoc of other crates get linked to here. | ||
#![doc(html_root_url = "https://docs.rs/serde_core/1.0.219")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute controls where Rustdoc will link to when Serialize
or another trait appears in trait bounds, such as the documentation of serde_json::to_string
.
I think docs.rs/serde would be a better choice for this. That will continue to be what most users have a dependency on when using a crate like serde_json, and the derive macros will be documented there.
#![doc(html_root_url = "https://docs.rs/serde_core/1.0.219")] | |
#![doc(html_root_url = "https://docs.rs/serde/1.0.219")] |
# Provide impls for Result<T, E>. Enabling these impls allows for serialization | ||
# and deserialization of Result types, which may be useful in certain contexts | ||
# but could lead to confusion if ? or unwrap are overused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
/unwrap
being "overused" is not really the right judgement of the code in which Result impls cause confusion. The quantity of use is not concerning to me. I would recommend framing this explanation more about carelessness of use i.e. inserting ?
/unwrap
just because then the code compiles. That better characterizes the users who run into trouble.
|
||
[features] | ||
default = ["std"] | ||
default = ["std", "result"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having serde = { version = "1", default-features = false }
change from having Result impls to not having Result impls is a breaking change. We cannot do that in 1.x.
|
||
pub use self::ignored_any::IgnoredAny; | ||
|
||
pub use crate::seed::InPlaceSeed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto — please find a way to avoid needing to expose this publicly.
pub use crate::ser::{Serialize, Serializer}; | ||
|
||
#[doc(hidden)] | ||
pub use lib::from_utf8_lossy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be exposed in the crate root with no indication visible in downstream code that this is not a public API. Dumb IDEs will happily insert imports of this.
#[macro_use] | ||
mod macros; | ||
#[doc(hidden)] | ||
pub use crate::lib::result::Result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rust-version = "1.56" | ||
|
||
[dev-dependencies] | ||
serde = { version = "1", path = "../serde" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a dev-dependency on serde_derive
. Without it, doctests are failing.
---- serde_core/src/de/mod.rs - de::IntoDeserializer (line 2298) stdout ----
error[E0432]: unresolved import `serde_derive`
--> serde_core/src/de/mod.rs:2300:5
|
4 | use serde_derive::Deserialize;
| ^^^^^^^^^^^^ use of unresolved module or unlinked crate `serde_derive`
|
help: there is a crate or module with a similar name
|
4 - use serde_derive::Deserialize;
4 + use serde_core::Deserialize;
serde_derive = { version = "1", optional = true, path = "../serde_derive" } | ||
|
||
[dev-dependencies] | ||
serde_derive = { version = "1", path = "../serde_derive" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am merging this with my own changes in #2962: please take a look.
The serde team recently released a `serde_core` crate that allows crates like this one to move forward in the dependency chain. serde-rs/serde#2608
The serde team recently released a `serde_core` crate that allows crates like this one to move forward in the dependency chain. serde-rs/serde#2608
The serde team recently released a `serde_core` crate that allows crates like this one to move forward in the dependency chain. serde-rs/serde#2608
This PR implements approach suggested in #2584 by @epage and @soqb. Also #2181
There were two changes initially that were since reverted during the lifetime of this PR:
serde_core
instead ofserde
. This has since been reverted as it would be a breaking change.serde
and notserde_core
. This however was not necessary following commit 2258bb4Before (11.4s for a clean release build with derive feature, 8 cores):

After (7.8s for a clean release build with derive feature, 8 cores):
