fix: raise UNREACHABLE #3194
fix: raise UNREACHABLE #3194
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3194 +/- ##
==========================================
+ Coverage 88.43% 88.93% +0.50%
==========================================
Files 88 84 -4
Lines 11046 10587 -459
Branches 2338 2208 -130
==========================================
- Hits 9769 9416 -353
+ Misses 822 760 -62
+ Partials 455 411 -44
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…t_reason commented out block in optimizer that causes this to fail.
|
I changed the condition passed in _assert_reason in parse_Raise from None to 0. This changes the value of test_expr passed in _assert_reason(self, test_expr, msg) from None to 0 (False also works). None is not a valid value for test_expr, whereas 0 is. This is what is causing the error "None is not allowed as an IRnode" Importantly, 0 used to be passed in parse_Raise when it was first implemented. My hypothesis is that it got changed when the block that I commented out in optimizer.py got introduced. This block raises a StaticAssertion error when 0 is passed into _assert_reason. I think that when this got introduced, None must've been added to parse_Raise to handle UNREACHABLE while avoiding 0, i.e. the optimization error. I am still combing through the history to verify this, but @charles-cooper if you have memories of doing this it would be helpful; I think it was potentially somewhere around efe1dbe -- feat: more arithmetic optimizations (#2647) Anyways, I'm still going through the history to find exactly when this change happened and why, but if you remember anything regarding reasoning let me know! Also, trying to understand if we can just remove the commented out block in optimizer.py because I don't really think that asserts or raises need to be optimized; if a dev is using them, they are using them on purpose and probably don't need them to be optimized. Let me know what you think about this. Thanks! |
|
btw i found the location of the change: Still working to understand why this happened. (note: when i run the script outlined in the issue which 2509 aimed to fix (assert message strings #2449), it compiles successfully with my recent changes) |
And added new usage of raise_unreachable to parse_Raise in codegen/stmt.py Added raise_unreachable to ir/README.md And added back the block I had commented out of optimizer.py
|
Because ir/optimizer.py is correct and needs to stay as it is to catch internal compiler assert bugs: |
very nice. that's clean, and mirrors the two way split between i think adding a new IR instruction |
Okay, thanks!! |
removed new IR instruction "raise_unreachable" removed documentation for "raise_unreachable" IR
…HABLE is handled in one block
vyper/codegen/stmt.py
Outdated
| if self.stmt.msg: | ||
| return self._assert_reason(test_expr, self.stmt.msg) | ||
| else: | ||
| return IRnode.from_list(["assert", test_expr], error_msg="user assert") | ||
|
|
||
| def parse_Raise(self): | ||
| if self.stmt.exc: | ||
| if isinstance(self.stmt.exc, vy_ast.Name) and self.stmt.exc.id == "UNREACHABLE": | ||
| self.stmt.exc.id = "raise UNREACHABLE" |
There was a problem hiding this comment.
why modify the ast here?
There was a problem hiding this comment.
as a way to tag it as raise unreachable so that it doesn't get passed into IRnode.from_list(["assert_unreachable", test_expr]); otherwise by the time you get to _assert_reason there's no way that I know of to distinguish it
There was a problem hiding this comment.
that's why the test expr is passed in as None, so assert_reason can distinguish -- see the final branch in the _assert_reason() implementation.
vyper/codegen/stmt.py
Outdated
| if test_expr is not None: | ||
| return IRnode.from_list( | ||
| ["assert_unreachable", test_expr], error_msg="assert unreachable" | ||
| ) | ||
| else: | ||
| return IRnode.from_list(["invalid"], error_msg="raise unreachable") |
There was a problem hiding this comment.
nice -- since this check is performed twice, and it's actually not entirely clear what the check is for, i would factor out the expression (test_expr is None) into a variable called is_raise and add a comment about what it is doing.
There was a problem hiding this comment.
Cool! Is it possible that None can be passed into parse_Assert from an internal compiler assert? I am just thinking that if so, it may be a little misleading if it got called 'is_raise', but also it shouldn't affect behavior.
There was a problem hiding this comment.
no, parse_Assert only handles user asserts -- but if you are paranoid (which is not a bad thing!) you can add a sanity check in parse_Assert on the test expr before passing down into assert_reason().
|
this is looking good. i think we want to add a test for |
Removed duplicate line in test_basic_call_unreachable Added sanity check in parse_Assert
|
cool, thanks! implemented your suggestions. Also, I removed a line from test_basic_call_unreachable which I am pretty certain was a duplicate and not necessary. Just want to make sure to point it out to y'all in case it was indeed necessary. |
|
thanks, @trocher what do you think about this fix? |
|
This is looking good to me! |
What I did
fix #3070: added handling for raise UNREACHABLE
How I did it
How to verify it
Commit message
fix: raise UNREACHABLE
Description for the changelog
raise UNREACHABLE was generating CompilerPanic because of how parse_Raise calls _assert_reason, and then got passed to the block which handles assert_unreachable (IRnode.from_list(["assert_unreachable", test_expr])). This was breaking because None was being passed as a test_expr to assert_unreachable, which is not valid.
Added a new block in _assert_reason which handles raise UNREACHABLE and generates "INVALID" directly without going through the same path as assert_unreachable.
Cute Animal Picture