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

https://github.com/ethereum/vyper/blob/089d0c4046a5e71412b46205788a89e76d7646ea/vyper/compile_lll.py#L273-L275

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
Copy link
Member

@fubuloubu fubuloubu commented Sep 22, 2019

Would this have been caught with MyPy?

@iamdefinitelyahuman
Copy link
Contributor Author

@iamdefinitelyahuman 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
Copy link
Collaborator

@charles-cooper 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 charles-cooper left a comment

@iamdefinitelyahuman
Copy link
Contributor Author

@iamdefinitelyahuman 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):
Copy link
Collaborator

@charles-cooper charles-cooper Sep 23, 2019

Choose a reason for hiding this comment

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

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):
Copy link
Collaborator

@charles-cooper charles-cooper Sep 23, 2019

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator

@charles-cooper charles-cooper left a comment

LGTM, will merge once CI passes

@charles-cooper charles-cooper merged commit efdc6e2 into vyperlang:master Sep 23, 2019
3 checks passed
@iamdefinitelyahuman iamdefinitelyahuman deleted the 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants