search

multiformats / rust-multihash

multihash implementation in Rust
https://github.com/multiformats/multihash
Other
150 stars 60 forks source link

Derive `Arbitrary` for `multihash_codetable::Code` #366

Closed expede closed 2 months ago

expede commented 2 months ago

I found myself writing a newtype wrapper over Code to get an impl proptest::Arbitrary, and thought "hey, why don't I upstream this one-liner?"

I tried to keep to your libraries and conventions as much as possible (e.g. arb), but please let me know if I missed anything.

(Some of the automated checks are failing, but I'm 90% sure those failures "shouldn't" be related to this change. It's late here, so I'll double check tomorrow)

vmx commented 2 months ago

The CI failures are unrelated, i've opened https://github.com/multiformats/rust-multihash/pull/367 for a fix.

expede commented 2 months ago

After #367, the tests got far enough to flag that there was a legitimate warning. However, I've now run into an edge case with the Cargo Hack action:

  50 | #[cfg_attr(feature = "arb", derive(arbitrary::Arbitrary))]
     |                                    ^^^^^^^^^^^^^^^^^^^^
     |                                    |
     |                                    unreachable call
     |                                    any code following this `match` expression is unreachable, as all arms diverge

I think this is a valid warning for the compiler to raise, but it only happens if none of the hash algorithms are enabled (which should never happen in normal use). The most pragmatic approach here is to allow the unreachable code (since it's useless without any hash algos anyway), so I did that. Please let me know if you'd prefer a different strategy. I wish that I could just allow the one derive, but it seems to not pick that up unless it's on the entire module (perhaps because it pertains to a macro?)

expede commented 2 months ago

Thank you!