feat!: Allow CustomConsts to (optionally) be hashable

[acl-cqc]

• Add trait TryHash as prereq for CustomConst
• Automatically impl'd if your const impl's Hash
• Can also trivially implement (i.e. impl TryHash for Foo { }) to say "no, not hashable"
• Derive Hash for most consts, but not ConstF64

BREAKING CHANGE: any impl CustomConst will need to either impl Hash or impl MaybeHash

[codecov]

Codecov Report

Attention: Patch coverage is 82.75862% with 10 lines in your changes missing coverage. Please review.

Project coverage is 87.28%. Comparing base (6bf6c82) to head (af7a75d).

Files with missing lines	Patch %	Lines
hugr-core/src/std_extensions/collections.rs	0.00%	5 Missing ⚠️
hugr-core/src/extension/prelude.rs	25.00%	1 Missing and 2 partials ⚠️
hugr-core/src/ops/constant.rs	94.44%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1397   +/-   ##
=======================================
  Coverage   87.28%   87.28%           
=======================================
  Files         128      128           
  Lines       21721    21774   +53     
  Branches    18721    18774   +53     
=======================================
+ Hits        18959    19006   +47     
- Misses       1990     1996    +6     
  Partials      772      772           

Flag	Coverage Δ
python	92.46% <ø> (ø)
rust	86.46% <82.75%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[acl-cqc]

This is not really needed yet but seems like a very good thing to have for dataflow analysis and not a bad idea anyway: most constants are hashable!

The approach taken with MaybeHash contrasts with equal_consts which requires every custom-const struct that impls PartialEq to also override equal_consts using the downcast_equal_consts helper, avoided here. (Instead, those that don't implement Hash have to empty-implement MaybeHash, so it's not a free lunch, but that effort is pretty minimal and applies only in the minority of cases, whereas downcast_equal_consts has to be used in the majority of cases...

Hence, picking @aborgna-q out here as he may know of better/rustier ways here. Can we Downcast to see if a thing impl's Hash ??

[aborgna-q]

I.e. wee need the trait to be object safe

[acl-cqc]

Comment updated.

[aborgna-q]

There is no reflection in Rust, so there's no way to branch on whether a type implements a trait or not (not with stable code at least).

This looks like a good alternative. We can discuss whether to define a new trait vs adding another trait method to CustomConst (more verbose vs less clear), but they are the same solution.

I may suggest renaming it to TryHash instead, to follow the existing name convention (e.g. TryFrom).

[aborgna-q]

Drop all those allocations!

[aborgna-q]

Suggested change

	fn maybe_hash(&self, st: &mut dyn Hasher) -> bool {
	Hash::hash(self, &mut Box::new(st));
	fn maybe_hash(&self, mut st: &mut dyn Hasher) -> bool {
	Hash::hash(self, &mut st);

[acl-cqc]

That gives me:

error[E0277]: the size for values of type `dyn Hasher` cannot be known at compilation time
   --> hugr-core/src/ops/constant/custom.rs:113:26
    |
113 |         Hash::hash(self, &mut *st);
    |         ----------       ^^^^^^^^ doesn't have a size known at compile-time
    |         |
    |         required by a bound introduced by this call
    |
    = help: the trait `Sized` is not implemented for `dyn Hasher`

which I admit is odd - I'm not trying to pass a Hasher, only a &mut to it. Am I missing something?

[acl-cqc]

Ah sorry I'd put in an extra *

[aborgna-q]

*st is trying to dereference an unsized value into the stack.
The suggestion avoids that by using T : Hasher $\implies$ &mut T : Hasher

[acl-cqc]

This does look to me as if I'm passing a mutable reference to a mutable reference!

[acl-cqc]

There is no reflection in Rust, so there's no way to branch on whether a type implements a trait or not (not with stable code at least).

This looks like a good alternative. We can discuss whether to define a new trait vs adding another trait method to CustomConst (more verbose vs less clear), but they are the same solution.

I may suggest renaming it to TryHash instead, to follow the existing name convention (e.g. TryFrom).

I like TryHash, and indeed try_hash.

Also I admit I'm not really clear why we don't require PartialEq as a prereq for CustomConst. You can always implement with return false, which is what CustomConst will give you by default if you don't...

[aborgna-q]

Patch coverage is 4.76190%

🫠

[acl-cqc]

Patch coverage is 4.76190%

🫠

Sorry, I really did think there was already some code hashing lists. Maybe I got confused with stuff in a previous iteration of "const_fold2" 😅 ...anyway, I've added enough to make it green, could dup much of that into collections too I suppose to get a final few percent.