Block scoping #601
Block scoping #601
Conversation
viper/parser/parser.py
Outdated
|
|
||
| def end_blockscope(self, blockscope_id): | ||
| if blockscope_id not in self.blockscopes: | ||
| return |
There was a problem hiding this comment.
Shouldn't this raise an error?
There was a problem hiding this comment.
I could probably remove that check completely?
There was a problem hiding this comment.
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.
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.
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 blockscope_id's that will be in self.blockscopes
There was a problem hiding this comment.
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.
You need to pop scopes from the list to prevent this from compiling:
def foo(b: bool):
if b:
a: num = 1
a += 2There was a problem hiding this comment.
@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.
Did for me, will be reworking this PR today ;)
viper/parser/parser.py
Outdated
| 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.
Simplify to self.blockscopes.pop(blockscope_id)
There was a problem hiding this comment.
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.
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 endedThere was a problem hiding this comment.
Looking at it now pop would be 100%, was just my thought process at that moment... hahaha 😸
| 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.
What does self.stmt.test do?
There was a problem hiding this comment.
@DavidKnott I believe it's basically the comparison expression or statement part: if (<test>)
viper/parser/parser.py
Outdated
|
|
||
| def end_blockscope(self, blockscope_id): | ||
| if blockscope_id not in self.blockscopes: | ||
| return |
There was a problem hiding this comment.
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 blockscope_id's that will be in self.blockscopes
|
@jacqueswww Tests look good! |
viper/parser/parser.py
Outdated
|
|
||
| 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: |
There was a problem hiding this comment.
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.
👍 yeah will do so, used the same for in-for-loop ;) as stated in the other comment I thought I required a sliceabke.
|
@DavidKnott I believe this one is ready now too :) |
- What I did
Partial PR for #545. I chose to separate this part for easier review.
The
a:num =1annotate assign will probably be a big onslaught of changes which I would rather keep separate from block scoping.- How I did it
Add a
blockscopeslist to context. Using thestart_blockscopeandend_blockscopemethod to set the start and end of a blockscope forforandifblocks.- How to verify it
Check the tests ;) (
test_blockscope.py).- Description for the changelog
- Cute Animal Picture