22 exception chain decomposition is not correct - #34
Merged
sahebbiswas merged 7 commits intoMar 29, 2026
Conversation
…ndling in decompiler
…compilation logic
…ducing an instruction lookahead check
Contributor
📝 WalkthroughWalkthroughThis PR adds a prescan-driven reconstruction pass for try/except/finally and with-statements, extends the instruction model with Changes
Sequence Diagram(s)sequenceDiagram
participant Decompiler as Decompiler.decompile()
participant Prescan as _prescan_try_structure()
participant Dispatcher as _handle_instruction()
participant Deferred as DeferredRenderers
participant Output as OutputBuffer
Decompiler->>Prescan: analyze bytecode for try/except/finally and with structure
Prescan-->>Decompiler: return suppression ranges, merge labels, mappings
loop per-instruction
Decompiler->>Dispatcher: process instruction at pc
Dispatcher->>Dispatcher: consult prescan maps
alt pc is finally/except merge
Dispatcher->>Deferred: emit deferred except/finally blocks
Deferred->>Output: append reconstructed handler/finally
else pc in suppression range
Dispatcher->>Dispatcher: skip inlined epilogue/wrapper instrs
else Normal
Dispatcher->>Output: emit decompiled line
end
end
Decompiler->>Decompiler: _close_blocks(0x7fffffff)
Decompiler->>Output: post-pass cleanup (remove/replace compiler return)
sequenceDiagram
participant Code as Bytecode
participant Decompiler as DecompilerGeneric
participant WithHandler as SETUP_WITH/BEFORE_WITH logic
participant Output as OutputBuffer
Code->>Decompiler: load bytecode with with/try/finally
Decompiler->>Prescan: prescan structure
Prescan-->>Decompiler: suppression & merge info
Decompiler->>WithHandler: encounter SETUP_WITH or BEFORE_WITH
WithHandler->>Decompiler: peek for STORE_* binding
WithHandler->>Output: emit "with ... as var:" and track block extents
Decompiler->>Output: later emit deferred finally/except at merge points
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…andling to specific error types
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…omprehensive test suite
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pycrefine.py`:
- Line 593: The unpacked variables from self.blocks.pop() (currently written as
block_end, block_type) are unused and triggering lint warnings; rename them to
underscore-prefixed placeholders (e.g., _block_end, _block_type) to indicate
intentionally unused values and silence RUF059/B007; apply the same
underscore-prefixed rename pattern to the other identical unpackings in this
module (the other occurrences that unpack two values but don't use them).
- Around line 1669-1672: The blanket except Exception around
_dis.Bytecode(self.code_obj).exception_entries hides real bugs; restrict the
fallback to only expected metadata-shape failures (e.g., AttributeError,
TypeError, ValueError) when computing _exc_entries: catch those specific
exceptions and set _exc_entries = [], but allow any other exception to propagate
(do not swallow). Locate the try block using _dis.Bytecode and
.exception_entries and replace the broad except with a narrow tuple of exception
types (or re-raise unexpected ones) so only metadata-related failures are
handled.
- Around line 410-414: The detection currently reads the mutable runtime stack
(self.stack) to decide if a RETURN_VALUE is returning None, which can
misclassify instructions; instead, stop inspecting self.stack and use
instruction-local evidence: for RETURN_CONST keep the existing instr.argval
check (e.g., instr.argval is None) and for RETURN_VALUE examine the immediately
preceding bytecode instruction (e.g., prev_instr = instructions[i-1]) to see if
it is a LOAD_CONST whose argval is None (prev_instr.opname == "LOAD_CONST" and
prev_instr.argval is None); remove the branch that reads self.stack and rely
only on instr, op, instr.argval and the previous instruction to decide to
continue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 244bf41e-c53e-47b0-b8ee-68e58b395738
📒 Files selected for processing (2)
pycrefine.pytests/test_pycrefine.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_pycrefine.py
Comment on lines
+410
to
+414
| stack = getattr(self, "stack", None) | ||
| if op == "RETURN_VALUE" and stack and str(stack[-1]) == "None": | ||
| continue | ||
| if op == "RETURN_CONST" and str(instr.argval) == "None": | ||
| continue |
Contributor
There was a problem hiding this comment.
Detach trailing-return detection from mutable runtime stack state.
With humility, this check can misclassify later RETURN_VALUE instructions because it consults the current self.stack instead of instruction-local evidence. That can silently suppress a real return None in edge control-flow.
Suggested fix
- # Skip subsequent None returns (common in 3.14 multiple exit paths)
- stack = getattr(self, "stack", None)
- if op == "RETURN_VALUE" and stack and str(stack[-1]) == "None":
- continue
+ # Skip subsequent implicit None returns using instruction-local evidence.
+ if op == "RETURN_VALUE":
+ prev = self.instructions[i - 1] if i > 0 else None
+ if prev and prev.opname in ("LOAD_CONST", "LOAD_SMALL_INT") and prev.argval is None:
+ continue
if op == "RETURN_CONST" and str(instr.argval) == "None":
continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pycrefine.py` around lines 410 - 414, The detection currently reads the
mutable runtime stack (self.stack) to decide if a RETURN_VALUE is returning
None, which can misclassify instructions; instead, stop inspecting self.stack
and use instruction-local evidence: for RETURN_CONST keep the existing
instr.argval check (e.g., instr.argval is None) and for RETURN_VALUE examine the
immediately preceding bytecode instruction (e.g., prev_instr =
instructions[i-1]) to see if it is a LOAD_CONST whose argval is None
(prev_instr.opname == "LOAD_CONST" and prev_instr.argval is None); remove the
branch that reads self.stack and rely only on instr, op, instr.argval and the
previous instruction to decide to continue.
| _NO_INDENT_TYPES = frozenset({"finally_wrapper"}) | ||
|
|
||
| while self.blocks and offset >= self.blocks[-1][0]: | ||
| block_end, block_type = self.blocks.pop() |
Contributor
There was a problem hiding this comment.
Clean unused loop/unpack variables to keep lint gates peaceful.
These names are currently unused and trigger Ruff warnings (RUF059 / B007). A tiny rename to underscore-prefixed placeholders keeps intent clear and CI quiet.
Suggested fix
- block_end, block_type = self.blocks.pop()
+ _block_end, block_type = self.blocks.pop()
@@
- for push_off, merge_off in self._push_exc_to_finally_merge.items():
+ for _push_off, merge_off in self._push_exc_to_finally_merge.items():
@@
- boff, btype = sub.blocks.pop()
+ _boff, _btype = sub.blocks.pop()
@@
- for i, ins in enumerate(self.instructions):
+ for _i, ins in enumerate(self.instructions):Also applies to: 1729-1729, 2044-2044, 3590-3590
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 593-593: Unpacked variable block_end is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pycrefine.py` at line 593, The unpacked variables from self.blocks.pop()
(currently written as block_end, block_type) are unused and triggering lint
warnings; rename them to underscore-prefixed placeholders (e.g., _block_end,
_block_type) to indicate intentionally unused values and silence RUF059/B007;
apply the same underscore-prefixed rename pattern to the other identical
unpackings in this module (the other occurrences that unpack two values but
don't use them).
Comment on lines
+1669
to
+1672
| try: | ||
| _exc_entries = _dis.Bytecode(self.code_obj).exception_entries # type: ignore[name-defined] | ||
| except Exception: | ||
| _exc_entries = [] |
Contributor
There was a problem hiding this comment.
Narrow this fallback catch to expected metadata failures.
With respect, except Exception here can bury genuine reconstruction defects. Please catch only expected metadata-shape failures and let unexpected faults surface.
Suggested fix
- except Exception:
+ except (AttributeError, IndexError, TypeError, ValueError, NameError):
_exc_entries = []🧰 Tools
🪛 Ruff (0.15.7)
[warning] 1671-1671: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pycrefine.py` around lines 1669 - 1672, The blanket except Exception around
_dis.Bytecode(self.code_obj).exception_entries hides real bugs; restrict the
fallback to only expected metadata-shape failures (e.g., AttributeError,
TypeError, ValueError) when computing _exc_entries: catch those specific
exceptions and set _exc_entries = [], but allow any other exception to propagate
(do not swallow). Locate the try block using _dis.Bytecode and
.exception_entries and replace the broad except with a narrow tuple of exception
types (or re-raise unexpected ones) so only metadata-related failures are
handled.
Contributor
|
Note Docstrings generation - SUCCESS |
coderabbitai Bot
added a commit
that referenced
this pull request
Mar 29, 2026
Docstrings generation was requested by @sahebbiswas. * #34 (comment) The following files were modified: * `pycrefine.py` * `tests/test_pycrefine.py`
This was referenced Mar 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests