-
Notifications
You must be signed in to change notification settings - Fork 131
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
Allow emit statements in conditions #2589
Allow emit statements in conditions #2589
Conversation
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/stable-cadence commit 759b814 Collapsed results for better readability
|
Codecov Report
@@ Coverage Diff @@
## feature/stable-cadence #2589 +/- ##
==========================================================
+ Coverage 78.99% 79.02% +0.03%
==========================================================
Files 340 340
Lines 81609 81724 +115
==========================================================
+ Hits 64467 64584 +117
Misses 14792 14792
+ Partials 2350 2348 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Ready for review, but going to add some more tests |
} | ||
|
||
return nil | ||
} | ||
|
||
func (checker *Checker) rewritePostConditions(postConditions ast.Conditions) PostConditionsRewrite { |
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.
Moved from https://github.com/onflow/cadence/pull/2589/files#diff-74d0954852bf407e9259a7c9bb55e8af093f080f00f5e35689c250886c29860aL1961 and refactored into sub-functions
return | ||
} | ||
|
||
func (checker *Checker) rewriteEmitPostCondition( |
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.
New
Added more tests in the last commits, but also fixed a parsing bug in 0638f55, where condition parse errors would not be propagated properly. @onflow/cadence Could you please have another look, especially at this fix? |
@SupunS could you please have another look? |
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!
a37f82d
to
379df98
Compare
Rebased on Stable Cadence, as the parsing fix is a breaking change |
Even though the FLIP says so, event emission was not considered a pure operation yet. I changed the event constructor to be always pure in 379df98. cc @dsainati1 |
Curious about what was causing the parser fix to break existing code? Were there any invalid codes that were treated as correct? |
@SupunS yes, some contracts have invalid trailing tokens in conditions, and those were just ignored, but now the errors for them are propagated and reported properly |
Closes #2069
Description
Implementation of onflow/flips#111
master
branchFiles changed
in the Github PR explorer