Added test to partially ensure HashableValue does not regress

[robert-e-davidson3]

Closes nothing (just ran into this)

Description

While looking into another issue, I came across the warning in hashablevalue.go and figured it could use a quick test to catch a possible regression.

I also added some language to the warning because it had a loophole.

---

• Targeted PR against master branch
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work
• Code follows the standards mentioned here
• Updated relevant documentation
• Re-reviewed Files changed in the Github PR explorer
• Added appropriate labels

[codecov-commenter]

Codecov Report

Merging #1431 (e9c8f65) into master (3ecb20d) will not change coverage.
The diff coverage is n/a.

❗ Current head e9c8f65 differs from pull request most recent head fd10f23. Consider uploading reports for the commit fd10f23 to get more accurate results

@@           Coverage Diff           @@
##           master    #1431   +/-   ##
=======================================
  Coverage   73.01%   73.01%           
=======================================
  Files         288      288           
  Lines       39786    39786           
=======================================
  Hits        29048    29048           
  Misses       9263     9263           
  Partials     1475     1475           

Flag	Coverage Δ
unittests	73.01% <ø> (ø)

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

Impacted Files	Coverage Δ
runtime/interpreter/encode.go	65.72% <ø> (ø)
runtime/interpreter/hashablevalue.go	100.00% <ø> (ø)
runtime/interpreter/primitivestatictype.go	72.42% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ecb20d...fd10f23. Read the comment docs.

[github-actions]

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 3ecb20d
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Results

	old.txt	new.txt
	time/op		delta
RuntimeResourceDictionaryValues-2	22.1ms ±16%	22.4ms ±17%	~	(p=0.620 n=7+7)
RuntimeFungibleTokenTransfer-2	2.22ms ±22%	2.01ms ±29%	~	(p=0.318 n=7+7)
ParseDeploy/byte_array-2	31.8ms ±15%	35.8ms ±18%	~	(p=0.073 n=7+7)
ParseDeploy/decode_hex-2	1.77ms ± 8%	1.82ms ±14%	~	(p=0.902 n=7+7)
ParseFungibleToken-2	333µs ±19%	344µs ±24%	~	(p=0.805 n=7+7)
ParseArray-2	21.2ms ±18%	23.0ms ±41%	~	(p=0.710 n=7+7)
ParseInfix-2	13.7µs ±27%	14.6µs ±23%	~	(p=0.535 n=7+7)
QualifiedIdentifierCreation/One_level-2	3.95ns ± 5%	3.98ns ± 9%	~	(p=0.945 n=6+7)
QualifiedIdentifierCreation/Three_levels-2	225ns ±13%	226ns ±11%	~	(p=0.902 n=7+7)
ContractInterfaceFungibleToken-2	68.7µs ±15%	72.0µs ±23%	~	(p=0.456 n=7+7)
CheckContractInterfaceFungibleTokenConformance-2	250µs ±25%	247µs ±27%	~	(p=1.000 n=7+7)
NewInterpreter/new_interpreter-2	1.93µs ±17%	1.89µs ±18%	~	(p=0.805 n=7+7)
NewInterpreter/new_sub-interpreter-2	3.63µs ±16%	3.49µs ±16%	~	(p=0.535 n=7+7)
InterpretRecursionFib-2	4.33ms ±14%	4.22ms ±11%	~	(p=0.805 n=7+7)
	alloc/op		delta
ContractInterfaceFungibleToken-2	26.6kB ± 0%	26.6kB ± 0%	+0.00%	(p=0.033 n=7+6)
RuntimeResourceDictionaryValues-2	4.05MB ± 0%	4.05MB ± 0%	~	(p=0.805 n=7+7)
RuntimeFungibleTokenTransfer-2	273kB ± 0%	273kB ± 0%	~	(p=0.902 n=7+7)
QualifiedIdentifierCreation/One_level-2	0.00B	0.00B	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	64.0B ± 0%	64.0B ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	66.2kB ± 0%	66.2kB ± 0%	~	(p=1.000 n=7+7)
NewInterpreter/new_interpreter-2	848B ± 0%	848B ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	1.32kB ± 0%	1.32kB ± 0%	~	(all equal)
InterpretRecursionFib-2	1.26MB ± 0%	1.26MB ± 0%	~	(p=0.767 n=7+6)
	allocs/op		delta
RuntimeResourceDictionaryValues-2	102k ± 0%	102k ± 0%	~	(p=0.849 n=7+7)
RuntimeFungibleTokenTransfer-2	4.52k ± 0%	4.52k ± 0%	~	(p=0.738 n=7+7)
QualifiedIdentifierCreation/One_level-2	0.00	0.00	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	2.00 ± 0%	2.00 ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	458 ± 0%	458 ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	1.07k ± 0%	1.07k ± 0%	~	(all equal)
NewInterpreter/new_interpreter-2	13.0 ± 0%	13.0 ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	39.0 ± 0%	39.0 ± 0%	~	(all equal)
InterpretRecursionFib-2	26.2k ± 0%	26.2k ± 0%	~	(all equal)

[robert-e-davidson3]

Maybe something like this would be a good idea too: https://github.com/onflow/cadence/pull/1374/files#diff-4c8b1e88f5b478844aa7afa3c59c0f6fee63bb07b0bef8bfc6e974782fdff155R8

[turbolent]

Great idea to avoid regressions there! 👍

Given that HashInputTypeUFix64 is currently the last "case in the enum", but might not be in the future, when types are added, maybe add an additional case which just indicates the "count" / number of cases, and a comment indicating it always must be last, e.g.

	// !!! *WARNING* !!!
	// ADD NEW TYPES *BEFORE* THIS WARNING.
	// DO *NOT* ADD NEW TYPES AFTER THIS LINE!
	HashInputType_Count
)

And then use HashInputType_Count in the new test case.

There are actually two more enumerations which have similar requirements. Could you please add the same "count case" there and a test case for each?

• interpreter.CBORTag*
• interpreter.PrimitiveStaticType

[turbolent]

Great idea and work!