Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix[lang]: allow type expressions inside pure functions #3906

Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Apr 2, 2024

#3895 introduced a regression where type expressions like the following would raise a compiler error instead of successfully compiling:

@pure
def f():
    convert(..., uint256)  # raises `not a variable or literal: 'uint256'`

the reason is because get_expr_info is called on uint256, which is not a regular expr. this commit introduces a fastpath return to address the issue. longer-term, we should generalize the rule so that ast traversal does not progress into type expressions.

What I did

How I did it

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

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

20432c5 introduced a regression where type expressions like the
following would raise a compiler error instead of successfully
compiling:
```
@pure
def f():
    convert(..., uint256)  # raises `not a variable or literal: 'uint256'`
```

the reason is because `get_expr_info` is called on `uint256`, which
is not a regular expr. this commit introduces a fastpath return to
address the issue. longer-term, we should generalize the rule so that
ast traversal does not progress into type expressions.
Copy link
Collaborator

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

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

Tested all snekmate contracts and they passed successfully.


[UNRELATED TO THIS PR; ADDED FOR DOCUMENTATION PURPOSES]

While reviewing this PR, I realised something else weird in the NatSpec output of the compiler. The __init__ function is ignored.

Example to reproduce:

vyper src/snekmate/governance/mocks/TimelockControllerMock.vy -f devdoc,userdoc

will produce:

{"title": "TimelockController Module Reference Implementation", "custom:contract-name": "TimelockControllerMock", "license": "GNU Affero General Public License v3.0 only", "author": "pcaversaccio"}

But it completely ignores my NatSpec comments in the ctor (see here). I opened an issue for tracking #3907.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.88%. Comparing base (9db9078) to head (84ca3cf).

❗ Current head 84ca3cf differs from pull request most recent head 39c3981. Consider uploading reports for the commit 39c3981 to get more accurate results

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3906      +/-   ##
==========================================
- Coverage   90.92%   88.88%   -2.04%     
==========================================
  Files          95       95              
  Lines       14386    14389       +3     
  Branches     3185     3187       +2     
==========================================
- Hits        13080    12790     -290     
- Misses        904     1138     +234     
- Partials      402      461      +59     

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

@charles-cooper charles-cooper enabled auto-merge (squash) April 3, 2024 12:40
@charles-cooper charles-cooper merged commit 45a225c into vyperlang:master Apr 3, 2024
148 checks passed
electriclilies pushed a commit to electriclilies/vyper that referenced this pull request Apr 27, 2024
20432c5 introduced a regression where type expressions like the
following would raise a compiler error instead of successfully
compiling:
```
@pure
def f():
    convert(..., uint256)  # raises `not a variable or literal: 'uint256'`
```

the reason is because `get_expr_info` is called on `uint256`, which
is not a regular expr. this commit introduces a fastpath return to
address the issue. longer-term, we should generalize the rules in
`vyper/semantics/analysis/local.py` so that AST traversal does not
progress into type expressions.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants