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 incorrect compile_to_assembly call args #1619

Merged
merged 3 commits into from Sep 23, 2019

Conversation

@iamdefinitelyahuman
Copy link
Contributor

iamdefinitelyahuman commented Sep 22, 2019

The Issue

As discovered by @michwill and reported on Gitter, the following code was not compiling due to an assertion error:

@public    
def g():
    ok: bool = False
    for i in range(5):
        assert ok, "Reasons"

The issue was in vyper/compile_lll.py - several calls to compile_to_assembly were not including the existing_labels argument:

vyper/vyper/compile_lll.py

Lines 273 to 275 in 089d0c4

o = compile_to_assembly(code.args[0], withargs, break_dest, height)
mem_start = compile_to_assembly(code.args[1], withargs, break_dest, height)
mem_len = compile_to_assembly(code.args[2], withargs, break_dest, height)

What I did

Added existing_labels as an argument in compile_to_assembly calls where it was missing.

How to verify it

See the new test cases in tests/parser/features/test_assert.py. I also added checks for proper behavior of the iterated assertions.

Cute Animal Picture

image

@fubuloubu

This comment has been minimized.

Copy link
Member

fubuloubu commented Sep 22, 2019

Would this have been caught with MyPy?

@iamdefinitelyahuman

This comment has been minimized.

Copy link
Contributor Author

iamdefinitelyahuman commented Sep 22, 2019

Would this have been caught with MyPy?

Probably - existing_labels is a set, break_dest is a tuple.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

charles-cooper commented Sep 23, 2019

Nice fix, thanks. While we're at it, could you please change the assertions to if <fail>: raise CompilerPanic('...')? Apparently some people run python with assertions turned off, plus the CompilerPanic has a more helpful error message.

Copy link
Collaborator

charles-cooper left a comment

These two asserts could be CompilerPanic

assert isinstance(withargs, dict)
if existing_labels is None:
existing_labels = set()
assert isinstance(existing_labels, set)

@iamdefinitelyahuman

This comment has been minimized.

Copy link
Contributor Author

iamdefinitelyahuman commented Sep 23, 2019

@charles-cooper good call 👍 see new commit

@@ -88,11 +91,13 @@ def apply_line_no_wrapper(*args, **kwargs):
def compile_to_assembly(code, withargs=None, existing_labels=None, break_dest=None, height=0):
if withargs is None:
withargs = {}
assert isinstance(withargs, dict)
elif not isinstance(withargs, dict):

This comment has been minimized.

Copy link
@charles-cooper

charles-cooper Sep 23, 2019

Collaborator

kind of a nitpick but to preserve the semantics I think these should be standalone if statements, not elif statements. Somebody could come in later and change the body of the previous branch to read withargs = [] and then we would no longer have the guarantee after this line that withargs is a dict.


if existing_labels is None:
existing_labels = set()
assert isinstance(existing_labels, set)
elif not isinstance(existing_labels, set):

This comment has been minimized.

Copy link
@charles-cooper

charles-cooper Sep 23, 2019

Collaborator

ditto

@iamdefinitelyahuman iamdefinitelyahuman force-pushed the iamdefinitelyahuman:compile-lll-bug branch from e83a6e5 to 7f63da6 Sep 23, 2019
Copy link
Collaborator

charles-cooper left a comment

LGTM, will merge once CI passes

@charles-cooper charles-cooper merged commit efdc6e2 into ethereum:master Sep 23, 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:compile-lll-bug branch Nov 4, 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.