-
Notifications
You must be signed in to change notification settings - Fork 140
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
Make indexing assignment entitled #2664
Make indexing assignment entitled #2664
Conversation
…onflow/cadence into supun/entitled-assigment
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/mutability-restrictions commit e8c73bd Collapsed results for better readability
|
Codecov Report
@@ Coverage Diff @@
## feature/mutability-restrictions #2664 +/- ##
===================================================================
+ Coverage 78.90% 78.92% +0.01%
===================================================================
Files 341 341
Lines 81860 81906 +46
===================================================================
+ Hits 64595 64644 +49
+ Misses 14909 14907 -2
+ Partials 2356 2355 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's safe to only require Insertable
access for indexed assignment, unfortunately. In the example in the test:
let array: [String] = ["foo", "bar"]
fun test() {
var arrayRef = &array as auth(Insertable) &[String]
arrayRef[0] = "baz"
}
This both inserts "baz"
and also removes "foo"
. Similarly, when doing a swap, you are both inserting one element and removing another. As such I think this operation needs to have Mutable
-only access. Ideally we could say that it has (Mutable) | (Insertable, Removable)
, but we don't support this level of complexity. I think we can just permit it only for Mutable
now, and later if we add the ability for entitlements to inherit or extend each other we can define Mutable
to be the intersection of Insertable
and Removable
to fix this oddity.
ah that's a good flag! I think internally we can make the |
Oh nice! If we can special case this internally that would be great. We should just make sure to make the error message here not produce the "raw" entitlement set so as not to mislead people about what is possible in user-land. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot one thing when reviewing this: because dictionaries and arrays now support entitled operations, ArrayType
and DictionaryType
need to be updated to conform to EntitlementSupportingType
in type.go
, and the implementation of SupportedEntitlements
for these types should include Mutable
, Insertable
and Removable
.
These can be tested by adding a test to TestCheckEntitlementConditions
to test that when returning a resource array or dictionary type, the created reference type of result
in a function postcondition contains these entitlements properly.
@dsainati1 Added in ae64ba2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
5b47d1a
into
feature/mutability-restrictions
Closes #2647
Work towards onflow/flips#86
Description
Make index assignments on references (
arrayRef[0] = something
ordictRef[index] = something
) to require entitlements.master
branchFiles changed
in the Github PR explorer