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

feat: replace AnnAssign with VariableDef #2881

Merged
merged 33 commits into from
Jul 18, 2022

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented May 28, 2022

What I did

Improve conversion of Python AST to Vyper AST.

How I did it

Replace Python's AnnAssign nodes for contract variable declarations with a newly introduced VariableDef Vyper AST node.

The VariableDef has four three additional attributes, which are set during semantics validation.

  • is_state_variable
  • is_constant
  • is_public
  • is_immutable

How to verify it

See updated tests.

Commit message

feat: add VariableDef Vyper AST node

Adds a VariableDef Vyper AST node for variable declarations.

Description for the changelog

Add VariableDef Vyper AST node

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #2881 (958fba6) into master (aac0be8) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2881      +/-   ##
==========================================
+ Coverage   88.13%   88.16%   +0.02%     
==========================================
  Files          97       97              
  Lines       10802    10820      +18     
  Branches     2562     2569       +7     
==========================================
+ Hits         9520     9539      +19     
+ Misses        824      823       -1     
  Partials      458      458              
Impacted Files Coverage Δ
vyper/semantics/types/bases.py 84.50% <ø> (ø)
vyper/semantics/types/utils.py 89.76% <ø> (ø)
vyper/ast/expansion.py 94.11% <100.00%> (ø)
vyper/ast/folding.py 91.73% <100.00%> (ø)
vyper/ast/nodes.py 92.84% <100.00%> (+0.22%) ⬆️
vyper/ast/utils.py 69.69% <100.00%> (+5.41%) ⬆️
vyper/builtin_functions/utils.py 100.00% <100.00%> (ø)
vyper/codegen/global_context.py 74.78% <100.00%> (ø)
vyper/semantics/types/function.py 87.12% <100.00%> (ø)
vyper/semantics/types/user/interface.py 83.19% <100.00%> (ø)
... and 4 more

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 aac0be8...958fba6. Read the comment docs.

@tserg tserg marked this pull request as ready for review July 14, 2022 05:13
@@ -168,7 +171,7 @@ def visit_AnnAssign(self, node):

# generate function type and add to metadata
# we need this when builing the public getter
node._metadata["func_type"] = ContractFunction.from_AnnAssign(node)
node._metadata["func_type"] = ContractFunction.from_VariableDef(node)
Copy link
Member

Choose a reason for hiding this comment

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

from_VariableDef could probably be renamed to something like getter_from_VariableDef

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

looks quite nice. my only question is whether local variables should also be defined with VariableDef (breaking from python) or stay as AnnAssign. the extra fields on VariableDef (is_constant etc) make a lot of sense on contract variables; not as much on local variables.

@tserg
Copy link
Collaborator Author

tserg commented Jul 15, 2022

looks quite nice. my only question is whether local variables should also be defined with VariableDef (breaking from python) or stay as AnnAssign. the extra fields on VariableDef (is_constant etc) make a lot of sense on contract variables; not as much on local variables.

I think another approach, taking from solc-typed-ast, is to consolidate is_constant and is_immutable into a mutability attribute that takes a VariableMutability enum of either (1) MUTABLE; (2) IMMUTABLE; or (3) CONSTANT. This would make it more relevant for local variables.

I think it would be clearer and more consistent to have both contract and local variable declarations be of VariableDef, than for local variable declarations to be of AnnAssign node type alongside e.g. struct/event members.

vyper/ast/utils.py Outdated Show resolved Hide resolved
vyper/ast/nodes.py Outdated Show resolved Hide resolved
vyper/ast/utils.py Outdated Show resolved Hide resolved
vyper/ast/utils.py Outdated Show resolved Hide resolved
@charles-cooper charles-cooper enabled auto-merge (squash) July 18, 2022 02:51
@charles-cooper charles-cooper disabled auto-merge July 18, 2022 03:25
@charles-cooper charles-cooper merged commit 1d1ef5d into vyperlang:master Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants