[fixed-hash] Major overhaul of `construct_hash!` macro #71

Robbepop · 2018-10-20T16:53:31Z

Overhaul of the entire sub-crate `fixed-hash`

Roadmap

Pre-merging

Sort out the last few nits with new features
- Finalize documentation
- Remove deprecated features
- Test new features
- Clean up public API
Merge this PR after
- Merge of PR
- After release of Rust 1.30
Bump version to 0.3-beta.0 and publish on crates.io.

Post-merging

Transit all below dependencies to the 0.3.0-beta.X version of fixed-hash. Note that the pwasm-* crates currently depend on parity-hash instead of fixed-hash version 0.2.

pwasm-std will provide new uint instances and updated fixed-hash instances without conversion between uint and fixed-hash since I cannot see that it has been provided yet
pwasm-ethereum will no longer link to parity-hash and instead use pwasm-std
pwasm-abi same as pwasm-ethereum
ethbloom simply update fixed-hash
ethereum-types update fixed-hash, but already provides uint conversions itself, so no need for providing them by fixed-hash either
substrate-primitives update fixed-hash but does not need uint conversion
Make any changes that might come up during the work on step 4 and publish 0.3-beta.X as needed
Bump version to 0.3, create a tag fixed-hash-0.3 and publish 0.3

Open Questions

What to do with impl_hash_uint_conversions macro?
- The problem: Why should it be defined in fixed-hash and not in uint?
- Possible solution: Maybe add another crate to combine fixed-hash and uint and allow for this macro.

Summary of changes

Major overhaul of the construct_hash macro
- Possibility to declare types with visibility modifier (not only pub)
- Possibility to add custom doc comment
- Syntactically more conforming to default Rust syntax
- Updated documentation and added examples
Major overhaul of the impl_hash_conversion macro
- No longer requires sizes of input types
- Added proper error handling for some conversions
- Updated documentation and added examples
The std feature is now a default feature for this crate
Removed items that have been deprecated in 0.2.5
Removed bad usage of AsRef<Self> impl
Removed serialize crate feature

Deref and DerefMut are only meant to be used by smart pointer types. Hash types as provided by this crate are no smart pointers and thus shouldn't provide Deref or DerefMut impls.

The AsRef<Self> impl is causing problems in generic code because the compiler fails to infer types because of it due to the other more useful AsRef and AsMut impls.

These methods are very similar to the ones provides by std implementations such as String::as_str that are also provided besides String::as_ref impl. This commit just mirrors this design decision. An advantage is that there is no type confusion on caller code with these new methods.

The compile errors were direct results of the API changes of the latest commits.

The macro now allows a more standard interface to constructing new types compared to before which is proposed by the Rust API guidelines.

Also added some more examples of new use cases possible by the new interface. Those include - private/public type definition - adding custom doc comment for the type - having more standard type definition

Using paths that were beginning with `::foo` had a wrong crate as root during the doc tests. Manually adding `$crate::foo` to all of them fixed the bugs and we can now successfully test those macros in doc comments. Special thanks to Sergei for pointing $crate out to me!

parity-cla-bot · 2018-10-20T16:53:33Z

It looks like @Robbepop signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Robbepop · 2018-10-20T17:07:56Z

As far as I can tell the build failures reported by TravisCI and Appveyor are not related to fixed-hash and the changes that have been done here.
To verify that yourself you can try to test this crate in isolation locally.

tomusdrw

Looks pretty cool, would be nice to have a similar overhaul for uint as well, as both types have some commonalities (at least in the way we use them in Parity products).

Robbepop · 2018-10-21T04:51:54Z

@tomusdrw Thank you for the feedback!

I have found some other things in the crate that could be improved.
Also the exported macro impl_hash_conversions could have a simpler interface and using path matchers instead of ident in its signature.

This is because all fixed-hash types created via the construct_hash! macro are attributed with the #[repr(C)] which has the effect that they offer a stable interface throughout different compilers and targets for their memory footprint meaning std::mem::size_of<H256>() == 32 is always true if H256 is created as a 32 byte hash type. Also since std::mem::size_of<T>() is a const fn we can replace the size parameters of all the macros with it with the same effect and same optimization level.

Read more: https://doc.rust-lang.org/std/mem/fn.size_of.html#size-of-reprc-items

I didn't fix it since this PR is already pretty big but in the end it should be fixed as well before we release another major version of the fixed-hash crate.

Macro matcher of macro_rules! construct_hash.

The macro no longer requires inputs of the type's sizes since they can be easily infered without overhead thus making this API much more easy to use. Also greatly improved the documentation and added a usage example for the new API.

The only single usage of it got replaced inline manually.

Robbepop · 2018-10-21T21:02:08Z

TravisCI and Appveyor are showing errors because of this issues which seems to be unrelated to the work in this PR.

dvdplm

Much better docs&examples, and I like the type visibility changes (although: nightly only is a no-no for this to be merged). Tests are failing and I'm not sure exactly why.
The feature set listed in Cargo.toml looks odd (not this PRs fault, but worth fixing if we're introducing breaking changes) and plain wrong (e.g. we don't have any serde code).
Past experience tells me new issues tend to pop up we start using the parity-common crates in parity-ethereum or wherever they're used; it'd be fantastic if you could start a PR in the projects that use fixed-hash to make sure the changes work well there. This crate in particular should work as a replacement for parity-hash so it'd be good to check what needs to be done for that to happen as well.

fixed-hash/src/hash.rs

dvdplm · 2018-10-22T13:00:21Z

fixed-hash/src/hash.rs

+	($large_ty:ident, $small_ty:ident) => {
+		impl From<$small_ty> for $large_ty {
+			fn from(value: $small_ty) -> $large_ty {
+				use std::mem;


Something fishy is going on with this macro; on this branch I get warnings about missing std. If the conversions need to work in both std/no_std we should use core::mem (and sort out the implications in lib.rs). I also get a bunch of type mismatch test failures when enabling this feature.
In the PR description you mentioned "testing in isolation". Not sure what you meant by that exactly but I copied the files out of parity-common and fixed up Cargo.toml and ran the tests: same failures. Can you elaborate further on this aspect?

The use std::mem; is probably wrong here and should be replaced with use $crate::core::mem are you have suggested I guess. Besides that I will look into the problems you have had.

Unfortunately I can't find where I have written something about "testing in isolation" and can't remember either. :(
I hope the GitHub downtime is over soon and things are syncing again correctly.

To verify that yourself you can try to test this crate in isolation locally.

@pepyakin and me were trying to find a solution together.

The solution is to include the "std" feature in the default features.
This means that you have to build the crate with the cargo flag --no-default-features for a proper no_std environment. Fortunately this fixes the problems with doc tests above.

Also note that this is how it is done in other no_std crates in the pwasm-* area.

This crate now generates a useful ApiDummy hash type for the purpose of API inspection at docs.rs. Also the Cargo.toml now refers to the documentation that is going to be generated at docs.rs once uploaded to crates.io.

Deprecated since this crate no longer supports interoperability with uint in version 0.3. This support has to be provided from the outside, like pwasm-std.

dvdplm · 2018-10-30T07:48:42Z

.travis.yml

@@ -9,7 +9,7 @@ script:
  - cargo check --all --tests
  - cargo build --all
  - cargo test --all --exclude uint --exclude fixed-hash
-  - cd fixed-hash/ && cargo test --features=std,heapsizeof,uint_conversions && cd ..
+  - cd fixed-hash/ && cargo test --features=quickcheck-support,heapsize-support && cd ..


This does not run all tests I think (24 vs 37). Should perhaps be cargo test --all-features?

fixed-hash/Cargo.toml

dvdplm · 2018-10-30T07:50:53Z

fixed-hash/Cargo.toml

-serde = { version = "1.0", optional = true }
-serde_derive = { version = "1.0", optional = true }
+rand = { version = "0.5", optional = true, default-features = false }
+rustc-hex = { version = "2.0", optional = true, default-features = false }


I kind of wish we could get rid of this dependency; I don't think it buys us much ergonomics. One day perhaps...

Well we could disable its feature guard by default.

From what I've seen in other places rustc-hex is all over the place so doing that would be quite some (boring) work. Probably not something we have time for right now... :)

dvdplm · 2018-10-30T07:51:13Z

fixed-hash/Cargo.toml

+rand = { version = "0.5", optional = true, default-features = false }
+rustc-hex = { version = "2.0", optional = true, default-features = false }
+quickcheck = { version = "0.7", optional = true }
+byteorder = { version = "1.2.7", optional = true, default-features = false }


Can we use version = "1.2" here?

dvdplm · 2018-10-30T07:51:51Z

fixed-hash/Cargo.toml

-serialize = ["serde", "serde_derive"]
-uint_conversions = ["uint"]
+default = ["std", "libc", "rand-support", "rustc-hex-support", "byteorder-support"]
+std = ["rustc-hex/std", "rand/std", "byteorder/std"]


I think this can be just:

default = ["std"] std = ["rustc-hex/std", "rand/std", "byteorder/std", "libc"]

So you say that all of those features should be disabled by default? I can do that but in my opinion it is always a good thing to have the common uses active by default. This is for example byteorder-support and I would even say rustc-hex-support.

No, that's not what I meant. I think that by having rand/std you tell cargo to "use the rand feature and use it with the std feature of rand enabled", i.e. default = ["std"] implies that rustc-hex, rand and byteorder will all be enabled (and will all be configured to use their respective std feature). Do double-check me on that though, I'm not 100% sure!

The problem is that this requires not having rand, byteorder and rustc-hex being activated but their features rand-support, byteorder-support and rustc-hex-support should be enabled since the entire code is guarded by them and not by their respective features which is more accurate I think. So this would cause problems I think.

Btw.: I think the normal use case is to either use std feature or libc but not both at the same time. So implying the one by the other seems to be wrong to me. But I might be confused about that!

dvdplm · 2018-10-30T08:42:35Z

fixed-hash/src/hash.rs

 				}
 				Ok(())
 			}
 		}

-		impl Copy for $from {}
+		impl $crate::core::marker::Copy for $name {}


Would it be crazy to make this optional? I.e. to let consumers decide if they want their hash type to be Copy or rather use move semantics? cc @debris

Maybe we could even do this with the simple addition of attributes via #[derive(Copy)] on the user side. This would be pretty cool. :)

According to the Copy reference we should keep the behaviour of implementing Copy by default for all hash types: https://doc.rust-lang.org/std/marker/trait.Copy.html#when-should-my-type-be-copy

dvdplm · 2018-10-30T08:43:26Z

fixed-hash/src/hash.rs

-		impl Clone for $from {
-			fn clone(&self) -> $from {
-				let mut ret = $from::new();
+		#[cfg_attr(feature = "dev", allow(expl_impl_clone_on_copy))]


There is no dev feature I think? Can we remove this code perhaps?

This is from an old revision and to be honest I thought that the dev feature is something magical. But ofc we can remove it if it doesn't exist at all.

@debris do you know the origin of features="dev" per chance?

dvdplm · 2018-10-30T08:53:48Z

fixed-hash/src/hash.rs

-				ret
+		/// Utilities using the `rand` crate.
+		#[cfg(feature = "rand-support")]
+		impl $name {


Here too there's an expansion of the API and while the code looks great I'm a bit hesitant to add new public methods like this without knowing more about which projects are going to use them. Can you elaborate?

I can understand your critics about expanding the API. I just did the same as I have done already in my crate apint. There the rand-based API is a good addition and allows for easy testing with the *_using APIs and allows for user friendly usage by the APIs with the default RNG. :)

Keep in mind that the rand API existed earlier but wasn't testable since it was missing the *_using methods. So basically the new API allows for greater control and better testability.

fixed-hash/src/hash.rs

dvdplm · 2018-10-30T09:00:39Z

fixed-hash/src/hash.rs


-/// Implements conversion to and from a hash type and the equally sized unsigned int.
-/// CAUTION: Bad things will happen if the two types are not of the same size!
-#[cfg(feature="uint_conversions")]


What is the plan for these? Move them to uint? Own crate? Just remove them? They are currently in use in ethereum-types so we need some kind of upgrade path for consumers.

I had the idea to move them to pwasm-std for the pwasm-* crates and also provide uint types by pwasm-std so it fits again for it. And for ethereum-types I would do the same I guess. Maybe just another crate to unify both? It actually is not feasible to make uint depending on fixed-hash since it is built up from u64 words instead of bytes and would require another major overhaul. However, uint is far more complex than fixed-hash.

So to summarize: The best way is to have this functionality in the crate that actually needs this interoperability or in another small utility crate featuring both types.

Hmm, a tricky one! My personal opinion is that it's probably less disruptive/more pragmatic to just keep these conversions in fixed-hash for the time being and refactor later – maybe we can make the consuming code use the conversions less or not at all and we can just remove this? If you feel strongly this is not a good path ahead, I can be swayed though (though we can't merge before we have a solution to this as it'd make upgrading impossible).

I am not strongly against having the conversions in fixed-hash as a conversion path to a better future. However, I do not see anyone of us implementing this better future anytime soon if we stay with this approach. :D

This now just tests the same things as travis CI.

This should avoid more name collission and is also more clear that the macro is linked to the `fixed-hash` crate.

Could be used by uint conversion to avoid length checks.

dvdplm · 2018-10-30T20:11:52Z

fixed-hash/src/hash.rs

-			feature = "libc",
-			not(target_os = "unknown")
-		))]
+		#[cfg(feature = "heapsize-support")]


The target_os need to be there I'm afraid. WASM can't use the heapsize crate.

okay will revert it

Seems like heapsize needs libc and target_os = "unknown" settings.

fixed-hash/README.md

dvdplm · 2018-10-30T20:38:51Z

LGTM. Great work and perseverence here @Robbepop! Many thanks.

Robbepop added 9 commits October 20, 2018 17:16

Remove Deref and DerefMut impl

ab4547e

Deref and DerefMut are only meant to be used by smart pointer types. Hash types as provided by this crate are no smart pointers and thus shouldn't provide Deref or DerefMut impls.

Remove AsRef<Self> impl

50ecdb3

The AsRef<Self> impl is causing problems in generic code because the compiler fails to infer types because of it due to the other more useful AsRef and AsMut impls.

Fixed some minor compile errors

b7a585a

The compile errors were direct results of the API changes of the latest commits.

Slightly improve doc comment of clean_0x fn

9503490

Standardized interface to construct_hash!

6101a3d

The macro now allows a more standard interface to constructing new types compared to before which is proposed by the Rust API guidelines.

Rename $from to $name and $size to $n_bytes

927690f

Update doc-comment of macro_rules! construct_hash

17e1c1a

Also added some more examples of new use cases possible by the new interface. Those include - private/public type definition - adding custom doc comment for the type - having more standard type definition

Robbepop changed the title ~~Api refactorings~~ Major overhaul of construct_hash! macro Oct 20, 2018

Robbepop requested review from dvdplm, NikVolf and pepyakin October 20, 2018 16:55

Robbepop added 6 commits October 20, 2018 19:09

Fixed small doc-comment typo

28a079f

Fix a confusion in the new doc comments

d7e404f

Update quickcheck dependency from version 0.6 to 0.7

0ad6d46

Add doc-comment to fn as_mut_ptr

a726e51

Add #[inline] annotations were appropriate

5b4802d

Simplify impl of impl_libc_for_hash! macro

be525ea

tomusdrw approved these changes Oct 20, 2018

View reviewed changes

Robbepop mentioned this pull request Oct 21, 2018

Replace parity-hash with fixed-hash openethereum/pwasm-std#77

Closed

2 tasks

Robbepop added 3 commits October 21, 2018 18:24

Add some whitespace to macro matcher to make it more readable

f1d1b8f

Macro matcher of macro_rules! construct_hash.

Major overhaul of macro_rules impl_hash_conversions

8cf5ca6

The macro no longer requires inputs of the type's sizes since they can be easily infered without overhead thus making this API much more easy to use. Also greatly improved the documentation and added a usage example for the new API.

Deprecate fn clean_0x since it doesn't fit into this crate

11cff70

The only single usage of it got replaced inline manually.

Robbepop changed the title ~~Major overhaul of construct_hash! macro~~ [fixed-hash] Major overhaul of construct_hash! macro Oct 21, 2018

dvdplm suggested changes Oct 22, 2018

View reviewed changes

Robbepop added 3 commits October 29, 2018 20:08

Add support for docs.rs

e18d568

This crate now generates a useful ApiDummy hash type for the purpose of API inspection at docs.rs. Also the Cargo.toml now refers to the documentation that is going to be generated at docs.rs once uploaded to crates.io.

Bump version to 0.3.0-beta.0

eaf04c7

Remove deprecated section of the README

cd4533a

Deprecated since this crate no longer supports interoperability with uint in version 0.3. This support has to be provided from the outside, like pwasm-std.

dvdplm suggested changes Oct 30, 2018

View reviewed changes

Robbepop added 16 commits October 30, 2018 12:08

travisCI: test all features of fixed-hash

00fdcff

Appveyor: add tests for fixed-hash, uint, hashdb and plain_hasher

1201ef2

This now just tests the same things as travis CI.

Use byteorder version 1.2 instead of 1.2.7

3e51054

Remove restriction to libc feature usage for heapsize impl

93d18df

Document the api-dummy feature

a82c72d

Improve doc-comment and doc-test for construct_hash macro

dda1009

Improve doc comment for Hash::repeat_byte

baff409

Fix docs for impl block of byteorder-support

7c76d2b

Rename test var to expected (more clear)

2d02a65

Replace all whitespace indentation with tabs

ec30a9f

Rename construct_hash macro to construct_fixed_hash

5713e33

This should avoid more name collission and is also more clear that the macro is linked to the `fixed-hash` crate.

Fix README for renaming of construct_hash to construct_fixed_hash

386a0eb

Rename impl_hash_conversions to impl_fixed_hash_conversions

dcccb0b

Add README section about available crate features and their purposes

0202383

Add readme section to Cargo.toml used by crates.io

bbe1442

Add Hash::to_bytes api

1badf67

Could be used by uint conversion to avoid length checks.

dvdplm reviewed Oct 30, 2018

View reviewed changes

Robbepop added 2 commits October 30, 2018 21:18

Revert heapsize-support changes

2beacfd

Seems like heapsize needs libc and target_os = "unknown" settings.

Remove libc feature req of heapsize-support

8431aea

dvdplm reviewed Oct 30, 2018

View reviewed changes

fixed-hash/README.md Show resolved Hide resolved

Add info what features are enabled by enabling the std feature

59bc634

dvdplm approved these changes Oct 30, 2018

View reviewed changes

Robbepop merged commit e101b64 into master Oct 30, 2018

Robbepop mentioned this pull request Nov 6, 2018

Update fixed-hash & uint utilities paritytech/substrate#1078

Merged

ordian deleted the api-refactorings branch May 11, 2019 14:21

[fixed-hash] Major overhaul of construct_hash! macro #71

[fixed-hash] Major overhaul of construct_hash! macro #71

Conversation

Robbepop commented Oct 20, 2018 • edited Loading

Overhaul of the entire sub-crate fixed-hash

Roadmap

Pre-merging

Post-merging

Open Questions

Summary of changes

parity-cla-bot commented Oct 20, 2018

Robbepop commented Oct 20, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

Robbepop commented Oct 21, 2018 • edited Loading

Robbepop commented Oct 21, 2018

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbepop Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm commented Oct 30, 2018

[fixed-hash] Major overhaul of `construct_hash!` macro #71

[fixed-hash] Major overhaul of `construct_hash!` macro #71

Robbepop commented Oct 20, 2018 •

edited

Loading

Overhaul of the entire sub-crate `fixed-hash`

Robbepop commented Oct 21, 2018 •

edited

Loading

Robbepop Oct 22, 2018 •

edited

Loading