Conflict with display_derive

[ErichDonGubler]

The Display procedural macro unfortunately conflicts with @withoutboats' display_derive procedural macro. In some code I'm working with on a personal project, the following will compile when #[macro_use]ing only display_derive, but not in conjunction with strum_derive:

#[derive(Clone, Debug, Deserialize, Display, Eq, Hash, PartialEq, Serialize)]
#[display(fmt = "{}", _0)]
pub struct MessageId(pub String);

Both of these crates are great additions to the ecosystem, in my opinion. It's a shame that we don't have macros as first-class items yet and that I have to choose between them. Perhaps disabling Display with a feature flag would be best for now?

EDIT: It's also possible to use the right crate's definition by making the desired derive be imported LAST...but that's a hacky workaround.

[lo48576]

Can we use more detailed name, such as EnumDisplay?
I'm not sure whether it should be recommended rather than generic name Display, or whether it should always be available.
But at least when Display is disabled, such alternative name may be useful because it lets strum to avoid degrading its feature.

[Peternator7]

@ErichDonGubler Thanks for the PR and being patient while I find time to look at it :)

@lo48576 Are you suggesting we change the name across the board or just when this feature flag is flipped? I wish I knew the roadmap for macros a better. To entirely future-proof this issue, we could just have a flag PrependStrum that for all the derives puts strum in front of the attribute: StrumToString , StrumDisplay, etc. It's a little ugly for sure, but it would make it easy to avoid collisions across the whole crate.

[lo48576]

I meant renaming only Display to some other name (and maybe also As{Static,Ref}str).
It's because Display and AsRefStr is too generic and other crates may implement them for non-enum types.

In contrast, EnumString and something like that is cleary for enum, and if other crates wanted to use that name, I think the feature is not provided by two crates for the same code.
For example, if both strum and other crate (call it alt_strum) provides derive(EnumDisplay), then users should not use them at the same time.
(It can happen, but it is not desirable to use two crates for same purpose, I think.
For such case, strum or alt_strum should cover all custom derives required by the user.)

To make strum future-proof (not only avoiding conflicts), using strum's own namespace (such as #[derive(Strum)] #[strum(derive(Display))]) may be good.

[ErichDonGubler]

My PR wholesale disables Display, but renaming to indicate that the proc-macro comes from StrumDisplay seems like an equally viable solution. I would personally prefer something like that to what I submitted -- simply omitting it was the simplest to implement at the time. :)

[daboross]

With rust 2018 edition, macros can now be imported individually.

For example, you could do

mod a {
    use strum_macros::Display;

    #[derive(Display)]
    enum UsesStrumDisplay {}
}
mod b {
    use derive_more::Display;

    #[derive(Display)]
    enum UsesDeriveMoreDisplay {}
}
mod c {
    use derive_more::Display;
    use strum_macros::EnumString;

    #[derive(Display, EnumString)]
    enum UsesStrumEnumStringAndDeriveMoreDisplay {}
}

I've tested importing just FromEnum and not Display from strum_macros and it works fine.

Could this issue be considered resolved?

[ErichDonGubler]

Just as a note, this is possible with Rust 2015 also -- I forget which release and I'm on mobile. Perhaps 1.30?

[Peternator7]

@ErichDonGubler , I'm going to close this. Let me know if you disagree.

[ErichDonGubler]

@daboross @Peternator7

My only concern with
simply closing this issue is where distros with older versions of Rust are left -- I've been doing some research for the oldest supported distros, and pre-1.25 Rust distributions are still (unfortunately) fairly common. I have no idea what the proportion of users are that are actually ON those versions, but whatever the case it boils down to a question of the support policy this crate has. I don't see any mention of it in the README. Is there one?

And to be honest, I don't care if you take a minimal support policy and say "This crate should only be expected to work on the latest Rust." So long as it's a conscious decision, then users can at least have a definitive answer to any questions about the version of their toolchain in relation to strum. :)

[ErichDonGubler]

@Peternator7: Haven't heard a response from this, just wanted to ping you.

[Peternator7]

Hey @ErichDonGubler , thanks for following up on this. It had slipped my mind. That's interesting official distros are so old. Let me figure out what the earliest version of rustc compiles strum. I don't plan on having extensive long term support, but at least making sure new features don't break back compat is a good start. I'll get a travis build set up for that version.

[Peternator7]

Following up on this. The earliest version of rust right now that can compile strum is 1.26 because that's when impl trait was stabilized so I made a note on the README.

It's should be possible to rename macros with some conditional feature flags like we discussed above. I'm planning on creating one flag per custom derive because the overhead for adding a feature is so small, and typically the conflict only exists with a single name so consumers won't need to refactor their entire library, only colliding names.

Thanks for your patience on this. I've been busy with other things lately.

#[cfg_attr(not(feature = "verbose-enumstring-name"), proc_macro_derive(EnumString, attributes(strum)))]
#[cfg_attr(feature = "verbose-enumstring-name", proc_macro_derive(StrumEnumString, attributes(strum)))]
pub fn from_string(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
    let ast = syn::parse(input).unwrap();

    let toks = from_string::from_string_inner(&ast);
    debug_print_generated(&ast, &toks);
    toks.into()
}

[Peternator7]

Housekeeping

This was implemented in 0.15.0 so I believe this issue is okay to close now.