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

chore: improve diagnostics for invalid for loop annotation #3721

Merged
merged 13 commits into from
Jan 11, 2024

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Jan 8, 2024

What I did

Improve diagnostics for invalid loop variable annotation and add tests for #3596.

Compiling this contract throws an error that does not make sense

@external
def foo():
    for i: uint9 in [1, 2, 3]:
        pass
vyper.exceptions.UnknownType: No builtin or user-defined type named '@exte'. 
  contract "tmp/old_for.vy:1", function "foo", line 1:0 
  ---> 1 @external
  -------^
       2 def foo():

This PR changes the error message to:

vyper.exceptions.UnknownType: No builtin or user-defined type named 'uint9'. Did you mean 'uint96', or maybe 'uint8'?
  contract "tmp/old_for.vy:3", function "foo", line 3:4 
       2 def foo():
  ---> 3     for i: uint9 in [1, 2, 3]:
  -----------^
       4         pass

How I did it

Propagate the annotation source code and For node locations to the annotation and all its child nodes

How to verify it

See tests.

Commit message

chore: improve diagnostics for invalid for loop annotation

Description for the changelog

Improve diagnostics for invalid for loop annotation

Cute Animal Picture

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

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2024

Codecov Report

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

Comparison is base (ddfce52) 84.22% compared to head (1f476fc) 84.21%.
Report is 1 commits behind head on master.

Files Patch % Lines
vyper/ast/pre_parser.py 83.33% 1 Missing and 1 partial ⚠️
vyper/ast/parse.py 95.65% 0 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3721      +/-   ##
==========================================
- Coverage   84.22%   84.21%   -0.01%     
==========================================
  Files          92       92              
  Lines       13073    13091      +18     
  Branches     2916     2921       +5     
==========================================
+ Hits        11011    11025      +14     
- Misses       1644     1646       +2     
- Partials      418      420       +2     

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

# this step improves the diagnostics during semantics analysis as otherwise
# the code displayed in the error message would be incorrectly based on the
# full source code with the location of the type annotation as an expression
annotation_children = python_ast.iter_child_nodes(node.target.annotation)
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 use python_ast.walk here

n.node_source_code = raw_annotation

# override the locations to show the `For` node
python_ast.copy_location(n, node)
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 python_ast.increment_lineno might be better here

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 new tests.

i think the annotation is a bit tricky to get right, since you're copying the location of the parent for node into the for target. here's a (contrived) example of where that doesn't work so well:

@external
def foo():
    for i: \
        \
        \
        \
        uint9 in [1, 2, 3]:
        pass

with the current implementation that results in an error message something like

vyper.exceptions.UnknownType: No builtin or user-defined type named 'uint9'. Did you mean 'uint96', or maybe 'uint8'?
  contract "repros/3721.vy:3", function "foo", line 3:4 
       2 def foo():
  ---> 3     for i: \
  -----------^
       4         \

i think what we really want to do is go into the tokens generated by ASTTokens (https://asttokens.readthedocs.io/en/latest/api-index.html#asttokens.ASTTokens.tokens) and modify all of them to have the correct start/end offsets.

vyper/ast/parse.py Fixed Show fixed Hide fixed
vyper/ast/parse.py Fixed Show fixed Hide fixed
@charles-cooper charles-cooper enabled auto-merge (squash) January 11, 2024 22:41
@charles-cooper
Copy link
Member

i have a solution for #3721 (review). i'll submit it in a follow up PR.

@charles-cooper charles-cooper merged commit a6dc432 into vyperlang:master Jan 11, 2024
84 checks passed
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