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: merge annotation.py and local.py #3456

Merged
merged 87 commits into from
Oct 19, 2023

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented May 25, 2023

What I did

Annotation at the module level is performed simultaneously with typechecking in vyper/semantics/analysis/module.py while annotation at the statement and expression level is performed in vyper/semantics/analysis/annotation.py with typechecking in vyper/semantics/analysis/local.py.

This PR merges annotation of statements and expressions into vyper/semantics/analysis/local.py for consistency with how it is done for modules, and makes it easier for other possible changes (e.g. moving some folding to after semantics analysis).

How I did it

Copy, paste and edit.

How to verify it

See tests.

Commit message

This commit merges two phases of analysis: typechecking
(`vyper/semantics/analysis/local.py`) and annotation of ast nodes with
types (`vyper/semantics/analysis/annotation.py`). This is both for
consistency with how it is done for `analysis/module.py`, and also
because it increases internal consistency, as some typechecking was
being done in `annotation.py`. It is also easier to maintain this way,
since bugfixes or modifications to typechecking need to only be done in
one place, rather than two passes. Lastly, it also probably improves
performance, because it collapses two passes into one (and calls
`get_*_type_from_node` less often).

This commit also fixes a bug with accessing the iterator when the
iterator is an empty list literal.

Description for the changelog

Cute Animal Picture

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

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2023

Codecov Report

Merging #3456 (54c1908) into master (5482bbc) will decrease coverage by 0.07%.
The diff coverage is 96.93%.

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

@@            Coverage Diff             @@
##           master    #3456      +/-   ##
==========================================
- Coverage   89.08%   89.01%   -0.07%     
==========================================
  Files          86       85       -1     
  Lines       11455    11364      -91     
  Branches     2605     2585      -20     
==========================================
- Hits        10205    10116      -89     
- Misses        830      834       +4     
+ Partials      420      414       -6     
Files Coverage Δ
vyper/builtins/_signatures.py 97.64% <100.00%> (ø)
vyper/builtins/functions.py 90.31% <100.00%> (-0.29%) ⬇️
vyper/semantics/analysis/common.py 100.00% <100.00%> (ø)
vyper/semantics/analysis/utils.py 92.04% <100.00%> (+0.07%) ⬆️
vyper/semantics/analysis/local.py 93.36% <96.74%> (+1.67%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator Author

@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.

Some comments before I forget

vyper/semantics/analysis/local.py Show resolved Hide resolved
vyper/semantics/analysis/local.py Show resolved Hide resolved
vyper/semantics/analysis/local.py Show resolved Hide resolved
@tserg
Copy link
Collaborator Author

tserg commented May 27, 2023

The mypy lints are a bit ugly, and we cannot do file level ignores yet because we are stuck on mypy 0.910 due to py-evm. Is it okay to remove the type annotations for _LocalExpressionVisitor instead?

@@ -475,6 +475,12 @@ def evaluate(self, node):

return vy_ast.Int.from_node(node, value=length)

def infer_arg_types(self, node):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

infer_arg_types of Len builtin was returning a python tuple of types.

@charles-cooper charles-cooper added this to the v0.4.0 milestone Sep 25, 2023
@charles-cooper charles-cooper changed the title refactor: merge annotation into local analysis refactor: merge annotation.py and local.py Oct 19, 2023
@charles-cooper charles-cooper changed the title refactor: merge annotation.py and local.py refactor: merge annotation.py and local.py Oct 19, 2023
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.

pushed some final changes/polishing. nice improvement to the typechecking code, great work!

@charles-cooper charles-cooper enabled auto-merge (squash) October 19, 2023 16:53
@charles-cooper charles-cooper merged commit 3ba1412 into vyperlang:master Oct 19, 2023
80 of 82 checks passed
@tserg tserg deleted the refactor/annotation branch October 20, 2023 01:51
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