-
-
Notifications
You must be signed in to change notification settings - Fork 810
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
Block scoping #601
Block scoping #601
Changes from 4 commits
c662e3b
2e1a62d
9e7d5aa
61b2de4
2248773
226f852
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
|
||
import pytest | ||
from pytest import raises | ||
|
||
from viper import compiler | ||
from viper.exceptions import VariableDeclarationException | ||
|
||
|
||
fail_list = [ | ||
""" | ||
@public | ||
def foo(choice: bool): | ||
if (choice): | ||
a = 1 | ||
a += 1 | ||
""", | ||
""" | ||
@public | ||
def foo(choice: bool): | ||
if (choice): | ||
a = 0 | ||
else: | ||
a = 1 | ||
a += 1 | ||
""", | ||
""" | ||
@public | ||
def foo(choice: bool): | ||
if (choice): | ||
a = 0 | ||
else: | ||
a += 1 | ||
""", | ||
""" | ||
@public | ||
def foo(choice: bool): | ||
|
||
for i in range(4): | ||
a = 0 | ||
a += 1 | ||
""", | ||
""" | ||
@public | ||
def foo(choice: bool): | ||
|
||
for i in range(4): | ||
a = 0 | ||
a += 1 | ||
""", | ||
""" | ||
a: num | ||
|
||
@public | ||
def foo(): | ||
a = 5 | ||
""", | ||
] | ||
|
||
|
||
@pytest.mark.parametrize('bad_code', fail_list) | ||
def test_fail_(bad_code): | ||
|
||
with raises(VariableDeclarationException): | ||
compiler.compile(bad_code) | ||
|
||
|
||
valid_list = [ | ||
""" | ||
@public | ||
def foo(choice: bool, choice2: bool): | ||
if (choice): | ||
a = 11 | ||
if choice2 and a > 1: | ||
a -= 1 # should be visible here. | ||
""" | ||
] | ||
|
||
|
||
@pytest.mark.parametrize('good_code', valid_list) | ||
def test_valid_blockscope(good_code): | ||
assert compiler.compile(good_code) is not None |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -277,13 +277,31 @@ def __init__(self, vars=None, globals=None, sigs=None, forvars=None, return_type | |
self.in_for_loop = set() | ||
# Count returns in function | ||
self.function_return_count = 0 | ||
# Current blockscope | ||
self.blockscopes = [] | ||
|
||
def set_in_for_loop(self, name_of_list): | ||
self.in_for_loop.add(name_of_list) | ||
|
||
def remove_in_for_loop(self, name_of_list): | ||
self.in_for_loop.remove(name_of_list) | ||
|
||
def start_blockscope(self, blockscope_id): | ||
if blockscope_id not in self.blockscopes: | ||
self.blockscopes.append(blockscope_id) | ||
|
||
def end_blockscope(self, blockscope_id): | ||
if blockscope_id not in self.blockscopes: | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this raise an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could probably remove that check completely? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, I mean this is an internal thing you added to keep track of blockscopes, which is something Python already does with the AST module to prevent uses from changing it arbitrarily. Wait, does AST provide any information for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I am aware no, since python doesn't use block scoping like we want to. I'll remove it - just a bit of 'defensive coding' ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think AST will help here as it only parses with python scoping. I think this check can be removed given that we're only calling it internally and inputting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Python does scoping, at least behind the scenes. All languages do some aspect of scoping. Python specifically tracks indenting to figure out block scoping. This might be handled in the lexer and not made present in the AST, but it's definitely in the Python front-end parser somewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to pop scopes from the list to prevent this from compiling: def foo(b: bool):
if b:
a: num = 1
a += 2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jacqueswww I don't think the above example would compile even with the aforementioned check removed. Does it currently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did for me, will be reworking this PR today ;) |
||
# Remove all variables that have specific blockscope_id attached. | ||
poplist = [] | ||
for name, var_record in self.vars.items(): | ||
if blockscope_id in var_record.blockscopes: | ||
poplist.append(name) | ||
for name in poplist: self.vars.pop(name) | ||
# Remove blockscopes | ||
self.blockscopes = self.blockscopes[self.blockscopes.index(blockscope_id):] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplify to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pop would probably give the desired effect. My idea when I coded was: If you have nested scopes, you want to slice awat all nested scopes as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, that's an interesting thought. but the blockscopes should still both appear in order so I don't think this would happen e.g. for i in range(5): # "for" block started
# "for" body
if i == 3: # "if" block started
# "if" body
return i
# "if" block ended
# "for" block ended There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at it now pop would be 100%, was just my thought process at that moment... hahaha 😸 |
||
|
||
def increment_return_counter(self): | ||
self.function_return_count += 1 | ||
|
||
|
@@ -293,7 +311,7 @@ def new_variable(self, name, typ): | |
raise VariableDeclarationException("Variable name invalid or reserved: " + name) | ||
if name in self.vars or name in self.globals: | ||
raise VariableDeclarationException("Duplicate variable name: %s" % name) | ||
self.vars[name] = VariableRecord(name, self.next_mem, typ, True) | ||
self.vars[name] = VariableRecord(name, self.next_mem, typ, True, self.blockscopes.copy()) | ||
pos = self.next_mem | ||
self.next_mem += 32 * get_size_of_type(typ) | ||
return pos | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,11 +100,23 @@ def parse_if(self): | |
from .parser import ( | ||
parse_body, | ||
) | ||
|
||
if self.stmt.orelse: | ||
block_scope_id = id(self.stmt.orelse) | ||
self.context.start_blockscope(block_scope_id) | ||
add_on = [parse_body(self.stmt.orelse, self.context)] | ||
self.context.end_blockscope(block_scope_id) | ||
else: | ||
add_on = [] | ||
return LLLnode.from_list(['if', Expr.parse_value_expr(self.stmt.test, self.context), parse_body(self.stmt.body, self.context)] + add_on, typ=None, pos=getpos(self.stmt)) | ||
|
||
block_scope_id = id(self.stmt) | ||
self.context.start_blockscope(block_scope_id) | ||
o = LLLnode.from_list( | ||
['if', Expr.parse_value_expr(self.stmt.test, self.context), parse_body(self.stmt.body, self.context)] + add_on, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DavidKnott I believe it's basically the comparison expression or statement part: |
||
typ=None, pos=getpos(self.stmt) | ||
) | ||
self.context.end_blockscope(block_scope_id) | ||
return o | ||
|
||
def call(self): | ||
from .parser import ( | ||
|
@@ -179,6 +191,8 @@ def parse_for(self): | |
len(self.stmt.iter.args) not in (1, 2): | ||
raise StructureException("For statements must be of the form `for i in range(rounds): ..` or `for i in range(start, start + rounds): ..`", self.stmt.iter) # noqa | ||
|
||
block_scope_id = id(self.stmt.orelse) | ||
self.context.start_blockscope(block_scope_id) | ||
# Type 1 for, eg. for i in range(10): ... | ||
if len(self.stmt.iter.args) == 1: | ||
if not isinstance(self.stmt.iter.args[0], ast.Num): | ||
|
@@ -206,6 +220,7 @@ def parse_for(self): | |
o = LLLnode.from_list(['repeat', pos, start, rounds, parse_body(self.stmt.body, self.context)], typ=None, pos=getpos(self.stmt)) | ||
del self.context.vars[varname] | ||
del self.context.forvars[varname] | ||
self.context.end_blockscope(block_scope_id) | ||
return o | ||
|
||
def _is_list_iter(self): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will blockscopes contain only distinct elements? If so, then consider changing it (self.blockscopes) into a set for hashmap complexity, O(log(n)) lookup time for the "not in" lookups (this expression degrades into a O(n) operation). More on this:
https://juliank.wordpress.com/2008/04/29/python-speed-x-in-list-vs-x-in-set/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 yeah will do so, used the same for in-for-loop ;) as stated in the other comment I thought I required a sliceabke.