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.

Sign up for GitHub

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

Jump to bottom

serde derive feature #86

Open
conradludgate opened this issue Aug 24, 2023 · 1 comment
Open

serde derive feature #86

conradludgate opened this issue Aug 24, 2023 · 1 comment

Comments

@conradludgate
Copy link

conradludgate commented Aug 24, 2023 •
edited
Loading

Inspired by serde-rs/serde#2584, I looked through my dep tree and saw that either includes serde with the "derive" feature.

Unfortunately, it's not so simple how to remove the feature here.

Removing derive altogether

Either uses serde for 3 things,

  1. Tagged Representation
  2. Untagged Representation
  3. Optional Untagged Representation

For 1, We can write a manual implementation of the de/serializers with no problem. For 2 and 3, the derived deserializer makes use of doc(hidden) APIs in order to cache the serde tree in case it needs to perform a second deserialisation.

Depending on serde_derive directly

dtolnay was initially against this idea because the versions of serde/derive need to be in lockstep.

There seems to be some effort to ensure it using some fancy target trickery. This is in the latest release of 1.0.186.

The trouble here is that either has 1 feature, serde. For compat with older resolvers, I think I would need to rename the serde package to serde_pkg and make the serde feature include both serde_pkg and serde_derive

Should I make a PR with the second option? The first option is possible but it requires a lot more effort to duplicate the private APIs
@cuviper
Copy link
Member

cuviper commented Aug 24, 2023 •
edited
Loading

I think the compile-time gains are way too small to justify any significant effort here, so manual implementations are out. I suppose direct serde_derive could be ok, as long as we make it clear that the remaining implicit features from optional dependencies are not considered part of the semver API. Or we could bump to MSRV 1.60 with dep: syntax, and this will also be hidden from earlier versions of the cargo resolver. (So we're not disruptive to any older MSRV crates using either, especially in public API.)

But seeing that serde-rs/serde#2584 is still open, maybe we should wait for the dust to settle a bit...

KamilaBorowska added a commit to KamilaBorowska/either that referenced this issue Sep 2, 2023
@KamilaBorowska
Compile serde_derive separately from serde
d08d2f4 
When serde's derive feature is used, serde_derive must be
compiled before serde can be, as serde with that feature has a
serde_derive dependency.

As of serde 1.0.186, this issue can be avoided by adding a separate
serde_derive dependency due to the fact that serde 1.0.186 has a
never-applicable dependency on serde_derive, which ensures that
there is no incompatible version of serde_derive in a program [1].

Because MSRV being set to 1.36, it's not possible to use the dep:
syntax in features, so serde crate needs to be renamed.

This should improve compilation times of programs that use either
with its serde feature, provided it doesn't have other crates that
use serde with its derive feature.

Fixes rayon-rs#86.

[1]: serde-rs/serde#2588
@KamilaBorowska KamilaBorowska mentioned this issue Sep 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Compile serde_derive separately from serde KamilaBorowska/either
2 participants
@cuviper @conradludgate