const generics #116

mriise · 2021-03-30T18:13:05Z

What this PR does

removes generic array
implements Default for things that needed it by making an empty array [0u8; SIZE]\
closes Use const generics #114

What it doesn't do:

~~modify user api to make use of const generics e.g. Blake3::<SIZE>::digest(&[u8])~~ (this can't be done without more modification of the proc macro, so instead should probably be another PR)
~~build~~
~~modify proc macro~~
~~fix SHA macro_rules!~~
~~some other things that need to be squashed to even build.~~

This reverts commit eebdb2c.

src/multihash.rs

src/hasher.rs

src/multihash.rs

src/multihash_impl.rs

src/hasher_impl.rs

mriise · 2021-03-31T16:50:41Z

sorry, had this up on my phone in my pocket which caused the closing and opening. Almost posted a chunk of garbled text as a comment too...

mriise · 2021-04-13T22:52:16Z

While stepping through the macro I noticed that to use hash<64> in the macro we need to be able to evaluate if alloc_size > SIZE(of hash), the support syn has for const generic syn::GenericArgument::Const() gives &syn:Expr which from what I can tell cant be evaluated in time to be used for our purposes.

As for alloc_size in the macro, I moved things to syn::LitInt so it can easily be parsed into a usize.

I honestly am not sure where to go from here, It could be something I don't know or it might just be that syn doesn't have support for what we need to do. Honestly this seems to be leaning a bit into const fns that might support what we need, but again i don't know enough about macros to be sure.

vmx · 2021-04-20T08:09:26Z

@mriise I had a look at the derive macro part (which is indeed quite complicated, it took me a while to get into again). You can cherry-pick this commit: 19a44ff

It only fixes the derive macro, the build still fails. I indeed just moved things to syn::LintInt. I hope that helps you to make further progress.

mriise · 2021-04-23T09:23:34Z

Thank you so much @vmx, IDK what it was that was confusing me so much.
I managed to fix the sha macros and now have run into problems with digests < max size. Will look into allowing different sized Digests tomorrow morning

mriise · 2021-04-24T05:57:55Z

Fix for above was as simple as using a different generic for the digest, and rust compiler being amazing as it is implies S from the passed in digest automatically so no breaking API for the most part. (does break method signature though)

  fn multihash_from_digest<'a, D, const S: usize>(digest: &'a D) -> Multihash<SIZE>
  where
      D: Digest<S>,
      Self: From<&'a D>;

I removed fn size() and replaced it with an associated const as it just returned SIZE anyways. We could go with changing it to a const fn, but i figured the less unstable features we use the better.

I believe the last mole to whack is serde.
It has a [T; 0] impl for Deserialize & Serialize so we cant just do it for ourselves without a work-around as mentioned in serde-rs/serde#1937, alternatively we could wait for serde-rs/serde#1860 to be merged and released, and even further alternative is to use https://github.com/est31/serde-big-array for its const-generic support in the meantime.

vmx · 2021-04-26T08:48:54Z

Thank you so much @vmx, IDK what it was that was confusing me so much.

It is confusing. It took my quite some time to dig into this code again. The final fix looks trivial, but getting there wasn't :)

I believe the last mole to whack is serde.

I can't really tell what's best from reading the threads, which also means I don't have a strong preference. So I suggest trying things out and see what works (best).

mriise · 2021-04-26T09:44:25Z

@vmx simplest option was to use serde-big-array, and maybe when serde merges the PR we can drop it for that.

Everything builds, passes all tests, and benches, so I think it should be good to review. ~~I still have a TODO of renaming S to SIZE, but that's not going to affect code review.~~

just saw this

After taking a look at the actual code, I change my mind. i think S is better than SIZE.

src/hasher.rs

src/hasher_impl.rs

src/multihash.rs

mriise · 2021-04-26T10:40:20Z

src/multihash.rs

@@ -73,27 +70,37 @@ pub trait MultihashDigest:
 /// ```
 #[cfg_attr(feature = "serde-codec", derive(serde::Deserialize))]
 #[cfg_attr(feature = "serde-codec", derive(serde::Serialize))]
-#[cfg_attr(feature = "serde-codec", serde(bound = "S: Size"))]


I dont think removing this has any Ill effects, but im not 100% sure.

src/multihash.rs

mriise · 2021-04-27T05:23:51Z

did not mean to force push that...

mriise · 2021-04-27T05:36:10Z

build fails because of Warning: can't set `format_code_in_doc_comments = true`, unstable features are only available in nightly channel.

dvc94ch

@vmx I think this answers your question

dvc94ch · 2021-06-10T13:53:21Z

examples/custom_table.rs

-impl StatefulHasher for Sha2_256Truncated20 {
-    type Size = U20;
-    type Digest = Sha2Digest<Self::Size>;
+impl StatefulHasher<20> for Sha2_256Truncated20 {


this should still be an associated constant

what's Self::SIZE?

Self::SIZE replaced size() since while it could be a const fn it made more sense (in my mind) to make it a public associated constant.

It would make sense to move back to const fn size() in the case of having SIZE be an associated constant without being repetitive.

It seems using an assosicated const as a paramater is still part of the incomplete const_generics feature in nightly, though with enough feature flags we can make it look something like this

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

Not being able to have it as associate constant in current stable Rust sucks :-/

I guess we leave it like that then, until it;s possible in stable Rust? What do others think?

are we in a hurry to merge this? it's a usability regression, and comparing it to the benefits, maybe we should hold off until more of const-generics is stabilized.

I'm not in a hurry. Though I fear that it will take a long time until it's usable on stable.

@mriise What do you think about perhaps fixing everything up a single commit (once we are happy with the state of it) and we just keep it open? Then people who want const generic support can easily cherry-pick it themselves and it should also be easier to rebase it if we make other changes (e.g. fixing the blake3 hashing). This way all your work is not lost, still usable and hopefully find it's way in some day.

Although it is disappointing, I would have to agree. I'll do the final bits of cleanup and request another review.

So, is this really a regression? Or could we use this to trim down our digest list a bit (e.g., for all the Blake2s variants).

src/hasher.rs

mriise · 2021-06-14T11:38:02Z

A different way to implement const generics for certian static sized algorithms could be this. This should compile fine and is probably part of the incomplete part. This differs a bit more in complexity but only requires one flag instead of the three in my last suggested option.

vmx · 2021-06-14T11:47:41Z

generic_associated_types also seems a lot closer to become a stable feature that full const_generics.

dvc94ch · 2021-06-14T11:57:40Z

@mriise thanks for your work on this PR even if it takes a while for it to get merged

mriise · 2021-06-14T12:00:15Z

@vmx Yep! While I will have to put this on the back burner to make room for some other projects, I will open another PR that tries the other approach on nightly at some point.

@dvc94ch thanks!

vmx · 2021-06-14T12:02:26Z

@mriise Sure, as we identified it surely isn't time sensitive. I'm also disappointed that it cannot just land. I was warned, but I had hoped we really only need the min_const_generics part. Anyway, I'd like to thank you too, for spending so much time on this and such long review cycles.

mriise · 2021-06-17T10:49:54Z

TL;DR for anyone in the future, this PR made the change of impl StatefulHasher -> impl<const S: usize> StatefulHasher<S> by consequence of Digest<const S: usize> not being able to infer from an associated const (currently we use an associated typenum). As stated above, if you may cherry pick this PR if you need const generic support.

koushiro · 2021-09-17T11:01:06Z

@vmx Can you consider merging this PR and releasing a new version? I really need it.

vmx · 2021-09-20T10:10:41Z

@koushiro As mentioned in this comment, this PR makes the library harder to use. So the folks currently involved in this PR decided to wait until generic_associated_types land in stable Rust. So it will be a long time until const generics support can land.

mriise · 2021-09-20T10:16:32Z

There may be an alternate approach i've been thinking of... I will play with it a bit and will let you guys know if it will work out.

mriise · 2021-09-22T18:17:23Z

Well, I about tried everything I could think. At this point it boils down to 3 alternatives:

Go back to using Generic Array only implementing const generics for a few items
Unstable features, you can minimally rely on GATs, but it is highly incomplete (I ran across a few ICEs)
An unholy mess of unsafe and pointer magic

~~The 1st option seems to be possible (I haven't checked but it should work fine),~~ (Option one would rely heavily on const expressions to do const -> typenum) but personally I am a fan of the current implementation simply for how clean it ends up.

~~I will put together a PR soon with the Generic Array approach.~~
@vmx @koushiro (@dvc94ch if you want to add anything)
This is still blocked if we want to keep the API the same, though It may be worth merging depending on how urgent @koushiro needs this.

vmx · 2021-09-30T08:25:01Z

I'd be in favour of keeping the current API. I don't see a reason to change the API, just for moving from GenericArray to const generics. Though I've little experience on that side of things, so if there would be a good reason, please let me know.

Thanks again @mriise for looking into that.

mriise · 2021-09-30T12:17:46Z

There was one last thing I thought of before setting this aside again that relied heavily on const expressions to convert typenum into const generics, but implementation details are a bit fuzzy and the current implementation isn't ready for it anyway.

So this may be a thing to look at again if full const generics gets stabalized before GATs do (it is anyone's guess at this point i think).

And no problem @vmx, this has been a good challenge for me.

mriise · 2021-10-06T12:19:20Z

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=819a4ceb088d68814c02726708ef0737 ?

still requires nightly features for into/from<[u8; S]> but otherwise OK

Stebalien · 2021-10-28T16:26:59Z

@vmx and I discussed this offline and, while annoying, the issues with the interface changes don't outweigh the benefits. We're going to try to move this forward.

Rebased and squashed from #116. Closes #114.

Rebased and squashed from #116. Closes #114. BREAKING CHANGE: This change replaces uses of typenum with const generics. Importantly, the MultihashDigest, Hasher, and StatefulHasher traits are now generic over the digest size due to compiler limitations. However, `Hasher` exposes this size as an associated const, so `MyHasherImpl::SIZE` can often be used to fill in the size parameter with some help from type inference. Additionally: - All usages of typenum must be replaced with usize constants. - The new MSRV is 1.51.0 due to const generics. - IdentityDigest is now a tuple struct of usize and the digest instead of u8 and a generic array.

Stebalien · 2021-10-29T15:37:38Z

Changes merged into the next branch.

mriise added 2 commits August 24, 2020 01:52

Revert "(cargo-release) version 0.11.3"

5c4cdcb

This reverts commit eebdb2c.

lazy move to const generics

18f51a2

mriise marked this pull request as draft March 30, 2021 18:16

remove std::usize

daa059e

mriise commented Mar 30, 2021

View reviewed changes

src/multihash.rs Outdated Show resolved Hide resolved

src/hasher.rs Outdated Show resolved Hide resolved

src/multihash.rs Outdated Show resolved Hide resolved

src/multihash_impl.rs Show resolved Hide resolved

use usize for maximum hash size instead of u8

ba1a6a9

dvc94ch reviewed Mar 31, 2021

View reviewed changes

src/hasher_impl.rs Show resolved Hide resolved

mriise closed this Mar 31, 2021

mriise reopened this Mar 31, 2021

mriise closed this Mar 31, 2021

mriise reopened this Mar 31, 2021

vmx and others added 2 commits April 23, 2021 01:22

fix: make derive macro work

2e193e5

fix sha macro

bfed660

final changes & cargo clippy/fmt

224dbe7

mriise marked this pull request as ready for review April 26, 2021 09:46

update proc macro messages and tests

f040069

mriise commented Apr 26, 2021

View reviewed changes

mriise mentioned this pull request Apr 27, 2021

chore(deps): update parity-scale-codec requirement from 1.3.5 to 2.1.1 #118

Closed

mriise added 2 commits April 26, 2021 21:44

fix syntax and wordings

0354a47

Merge branch '0354a47'

72108e6

Merge branch 'master' into master

abd0fa6

dvc94ch reviewed Jun 10, 2021

View reviewed changes

mriise added 3 commits June 10, 2021 21:06

rewrite sha finalize()

5cea7ed

raise limit of varint to u16, use associated const

65f3127

derive copy for Multihash & Digest

7adfd4b

vmx reviewed Jun 11, 2021

View reviewed changes

src/hasher.rs Outdated Show resolved Hide resolved

mriise mentioned this pull request Jun 11, 2021

Associated constant generic as parameter of StatefulHasher #131

Closed

mriise added 2 commits June 14, 2021 03:20

revert max size

9162fe3

dont update README in this pr"

750f5be

johnchandlerburnham mentioned this pull request Oct 14, 2021

Merging work from yatima-inc/sp-multihash #144

Closed

Stebalien pushed a commit that referenced this pull request Oct 29, 2021

implement const generics

ac89aac

Rebased and squashed from #116. Closes #114.

Stebalien mentioned this pull request Oct 29, 2021

implement const generics #148

Merged

Stebalien pushed a commit that referenced this pull request Oct 29, 2021

implement const generics

68db238

Rebased and squashed from #116. Closes #114.

Stebalien closed this Oct 29, 2021

compiler-errors mentioned this pull request Jul 3, 2022

Stabilize generic associated types rust-lang/rust#96709

Merged

5 tasks

const generics #116

const generics #116

Conversation

mriise commented Mar 30, 2021 • edited Loading

mriise commented Mar 31, 2021

mriise commented Apr 13, 2021 • edited Loading

vmx commented Apr 20, 2021

mriise commented Apr 23, 2021

mriise commented Apr 24, 2021 • edited Loading

vmx commented Apr 26, 2021

mriise commented Apr 26, 2021 • edited Loading

mriise Apr 26, 2021

Choose a reason for hiding this comment

mriise commented Apr 27, 2021

mriise commented Apr 27, 2021

dvc94ch left a comment

Choose a reason for hiding this comment

dvc94ch Jun 10, 2021

Choose a reason for hiding this comment

dvc94ch Jun 10, 2021

Choose a reason for hiding this comment

mriise Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

mriise Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

vmx Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

dvc94ch Jun 11, 2021

Choose a reason for hiding this comment

vmx Jun 14, 2021

Choose a reason for hiding this comment

mriise Jun 14, 2021

Choose a reason for hiding this comment

Stebalien Oct 14, 2021

Choose a reason for hiding this comment

mriise commented Jun 14, 2021

vmx commented Jun 14, 2021

dvc94ch commented Jun 14, 2021

mriise commented Jun 14, 2021

vmx commented Jun 14, 2021

mriise commented Jun 17, 2021

koushiro commented Sep 17, 2021 • edited Loading

vmx commented Sep 20, 2021

mriise commented Sep 20, 2021

mriise commented Sep 22, 2021 • edited Loading

vmx commented Sep 30, 2021

mriise commented Sep 30, 2021

mriise commented Oct 6, 2021 • edited Loading

Stebalien commented Oct 28, 2021

Stebalien commented Oct 29, 2021

mriise commented Mar 30, 2021 •

edited

Loading

mriise commented Apr 13, 2021 •

edited

Loading

mriise commented Apr 24, 2021 •

edited

Loading

mriise commented Apr 26, 2021 •

edited

Loading

mriise Jun 11, 2021 •

edited

Loading

mriise Jun 11, 2021 •

edited

Loading

vmx Jun 11, 2021 •

edited

Loading

koushiro commented Sep 17, 2021 •

edited

Loading

mriise commented Sep 22, 2021 •

edited

Loading

mriise commented Oct 6, 2021 •

edited

Loading