feat: introduce `Multihash::new` which can be called from const contexts #331

aatifsyed · 2023-07-19T15:32:32Z

Resolves #330.

aatifsyed · 2023-07-19T15:44:22Z

Last use of wrap standing is

rust-multihash/derive-impl/src/multihash.rs

Line 236 in f78d455

Multihash::wrap((*self).into(), digest)

Which I think is fine, if not necessary because Error::invalid_size is pub(crate)

thomaseizinger · 2023-07-19T15:46:48Z

Last use of wrap standing is

We should also change that to not generate code that is using deprecated functionality.

thomaseizinger

Thank you!

This is fine with me but I'd like more input from other maintainers. cc @vmx @mxinden

src/multihash.rs

thomaseizinger · 2023-07-19T15:49:05Z

Do we have other const functions that might be affected by the same problem? I wonder what a long-term solution could be here.

In the issue, you said that the problem is the io::Error in Kind? I tried previously to remove that one because it only appears in certain functions. Perhaps that is worth exploring because most functions don't actually return an io::Error.

aatifsyed · 2023-07-19T17:33:45Z

Last use of wrap standing is

We should also change that to not generate code that is using deprecated functionality.

This is not possible without further changes to Error, which has no public constructors. I think that's outside the scope of this PR. But, to outline the problem,
Error can only be created using methods like wrap.
It must be created to implement MultihashDigest.

Neither rustc -Dwarnings nor clippy -Dclippy::pedantic complain about the use of this method in derived code. I can add #[automatically_derived] or #[allow(deprecated)] if you would prefer.

As an aside, the docs for multihash-derive are mangled

aatifsyed · 2023-07-19T17:35:29Z

Do we have other const functions that might be affected by the same problem? I wonder what a long-term solution could be here.

In the issue, you said that the problem is the io::Error in Kind? I tried previously to remove that one because it only appears in certain functions. Perhaps that is worth exploring because most functions don't actually return an io::Error.

If you'd like my opinion, you should impl From<YourErrorType> for std::io::Error, and have io functions return and io::Result, which may be InvalidData with a source = Some(YourErrorType), where YourErrorType is a slimmer Error

(This would be a breaking change.)
((I think that's fine, you're pre 1.0.0, and the API should be allowed to evolve past current limitations))

aatifsyed · 2023-07-19T18:48:35Z

I've gone ahead and implemented the required changes:

DigestTooBig now contains a usize so it can match the error message from Error. I think this is an odd choice - you're telling the caller the length of the slice, but they can already find that out because they retain slice ownership.
Added a new constructor (From::from) for Error so that the derive macro doesn't use wrap

thomaseizinger · 2023-07-19T19:13:23Z

If you'd like my opinion, you should impl From<YourErrorType> for std::io::Error, and have io functions return and io::Result, which may be InvalidData with a source = Some(YourErrorType), where YourErrorType is a slimmer Error

Good idea! Would you mind capturing this in an issue?

((I think that's fine, you're pre 1.0.0, and the API should be allowed to evolve past current limitations))

Yes-ish. multihash is a foundational crate of the libp2p ecosystem. It appears pretty much in all public APIs because multiaddr and PeerId depend on it. A breaking change is a breaking change for every dependency. We just released a breaking change a few weeks ago and I'd like to not go through that effort again for a couple of months unless really necessary.

mxinden

No objections from my end.

vmx · 2023-07-25T12:56:24Z

If you'd like my opinion, you should impl From<YourErrorType> for std::io::Error, and have io functions return and io::Result, which may be InvalidData with a source = Some(YourErrorType), where YourErrorType is a slimmer Error

That sounds like a better solution. I would actually prefer that and keep the wrap name and just doing a breaking change. I agree that breaking changes are a hassle, but it shouldn't prevent us from going for a clean API. As this is very specific, also project (like libp2p) don't need to upgrade, they can just keep using the current version as long as they are happy with it.

I'd also be OK with just having the proper solution merged into master and just keep it there before doing a release, in case more breaking changes are uncovered.

thomaseizinger · 2023-07-19T19:15:32Z

src/multihash.rs

+        if size > S {
+            return Err(DigestTooBig(size));
+        }


I guess we could also flip it and say BufferTooSmall? The thing is, the size of the internal buffer is part of the public API and likely, cannot be changed as easily so it makes more sense that the digest is too long. Really, this is one of those "should almost never happen"-errors.

The buffer is likely 32 or 64 bytes so you can fit the digests of SHA256 or SHA512 in it.

thomaseizinger · 2023-07-31T19:44:30Z

If you'd like my opinion, you should impl From<YourErrorType> for std::io::Error, and have io functions return and io::Result, which may be InvalidData with a source = Some(YourErrorType), where YourErrorType is a slimmer Error

That sounds like a better solution. I would actually prefer that and keep the wrap name and just doing a breaking change. I agree that breaking changes are a hassle, but it shouldn't prevent us from going for a clean API. As this is very specific, also project (like libp2p) don't need to upgrade, they can just keep using the current version as long as they are happy with it.

I'd also be OK with just having the proper solution merged into master and just keep it there before doing a release, in case more breaking changes are uncovered.

The issue is that you will have to immediately make a release because I guess that @aatifsyed would want to use this right away.

Not upgrading to latest solution is at best a band-aid and means what we might have to do more work later by backporting PRs instead of just being able to merge them into master.

An alternative suggestion could be to temporarily introduce a wrap_const function, not deprecate wrap and remove it again on the next breaking change.

aatifsyed · 2023-08-01T10:17:32Z

Sorry for the week's silence, I was on vacation.

because I guess that @aatifsyed would want to use this right away.

You're correct, but as the rust-cid folks pointed-out¹, there's a useable workaround for Copy types ²:

const fn const_unwrap<T: Copy, E>(r: Result<T, E>) -> T {
    let v = match r {
        Ok(r) => r,
        Err(_) => panic!(),
    };
    mem::forget(r);
    v
}

That means forest features are no longer blocked - we can just use const_unwrap().

I think a lot of the discussion above about error types/API design is valuable, and maybe could be addressed in #332, but this PR (or something like it) can wait until after that.

thomaseizinger · 2023-08-07T10:00:34Z

I think a lot of the discussion above about error types/API design is valuable, and maybe could be addressed in #332, but this PR (or something like it) can wait until after that.

I agree with that. Thank you for the feedback and input on the error design. We'll incorporate it on the next breaking change :)

aatifsyed added 7 commits July 19, 2023 16:30

feat: usable const constructor

9e303ed

feat: CreationError implements std::error::Error

349423c

refactor: Multihash::wrap uses Multihash::new internally

f3ac22e

chore: mark Multihash::wrap as deprecated, and hide it

f501ff3

fix: clippy linting use of deprecated Multihash::wrap

82df0ea

doc: add note to deprecation

372a057

chore: cleanup stray uses of Multihash::wrap

3646840

thomaseizinger changed the title ~~Usable const constructor~~ feat: introduce Multihash::new which can be called from const contexts Jul 19, 2023

thomaseizinger approved these changes Jul 19, 2023

View reviewed changes

src/multihash.rs Outdated Show resolved Hide resolved

src/multihash.rs Outdated Show resolved Hide resolved

src/multihash.rs Show resolved Hide resolved

src/multihash.rs Outdated Show resolved Hide resolved

review: markups

24a433b

aatifsyed mentioned this pull request Jul 19, 2023

"impractical" const fn rust-lang/rust-clippy#11192

Open

This was referenced Jul 19, 2023

Redesign multihash::Error #332

Open

feat: introduce Cid::const_new, which allows accessing Cids from const contexts multiformats/rust-cid#139

Closed

mxinden reviewed Jul 23, 2023

View reviewed changes

aatifsyed marked this pull request as draft July 25, 2023 08:33

thomaseizinger reviewed Jul 31, 2023

View reviewed changes

aatifsyed closed this Aug 1, 2023

This was referenced Aug 1, 2023

multihash const constructors unusable #330

Closed

cid const constructors unusable multiformats/rust-cid#138

Closed

bake known checkpoints ChainSafe/forest#3227

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce `Multihash::new` which can be called from const contexts #331

feat: introduce `Multihash::new` which can be called from const contexts #331

aatifsyed commented Jul 19, 2023 •

edited by thomaseizinger

Loading

aatifsyed commented Jul 19, 2023

thomaseizinger commented Jul 19, 2023

thomaseizinger left a comment

thomaseizinger commented Jul 19, 2023

aatifsyed commented Jul 19, 2023

aatifsyed commented Jul 19, 2023 •

edited

Loading

aatifsyed commented Jul 19, 2023

thomaseizinger commented Jul 19, 2023

mxinden left a comment

vmx commented Jul 25, 2023

thomaseizinger Jul 19, 2023

thomaseizinger commented Jul 31, 2023 •

edited

Loading

aatifsyed commented Aug 1, 2023

thomaseizinger commented Aug 7, 2023

feat: introduce Multihash::new which can be called from const contexts #331

feat: introduce Multihash::new which can be called from const contexts #331

Conversation

aatifsyed commented Jul 19, 2023 • edited by thomaseizinger Loading

aatifsyed commented Jul 19, 2023

thomaseizinger commented Jul 19, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Jul 19, 2023

aatifsyed commented Jul 19, 2023

aatifsyed commented Jul 19, 2023 • edited Loading

aatifsyed commented Jul 19, 2023

thomaseizinger commented Jul 19, 2023

mxinden left a comment

Choose a reason for hiding this comment

vmx commented Jul 25, 2023

thomaseizinger Jul 19, 2023

Choose a reason for hiding this comment

thomaseizinger commented Jul 31, 2023 • edited Loading

aatifsyed commented Aug 1, 2023

Footnotes

thomaseizinger commented Aug 7, 2023

feat: introduce `Multihash::new` which can be called from const contexts #331

feat: introduce `Multihash::new` which can be called from const contexts #331

aatifsyed commented Jul 19, 2023 •

edited by thomaseizinger

Loading

aatifsyed commented Jul 19, 2023 •

edited

Loading

thomaseizinger commented Jul 31, 2023 •

edited

Loading