0.24.1 change to automatically implement TryFrom has unforeseen migration issues

[AlexTMjugador]

I develop a Rust library that used to implement TryFrom manually on types that were annotated with strum's FromRepr macro because I needed to handle the conversion failure with my specific error type. This provides for easier troubleshooting and better error messages than a generic "invalid variant" error type.

Today I pushed commits to its repo, which caused CI to run and fail due to this breaking change. Because the project is a library, the Cargo.lock was not committed to the repository, as recommended in the Cargo book, so I was puzzled at first because everything appeared to work fine on my machine, whose Cargo.lock still pointed to the older patch version of this crate.

Reading strum's changelog I stumbled upon PR #217, which implemented this change. In my humble opinion, I think the way that PR was deployed is not ideal because:

• It represents a breaking change for an otherwise patch-compatible version that may cause apparently random build failures for Rust library projects. The semver expectations are rigid: a "minor inconvenience" is not enough justification for a breaking change in an otherwise compatible version to sensible client code. Moreover, a "minor inconvenience" can turn into a "major annoyance" when factoring in that strum has many reverse dependencies that may be on the verge of breaking, and end-users that depend on a third-party library that happens to use strum can't do much about it other than patching things themselves and nagging the library author(s).
• It provides no migration path for those that desire to use a custom error type. Using a custom error type is useful to attach additional information about the conversion failure to the error itself, which in my case is a necessity. Otherwise, I'd have to refactor the error handling code of my library to convey that information elsewhere, which is much less ergonomic and elegant.

To improve the situation while honoring the improvement made by #217, I'd suggest making FromRepr not implement TryFrom, and adding a new macro, such as TryFromRepr, which does. This way the users of the crate can choose whether they want the nice convenience of a TryFrom implementation or not, and no breaking change is made to FromRepr.

[Peternator7]

Hello @AlexTMjugador, sorry that this broke your CI :( I've received feedback in the past that strum too aggressively rev's semver so I was trying to make a compromise here, but clearly it didn't quite pan out correctly. My original thinking was that you could newtype it and then TryFrom on the newtype. As I think more about it, I can appreciate it's a non-trivial refactor.

I think your suggestion to split the 2 is probably the best, but I'll need to think a little more about that. In the short term, I yanked version 0.24.1 to unblock people's CI.

[AlexTMjugador]

No problem, and thank you for sharing your thoughts!

I understand that semver expectations can be quite hard to manage at times. Indeed, after writing the issue I thought on using newtypes too, but sadly that refactor can get more complicated than it might appear at a first glance. Take the time you need to come up with a great solution for everyone :)

[Peternator7]

Going to close this since the offending version was yanked

[robertvazan]

TryFrom is however still advertised in the latest docs, which made me wonder why it does not work.

BTW, I could use TryFrom myself, perhaps as an optional feature. Or at least some custom strum trait, so that I can use this feature in generic functions.