Add support for hashing from a reader

[Stebalien]

fixes #141

The io import stuff is a bit annoying, but I can't find a better way to do this.

[vmx]

In regards to the io imports. This seems to work:

+++ b/derive/src/multihash.rs
@@ -238,7 +238,8 @@ pub fn multihash(s: Structure) -> TokenStream {
     let code_into_u64 = hashes.iter().map(|h| h.code_into_u64(&params));
     let code_from_u64 = hashes.iter().map(|h| h.code_from_u64());
     let code_digest = hashes.iter().map(|h| h.code_digest());
-    let code_reader = hashes.iter().map(|h| h.code_reader());
+    let code_reader_core = hashes.iter().map(|h| h.code_reader());
+    let code_reader_std = hashes.iter().map(|h| h.code_reader());
 
     quote! {
         /// A Multihash with the same allocated size as the Multihashes produces by this derive.
@@ -253,11 +254,22 @@ pub fn multihash(s: Structure) -> TokenStream {
                 }
             }
 
-            fn digest_reader<R: #io_path::Read>(&self, reader: &mut R) -> #io_path::Result<Multihash> {
-                use #io_path;
+            #[cfg(feature = "std")]
+            fn digest_reader<R: std::io::Read>(&self, reader: &mut R) -> std::io::Result<Multihash> {
+                use std::io;
                 use #mh_crate::Hasher;
                 match self {
-                    #(#code_reader,)*
+                    #(#code_reader_core,)*
+                    _ => unreachable!(),
+                }
+            }
+
+            #[cfg(not(feature = "std"))]
+            fn digest_reader<R: core2::io::Read>(&self, reader: &mut R) -> core2::io::Result<Multihash> {
+                use core2::io;
+                use #mh_crate::Hasher;
+                match self {
+                    #(#code_reader_std,)*
                     _ => unreachable!(),
                 }
             }

[Stebalien]

@vmx so, that brings up an interesting question.

The first two are "safe" (except for the identity hash) because we know the digests will fit. But the last one isn't as a digest could have an arbitrary size. This isn't a new issue, but it would be nice to fix it while we're changing these APIs.

Options:

• Return a Result.
• Remove the method. The user can always call Multihash::wrap(code.into(), digest).

Thoughts?

[Stebalien]

For now I'm returning a result.

[vmx]

The first two are "safe" (except for the identity hash) because we know the digests will fit.

I'm not sure about this. In the default table we would know. But if someone defines their own table (and people really should), then they could define an allocation size, which is smaller than what the hasher returns. Multihash::wrap() would the return an error. Then the unwrap will lead to a panic.

This behaviour might be fine, but we need documentation for the panic cases.

[Stebalien]

You know what, I think I have a good solution. I'm going to codegen tests.

[Stebalien]

So, actually, what I can do is:

1. Have hashers specify a maximum digest size.
2. Statically assert that the maximum digest size of all hashers is less than the size of the Multihash type. That's basically what we used to do anyways.

[vmx]

2. Statically assert that the maximum digest size of all hashers is less than the size of the Multihash type. That's basically what we used to do anyways.

@Stebalien what exactly do you mean with "statically assert"? At compile time or at run-time via assert!()? If it's at compile time, could you give me a hint on how to do that? Then I'll try to implement that.

[Stebalien]

I mean at compile time. I can't remember exactly what I was planning, but I think it was something like: when deriving MultihashDigest, emit some static assertions that Digest::SIZE <= target_multihash_size.

But I can't remember exactly how I intended to do it. Something like:

trait Digest {
    const SIZE: usize;
}

struct MyDigest;

impl Digest for MyDigest {
    const SIZE: usize = 20;
}

trait Assert<const T: bool> {
    fn assert() {}
}

impl Assert<true> for () {}

fn main() {
    <() as Assert<{ MyDigest::SIZE < 64 }>>::assert();
    println!("Results:")
}

But I'm not sure if there's a way to get a better error.

[vmx]

@mriise On one hand I don't want to drag you into yet another const generics related discussion, on the other hand I'm pretty sure you would have a good idea on how to check if a certain size is smaller then other nicely at compile time :)

[mriise]

here is a playground link to the best solution right now IMO: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=de22846e5857a2b607e884e0589e9aae

The error it puts out is a bit confusing at first glance but its not too terrible, might be even better with different ident names.

as for shrinking arrays, since it can fail at runtime assuming we are shrinking it to the mh.size() it can be covered by a resize function that does checks at runtime for both growing and shrinking the backing array.

[vmx]

I've pushed two new commits (I intentionally didn't rebase this PR for easier review. Once reviewed, I'll rebase and squash it into a single commit).

The first one is about the io_path, I hope my solution is a working replacement.

The second one adds those compile-time asserts. Thanks @mriise your playground like was really useful.

[vmx]

@mriise I've added you a few times as a reviewer, when I'd appreciate your input. Though please don't feel obliged to do a review.

[vmx]

While testing the std/core2 expansion, I found out that core2 does support io::copy only on nightly Rust. @Stebalien is that a problem?

Perhaps we should also check examples being compiled without the std feature. I discovered the issue by running:

cargo build --example custom_table --no-default-features --features derive,secure-hashes,alloc

[Stebalien]

While testing the std/core2 expansion, I found out that core2 does support io::copy only on nightly Rust. @Stebalien is that a problem?

Hm. Yes, it is. Especially because the unstable API is being replaced. Maybe upstream would accept a stable version? There's no good reason it can't be supported on stable.

[vmx]

Maybe upstream would accept a stable version?

I've opened https://github.com/technocreatives/core2/pull/17, let's see what happens.

[vmx]

Solving this properly probably will take a while. What do you folks think about releasing current master and leaving this one for the release after (especially since this change is just and addition, without breaking APIs)?

[Stebalien]

Yeah, that makes sense.