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.

Sign up for GitHub

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

Jump to bottom

Parsing for attachments and related operations #1931

Merged
merged 32 commits into from
Nov 18, 2022
Merged

Parsing for attachments and related operations #1931

merged 32 commits into from
Nov 18, 2022
+1,678 −130

Conversation

dsainati1
Copy link
Contributor

@dsainati1 dsainati1 commented Sep 1, 2022 •
edited
Loading

Part of #357

Closes #2060

Description

As described in https://github.com/onflow/flips/blob/main/cadence/2022-09-21-attachments.md, this adds parsing for three new syntax forms:

  • AttachmentDeclarations declare a new attachment and can have a name, a base type, a list of conformances, and a list of members
  • AttachExpressions attach an attachment to a type and are denoted with the attach ... to ... syntax, and can have a left-hand side expression and a right-hand side expression
  • RemoveStatements remove an attachment from a value.
  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@dsainati1 dsainati1 self-assigned this Sep 1, 2022
@codecov
Copy link

codecov bot commented Sep 1, 2022 •
edited
Loading

Codecov Report

Merging #1931 (53c4d10) into feature/attachments (f839871) will decrease coverage by 0.01%.
The diff coverage is 74.57%.

@@                   Coverage Diff                   @@
##           feature/attachments    #1931      +/-   ##
=======================================================
- Coverage                77.94%   77.92%   -0.02%     
=======================================================
  Files                      307      308       +1     
  Lines                    64026    64362     +336     
=======================================================
+ Hits                     49902    50156     +254     
- Misses                   12331    12401      +70     
- Partials                  1793     1805      +12
Flag Coverage Δ
unittests 77.92% <74.57%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
runtime/ast/elementtype_string.go 0.00% <ø> (ø)
runtime/common/declarationkind.go 78.07% <0.00%> (-2.84%) ⬇️
runtime/common/declarationkind_string.go 40.00% <ø> (ø)
runtime/common/memorykind_string.go 40.00% <ø> (ø)
runtime/common/metering.go 92.23% <ø> (ø)
runtime/parser/declaration.go 81.71% <58.00%> (-1.82%) ⬇️
runtime/ast/attachment.go 79.45% <79.45%> (ø)
runtime/parser/expression.go 90.13% <87.50%> (-0.05%) ⬇️
runtime/parser/statement.go 80.85% <92.68%> (+0.82%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

github-actions bot commented Sep 1, 2022 •
edited
Loading

Cadence Benchstat comparison

This branch with compared with the base branch onflow:feature/attachments commit f839871
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.txtnew.txt
time/opdelta
CheckContractInterfaceFungibleTokenConformance-2117µs ± 1%122µs ± 8%~(p=0.073 n=7+6)
ContractInterfaceFungibleToken-242.2µs ± 0%42.9µs ± 4%~(p=0.394 n=6+6)
InterpretRecursionFib-22.48ms ± 1%2.49ms ± 2%~(p=0.945 n=7+6)
NewInterpreter/new_interpreter-21.11µs ± 0%1.12µs ± 5%~(p=1.000 n=6+6)
NewInterpreter/new_sub-interpreter-2593ns ± 9%577ns ± 1%~(p=0.126 n=7+6)
ParseArray-27.51ms ± 6%7.52ms ± 1%~(p=0.234 n=7+6)
ParseDeploy/byte_array-211.1ms ± 1%11.3ms ± 2%+1.56%(p=0.014 n=6+7)
ParseDeploy/decode_hex-21.20ms ± 2%1.20ms ± 1%~(p=0.628 n=7+6)
ParseFungibleToken/With_memory_metering-2181µs ± 2%183µs ± 2%~(p=0.128 n=7+7)
ParseFungibleToken/Without_memory_metering-2141µs ± 1%143µs ± 1%+1.47%(p=0.002 n=7+7)
ParseInfix-26.86µs ± 1%6.98µs ± 1%+1.76%(p=0.001 n=6+7)
QualifiedIdentifierCreation/One_level-22.41ns ± 0%2.42ns ± 0%+0.22%(p=0.041 n=6+7)
QualifiedIdentifierCreation/Three_levels-2115ns ± 0%117ns ± 1%+1.11%(p=0.001 n=7+6)
RuntimeFungibleTokenTransfer-2534µs ±42%529µs ±23%~(p=0.805 n=7+7)
RuntimeResourceDictionaryValues-25.03ms ± 1%5.15ms ± 1%+2.47%(p=0.001 n=6+7)
RuntimeScriptNoop-218.8µs ±46%17.7µs ±54%~(p=0.836 n=7+6)
SuperTypeInference/arrays-2289ns ± 4%298ns ± 3%+3.06%(p=0.025 n=7+7)
SuperTypeInference/composites-2129ns ± 0%128ns ± 2%~(p=0.061 n=6+6)
SuperTypeInference/integers-294.1ns ± 0%92.4ns ± 0%−1.81%(p=0.002 n=6+6)
ValueIsSubtypeOfSemaType-287.3ns ± 1%85.8ns ± 1%−1.74%(p=0.002 n=7+7)
 
alloc/opdelta
CheckContractInterfaceFungibleTokenConformance-251.0kB ± 0%51.0kB ± 0%~(all equal)
ContractInterfaceFungibleToken-224.7kB ± 0%24.7kB ± 0%~(all equal)
InterpretRecursionFib-21.00MB ± 0%1.00MB ± 0%~(p=0.385 n=7+6)
NewInterpreter/new_interpreter-2752B ± 0%752B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-2200B ± 0%200B ± 0%~(all equal)
ParseArray-22.72MB ± 3%2.74MB ± 3%~(p=0.383 n=7+7)
ParseDeploy/byte_array-24.13MB ± 3%4.13MB ± 3%~(p=0.535 n=7+7)
ParseDeploy/decode_hex-2214kB ± 0%214kB ± 0%~(p=0.563 n=7+7)
ParseFungibleToken/With_memory_metering-229.4kB ± 0%29.4kB ± 0%~(p=0.409 n=7+7)
ParseFungibleToken/Without_memory_metering-229.4kB ± 0%29.4kB ± 0%~(p=1.000 n=7+7)
ParseInfix-21.92kB ± 0%1.92kB ± 0%+0.11%(p=0.034 n=7+7)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
RuntimeFungibleTokenTransfer-2102kB ± 1%102kB ± 1%~(p=0.902 n=7+7)
RuntimeResourceDictionaryValues-22.28MB ± 0%2.28MB ± 0%~(p=0.805 n=7+7)
RuntimeScriptNoop-28.51kB ± 0%8.50kB ± 0%~(p=1.000 n=7+7)
SuperTypeInference/arrays-296.0B ± 0%96.0B ± 0%~(all equal)
SuperTypeInference/composites-20.00B 0.00B ~(all equal)
SuperTypeInference/integers-20.00B 0.00B ~(all equal)
ValueIsSubtypeOfSemaType-248.0B ± 0%48.0B ± 0%~(all equal)
 
allocs/opdelta
CheckContractInterfaceFungibleTokenConformance-2920 ± 0%920 ± 0%~(all equal)
ContractInterfaceFungibleToken-2444 ± 0%444 ± 0%~(all equal)
InterpretRecursionFib-218.9k ± 0%18.9k ± 0%~(all equal)
NewInterpreter/new_interpreter-213.0 ± 0%13.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-24.00 ± 0%4.00 ± 0%~(all equal)
ParseArray-259.6k ± 0%59.6k ± 0%~(p=1.000 n=7+7)
ParseDeploy/byte_array-289.4k ± 0%89.4k ± 0%~(p=1.000 n=7+7)
ParseDeploy/decode_hex-264.0 ± 0%64.0 ± 0%~(all equal)
ParseFungibleToken/With_memory_metering-2779 ± 0%779 ± 0%~(all equal)
ParseFungibleToken/Without_memory_metering-2779 ± 0%779 ± 0%~(all equal)
ParseInfix-248.0 ± 0%48.0 ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
RuntimeFungibleTokenTransfer-22.01k ± 0%2.01k ± 0%~(p=1.000 n=7+7)
RuntimeResourceDictionaryValues-237.0k ± 0%37.0k ± 0%~(p=0.972 n=7+6)
RuntimeScriptNoop-2137 ± 0%137 ± 0%~(all equal)
SuperTypeInference/arrays-23.00 ± 0%3.00 ± 0%~(all equal)
SuperTypeInference/composites-20.00 0.00 ~(all equal)
SuperTypeInference/integers-20.00 0.00 ~(all equal)
ValueIsSubtypeOfSemaType-21.00 ± 0%1.00 ± 0%~(all equal)
 

dreamsmasher
dreamsmasher approved these changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@dreamsmasher dreamsmasher left a comment

Choose a reason for hiding this comment

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

LGTM

@dsainati1 dsainati1 changed the title Parsing for extensions and related operations Parsing for attachments and related operations Oct 13, 2022
@dsainati1 @dreamsmasher
Update runtime/ast/attachment.go
8c9ead2 
Co-authored-by: Naomi Liu <57917002+dreamsmasher@users.noreply.github.com>
@dsainati1 dsainati1 marked this pull request as draft October 17, 2022 13:39
@dsainati1 dsainati1 marked this pull request as ready for review November 14, 2022 15:45
turbolent
turbolent reviewed Nov 15, 2022
View reviewed changes
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.

Nice!

Just one question regarding the new precedence level

runtime/ast/precedence.go Outdated Show resolved Hide resolved
@turbolent
Copy link
Member

Could you please target a feature branch instead of master? That way we won't end up releasing the attachments feature in an incomplete state or have to block the release on completing it. Thanks!

@dsainati1 dsainati1 changed the base branch from master to feature/attachments November 15, 2022 15:54
SupunS
SupunS reviewed Nov 17, 2022
View reviewed changes
runtime/ast/attachment_test.go Show resolved Hide resolved
runtime/parser/declaration.go Outdated Show resolved Hide resolved
runtime/parser/statement.go Outdated Show resolved Hide resolved
runtime/parser/expression.go Outdated Show resolved Hide resolved
runtime/parser/declaration.go Outdated Show resolved Hide resolved
runtime/parser/declaration.go Outdated Show resolved Hide resolved
turbolent
turbolent reviewed Nov 17, 2022
View reviewed changes
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.

Looks good, just some minor suggestions

runtime/parser/expression.go Outdated Show resolved Hide resolved
runtime/ast/attachment_test.go Show resolved Hide resolved
runtime/ast/attachment_test.go Show resolved Hide resolved
runtime/ast/attachment_test.go Show resolved Hide resolved
runtime/parser/declaration.go Outdated Show resolved Hide resolved
runtime/parser/declaration.go Outdated Show resolved Hide resolved
runtime/parser/declaration.go Outdated Show resolved Hide resolved
runtime/parser/statement.go Show resolved Hide resolved
runtime/parser/statement.go Outdated Show resolved Hide resolved
runtime/parser/statement.go Outdated Show resolved Hide resolved
@dsainati1 dsainati1 requested a review from turbolent November 18, 2022 16:03
turbolent
turbolent approved these changes Nov 18, 2022
View reviewed changes
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.

LGTM!

runtime/parser/declaration.go Outdated Show resolved Hide resolved
@dsainati1 @turbolent
Update runtime/parser/declaration.go
53c4d10 
Co-authored-by: Bastian Müller <bastian@axiomzen.co>
@dsainati1 dsainati1 merged commit 269df27 into feature/attachments Nov 18, 2022
@dsainati1 dsainati1 deleted the sainati/extension-parsing branch November 18, 2022 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamsmasher dreamsmasher dreamsmasher approved these changes

@robert-e-davidson3 robert-e-davidson3 robert-e-davidson3 approved these changes

@SupunS SupunS SupunS approved these changes

@turbolent turbolent turbolent approved these changes

Assignees

@dsainati1 dsainati1

Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Parsing for Attachments
5 participants
@dsainati1 @turbolent @SupunS @robert-e-davidson3 @dreamsmasher