multiformats / rust-multihash

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

multihash const constructors unusable #330

Closed aatifsyed closed 1 year ago

aatifsyed commented 1 year ago

Multihash's constructors are const fn, but they can't actually be used as such:

use multihash::Multihash;

const _: Multihash<64> = match Multihash::wrap(
    0x16,
    &[
        0x16, 0x20, 0x64, 0x4b, 0xcc, 0x7e, 0x56, 0x43, 0x73, 0x04, 0x09, 0x99, 0xaa, 0xc8, 0x9e,
        0x76, 0x22, 0xf3, 0xca, 0x71, 0xfb, 0xa1, 0xd9, 0x72, 0xfd, 0x94, 0xa3, 0x1c, 0x3b, 0xfb,
        0xf2, 0x4e, 0x39, 0x38,
    ],
) {
    Ok(o) => o,
    Err(_) => panic!(),
};

Fails to compile:

error[E0493]: destructor of `Result<Multihash<64>, multihash::Error>` cannot be evaluated at compile-time
  --> src/lib.rs:3:32
   |
3  |   const _: Multihash<64> = match Multihash::wrap(
   |  ________________________________^
4  | |     0x16,
5  | |     &[
6  | |         0x16, 0x20, 0x64, 0x4b, 0xcc, 0x7e, 0x56, 0x43, 0x73, 0x04, 0x09, 0x99, 0xaa, 0xc8, 0x9e,
...  |
9  | |     ],
10 | | ) {
   | |_^ the destructor for this type cannot be evaluated in constants
...
13 |   };
   |   - value is dropped here

For more information about this error, try `rustc --explain E0493`.

This is presumably because the top level error type is non-copy/contains an io::Error which cannot be const https://github.com/multiformats/rust-multihash/blob/16e68d8203c787768688dab8aed4295a32f33bf0/src/error.rs#L42-L50

Here is the source for the constructor https://github.com/multiformats/rust-multihash/blob/16e68d8203c787768688dab8aed4295a32f33bf0/src/multihash.rs#L59-L75

Fix is either:

aatifsyed commented 1 year ago

forest have interest in const-construction of CIDs. See also https://github.com/multiformats/rust-cid/issues/138

thomaseizinger commented 1 year ago

I'd really like to avoid making a breaking change. Can we instead introduce a new function and deprecate wrap? Such a new function can then have a different signature and introduce a new const compatible error type.

I'd also like to see a test for this :)

vmx commented 1 year ago
  • drop the illusion of const from the wrap constructor. This is a breaking change

Why is that a breaking change. Given that it's not really const, wouldn't removing the const just be a bugfix? It shouldn't break any code as code using it as const wouldn't compile. What am I missing?

aatifsyed commented 1 year ago

Why is that a breaking change. Given that it's not really const, wouldn't removing the const just be a bugfix? It shouldn't break any code as code using it as const wouldn't compile. What am I missing?

const calling the function would compile, but not accessing the result, so it's a breaking change

aatifsyed commented 1 year ago

Closing - the constructors aren't unusable. Maybe there should be some user-facing documentation for the const case so this issue isn't created again?

See https://github.com/multiformats/rust-multihash/pull/331#issuecomment-1660013138 for closing comment.