load, copy and borrow cause a force cast

[dsainati1]

Closes #1247

Description

This changes the storage API to cause load, copy and borrow to produce a runtime error if the supplied type argument does not match the stored value.

---

• 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 #1286 (20e7284) into master (4fffc3b) will decrease coverage by 0.01%.
The diff coverage is 95.45%.

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

@@            Coverage Diff             @@
##           master    #1286      +/-   ##
==========================================
- Coverage   77.43%   77.41%   -0.02%     
==========================================
  Files         279      279              
  Lines       36159    36129      -30     
==========================================
- Hits        27998    27969      -29     
+ Misses       7069     7068       -1     
  Partials     1092     1092              

Flag	Coverage Δ
unittests	77.41% <95.45%> (-0.02%)	⬇️

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

Impacted Files	Coverage Δ
runtime/interpreter/interpreter.go	89.53% <91.66%> (-0.14%)	⬇️
runtime/interpreter/value.go	80.24% <100.00%> (+<0.01%)	⬆️
runtime/stdlib/builtin.go	95.93% <0.00%> (-0.51%)	⬇️
runtime/runtime.go	87.01% <0.00%> (-0.02%)	⬇️

---

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 4fffc3b...ded629d. Read the comment docs.

[github-actions]

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 4fffc3b
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	16.3ms ± 9%	16.8ms ±12%	~	(p=0.456 n=7+7)
RuntimeFungibleTokenTransfer-2	1.52ms ±18%	1.64ms ±27%	~	(p=0.456 n=7+7)
ParseArray-2	22.9ms ± 7%	23.2ms ±11%	~	(p=1.000 n=7+7)
ParseInfix-2	25.1µs ± 8%	25.3µs ± 9%	~	(p=0.710 n=7+7)
ParseDeploy/byte_array-2	35.1ms ± 6%	35.3ms ±11%	~	(p=0.620 n=7+7)
ParseDeploy/decode_hex-2	1.45ms ± 7%	1.46ms ± 8%	~	(p=1.000 n=7+7)
ParseFungibleToken-2	461µs ± 5%	479µs ±10%	~	(p=0.295 n=6+7)
QualifiedIdentifierCreation/One_level-2	3.24ns ±13%	3.23ns ± 8%	~	(p=1.000 n=7+7)
QualifiedIdentifierCreation/Three_levels-2	163ns ±10%	162ns ±10%	~	(p=0.934 n=7+7)
ContractInterfaceFungibleToken-2	47.7µs ±10%	46.5µs ± 2%	~	(p=0.534 n=7+6)
CheckContractInterfaceFungibleTokenConformance-2	171µs ±11%	164µs ± 6%	~	(p=0.234 n=7+6)
NewInterpreter/new_interpreter-2	1.20µs ±15%	1.17µs ± 4%	~	(p=0.973 n=7+6)
NewInterpreter/new_sub-interpreter-2	2.42µs ±11%	2.30µs ± 8%	~	(p=0.209 n=7+7)
InterpretRecursionFib-2	2.83ms ±13%	2.65ms ± 3%	~	(p=0.051 n=7+6)
	alloc/op		delta
CheckContractInterfaceFungibleTokenConformance-2	65.7kB ± 0%	65.7kB ± 0%	+0.00%	(p=0.021 n=7+7)
RuntimeResourceDictionaryValues-2	4.04MB ± 0%	4.04MB ± 0%	~	(p=0.318 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)
ContractInterfaceFungibleToken-2	26.5kB ± 0%	26.5kB ± 0%	~	(all equal)
NewInterpreter/new_interpreter-2	720B ± 0%	720B ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	1.18kB ± 0%	1.18kB ± 0%	~	(all equal)
InterpretRecursionFib-2	1.24MB ± 0%	1.24MB ± 0%	~	(all equal)
RuntimeFungibleTokenTransfer-2	238kB ± 0%	238kB ± 0%	−0.01%	(p=0.017 n=6+6)
	allocs/op		delta
RuntimeResourceDictionaryValues-2	102k ± 0%	102k ± 0%	~	(p=0.913 n=6+7)
RuntimeFungibleTokenTransfer-2	4.54k ± 0%	4.54k ± 0%	~	(p=0.318 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	457 ± 0%	457 ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	1.07k ± 0%	1.07k ± 0%	~	(all equal)
NewInterpreter/new_interpreter-2	11.0 ± 0%	11.0 ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	36.0 ± 0%	36.0 ± 0%	~	(all equal)
InterpretRecursionFib-2	25.0k ± 0%	25.0k ± 0%	~	(all equal)

[dsainati1]

Does this change merit a FLIP? On the one hand, it is a pretty small change design/implementation wise, and there is not much to discuss on those fronts. On the other hand, it has a pretty widespread affect on how people write code. I am curious whether or not this would benefit from going through the full FLIP process.

[SupunS]

Looks good! 🎉

[turbolent]

Does this change merit a FLIP?

Yes, we should definitely pitch this to the community, then if there is interest, propose the breaking change through a FLIP and get agreement on it.

Thank you for already creating a PR with the changes 👍

I could imagine putting this change behind a feature flag / option, then release it, disabled by default, in the emulator.
This would allow users to evaluate the impact of the changes on their code.

[dsainati1]

The FLIP outlining this change has been approved and merged: onflow/flow#718

[turbolent]

Nice, looks good! Just one comment regarding generating the error value, which requires the location range

[turbolent]

Nice! Ready to get merged once the FLIP has been approved