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 commented Sep 2, 2019

What I did

  • Fixed #1469
  • Fixed #1466 (comment)
  • Added a type check for integer arrays, prior to this PR the following code would compile:
@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

This comment has been minimized.

Copy link
Collaborator

charles-cooper commented Sep 3, 2019

I like this code, but note that the method for typechecking is (slightly) different than the approach in ann_assign:

vyper/vyper/parser/stmt.py

Lines 141 to 151 in ab39d4e

# Check that the integer literal, can be assigned to uint256 if necessary.
elif (self.stmt.annotation.id, sub.typ.typ) == ('uint256', 'int128') and sub.typ.is_literal:
if not SizeLimits.in_bounds('uint256', sub.value):
raise InvalidLiteralException(
'Invalid uint256 assignment, value not in uint256 range.', self.stmt
)
elif self.stmt.annotation.id != sub.typ.typ and not sub.typ.unit:
raise TypeMismatchException(
'Invalid type %s, expected: %s' % (sub.typ.typ, self.stmt.annotation.id),
self.stmt,
)

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

This comment has been minimized.

Copy link
Collaborator

charles-cooper commented Sep 3, 2019

linking #764 (comment)

@iamdefinitelyahuman

This comment has been minimized.

Copy link
Contributor Author

iamdefinitelyahuman commented Sep 3, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

iamdefinitelyahuman commented Sep 4, 2019

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

This comment has been minimized.

Copy link
Collaborator

charles-cooper commented Sep 9, 2019

LGTM

@charles-cooper charles-cooper merged commit e9c8a4c into ethereum:master Sep 9, 2019
3 checks passed
3 checks passed
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
@iamdefinitelyahuman iamdefinitelyahuman deleted the iamdefinitelyahuman:type-checks branch Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.