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: improve ast output #2824

Merged
merged 16 commits into from May 5, 2022
Merged

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Apr 23, 2022

What I did

Partial resolution for #2276, by adding type to the AST

Include more relevant information in the AST output under a new AST format that is partially unfolded (only performs folding of
builtin constants and builtin functions) and after type annotation and validation

How I did it

  • Add a new output format unfolded_ast.
  • Set type key in metadata type for more nodes.
  • Modify __repr__() for Event and ContractFunction to show the argument types and return type (for ContractFunction only).
  • Modify node's to_dict function.
  • Flatten _metadata fields when outputting the AST in a new format ast_extended, which is after constant folding and annotation.

Some limitations

  • Builtin constants and functions need to be injected for type-checking.

How to verify it

Existing tests pass.

Commit message

Modify AST output format to be partially folded (only performs folding  
of builtin constants and builtin functions) and after type annotation   
and validation. The purpose is to expose type information on expressions
to downstream tooling.
 
Partial fix for #2276

Description for the changelog

Add new AST output format after constant folding and type annotation with additional information.

Cute Animal Picture

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

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2022

Codecov Report

Merging #2824 (548ba23) into master (d366899) will increase coverage by 0.08%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##           master    #2824      +/-   ##
==========================================
+ Coverage   87.32%   87.40%   +0.08%     
==========================================
  Files          94       94              
  Lines       10053    10102      +49     
  Branches     2490     2497       +7     
==========================================
+ Hits         8779     8830      +51     
+ Misses        805      803       -2     
  Partials      469      469              
Impacted Files Coverage Δ
vyper/compiler/output.py 93.78% <ø> (ø)
vyper/compiler/phases.py 93.22% <90.90%> (-0.24%) ⬇️
vyper/ast/nodes.py 94.24% <100.00%> (+0.02%) ⬆️
vyper/builtin_functions/functions.py 89.23% <100.00%> (+0.20%) ⬆️
vyper/cli/vyper_compile.py 68.14% <100.00%> (ø)
vyper/semantics/types/function.py 87.15% <100.00%> (+0.05%) ⬆️
vyper/semantics/types/user/event.py 79.36% <100.00%> (+1.03%) ⬆️
vyper/ir/compile_ir.py 94.60% <0.00%> (-0.11%) ⬇️
vyper/semantics/types/user/struct.py 83.11% <0.00%> (ø)
vyper/semantics/validation/annotation.py 92.20% <0.00%> (+0.05%) ⬆️
... and 5 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 d366899...548ba23. Read the comment docs.

@tserg tserg marked this pull request as ready for review April 29, 2022 11:41
@tserg tserg marked this pull request as draft May 1, 2022 16:13
@tserg tserg marked this pull request as ready for review May 2, 2022 07:20
@@ -65,11 +66,13 @@ def visit_Assert(self, node):

def visit_Assign(self, node):
type_ = get_exact_type_from_node(node.target)
node._metadata["type"] = type_
Copy link
Member

Choose a reason for hiding this comment

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

why would Assign have a type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added it as an extension to the request in 2276 for more information on the type of AnnAssign nodes (variable declarations) since they are similar in that the variable type is usually derived from the child nodes.

Copy link
Member

Choose a reason for hiding this comment

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

i don't think 2276 was literally asking for vyper type info on AnnAssign nodes :) i think they were more asking "what kind of declaration is this". it doesn't make really make sense for statements to have types attached to them -- types generally go on expressions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah okay got it, I will revert the changes made to the various *Assign nodes.

@@ -141,7 +141,34 @@ def _to_dict(value):
# if value is a Vyper node, convert to a dict
if isinstance(value, VyperNode):
return value.to_dict()
return value

Copy link
Member

Choose a reason for hiding this comment

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

kind of weird to import type modules here. is there some way we can rethink this structure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, will look into this

Copy link
Member

Choose a reason for hiding this comment

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

i think the real issue here is that the representation of the _metadata["type"] field is not implemented. i think it will be cleaner if we add a method to all vyper types which is like "serialize_for_ast" or something (i didn't think very hard about the name, feel free to come up with alternatives).

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.

i think that the ast output should be replaced with the new ast_unfolded functionality. people who are looking for the AST will generally want the new "unfolded" AST.

@@ -23,6 +23,14 @@ def build_ast_dict(compiler_data: CompilerData) -> dict:
return ast_dict
Copy link
Member

Choose a reason for hiding this comment

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

i think now build_ast_dict is dead code? can we just replace the old build_ast_dict body with the new build_unfolded_ast_dict body?

vy_ast.validation.validate_literal_nodes(vyper_module)
vy_ast.folding.replace_builtin_constants(vyper_module)
vy_ast.folding.replace_builtin_functions(vyper_module)
validate_semantics(vyper_module, interface_codes)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
validate_semantics(vyper_module, interface_codes)
# note: validate_semantics does type inference on the AST
validate_semantics(vyper_module, interface_codes)

@@ -184,6 +193,19 @@ def generate_ast(source_code: str, source_id: int, contract_name: str) -> vy_ast
return vy_ast.parse_to_ast(source_code, source_id, contract_name)


def generate_unfolded_ast(
Copy link
Member

Choose a reason for hiding this comment

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

could you add a comment describing that the purpose of this phase is to generate an AST for tooling use?

now that i think about it, i wonder if it breaks other downstream tools. is it possible to check, e.g. whether brownie still works with the unfolded AST?

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.

nice work. thanks!

@charles-cooper charles-cooper enabled auto-merge (squash) May 5, 2022 13:09
@charles-cooper charles-cooper enabled auto-merge (squash) May 5, 2022 13:12
@charles-cooper charles-cooper merged commit 3cbdf35 into vyperlang:master May 5, 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.

None yet

3 participants