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

Typechecks for default parameters and int arrays #1596

Merged
merged 5 commits into from
Sep 9, 2019

Conversation

iamdefinitelyahuman
Copy link
Contributor

@iamdefinitelyahuman iamdefinitelyahuman commented Sep 2, 2019

What I did

@public
def foo():
   a: uint256[2] = [-1, -1]

How I did it

  • vyper/parser/parser_utils.py in make_setter, raise InvalidLiteralException when left type is BaseType and right value is out of bounds. This works for arrays as well because make_setter recursively calls itself until the left type is reduced to a BaseType.
  • vyper/signatures/function_signature.py moved param check to validate_default_values to allow recursive calls for checking arrays

How to verify it

I added test cases for default parameters and array assignment.

Cute Animal Picture

image

@charles-cooper
Copy link
Member

I like this code, but note that the method for typechecking is (slightly) different than the approach in ann_assign: https://github.com/ethereum/vyper/blob/ab39d4e9c5168eff2646dd29e23d7212654b636d/vyper/parser/stmt.py#L141-L151

I am good merging this so long as we add a note to this effect since we should reify the two code paths at some point.

@charles-cooper
Copy link
Member

linking #764 (comment)

@iamdefinitelyahuman
Copy link
Contributor Author

@charles-cooper I think some refactoring is possible so one of these checks isn't needed. It's late here, will have a look and hopefully push more tomorrow morning.

@iamdefinitelyahuman
Copy link
Contributor Author

OK I take it back.. There are a lot of type checks happening in stmt._check_valid_assign, stmt.parse_return and parser_utils.make_setter that potentially overlap, and refactoring out the check in stmt._check_valid_assign that @charles-cooper pointed out only leads to more fragmented logic. This whole area should be refactored at some point but that's a big job. So going with the short-term fix and leaving a comment to the future refactor-er :)

@charles-cooper
Copy link
Member

LGTM

@charles-cooper charles-cooper merged commit e9c8a4c into vyperlang:master Sep 9, 2019
@iamdefinitelyahuman iamdefinitelyahuman deleted the type-checks branch September 10, 2019 02:45
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.

Default parameters do not get properly typechecked
3 participants