vyper icon indicating copy to clipboard operation
vyper copied to clipboard

fix: reorder AST folding after semantics validation

Open tserg opened this issue 3 years ago • 14 comments

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_assignable flag to BaseTypeDefinition.
  • 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

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

tserg avatar Apr 30 '22 16:04 tserg

Codecov Report

Merging #2832 (2001f11) into master (f31f0ec) will decrease coverage by 0.18%. The diff coverage is 83.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.

codecov-commenter avatar Apr 30 '22 16:04 codecov-commenter

it is also very weird to see typechecking code in AST construction. is it possible to move these to the constant folder module?

charles-cooper avatar May 04 '22 06:05 charles-cooper

looks like there is some code that is duplicated in structure. is there some way we can refactor _validate_numeric_bounds to address these cases?

I have moved the typechecking to folding.py so it only runs after _validate_numeric_bounds for BinOp and Compare nodes.

tserg avatar May 05 '22 07:05 tserg

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

lgtm-com[bot] avatar Jun 10 '22 03:06 lgtm-com[bot]

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

lgtm-com[bot] avatar Jun 10 '22 04:06 lgtm-com[bot]

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

lgtm-com[bot] avatar Jun 10 '22 08:06 lgtm-com[bot]

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.

tserg avatar Jun 11 '22 09:06 tserg

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]

tserg avatar Jun 13 '22 05:06 tserg

This pull request introduces 1 alert when merging 108449ee2ab65c66cdf0012e33bf0ba4faef0270 into 573d77f7af177fb3bf2be2a14d16e3b6c477a0fc - view on LGTM.com

new alerts:

  • 1 for Unused import

lgtm-com[bot] avatar Jul 06 '22 15:07 lgtm-com[bot]

This pull request introduces 1 alert when merging 4d0d1ed2bea5eed6c3640ddd68fb49c421445a96 into 573d77f7af177fb3bf2be2a14d16e3b6c477a0fc - view on LGTM.com

new alerts:

  • 1 for Unreachable code

lgtm-com[bot] avatar Jul 07 '22 11:07 lgtm-com[bot]

This pull request introduces 1 alert when merging 3d2428c11dda66c131899786ab2e7f82844ecf61 into 573d77f7af177fb3bf2be2a14d16e3b6c477a0fc - view on LGTM.com

new alerts:

  • 1 for Unreachable code

lgtm-com[bot] avatar Jul 07 '22 13:07 lgtm-com[bot]

This pull request introduces 1 alert when merging 27dff52ae38d8109dc7b3dc088e7b37c74496509 into 573d77f7af177fb3bf2be2a14d16e3b6c477a0fc - view on LGTM.com

new alerts:

  • 1 for Unreachable code

lgtm-com[bot] avatar Jul 07 '22 13:07 lgtm-com[bot]

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.

Pending #2935. Tests for bounds should be replaced with this builtin once merged.

tserg avatar Jul 08 '22 01:07 tserg

This pull request introduces 1 alert when merging a308b96ddc5713d3ad6be75230af837dceb001dc into 6c363614afafc9c75579c75a39a13bbe2073a40e - view on LGTM.com

new alerts:

  • 1 for Unused import

lgtm-com[bot] avatar Jul 10 '22 06:07 lgtm-com[bot]