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

Bugfix - calls to private functions with multiple lists as parameters #1560

Merged

Conversation

iamdefinitelyahuman
Copy link
Contributor

What I did

Fixed an bug stemming from #1470 - calls to private functions that had a bytes list following an integer list of length > 1 were underflowing.

In the following example calls to bar would underflow, but bar2 would succeed:

@private
def _foo(y: int128[2], x: bytes[5]) -> int128:
    return y[0]

@private
def _foo2(x: bytes[5], y: int128[2]) -> int128:
    return y[0]

@public
def bar() -> int128:
    return self._foo([1, 2], b"hello")

@public
def bar2() -> int128:
    return self._foo2(b"hello", [1, 2])

How I did it

Used get_static_size_of_type to verify the memory size of each argument during iteration.

How to verify it

I have included a test as the first commit, if you run it without the 2nd commit you can see the bug in action.

Description for the changelog

  • Bugfix - multiple arrays as inputs in a private function

Cute Animal Picture

image

@iamdefinitelyahuman iamdefinitelyahuman changed the title Private multiple lists Bugfix - calls to private functions with multiple lists as parameters Aug 3, 2019
@fubuloubu
Copy link
Member

Only python 3.6 build failed, with the following message:

E           hypothesis.errors.Flaky: Hypothesis test_sqrt_valid_range(sqrt_contract=<tests.base_conftest.VyperContract at 0x7fea210e60b8>, value=Decimal('1760211996687216922475948105605887618.3833309761')) produces unreliable results: Falsified on the first call but did not on a subsequent one

Retriggering to see if results repeat.

@iamdefinitelyahuman
Copy link
Contributor Author

@fubuloubu I get a similar fail output on that test about 20% of the time when I run tests locally, including on master 🤷‍♂️ From the output it seems it's just failing because the test takes too long to finish, so I've been ignoring it.

@charles-cooper
Copy link
Member

@fubuloubu I get a similar fail output on that test about 20% of the time when I run tests locally, including on master man_shrugging From the output it seems it's just failing because the test takes too long to finish, so I've been ignoring it.

I think I've run into a similar bug. I think we need to increase the timeout on that test to like 1s (currently 400ms I believe).

if isinstance(arg.typ, ByteArrayLike):
last_idx = idx
idx += get_static_size_of_type(arg.typ)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch. thanks

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.

The change LGTM. I found the test case confusing though, I think it will be clearer if you just use the test from the PR description - unless this tests something that is not in the PR description and I am missing something. Thanks.

@iamdefinitelyahuman
Copy link
Contributor Author

My goal in the test was to ensure I had many lists with length > 1, and started one with a bytes list and another with a not-bytes list. In hindsight yes, it's maybe a bit overkill. Will update.

@iamdefinitelyahuman
Copy link
Contributor Author

@charles-cooper i've revised the test per your comments

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.

LGTM - thanks!

@charles-cooper
Copy link
Member

@jacqueswww can I merge this?

Copy link
Contributor

@jacqueswww jacqueswww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@charles-cooper charles-cooper merged commit ec3f494 into vyperlang:master Aug 6, 2019
@iamdefinitelyahuman iamdefinitelyahuman deleted the private-multiple-lists branch August 8, 2019 04:25
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.

4 participants