vyper
vyper copied to clipboard
fix: reorder AST folding after semantics validation
What I did
Fix #2830, and stricter type safety for literal operations.
How I did it
- Reorder AST folding to after semantics validation.
- Added
not_assignableflag toBaseTypeDefinition. - Added annotation of constants, and helper functions to derive foldable values where required for semantics validation before AST nodes are folded.
How to verify it
See new tests, and updated tests.
Commit message
fix: reorder AST folding after semantics validation
Fixes an issue with type-checking and bounds-check of literal ops.
With the reordering, a `not_assignable` flag has been added to
`BaseTypeDefinition`, aligning with the intent behind `check_kwargable`.
This allows true constants (builtin or user-defined) to be differentiated
from calldata arguments which are not assignable, but not constants.
Helper functions have also been added to support the derivation of foldable
values for semantics validation and to determine whether a node is foldable
prior to their AST folding.
Fix #2830
Description for the changelog
Type-check and bounds-check literal ops involving literals from constant folding.
Cute Animal Picture

Codecov Report
Merging #2832 (2001f11) into master (f31f0ec) will decrease coverage by
0.18%. The diff coverage is83.09%.
@@ Coverage Diff @@
## master #2832 +/- ##
==========================================
- Coverage 88.29% 88.11% -0.19%
==========================================
Files 97 97
Lines 10919 10988 +69
Branches 2583 2558 -25
==========================================
+ Hits 9641 9682 +41
- Misses 830 864 +34
+ Partials 448 442 -6
| Impacted Files | Coverage Δ | |
|---|---|---|
| vyper/semantics/types/function.py | 87.12% <ø> (ø) |
|
| vyper/semantics/types/indexable/mapping.py | 96.42% <ø> (ø) |
|
| vyper/semantics/types/value/address.py | 100.00% <ø> (ø) |
|
| vyper/semantics/types/indexable/sequence.py | 87.23% <50.00%> (-0.71%) |
:arrow_down: |
| vyper/builtin_functions/functions.py | 90.68% <69.82%> (+0.28%) |
:arrow_up: |
| vyper/semantics/validation/annotation.py | 91.25% <70.00%> (+1.58%) |
:arrow_up: |
| vyper/semantics/types/utils.py | 88.35% <82.60%> (-1.41%) |
:arrow_down: |
| vyper/semantics/validation/local.py | 91.34% <83.33%> (+0.13%) |
:arrow_up: |
| vyper/semantics/validation/utils.py | 90.00% <88.23%> (-1.35%) |
:arrow_down: |
| vyper/ast/folding.py | 87.90% <100.00%> (-4.72%) |
:arrow_down: |
| ... and 18 more |
Help us with your feedback. Take ten seconds to tell us how you rate us.
it is also very weird to see typechecking code in AST construction. is it possible to move these to the constant folder module?
looks like there is some code that is duplicated in structure. is there some way we can refactor
_validate_numeric_boundsto address these cases?
I have moved the typechecking to folding.py so it only runs after _validate_numeric_bounds for BinOp and Compare nodes.
This pull request introduces 1 alert when merging bc4ab0887b2f5727bf736a325b083e68dd95e6a0 into b8dea0cec9b4a4a07b70f28aa164894b98d4ff80 - view on LGTM.com
new alerts:
- 1 for Wrong number of arguments in a call
This pull request introduces 1 alert when merging 129835d1a7a1ea970c51af9f3ff396d376b1a417 into b8dea0cec9b4a4a07b70f28aa164894b98d4ff80 - view on LGTM.com
new alerts:
- 1 for Wrong number of arguments in a call
This pull request introduces 1 alert when merging ff92f85b5d90dc9fb2467f7775f403394148f476 into b8dea0cec9b4a4a07b70f28aa164894b98d4ff80 - view on LGTM.com
new alerts:
- 1 for Wrong number of arguments in a call
Note: We may want to add builtin constants for MAX_UINT, MIN_INT and MAX_INT for all integer types after this change. Otherwise, it is difficult to assign the min/max value in an easy way because the exponentiation value will be out of bounds.
Seems like there is regression in bytecode size - e.g.
@external
def foo() -> uint256:
a: uint256 = 2 ** 200
b: uint256 = 5
return max(a, 5) + max(b, 5)
@external
def goo() -> uint256:
return min(3, 5) + max(40, 80)
[update: seems to have been resolved as of de97286]
This pull request introduces 1 alert when merging 108449ee2ab65c66cdf0012e33bf0ba4faef0270 into 573d77f7af177fb3bf2be2a14d16e3b6c477a0fc - view on LGTM.com
new alerts:
- 1 for Unused import
This pull request introduces 1 alert when merging 4d0d1ed2bea5eed6c3640ddd68fb49c421445a96 into 573d77f7af177fb3bf2be2a14d16e3b6c477a0fc - view on LGTM.com
new alerts:
- 1 for Unreachable code
This pull request introduces 1 alert when merging 3d2428c11dda66c131899786ab2e7f82844ecf61 into 573d77f7af177fb3bf2be2a14d16e3b6c477a0fc - view on LGTM.com
new alerts:
- 1 for Unreachable code
This pull request introduces 1 alert when merging 27dff52ae38d8109dc7b3dc088e7b37c74496509 into 573d77f7af177fb3bf2be2a14d16e3b6c477a0fc - view on LGTM.com
new alerts:
- 1 for Unreachable code
Note: We may want to add builtin constants for
MAX_UINT,MIN_INTandMAX_INTfor all integer types after this change. Otherwise, it is difficult to assign the min/max value in an easy way because the exponentiation value will be out of bounds.
Pending #2935. Tests for bounds should be replaced with this builtin once merged.
This pull request introduces 1 alert when merging a308b96ddc5713d3ad6be75230af837dceb001dc into 6c363614afafc9c75579c75a39a13bbe2073a40e - view on LGTM.com
new alerts:
- 1 for Unused import