Add more purity annotations to built-in and standard library function types #2125
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
turbolent
commented
Nov 7, 2022
| @@ -145,8 +149,8 @@ func NewChecker( | |||
| } | |||
|
|
|||
| functionActivations := &FunctionActivations{} | |||
| functionActivations.EnterFunction(&FunctionType{ | |||
There was a problem hiding this comment.
Can save an allocation for each new checker
| sema.ByteArrayType, | ||
| ), | ||
| }, | ||
| sema.ToBigEndianBytesFunctionType, |
There was a problem hiding this comment.
Use existing function type
| @@ -3059,10 +3059,7 @@ var typeFunction = NewUnmeteredHostFunctionValue( | |||
| staticType := ConvertSemaToStaticType(invocation.Interpreter, ty) | |||
| return NewTypeValue(invocation.Interpreter, staticType) | |||
| }, | |||
| &sema.FunctionType{ | |||
There was a problem hiding this comment.
Move to sema, define a function type like for all other run-time type constructors
| @@ -661,8 +662,10 @@ func OptionalTypeMapFunctionType(typ Type) *FunctionType { | |||
| TypeParameter: typeParameter, | |||
| } | |||
|
|
|||
| const functionPurity = FunctionPurityImpure | |||
There was a problem hiding this comment.
Ensure the purities stay in sync
dsainati1
approved these changes
Nov 7, 2022
There was a problem hiding this comment.
Thanks for updating these.
One quick suggestion, AccountForEachFunctionType is defined in publicaccount_type.go and should be impure. It is in practice because that's the default, but since we are making the default explicit elsewhere in this PR it would be nice to update that function as well.
|
@dsainati1 Good point! Added in 46069fb |
Codecov Report
@@ Coverage Diff @@
## feature/stable-cadence #2125 +/- ##
=======================================================
Coverage 77.99% 77.99%
=======================================================
Files 306 306
Lines 64134 64138 +4
=======================================================
+ Hits 50021 50027 +6
+ Misses 12353 12352 -1
+ Partials 1760 1759 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/stable-cadence commit ce3978a Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SupunS
approved these changes
Nov 7, 2022
| @@ -193,6 +195,7 @@ Returns nil if no contract/contract interface with the given name exists in the | |||
| ` | |||
|
|
|||
| var AuthAccountContractsTypeRemoveFunctionType = &FunctionType{ | |||
| Purity: FunctionPurityImpure, | |||
There was a problem hiding this comment.
Doesn't have to be in this PR, but maybe we could replace the struct literals with a constructor to enforce the proper purity status
This was referenced Nov 9, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Work towards #2084
Description
Updating the NFT Storefront contracts identified that some more built-in and standard library functions are impure, but could be view: onflow/nft-storefront#74 (comment)
Add explicit purity annotations to more built-in and standard library function types, make functions view when possible.
masterbranchFiles changedin the Github PR explorer