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: require type annotations for loop variables #3596

Merged
merged 56 commits into from
Jan 7, 2024

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Sep 7, 2023

What I did

Fix #3475, #3441

How I did it

How to verify it

Commit message

feat: require type annotations for loop variables

this commit changes the vyper language to require type annotations for
loop variables. that is, before, the following was allowed:
```vyper
for i in [1, 2, 3]:
    pass
```

now, `i` is required to have a type annotation:
```vyper
for i: uint256 in [1, 2, 3]:
    pass
```

this makes the annotation of loop variables consistent with the rest of
vyper (it was previously a special case, that loop variables did not
need to be annotated).

the approach taken in this commit is to add a pre-parsing step which
lifts out the type annotation into a separate data structure, and then
splices it back in during the post-processing steps in
`vyper/ast/parse.py`.

this commit also simplifies a lot of analysis regarding for loops.
notably, the possible types for the loop variable no longer needs to be
iterated over, we can just propagate the type provided by the user. for
this reason we also no longer need to use the typechecker speculation
machinery for inferring the type of the loop variable. however, the
NodeMetadata code is not removed because it might come in handy at a
later date.

Description for the changelog

Cute Animal Picture

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

@tserg tserg marked this pull request as ready for review September 7, 2023 16:06
vyper/ast/annotation.py Fixed Show fixed Hide fixed
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (0c82d0b) 84.23% compared to head (9678f91) 84.25%.

Files Patch % Lines
vyper/ast/parse.py 91.30% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3596      +/-   ##
==========================================
+ Coverage   84.23%   84.25%   +0.02%     
==========================================
  Files          92       92              
  Lines       13034    13073      +39     
  Branches     2919     2916       -3     
==========================================
+ Hits        10979    11015      +36     
- Misses       1636     1640       +4     
+ Partials      419      418       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vyper/semantics/analysis/local.py Fixed Show fixed Hide fixed
vyper/semantics/analysis/local.py Fixed Show fixed Hide fixed
vyper/semantics/analysis/local.py Fixed Show fixed Hide fixed
vyper/semantics/analysis/local.py Fixed Show fixed Hide fixed
@@ -82,7 +83,7 @@ def parse_to_ast_with_settings(
module = vy_ast.get_node(py_ast)
assert isinstance(module, vy_ast.Module) # mypy hint

return settings, module
return settings, module, loop_var_annotations
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 instead of passing around loop_var_annotations everywhere we should just tag it on the AST during the annotate_vyper_ast portion of ast parsing.

vyper/semantics/analysis/local.py Fixed Show fixed Hide fixed
vyper/compiler/phases.py Fixed Show fixed Hide fixed
@@ -485,13 +485,13 @@ def _load_import_helper(


def _parse_and_fold_ast(file: FileInput) -> vy_ast.VyperNode:
ret = vy_ast.parse_to_ast(
ast = vy_ast.parse_to_ast(
Copy link
Member

Choose a reason for hiding this comment

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

i guess since folding isn't performed any longer we can kind of skip the intermediate variable assignment (and rename the function!)

Copy link
Member

Choose a reason for hiding this comment

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

i just reverted the change since it's not relevant to this PR

loop_var_annotations[k]["vy_ast"] = loop_var_vy_ast
del loop_var_annotations[k]["parsed_ast"]

module._metadata["loop_var_annotations"] = loop_var_annotations
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 we need to attach this into the metadata tbh. this should all be handled before analysis, and ideally before we even get to vyper AST construction.

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 we can accomplish all this just with adding a visit_For into the visitor in this file

@charles-cooper charles-cooper changed the title feat: loop variable annotation feat: require annotations of loop variables Jan 7, 2024
@charles-cooper charles-cooper enabled auto-merge (squash) January 7, 2024 18:16
@charles-cooper charles-cooper changed the title feat: require annotations of loop variables feat: require type annotations for loop variables Jan 7, 2024
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) January 7, 2024 18:22
@charles-cooper charles-cooper merged commit ddfce52 into vyperlang:master Jan 7, 2024
84 checks passed
@tserg tserg deleted the feat/loop_var_annotation2 branch January 8, 2024 02:40
DanielSchiavini added a commit to DanielSchiavini/vyper-plugin that referenced this pull request Jul 9, 2024
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.

VIP: require loop variable annotation
3 participants