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

refactor: for loop target parsing #3724

Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Jan 11, 2024

What I did

use a parser trick to get always-correct source locations into for-loop target annotations.

follow-up to #3721; reverts the AnnAssign pre-parser change from there.

How I did it

see commit

How to verify it

check new test will not pass without the parser changes.

Commit message

use a parser trick to get always-correct source locations into for-loop
target annotations.

this fixes an issue with parsing the for loop target (introduced in
ddfce5273b3), where the target annotation is not decorated with the
correct line/column offsets. a6dc432db2df partially fixed the issue, but
does not decorate correctly when the annotation is split across multiple
lines. this commit approaches the problem differently, by using
`untokenize()` to produce a source code for the annotation during AST
massaging which "happens" to have the annotation in exactly the same
place as it appeared in the original source.

small refactors:
- move annotate_python_ast to be earlier in the file. it's somehow
  easier to navigate to it when it comes right before the definition of
  AnnotatingVisitor.
- revert changes from a6dc432db2df to ast/pre_parser.py; go back to
  slurping just the annotation as opposed to the target+annotation
  together.

Description for the changelog

Cute Animal Picture

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

use a parser trick to get always-correct source locations into for-loop
target annotations.
@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a6dc432) 84.11% compared to head (4dcb201) 84.16%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3724      +/-   ##
==========================================
+ Coverage   84.11%   84.16%   +0.05%     
==========================================
  Files          92       92              
  Lines       13123    13106      -17     
  Branches     2926     2922       -4     
==========================================
- Hits        11038    11031       -7     
+ Misses       1663     1656       -7     
+ Partials      422      419       -3     

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

Copy link
Collaborator

@tserg tserg left a comment

Choose a reason for hiding this comment

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

This is truly black magic.

I tried it with a nested error and it works too:

@external
def foo():
    for i: DynArray[\
        uint256, 3] in [[1], [2], [3]]:
        for j: \
            uint247 in i:
            pass

vyper.exceptions.UnknownType: No builtin or user-defined type named 'uint247'. Did you mean 'uint24', or maybe 'uint240'?
  contract "tmp/old_for3.vy:7", function "foo", line 7:12 
       6         for j: \
  ---> 7             uint247 in i:
  -------------------^
       8

vyper/ast/pre_parser.py Outdated Show resolved Hide resolved
it's somehow easier to navigate to it when it comes right before the
definition of AnnotatingVisitor.
@charles-cooper charles-cooper merged commit 07ab92f into vyperlang:master Jan 12, 2024
82 checks passed
@charles-cooper charles-cooper changed the title refactor: for target parsing refactor: for loop target parsing Jan 12, 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.

4 participants