bluejekyll / enum-as-inner

Macros for deriving as functions to access Enums as their inner components
Other
92 stars 11 forks source link

0.5.2 breaks trust-dns-proto v0.22.0 #98

Closed leptonyu closed 1 year ago

leptonyu commented 1 year ago
error[E0592]: duplicate definitions with name `is_soa`
   --> /home/lepton/.local/registry/src/github.com-1ecc6299db9ec823/trust-dns-proto-0.22.0/src/rr/record_data.rs:55:17
    |
55  | #[derive(Debug, EnumAsInner, PartialEq, Clone, Eq)]
    |                 ^^^^^^^^^^^ duplicate definitions for `is_soa`
...
994 |     pub fn is_soa(&self) -> bool {
    |     ---------------------------- other definition for `is_soa`
    |
    = note: this error originates in the derive macro `EnumAsInner` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0592`.
NobodyXu commented 1 year ago

@bluejekyll Can you yank enum-as-inner v0.5.1 please? It is breaking cargo install cargo-binstall, causing it to fail with the error in description.

daxpedda commented 1 year ago

91 in my opinion is a breaking change. Adding methods through a derive macro on a user-controlled type can easily collide with user defined methods, as shown in #1946.

Porges commented 1 year ago

There are a lot of downstream dependents of trust-dns-proto, so this is likely to impact many things.

dariusc93 commented 1 year ago

This does indeed affect many projects that depends on trust-dns-proto. Yanking the version and bumping the minor to the next minor version iteration would probably be the best course here since https://github.com/bluejekyll/enum-as-inner/commit/6a9800615cb92d0ec24098a9d033580c4d2d5c26 is the breaking change.

NobodyXu commented 1 year ago

Turns out that enum-as-inner v0.5.2 only breaks release build of cargo-binstall, but the dev-build seems fine https://github.com/cargo-bins/cargo-binstall/actions/runs/5107730096/jobs/9180899613

decathorpe commented 1 year ago

Maybe it would make sense to split this into two separate traits, something like EnumAsInner and EnumIs? Adding is_*() functions when deriving EnumAsInner is a subtle breaking change as others have noted, but having a separate trait and derive macro would make this a non-breaking / opt-in change. On the other hand, it's already implemented the way it is, so yanking 0.5.2 and re-releasing this as 0.6.0 would probably be easier ...

djc commented 1 year ago

I think yanking 0.5.2 and rereleasing it as 0.6 is the right way forward here.

bluejekyll commented 1 year ago

Thanks everyone. I definitely made a mistake in understanding the impact of this change on semver. I’ll yank and release with a minor change version bump.

bluejekyll commented 1 year ago

Yanked. @decathorpe, I have been considering other ways of controlling the set of generated functions in this library. Your idea is good, I wonder if there are any others that we should consider?

decathorpe commented 1 year ago

Yanked. @decathorpe, I have been considering other ways of controlling the set of generated functions in this library. Your idea is good, I wonder if there are any others that we should consider?

Thanks!

I'm not that familiar with writing procedural macros, but I think using custom attributes to somewhat affect their behavior (similar to container attributes in serde) would work.

djc commented 1 year ago

Yeah, additional attributes to opt in to the extra methods sounds more appealing than a second derive macro.

bluejekyll commented 1 year ago

Fixed in #99