Support building multihash with no hashes enabled.

[dvc94ch]

@vmx please merge and release 0.13.2, this is blocking a new libipld release

[vmx]

Is that PR still needed? Is this about that you need to specify a hasher if you use the multihash-impl feature? I just saw that the documentation could be improved on that. Would better documentation solve this problem or is there a use case where you would want to use multihash-impl without any hasher?

[dvc94ch]

Yes there is. The user can select the hashes later, but we don't want to prescribe any in libipld

[vmx]

I think I got it now. So users could just use e.g. the sha2 feature and wouldn't need to explicitly specify the multihash-impl feature. So it's about convenience and not not about something that wouldn't be possible without this PR. Correct?

[dvc94ch]

The default params sets the hashes enum to the multihash code, that's a problem if it doesn't exist

[vmx]

I still don't fully get the problem that is solved here, but the fix looks sensible. I'll look at it tomorrow again, but I also don't want to block @dvc94ch, hence merging.

[dvc94ch]

Thanks

[Ericson2314]

IMO this is kind of an unfortunate fix --- you're effectively turning a static errors into dynamic errors.

An "empty" enum like enum Never {} already can be matched producing any value with match my_never_value {}, since it's dead code, so I don't think this should be needed in principle?

Would you mind pasting the build error without this?

[dvc94ch]

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f41eab90f5fcd0a3c279256b0476bd1c

It is not a dynamic error, it is indeed unreachable!

[Ericson2314]

The unreachble macro creates a dynamic error.

If you deref self, the empty case works: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=04dac198be4a65584265aa38b1399127

[Ericson2314]

I guess do that, and change the match arm patterns to use ref, and then the problem is solved without unreachable!().

[dvc94ch]

I'm on it. Any other recommendations?

[vmx]

@Ericson2314 thanks for chiming in. I was also under the impression that it will steal lead to compile time errors.

[Ericson2314]

@dvc94ch

I'm on it. Any other recommendations?

Thanks!

@vmx

I was also under the impression that it will still lead to compile time errors.

Heh yes it's all quite subtle, but from following rust-lang/rust#57012 's incredibly long saga has beaten these things into my head. :)

[vmx]

@dvc94ch any news? I'm thinking about reverting this one again and doing a new release.

I also still don't fully understand the problem in relation to rust-ipld. What wouldn't work if this PR would be reverted, can you provide a example? Perhaps we can find a way to fix the problem from the rust-ipld side.

[dvc94ch]

I'm thinking about reverting this one again and doing a new release.

Are you kidding me? What's with the bike shedding. I really wonder sometimes where these random people on the internet come from that have nothing better to do.

Since rust actually is not smart enough to determine that it is dead code, there is a begin_unwind with a string in the mir code. In release mode llvm takes care of it. In debug mode you have a string with the panic message, increasing your code size by a couple of bytes. However if you used unimplemented anywhere in your code you already have that message in your binary.

[vmx]

Are you kidding me? What's with the bike shedding. I really wonder sometimes where these random people on the internet come from that have nothing better to do.

This is not about bike shedding. I clearly indicated that I'm not sure about this change, but I trust you enough that it is a sensible change, so I went ahead merged and released.

Then another person (@Ericson2314) chimes in and raises concerns about this change (which I think is great, I like to get feedback from others).

@dvc94ch replies with:

I'm on it. Any other recommendations?

Which I read as "I look into this, I'll provide an update with my findings".

I haven't seen an update and as I still haven't looked closely into it, hence I was asking if @dvc94ch could perhaps provide an example why this change is exactly needed. This way I would then be able whether this code base is better with or without that change.

---

Now that I had a closer look, I'm not sure about this change. To me it looks like it's changing something I had there intentionally. To me the usual user should just use the default features and things work. If you are a power user, you should know what you're doing. Hence prior to this change no Multihash with a specific size was exported. You would need to do type Multihash = Multihash<U64> in your code to have that.

@dvc94ch so is this change exactly about that? That you want to have the Multihash<U64> exported as Multihash if multihash-impl is enabled without having a hasher enabled?

[vmx]

I forgot to add that it might be totally fine to export an empty Code enum and the Multihash<U64> as Multihash, I just wonder it makes things simpler or more confusing.

[dvc94ch]

The default params sets the hashes enum to the multihash code, that's a problem if it doesn't exist

Cargo features are additive. That means it will based on the dependency graph create a union of features that need to be enabled. libipld doesn't care about which specific hashes are enabled, and depending on multihash without --no-default-features will prevent anyone using libipld to remove unnecessary hashes from their dependency tree. But since by default the Multihash enum is used (which is a sensible default and makes things just work) we need a Multihash enum even when no hashes are enabled. I really can't explain it any better than that.

[vmx]

[removed due to Code of Conduct violation]

@dvc94ch Please chose your words more wisely. Such comments are unacceptable. This is a welcoming project, where people are free to have different opinions and priorities.

[vmx]

@dvc94ch Thanks for the explanation. Though wouldn't it make sense if IPLD would use MultihashGeneric so that it isn't bound to fixed size? E.g. people might want to use a multihash which is 32 bytes only.

[dvc94ch]

@dvc94ch Please chose your words more wisely. Such comments are unacceptable. This is a welcoming project, where people are free to have different opinions and priorities.

I guess do that

But it's ok for random people to tell other people what to do? He is welcome to open his own PR. This work was paid for, meaning I was on the clock. No time to spend hours thinking about what is to many people completely irrelevant stuff.

@dvc94ch Thanks for the explanation. Though wouldn't it make sense if IPLD would use MultihashGeneric so that it isn't bound to fixed size? E.g. people might want to use a multihash which is 32 bytes only.

Maybe, PR's welcome.

[vmx]

But it's ok for random people to tell other people what to do?

It is not. I have to admit that when I first read @Ericson2314 comment I read it as ""hey, that's a simple fix that might solve the problem I'm seeing". I didn't read it as a request, but I can see now that it could be read as such.

[vmx]

Coming back to the original point from @Ericson2314. I read it as "there will be runtime rather than compile time errors", but that's not the case, right?

[dvc94ch]

There is no way that it can ever panic in practice (as in I did not introduce a bug). You can argue about if one solution is mildly better than the other.