2018 edition custom derives and shadowy import UX

1

This follow up from a twitter discussion here https://twitter.com/softprops/status/1077533430924009473 I was directed here by @rustlang for a further discussion

While working on updating a crate to 2018 edition and removing extern crate usage, I discovered a weird quirk with custom derive name resolution that seems to conflict/cause confusion with typical trait name usage resolution.

This recipe that produces this conflict feels pretty common. I publish a crate called “foo” with a public trait called “Bar”. As a service, I publish another crate called “foo_derive” which is a proc macro crate that derives an implementations for “Bar” for the consumer crate types. Here’s the rub.

Since we no longer recommend #[macro_use] extern crate foo_derive; The recommendation for consumers is to use standard use statements in their stead. Using serde is a more canonical example it’s now recommended to use serde_derive::{Serialize, Deserialize}; instead of extern crate. On first glance this looks great. That is until, in the same consumer crate you need to invoke a method on an instance of the derived trait. At that point standard trait name resolution kicks in and users will have to use serde::{Serialize,Deserialize} ( or insert your trait here )

I’ve since learned that the serde crate has published a work around that avoids the UX issue by adding a feature flag to re-export the entire proc macro crates top level within the core serde crate.

With the scene’s characters set, what is the recommended script crate authors should follow (for now at least) to avoid having crate consumers having to both use foo::Bar and use foo_derive::Bar in the same lexical scope in order to achieve what we could in Rust 2015. Is the feature flag + re-export a recommended pattern? Whatever the recommendation is, I’d recommend promoting it strongly to ensure a consistent consumer experience for users pulling various trait deriving dependencies into their consumer crates.

1 Like
2

What serde does is the recommended pattern: re-export from the derive crate in the same scope as the trait definition under an on-by-default feature flag (so that libraries that don’t need it can turn it off to improve their compile times). That way, the trait and its derive can be imported as a unit. This is the UX we’d like derives to have: for them to seem like they are manifested by the trait itself.

It’s been a common pattern for people to see the removal of #[macro_use] and its “implicitly import everything everywhere” behavior as a UX regression. That’s interesting; I think if we suggested we should eliminate use statements and adopt the macro_use behavior for all items, people would not regard that as a UX improvement. :wink: The behavior of macro_use always caused consternation when multiple crates define macros with the same name.

2 Likes
3

Thanks for the quick response!

for them to seem like they are manifested by the trait itself

That's exactly what I want :slight_smile: I just wasn't sure what the recommended path was to get there. I'll take this route. I'm not sure where it should go, but we should try and encourage this pattern in some documentation way so crate authors publish these custom derives in a way that's consistent across crates come 2019 when everyone will be migrating towards 2018 edition.

1 Like
4

The serde_derive re-exports in Serde were added a long time ago for a totally different reason unrelated to 2018-style macro imports. It is a workaround for a limitation of Cargo cfgs. To the extent that they provide better import UX in 2018 crates, that is a fortunate coincidence, but let's not see it as Serde's solution to derive import UX just yet.

We have a thread in serde-rs/serde#1441 discussing how to proceed.


One observation from that thread is that a major drawback of a derive cfg is that it is non-additive when combined with 2018-style macro imports, and we have seen this cause real breakage. Consider a crate that compiles fine without the derive feature enabled:

use serde::Serialize;
use serde_derive::Serialize;

#[derive(Serialize)]
struct S;

fn f<T: Serialize>(...) {}

If some unrelated crate somewhere in your dependency tree decides to enable the derive cfg, the code here no longer compiles.

error[E0252]: the name `Serialize` is defined multiple times
 --> src/main.rs:2:5
  |
1 | use serde::Serialize;
  |     ---------------- previous import of the macro `Serialize` here
2 | use serde_derive::Serialize;
  |     ^^^^^^^^^^^^^^^^^^^^^^^ `Serialize` reimported here
  |
  = note: `Serialize` must be defined only once in the macro namespace of this module
help: you can use `as` to change the binding name of the import
  |
2 | use serde_derive::Serialize as OtherSerialize;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I would like to see this fixed in the compiler before declaring derive cfg the recommended pattern. Both imports refer to the exact same macro so it may be fine for them not to conflict each other.


I think this would be nice but is not currently how things work. For example in the context of Serde, the Serialize trait is exported at serde::ser::Serialize and re-exported from the crate root as serde::Serialize. They are the same trait but you only get the derive macro if you import the latter way, not serde::ser::Serialize. We also can't fix it because macros are limited to being exported from crate root.

I would be excited to see someone flesh out a design in an RFC by which trait definitions could couple themselves with a preferred derive macro for that trait. Basic straw man:

#[cfg_attr(feature = "derive", derive = "serde_derive::Serialize")]
pub trait Serialize {
    /* ... */
}

This way any time you have trait Serialize in scope (whether from serde::Serialize or serde::ser::Serialize or any other path) and the derive cfg enabled, you have the derive macro in scope as well. This would fully realize the model that @withoutboats described: derive macros "seem like they are manifested by the trait itself."

3 Likes
5

This is only the case for re-exported proc-macros with older compilers, you should be able to use something like pub mod ser { pub use serde_derive::Serialize; } if you are supporting only compilers with the stabilised use_extern_macros feature (I’m not sure of the min version off the top of my head).

6

Ah thanks! I’ll hold off until the conflicting imports thing is fixed because in the thread @sfackler mentioned importing serde::ser::Serialize as a workaround to avoid an import that conflicts with serde_derive::Serialize. At present, we would be breaking that workaround by adding a re-export of serde_derive::Serialize as serde::ser::Serialize.

7

Yea, something like this would be a pretty cool feature, rather than the hacky solution we have now.

1 Like
8

Thanks for the extra context @dtolnay this gave me a lot more knowledge that I had before. I’m wondering if there was a recommended pattern that it may come with some small print outlining the drawbacks. I can’t speak clearly on these myself but I’m getting a feeling like we may have a better solution in the future.

9

coming back to this after publishing a new version of my crate with the suggested pattern ( crate feature flag for re-exporting my derive on by default ) but this brought to my attention a new concern.

How does one unit test their derive crate when it derives interfaces from a core crate which now depends on the derive crate? A cyclic testing dependency is born!

I was able to work around some things like rustdoc examples by annotating with ignore but this puts the orange warning label “this example is not tested” which users reading take as a test of faith. I found that cargo does let you declare dev-dependency on the core crate and let’s you run tests ( which somehow works ) but does not let you publish the derive crate when it has a dev-dependency on the core crate. A crate that can not be published is of little value to others so I’ve punted on the unit test and code coverage story for the derive crate but that seems like a very bad habit to make.

How are others handling this?

10

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.