Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit mutation of container-typed fields to the scope of the composite #1267

Merged
merged 36 commits into from
Feb 3, 2022

Conversation

dsainati1
Copy link
Contributor

@dsainati1 dsainati1 commented Nov 22, 2021

Closes #1260

Description

This adds a new error in the Cadence checker that prevents values like arrays and dictionaries from being mutated outside of the scope they were created in.

Currently, the let declaration kind does not allow writes anywhere, while the var declaration kind allows writes in current and inner scopes. This does not, however, impose any kind of restriction on where dictionary and array values can be mutated, in affect allowing these values to be mutated anywhere they can be read.

This change alters this behavior by only allowing values to be mutated in the context that they can be written. As such, the following program is now an error:

pub struct Foo {
	pub let x : [Int]

	init() {
	    self.x = [3]
	}
}

pub fun bar() {
        let foo = new Foo()
	foo.x[0] = 1 // error: cannot mutate `x`: field was defined inside `Foo`
}

A full specification and discussion of this change can be found in this FLIP: onflow/flow#703

@github-actions
Copy link

github-actions bot commented Nov 22, 2021

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit d3124c5
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.txtnew.txt
time/opdelta
RuntimeFungibleTokenTransfer-21.27ms ±30%1.30ms ±22%~(p=0.805 n=7+7)
RuntimeResourceDictionaryValues-214.7ms ± 5%14.5ms ± 4%~(p=0.710 n=7+7)
ParseFungibleToken-2185µs ± 1%182µs ± 2%~(p=0.051 n=6+7)
ParseArray-213.4ms ± 3%13.0ms ± 3%~(p=0.073 n=6+7)
ParseInfix-29.78µs ± 1%10.13µs ± 5%~(p=0.456 n=7+7)
ParseDeploy/decode_hex-21.17ms ± 2%1.16ms ± 1%~(p=0.138 n=7+6)
QualifiedIdentifierCreation/One_level-22.34ns ± 0%2.34ns ± 0%~(p=0.371 n=7+6)
QualifiedIdentifierCreation/Three_levels-2140ns ± 3%140ns ± 2%~(p=0.735 n=7+7)
ContractInterfaceFungibleToken-240.7µs ± 3%40.4µs ± 2%~(p=0.836 n=7+6)
CheckContractInterfaceFungibleTokenConformance-2135µs ± 5%134µs ± 2%~(p=0.535 n=7+7)
InterpretRecursionFib-22.45ms ± 4%2.43ms ± 1%~(p=0.268 n=7+5)
NewInterpreter/new_interpreter-21.07µs ± 2%1.06µs ± 1%~(p=0.159 n=7+7)
NewInterpreter/new_sub-interpreter-22.14µs ± 3%2.08µs ± 1%−2.75%(p=0.001 n=7+7)
ParseDeploy/byte_array-222.7ms ±10%21.0ms ± 3%−7.68%(p=0.017 n=7+7)
 
alloc/opdelta
CheckContractInterfaceFungibleTokenConformance-265.7kB ± 0%65.9kB ± 0%+0.29%(p=0.001 n=7+7)
RuntimeFungibleTokenTransfer-2277kB ± 0%277kB ± 0%+0.10%(p=0.001 n=7+7)
RuntimeResourceDictionaryValues-24.05MB ± 0%4.05MB ± 0%~(p=0.259 n=7+7)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
InterpretRecursionFib-21.24MB ± 0%1.24MB ± 0%~(all equal)
NewInterpreter/new_interpreter-2768B ± 0%768B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-21.24kB ± 0%1.24kB ± 0%~(all equal)
ContractInterfaceFungibleToken-226.5kB ± 0%26.5kB ± 0%−0.00%(p=0.042 n=7+6)
 
allocs/opdelta
RuntimeFungibleTokenTransfer-24.61k ± 0%4.61k ± 0%~(p=0.735 n=7+7)
RuntimeResourceDictionaryValues-2102k ± 0%102k ± 0%~(p=0.365 n=7+7)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
ContractInterfaceFungibleToken-2457 ± 0%457 ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-21.07k ± 0%1.07k ± 0%~(all equal)
InterpretRecursionFib-225.0k ± 0%25.0k ± 0%~(all equal)
NewInterpreter/new_interpreter-212.0 ± 0%12.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-238.0 ± 0%38.0 ± 0%~(all equal)
 

@dsainati1
Copy link
Contributor Author

One possible solution for fields with pub(set) access mode is to allow them to be mutated anywhere, the same way that pub(set) var fields can be written to from any scope. The upside of this is that it is simple and easily explainable to users. The possible downside is this does not allow the user to express a field that can be mutated to from any context, but not entirely overwritten (the way that let originally worked). One possible solution to this would be to add a pub(set) let declaration (currently not allowed), that forbids writing to the field in any scope, but allows mutating it in all scopes.

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2021

Codecov Report

Merging #1267 (01dbc6a) into master (d3124c5) will increase coverage by 0.03%.
The diff coverage is 90.00%.

❗ Current head 01dbc6a differs from pull request most recent head 55be095. Consider uploading reports for the commit 55be095 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1267      +/-   ##
==========================================
+ Coverage   75.81%   75.85%   +0.03%     
==========================================
  Files         279      279              
  Lines       38780    38823      +43     
==========================================
+ Hits        29402    29450      +48     
+ Misses       8016     8013       -3     
+ Partials     1362     1360       -2     
Flag Coverage Δ
unittests 75.85% <90.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
runtime/sema/errors.go 58.19% <54.54%> (-0.06%) ⬇️
runtime/common/declarationkind.go 79.80% <100.00%> (+3.84%) ⬆️
runtime/sema/check_assignment.go 96.38% <100.00%> (+0.30%) ⬆️
runtime/sema/check_member_expression.go 97.53% <100.00%> (+0.21%) ⬆️
runtime/sema/type.go 88.60% <100.00%> (+0.15%) ⬆️
runtime/sema/simple_type.go 92.85% <0.00%> (-3.58%) ⬇️
runtime/sema/checker.go 89.27% <0.00%> (+0.15%) ⬆️
runtime/ast/access.go 87.09% <0.00%> (+6.45%) ⬆️

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 d3124c5...55be095. Read the comment docs.

@dsainati1 dsainati1 changed the title demonstration of external mutation error Limit mutation of structure fields to the scope of that structure Dec 6, 2021
@dsainati1 dsainati1 marked this pull request as ready for review December 6, 2021 19:20
Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exciting! I just had one comment about the docs

@dsainati1 dsainati1 changed the title Limit mutation of structure fields to the scope of that structure Limit mutation of container-typed fields to the scope of the composite Jan 24, 2022
@dsainati1 dsainati1 requested a review from turbolent January 24, 2022 17:33
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed this in the last request: Please use the existing naming conventions for the tests and parallelize all tests.

@SupunS Could you also please have a look, as this is a critical piece?

@dsainati1 dsainati1 requested a review from turbolent January 28, 2022 15:13
@dsainati1 dsainati1 requested a review from SupunS February 1, 2022 17:54
Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Breaking Change Breaks Cadence contracts deployed on Mainnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate mutability improvements
5 participants