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

fix: reorder AST folding after semantics validation #2832

Closed
wants to merge 185 commits into from

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Apr 30, 2022

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

@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2022

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%) ⬇️
vyper/builtin_functions/functions.py 90.68% <69.82%> (+0.28%) ⬆️
vyper/semantics/validation/annotation.py 91.25% <70.00%> (+1.58%) ⬆️
vyper/semantics/types/utils.py 88.35% <82.60%> (-1.41%) ⬇️
vyper/semantics/validation/local.py 91.34% <83.33%> (+0.13%) ⬆️
vyper/semantics/validation/utils.py 90.00% <88.23%> (-1.35%) ⬇️
vyper/ast/folding.py 87.90% <100.00%> (-4.72%) ⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@tserg tserg marked this pull request as ready for review May 2, 2022 06:57
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.

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?

@charles-cooper
Copy link
Member

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

@tserg
Copy link
Collaborator Author

tserg commented May 5, 2022

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 tserg marked this pull request as draft June 9, 2022 04:16
@tserg tserg marked this pull request as ready for review July 13, 2022 08:02
@tserg tserg marked this pull request as draft July 28, 2022 04:18
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.

Type of integer constant is ignored during folding of literal ops
3 participants