fix(codegen): use safe_pop for loop body lowering; use fadd/fsub for float unary inc and dec#264
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses codegen crashes and incorrect control-flow in loop lowering by ensuring loop bodies don’t underflow result_stack and by removing a guard that incorrectly treated valid “falsy” LLVM values as missing results.
Changes:
- Replace raw
result_stack.pop()withsafe_pop()for loop body result handling inWhileStmt,ForCountLoopStmt, andForRangeLoopStmt. - Remove the
if not body_val: returnguard inWhileStmtso codegen doesn’t skip the back-edge on falsy values. - Add a regression test intended to cover loops whose body doesn’t leave a value on the stack.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/irx/builders/llvmliteir.py |
Makes loop codegen tolerant to empty body results by using safe_pop() and modernizes some type annotations. |
tests/test_while.py |
Adds a new regression test aimed at preventing WhileStmt crashes when the loop body doesn’t produce a value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@yuvimittal I have made the changes can you have a look? |
|
Hey @omsherikar, i was going through your PR and i analyzed that the safe_pop change in all three loop types makes good sense, and good call removing the if not body_val: return guard ->that would also break loops when the body produced a falsy value like 0, not just on empty stacks. but there were a couple of things I noticed: first is that regression test only covers WhileStmt — since ForCountLoopStmt and ForRangeLoopStmt were also changed to use safe_pop,i think it would be good to add similar void-body tests for those two as well , what do you think and the Optional → X | None type hint changes are valid modernization but maybe unrelated to the bug fix — might be cleaner as a separate commit to keep the fix-focused diff small. rest of the code change looks good overall 👍 |
safe_pop()
|
Hii @yuvimittal can you review this pr please |
|
|
||
| self.visit(node.body) | ||
| _ = self.result_stack.pop() | ||
| safe_pop(self.result_stack) |
There was a problem hiding this comment.
i am sorry @omsherikar , i missed that these are the exact changes you made!
|
hii @yuvimittal I have made the changes as discussed, resolved the merge conflicts . ready for review |
safe_pop()|
thanks @omsherikar |
|
🎉 This PR is included in version 1.9.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
.pop()withsafe_pop()in WhileStmt, ForCountLoopStmt, and ForRangeLoopStmt body handlingsolves Fix: WhileStmt body pop crashes on empty result stack #241