Return statements check #1384
Return statements check #1384
Conversation
…king balanced return statements.
|
@charles-cooper have a look, I think this looks pretty decent! :) |
|
@fubuloubu @charles-cooper I have fixed all above comments, I think it's ready to merge. Dead code discussion goes to an issue instead. |
vyper/parser/parser_utils.py
Outdated
| @@ -709,6 +712,23 @@ def make_setter(left, right, location, pos, in_function_call=False): | |||
| raise Exception("Invalid type for setters") | |||
|
|
|||
|
|
|||
| def is_return_from_function(node: Union[ast.AST, List[Any]]) -> bool: | |||
| is_selfdesctruct = ( | |||
There was a problem hiding this comment.
is_selfdesctruct -> is_selfdestruct
|
Could we add a couple tests for the single exit checker? Here's a couple examples which should fail the single exit checker def test1() :
return
return
def test2() :
return
selfdestruct(msg.sender)
def test3() -> int128 :
if block.number % 2 == 1 :
return 1
else :
raise "even block number"
return 0 |
|
Although at this point, the single exit checker looks so much like a dead code checker that we could easily strengthen it to catch more dead code conditions, e.g. def foo() :
return 1
self.x += 1I believe the single exit checker only needs to be modified to read def check_return_body(self, node, node_list):
for i, n in enumerate(node_list) :
if is_return_from_function(n) and i + 1 < len(node_list) :
raise StructureException(
f'Code cannot be reached after return, raise or selfdestruct',
node
)Other kinds of dead code checking are still out of scope (especially involving branches, e.g. the following) if False :
<body> |
|
@fubuloubu OK I think all of the above with additional steps as highlighted by @charles-cooper, I will add them :) |
|
I resolved all my comments. It seems like the test cases will be rehashed in #1390, which is not ideal to me but it's okay. |
|
@charles-cooper @fubuloubu I added the check for succeeding code after exit statements, Will fail. I leave the check for type checking of selfdestruct & "return None" for #1390. :) |
|
Awesome! I think the only thing left which is bugging me is the variable named |
|
Perfect, will fix then merge hehe. |
What I did
Fixes #590, and indirectly fixes #1383 .
Special thanks to @yzhang90 this is working really well 💯 👌
How I did it
See discussion in #590.
How to verify it
Check tests.
Description for the changelog
Cute Animal Picture