cid const constructors unusable

[aatifsyed]

Cid's new_v0 constructor is const fn, but it can't actually be used as such:

use cid::{multihash::Multihash, Cid};

const fn multihash() -> Multihash {
    panic!("https://github.com/multiformats/rust-multihash/issues/330")
}

const _: Cid = match Cid::new_v0(multihash()) {
    Ok(o) => o,
    Err(_) => panic!(),
};

Fails to compile:

error[E0493]: destructor of `Result<CidGeneric<64>, cid::Error>` cannot be evaluated at compile-time
  --> src/lib.rs:7:22
   |
7  | const _: Cid = match Cid::new_v0(multihash()) {
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^ the destructor for this type cannot be evaluated in constants
...
10 | };
   | - 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-cid/blob/1df4e3fe0f6bcb8845655d2ccefd9da5ef81a1cd/src/error.rs#L32

Here is the root cause of the error in the constructor https://github.com/multiformats/rust-cid/blob/1df4e3fe0f6bcb8845655d2ccefd9da5ef81a1cd/src/cid.rs#L78-L87

Fix is either:

• drop the illusion of const from the new_v0 constructor. This would propogate to new, and is a breaking change
• Fix multihash (see issue below), take a new multihash version[^1]. Then have new_v0 return a new, const-compatible InvalidCidV0Multihash error in its constructor, and propogate that to new. This is also a breaking change. This is my preference, and I'm happy to create a PR for such a change.

[^1]: note that this crate is on multihash 0.18.1, but latest is 0.19.0 - is there a reason for the lag?

[aatifsyed]

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

[Stebalien]

I believe what you need is https://github.com/filecoin-project/builtin-actors/blob/f84baa92638f85845bebeed2399b83e9768a40bd/runtime/src/runtime/empty.rs#L8-L15. I.e., a way to ensure that the error is never actually dropped.

[Stebalien]

In general, I'm not a fan of adding a const_x function while deprecating the existing one (a semi-permanent wart). I'd rather just break it if absolutely necessary, but I don't think it is in this case.

[vmx]

• drop the illusion of const from the new_v0 constructor. This would propogate to new, and is a breaking change

If we would just remove the const, would it really be a breaking change? Given that no one can compile code that uses it as const, it sounds to me like a bug fix.

[Stebalien]

See https://github.com/multiformats/rust-cid/issues/138#issuecomment-1646977310. We're already using the const.

[vmx]

See #138 (comment). We're already using the const.

I actually saw that and thought you're using it only for multihash and totally missed that it's wrapped in a CID. Nonetheless you only use new_v1, so if new_v0 would be non-const, it wouldn't be a problem for that case at least.

[Stebalien]

I mean, it wouldn't be a problem, but there's still no reason to make this change. new_v0 does work as a const function, it's just slightly painful because you have to be careful about how you drop the error (I.e., you need to explicitly forget it).

[vmx]

it's just slightly painful because you have to be careful about how you drop the error (I.e., you need to explicitly forget it).

I'd say it's more than "slightly painful". I think changing the error enum to be const friendly would me cleaner. Though there's still the question: does that deserve a breaking change, creating CIDv0 in a const context seems pretty niche? On the other hand, changing the errors shouldn't break much, so perhaps we could prepare a PR and merge it only if we need a breaking release for other reasons?

[Stebalien]

I agree that changing the enum would be cleaner, if possible. I just don't want to:

1. Remove the const here (unnecessary breakage).
2. Add a const_ function and deprecate this one (adds a wart).

But yeah, I agree that changing the error in the next breaking release makes sense. Especially if there's a From<NewV0Error> for Error implementation.

(in general, I'd be in favor of introducing multiple error types anyways...)

[vmx]

I agree that changing the enum would be cleaner, if possible. I just don't want to:

1. Remove the `const` here (unnecessary breakage).
2. Add a `const_` function and deprecate this one (adds a wart).

Agreed.

(in general, I'd be in favor of introducing multiple error types anyways...)

It's the first time I see the point of having multiple error types. Though would you do it the way #139 does it, or would you overhaul the errors and maybe structure them even differently?

[Stebalien]

Ideally, if we're going to be changing errors, I'd consider overhauling the errors entirely. But I wouldn't block on that.

What I wouldn't do is introduce that new function. The error definitions there look fine (except the lack of From<NewV0Error> for Error implementation).

[aatifsyed]

I believe what you need is https://github.com/filecoin-project/builtin-actors/blob/f84baa92638f85845bebeed2399b83e9768a40bd/runtime/src/runtime/empty.rs#L8-L15. I.e., a way to ensure that the error is never actually dropped.

Yep, that means this issue is wrong, the const constructors are actually useable if someone uses const_unwrap. I'll leave error redesign up to you, the maintainers.

Closing.

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