cadence
cadence copied to clipboard
onflow
Detect invalidated references statically
Working towards the FLIP: https://github.com/onflow/flow/pull/1043
Description
Statically check for invalidated references, whenever possible.
All invalidations may not be caught by the static checker. Hence the runtime validation is also added in: https://github.com/onflow/cadence/pull/1999
- [ ] Targeted PR against
masterbranch - [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work
- [x] Code follows the standards mentioned here
- [x] Updated relevant documentation
- [x] Re-reviewed
Files changedin the Github PR explorer - [x] Added appropriate labels
Codecov Report
Merging #2037 (c24f027) into feature/stable-cadence (abc0b07) will decrease coverage by
0.02%. The diff coverage is98.40%.
@@ Coverage Diff @@
## feature/stable-cadence #2037 +/- ##
==========================================================
- Coverage 77.70% 77.67% -0.03%
==========================================================
Files 304 304
Lines 63125 63252 +127
==========================================================
+ Hits 49050 49133 +83
- Misses 12364 12411 +47
+ Partials 1711 1708 -3
| Flag | Coverage Δ | |
|---|---|---|
| unittests | 77.67% <98.40%> (-0.03%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| runtime/sema/checker.go | 92.04% <ø> (ø) |
|
| runtime/sema/check_variable_declaration.go | 97.55% <98.13%> (+0.34%) |
:arrow_up: |
| runtime/sema/check_assignment.go | 97.26% <100.00%> (+0.01%) |
:arrow_up: |
| runtime/sema/check_expression.go | 100.00% <100.00%> (ø) |
|
| runtime/sema/errors.go | 73.52% <100.00%> (+0.04%) |
:arrow_up: |
| runtime/interpreter/storage.go | 69.04% <0.00%> (-1.59%) |
:arrow_down: |
| runtime/interpreter/errors.go | 55.94% <0.00%> (-0.89%) |
:arrow_down: |
| runtime/interpreter/value.go | 71.49% <0.00%> (-0.32%) |
:arrow_down: |
| runtime/interpreter/interpreter.go | 88.89% <0.00%> (-0.11%) |
:arrow_down: |
| runtime/parser/statement.go | 82.97% <0.00%> (+0.05%) |
:arrow_up: |
| ... and 1 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Cadence Benchstat comparison
This branch with compared with the base branch onflow:feature/stable-cadence commit 72b3292fff3a22bc8e7982842feb155a45c36937
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -benchmem -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.
Collapsed results for better readability
| old.txt | new.txt | |||
|---|---|---|---|---|
| time/op | delta | |||
| CheckContractInterfaceFungibleTokenConformance-2 | 167µs ± 4% | 159µs ± 3% | −4.32% | (p=0.038 n=7+7) |
| ContractInterfaceFungibleToken-2 | 49.5µs ± 4% | 50.4µs ± 9% | ~ | (p=0.805 n=7+7) |
| InterpretRecursionFib-2 | 3.18ms ± 9% | 2.93ms ± 3% | −7.95% | (p=0.001 n=7+6) |
| NewInterpreter/new_interpreter-2 | 1.51µs ± 9% | 1.42µs ± 3% | −5.89% | (p=0.001 n=7+7) |
| NewInterpreter/new_sub-interpreter-2 | 808ns ± 6% | 806ns ± 6% | ~ | (p=0.836 n=6+7) |
| ParseArray-2 | 9.71ms ± 3% | 9.92ms ± 9% | ~ | (p=0.902 n=7+7) |
| ParseDeploy/byte_array-2 | 15.0ms ± 4% | 14.6ms ± 3% | ~ | (p=0.165 n=7+7) |
| ParseDeploy/decode_hex-2 | 1.44ms ± 7% | 1.42ms ± 3% | ~ | (p=0.366 n=7+6) |
| ParseFungibleToken/With_memory_metering-2 | 229µs ± 3% | 231µs ± 6% | ~ | (p=0.731 n=6+7) |
| ParseFungibleToken/Without_memory_metering-2 | 184µs ± 3% | 184µs ± 2% | ~ | (p=1.000 n=7+7) |
| ParseInfix-2 | 8.45µs ± 4% | 8.10µs ± 0% | −4.23% | (p=0.003 n=7+5) |
| QualifiedIdentifierCreation/One_level-2 | 2.73ns ± 4% | 2.73ns ± 4% | ~ | (p=1.000 n=7+7) |
| QualifiedIdentifierCreation/Three_levels-2 | 162ns ± 5% | 164ns ± 4% | ~ | (p=0.535 n=7+7) |
| RuntimeFungibleTokenTransfer-2 | 766µs ±21% | 748µs ±20% | ~ | (p=0.535 n=7+7) |
| RuntimeResourceDictionaryValues-2 | 6.06ms ± 3% | 6.06ms ± 2% | ~ | (p=0.937 n=6+6) |
| RuntimeScriptNoop-2 | 20.8µs ±41% | 28.6µs ± 7% | +37.26% | (p=0.035 n=7+6) |
| ValueIsSubtypeOfSemaType-2 | 109ns ± 4% | 108ns ± 5% | ~ | (p=0.776 n=7+7) |
| alloc/op | delta | |||
| CheckContractInterfaceFungibleTokenConformance-2 | 61.4kB ± 0% | 62.8kB ± 0% | +2.19% | (p=0.001 n=7+7) |
| ContractInterfaceFungibleToken-2 | 24.1kB ± 0% | 24.8kB ± 0% | +2.79% | (p=0.001 n=6+7) |
| InterpretRecursionFib-2 | 1.23MB ± 0% | 1.23MB ± 0% | ~ | (p=0.592 n=7+7) |
| NewInterpreter/new_interpreter-2 | 848B ± 0% | 848B ± 0% | ~ | (all equal) |
| NewInterpreter/new_sub-interpreter-2 | 264B ± 0% | 264B ± 0% | ~ | (all equal) |
| ParseArray-2 | 2.76MB ± 4% | 2.74MB ± 4% | ~ | (p=0.535 n=7+7) |
| ParseDeploy/byte_array-2 | 4.23MB ± 3% | 4.17MB ± 4% | ~ | (p=0.364 n=7+7) |
| ParseDeploy/decode_hex-2 | 214kB ± 0% | 214kB ± 0% | ~ | (p=0.707 n=7+7) |
| ParseFungibleToken/With_memory_metering-2 | 29.8kB ± 0% | 29.8kB ± 0% | ~ | (p=0.592 n=7+7) |
| ParseFungibleToken/Without_memory_metering-2 | 29.8kB ± 0% | 29.8kB ± 0% | ~ | (p=0.185 n=7+7) |
| ParseInfix-2 | 1.93kB ± 0% | 1.93kB ± 0% | +0.08% | (p=0.041 n=6+7) |
| QualifiedIdentifierCreation/One_level-2 | 0.00B | 0.00B | ~ | (all equal) |
| QualifiedIdentifierCreation/Three_levels-2 | 64.0B ± 0% | 64.0B ± 0% | ~ | (all equal) |
| RuntimeFungibleTokenTransfer-2 | 108kB ± 1% | 108kB ± 1% | ~ | (p=0.535 n=7+7) |
| RuntimeResourceDictionaryValues-2 | 2.27MB ± 0% | 2.28MB ± 0% | ~ | (p=0.445 n=6+7) |
| RuntimeScriptNoop-2 | 8.87kB ± 1% | 8.86kB ± 0% | ~ | (p=0.731 n=7+6) |
| ValueIsSubtypeOfSemaType-2 | 48.0B ± 0% | 48.0B ± 0% | ~ | (all equal) |
| allocs/op | delta | |||
| CheckContractInterfaceFungibleTokenConformance-2 | 1.03k ± 0% | 1.03k ± 0% | +0.29% | (p=0.001 n=7+7) |
| ContractInterfaceFungibleToken-2 | 431 ± 0% | 431 ± 0% | ~ | (all equal) |
| InterpretRecursionFib-2 | 22.5k ± 0% | 22.5k ± 0% | ~ | (all equal) |
| NewInterpreter/new_interpreter-2 | 14.0 ± 0% | 14.0 ± 0% | ~ | (all equal) |
| NewInterpreter/new_sub-interpreter-2 | 5.00 ± 0% | 5.00 ± 0% | ~ | (all equal) |
| ParseArray-2 | 69.6k ± 0% | 69.6k ± 0% | ~ | (p=0.385 n=7+6) |
| ParseDeploy/byte_array-2 | 104k ± 0% | 104k ± 0% | ~ | (p=0.538 n=7+6) |
| ParseDeploy/decode_hex-2 | 75.0 ± 0% | 75.0 ± 0% | ~ | (all equal) |
| ParseFungibleToken/With_memory_metering-2 | 1.01k ± 0% | 1.01k ± 0% | ~ | (all equal) |
| ParseFungibleToken/Without_memory_metering-2 | 1.01k ± 0% | 1.01k ± 0% | ~ | (all equal) |
| ParseInfix-2 | 60.0 ± 0% | 60.0 ± 0% | ~ | (all equal) |
| QualifiedIdentifierCreation/One_level-2 | 0.00 | 0.00 | ~ | (all equal) |
| QualifiedIdentifierCreation/Three_levels-2 | 2.00 ± 0% | 2.00 ± 0% | ~ | (all equal) |
| RuntimeFungibleTokenTransfer-2 | 2.16k ± 0% | 2.16k ± 0% | ~ | (p=0.462 n=7+7) |
| RuntimeResourceDictionaryValues-2 | 36.9k ± 0% | 36.9k ± 0% | ~ | (p=0.878 n=7+7) |
| RuntimeScriptNoop-2 | 145 ± 0% | 145 ± 0% | ~ | (all equal) |
| ValueIsSubtypeOfSemaType-2 | 1.00 ± 0% | 1.00 ± 0% | ~ | (all equal) |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
I had the same thought initially. But after seeing how many contracts/obvious-cases can be detected statically, I actually changed that stance, and feels like it would be a sin not to catch them statically and enforce them to fix it (vs waiting for the run-time check to catch them on the act.)
The known scenario we don't catch right now is: Taking storage references and changing the storage. e.g: https://github.com/onflow/cadence/blob/91cf4a2ce69d72ade9fd852177732d60535767fc/runtime/tests/checker/reference_test.go#L1573-L1585 Change of this FLIP only targets ephemeral references, so it could argue that no change is done for storage references.
On the other hand, there are similar places where the best-effort analysis is done; for eg: certain invalid casting operations can be caught statically. And then there are also ones that cannot be, and those are caught dynamically at run-time.
I see! If the only case we can't detect statically is the storage change, then it makes sense to add this check, especially since as you pointed out this whole FLIP only proposed to change ephemeral references anyways. We should document this limitation explicitly though, so people are aware of what the boundaries of the analysis are.
On that point, the documentation boxes on this PR (and the underlying PR) are checked but I don't see any changes to the documentation in them. Are we planning to add a section to the reference documentation about this invalidation?
Actually, realized one more case: reference-typed fields are not possible to track. Added a test-case: https://github.com/onflow/cadence/pull/2037/commits/df5bd3899c7c95ffa7569677dcb3091c964e8f2d