#27 Add prescan phase for short-circuit boolean expression decompilation - #31
Merged
sahebbiswas merged 7 commits intoMar 27, 2026
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughA prescan pass detects contiguous POP_JUMP_IF_* short‑circuit chains, evaluates and assembles combined boolean-condition strings, records suppressed clause offsets, and the instruction handler uses these maps to skip intermediate emits and produce single combined if/while headers during decompilation. Changes
Sequence Diagram(s)sequenceDiagram
participant Decompiler as Decompiler (decompile)
participant Prescan as Prescan (_prescan_compound_conds)
participant Handler as InstructionHandler (_handle_instruction)
participant Emitter as Emitter (emit)
Decompiler->>Prescan: scan bytecode for POP_JUMP_IF_* chains
Prescan-->>Decompiler: return _compound_cond_map and _compound_suppress
Decompiler->>Handler: iterate instructions
Handler->>Handler: if offset in _compound_suppress -> pop stack, skip emit
Handler->>Emitter: if offset in _compound_cond_map -> emit combined condition header
Handler->>Emitter: else -> emit normal instruction handling
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 957-1007: The _is_compound_cjump check must be extended to
recognize Python 3.11+ opnames (e.g. POP_JUMP_FORWARD_IF_TRUE /
POP_JUMP_FORWARD_IF_FALSE / POP_JUMP_FORWARD_IF_NONE /
POP_JUMP_FORWARD_IF_NOT_NONE) in addition to the existing POP_JUMP_IF_* variants
so the prescan triggers on 3.11+. Update the method _is_compound_cjump to
include those OP name patterns and any helpers that rely on it
(_get_jump_target, the prescan loop that builds group) so group detection runs
on modern bytecode; then adjust the reconstruction in the cond-combining code
(the block that calls _eval_cond_expr and builds parts/combined) to preserve
subgroup boundaries for (a or b) and c vs a and (b or c) by keeping connectors
local to each subgroup rather than flattening across differing connectors, and
add regression tests exercising both forms across Python 3.10–3.13 to ensure
semantic equivalence.
🪄 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: 1cc747ee-fe37-456b-a19e-e4f8b9eb8df9
📒 Files selected for processing (2)
pycrefine.pytests/test_pycrefine.py
…ions by correctly applying operator precedence and parenthesization.
Contributor
|
Note Docstrings generation - SUCCESS |
…-in-multiple-lines` Docstrings generation was requested by @sahebbiswas. The following files were modified: * `pycrefine.py` * `tests/test_pycrefine.py`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pycrefine.py (1)
1023-1055:⚠️ Potential issue | 🔴 CriticalDear friend, this combiner still changes mixed
and/orsemantics.
combined_contains_oris set once and never updated, so a laterandwraps the whole accumulated expression instead of the local subgroup. That makes shapes likea or b and cdecompile as(a or b) and c, which is a different program. Please derive subgroup boundaries from the jump targets rather than from one globalorflag, and add a regression for botha or b and cand(a or b) and c.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 1023 - 1055, The current linear combiner in the loop (using variables combined, combined_contains_or, prev_was_or and conn over parts) uses a single combined_contains_or flag that is never updated, causing incorrect parenthesization for mixed and/or sequences; modify the builder inside the for k in range(...) loop to compute subgroup boundaries from the parts' jump targets (i.e., inspect parts[k] and parts[k+1] metadata or explicit jump target info) instead of relying on the global combined_contains_or flag, update the local flag/state as you append each next_expr (so parentheses are applied only to the immediate subgroup), and remove/replace the stale combined_contains_or usage; add regression tests that validate both "a or b and c" and "(a or b) and c" decompilation results to ensure correct precedence handling.
🤖 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`:
- Around line 2176-2187: When a precomputed normalized condition is pulled from
self._compound_cond_map (via compound_cond = compound_cond_map.get(instr.offset)
and assigned to cond), mark that this condition is precomputed (e.g., set a flag
like compound_precomputed = True) and ensure the subsequent is_true rewrite
branch (the logic that inverts or appends "is True"/"is not None") checks this
flag and skips modifying cond if compound_precomputed is True; update the code
paths around compound_cond, instr.offset, and the is_true handling to respect
this flag so precomputed conditions from _prescan_compound_conds() are not
rewritten a second time.
---
Duplicate comments:
In `@pycrefine.py`:
- Around line 1023-1055: The current linear combiner in the loop (using
variables combined, combined_contains_or, prev_was_or and conn over parts) uses
a single combined_contains_or flag that is never updated, causing incorrect
parenthesization for mixed and/or sequences; modify the builder inside the for k
in range(...) loop to compute subgroup boundaries from the parts' jump targets
(i.e., inspect parts[k] and parts[k+1] metadata or explicit jump target info)
instead of relying on the global combined_contains_or flag, update the local
flag/state as you append each next_expr (so parentheses are applied only to the
immediate subgroup), and remove/replace the stale combined_contains_or usage;
add regression tests that validate both "a or b and c" and "(a or b) and c"
decompilation results to ensure correct precedence handling.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a5ca38d-41fa-48e6-a0bc-db3d7a434eae
📒 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
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pycrefine.py (2)
1006-1038:⚠️ Potential issue | 🔴 CriticalKindly preserve subgroup boundaries instead of flattening the chain.
This left-to-right builder still changes semantics for mixed short-circuit groups.
a and (b or c)becomesa and b or c, whilea or b and ccan be forced into(a or b) and c. One precedence flag cannot distinguish those bytecode shapes; this needs grouping by jump target/sub-expression, with regressions fora and (b or c)and(a or b) and c.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 1006 - 1038, The builder flattens mixed short-circuit groups because it only tracks a single precedence flag; update the combine loop to preserve subgroup boundaries (use the subgroup/jump-target info stored in parts rather than a single combined_contains_or flag). Specifically, change the logic around combined, parts, curr_is_or, next_is_or and prev_was_or so you treat any part that represents a grouped sub-expression as atomic (wrap it in parens when it came from a subgroup/jump-target) and when joining use the actual next_is_or/curr_is_or pair to decide wrapping instead of a single top-level flag; ensure you never transform a true subgroup (e.g., a and (b or c) or (a or b) and c) into a flattened chain by consulting each part's subgroup marker before concatenation.
2190-2203:⚠️ Potential issue | 🔴 CriticalKindly skip the second inversion when
condcame from_compound_cond_map.Once
condis replaced with a precomputed compound expression, the lateris_truebranch still negates it again.if a or not b:then emits asif not a or not b:, which is a different predicate.Suggested fix
compound_cond_map = getattr(self, "_compound_cond_map", {}) compound_cond = compound_cond_map.get(instr.offset) - if compound_cond is not None: + compound_precomputed = compound_cond is not None + if compound_precomputed: cond = compound_cond else: if "IF_NONE" in opname and "NOT" not in opname: # Fires on None; body runs on NOT None cond = f"{cond} is not None" elif "IF_NOT_NONE" in opname: # Fires on NOT None; body runs on None cond = f"{cond} is None" - is_true = "IF_TRUE" in opname + is_true = "IF_TRUE" in opname and not compound_precomputed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 2190 - 2203, When a precomputed condition from _compound_cond_map (compound_cond) is used to set cond, skip any further inversion logic: do not run the IF_NONE/IF_NOT_NONE branches nor apply the is_true negation; instead leave cond as the compound expression. Concretely, in the block around compound_cond_map/_compound_cond_map and opname, ensure that if compound_cond is not None you set cond and bypass the code that mutates cond based on "IF_NONE"/"IF_NOT_NONE" and the is_true ("IF_TRUE" in opname) handling that currently flips the predicate; only run those transformations when compound_cond is None. This prevents double-negation cases like when a compound cond has already encoded the correct polarity.
🤖 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`:
- Around line 971-991: The fallback that sets is_or_jump = is_success_type
(based solely on "IF_TRUE" in op) misclassifies mixed None-chains; instead,
determine polarity for intermediate jumps by examining the grouped jump
structure/subgroup membership rather than raw truthiness: when t is an
intermediate target, consult the subgroup start/edges that t belongs to (the
jump chain that includes body_target/end_target) and derive whether a None-edge
represents success or failure for that subgroup (use the existing grouping logic
that identifies subgroup boundaries rather than the is_success_type flag);
update the branch that handles "IF_NONE"/"IF_NOT_NONE" to use that derived
subgroup polarity to set cond_str, and add a regression test covering patterns
like (x is None or y) and z to prevent future regressions.
---
Duplicate comments:
In `@pycrefine.py`:
- Around line 1006-1038: The builder flattens mixed short-circuit groups because
it only tracks a single precedence flag; update the combine loop to preserve
subgroup boundaries (use the subgroup/jump-target info stored in parts rather
than a single combined_contains_or flag). Specifically, change the logic around
combined, parts, curr_is_or, next_is_or and prev_was_or so you treat any part
that represents a grouped sub-expression as atomic (wrap it in parens when it
came from a subgroup/jump-target) and when joining use the actual
next_is_or/curr_is_or pair to decide wrapping instead of a single top-level
flag; ensure you never transform a true subgroup (e.g., a and (b or c) or (a or
b) and c) into a flattened chain by consulting each part's subgroup marker
before concatenation.
- Around line 2190-2203: When a precomputed condition from _compound_cond_map
(compound_cond) is used to set cond, skip any further inversion logic: do not
run the IF_NONE/IF_NOT_NONE branches nor apply the is_true negation; instead
leave cond as the compound expression. Concretely, in the block around
compound_cond_map/_compound_cond_map and opname, ensure that if compound_cond is
not None you set cond and bypass the code that mutates cond based on
"IF_NONE"/"IF_NOT_NONE" and the is_true ("IF_TRUE" in opname) handling that
currently flips the predicate; only run those transformations when compound_cond
is None. This prevents double-negation cases like when a compound cond has
already encoded the correct polarity.
🪄 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: ed546f90-b072-4815-8880-029554ca6907
📒 Files selected for processing (2)
pycrefine.pytests/test_pycrefine.py
Comment on lines
+971
to
+991
| is_success_type = ("IF_TRUE" in op) | ||
|
|
||
| # Logic: if jump targets body (OR), it's definitely an OR jump. | ||
| # If it targets END, it's definitely an AND jump. | ||
| # If it targets a later part of the chain: | ||
| # - Success-type jump to later = Progress to next OR sub-group. | ||
| # - Failure-type jump to later = Progress to next part of AND-chain. | ||
| if t == body_target: | ||
| is_or_jump = True | ||
| elif t == end_target: | ||
| is_or_jump = False | ||
| else: | ||
| # Intermediate target. | ||
| is_or_jump = is_success_type | ||
|
|
||
| if "IF_NONE" in op and "NOT" not in op: | ||
| # Fires on None; if OR jump, success is None. If AND, failure is None -> success is not None. | ||
| cond_str = f"{raw_expr} is None" if is_or_jump else f"{raw_expr} is not None" | ||
| elif "IF_NOT_NONE" in op: | ||
| # Fires on NOT None; if OR jump, success is NOT None. If AND, failure is NOT None -> success is None. | ||
| cond_str = f"{raw_expr} is not None" if is_or_jump else f"{raw_expr} is None" |
Contributor
There was a problem hiding this comment.
Kindly do not infer every intermediate jump from IF_TRUE alone.
The fallback here still misclassifies mixed None-chains that jump to a later clause start. A shape like (x is None or y) and z can be reconstructed with the first clause inverted, because the intermediate IF_NONE / IF_NOT_NONE edge carries subgroup meaning, not just raw truthiness. Please add a regression in this family and derive that polarity from the grouped jump structure instead of "IF_TRUE" alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pycrefine.py` around lines 971 - 991, The fallback that sets is_or_jump =
is_success_type (based solely on "IF_TRUE" in op) misclassifies mixed
None-chains; instead, determine polarity for intermediate jumps by examining the
grouped jump structure/subgroup membership rather than raw truthiness: when t is
an intermediate target, consult the subgroup start/edges that t belongs to (the
jump chain that includes body_target/end_target) and derive whether a None-edge
represents success or failure for that subgroup (use the existing grouping logic
that identifies subgroup boundaries rather than the is_success_type flag);
update the branch that handles "IF_NONE"/"IF_NOT_NONE" to use that derived
subgroup polarity to set cond_str, and add a regression test covering patterns
like (x is None or y) and z to prevent future regressions.
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pycrefine.py (1)
2266-2280:⚠️ Potential issue | 🟡 MinorDear friend, this code will never see the light of execution.
Lines 2146-2151 already handle
POP_JUMP_IF_NONEandPOP_JUMP_IF_NOT_NONEwithin the main conditional block. This subsequentelifblock at lines 2266-2280 checking for the same opcodes is unreachable dead code, as the earlier condition will always match first.In the pursuit of truth and simplicity, I humbly suggest removing this unreachable code.
🗑️ Suggested removal
- # FIX-01: POP_JUMP_IF_NONE / POP_JUMP_IF_NOT_NONE — was swapped - elif "POP_JUMP_IF_NONE" in opname or "POP_JUMP_IF_NOT_NONE" in opname: - if self.stack: - cond = self.stack.pop() - is_not_none = "NOT_NONE" in opname - # POP_JUMP_IF_NOT_NONE: jumps when not-None → body runs when None - # POP_JUMP_IF_NONE: jumps when None → body runs when not-None - if is_not_none: - self._append_reconstructed(f"if {cond} is None:") - else: - self._append_reconstructed(f"if {cond} is not None:") - - self.indent_level += 1 - jump_target = self._get_jump_target(instr) - self.blocks.append((jump_target, "if"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 2266 - 2280, The elif block that tests for "POP_JUMP_IF_NONE" / "POP_JUMP_IF_NOT_NONE" (which pops self.stack, sets is_not_none, calls self._append_reconstructed, increments self.indent_level, computes jump_target via self._get_jump_target, and appends to self.blocks) is unreachable because those opcodes are already handled earlier in the main conditional; remove this entire duplicate elif branch to eliminate dead code and avoid double-handling of opname related to POP_JUMP_IF_NONE / POP_JUMP_IF_NOT_NONE.
🧹 Nitpick comments (1)
pycrefine.py (1)
961-961: Unused loop variables, my friend.The loop control variables
kandjump_idxare unpacked but never used within the loop body. In the spirit of simplicity and clarity, consider renaming them with underscore prefixes to indicate they are intentionally unused.🔧 Suggested fix
- for k, (jump_idx, jinstr, expr_instrs, _) in enumerate(group): + for _k, (_jump_idx, jinstr, expr_instrs, _) in enumerate(group):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` at line 961, The loop unpacks unused variables k and jump_idx; rename them to indicate they are intentionally unused (e.g., _k and _jump_idx or _ and _jump_idx) in the for statement "for k, (jump_idx, jinstr, expr_instrs, _) in enumerate(group):" so the intent is clear and lint warnings are suppressed—update any references (there should be none) and run tests/linter to confirm no other uses exist.
🤖 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`:
- Around line 1006-1037: The loop's parenthesis tracking is broken because
next_is_or is never used, prev_was_or is assigned but not consulted, and
combined_contains_or is never updated after initialization; fix the for-loop
that builds combined (referencing variables combined, combined_contains_or,
prev_was_or, curr_is_or, next_is_or, conn, next_expr) by updating
combined_contains_or whenever an OR is encountered (e.g., combined_contains_or =
combined_contains_or or curr_is_or or next_is_or), actually using prev_was_or to
record the previous connector state and setting it each iteration (prev_was_or =
conn == "or"), and ensure the wrapping check before appending uses the
up-to-date combined_contains_or (if conn == "and" and combined_contains_or then
wrap combined); remove any redundant assignments if made unnecessary by this
logic.
---
Outside diff comments:
In `@pycrefine.py`:
- Around line 2266-2280: The elif block that tests for "POP_JUMP_IF_NONE" /
"POP_JUMP_IF_NOT_NONE" (which pops self.stack, sets is_not_none, calls
self._append_reconstructed, increments self.indent_level, computes jump_target
via self._get_jump_target, and appends to self.blocks) is unreachable because
those opcodes are already handled earlier in the main conditional; remove this
entire duplicate elif branch to eliminate dead code and avoid double-handling of
opname related to POP_JUMP_IF_NONE / POP_JUMP_IF_NOT_NONE.
---
Nitpick comments:
In `@pycrefine.py`:
- Line 961: The loop unpacks unused variables k and jump_idx; rename them to
indicate they are intentionally unused (e.g., _k and _jump_idx or _ and
_jump_idx) in the for statement "for k, (jump_idx, jinstr, expr_instrs, _) in
enumerate(group):" so the intent is clear and lint warnings are
suppressed—update any references (there should be none) and run tests/linter to
confirm no other uses exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…ly handle operator precedence and parenthesization, and add a comprehensive test suite including specific precedence tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pycrefine.py (1)
982-984:⚠️ Potential issue | 🔴 CriticalIntermediate-target polarity is still inferred from
IF_TRUEalone.Dear friend, this keeps the same semantic risk for mixed None-chains (for example, shapes like
(x is None or y) and z) when the jump target is intermediate, not final. Please deriveis_or_jumpfrom subgroup structure/target boundaries instead ofis_success_typefallback at Line 983.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 982 - 984, The assignment of is_or_jump currently uses is_success_type as a fallback which misclassifies intermediate jump targets; change the logic so is_or_jump is computed from the subgroup/target boundary information instead: inspect the current subgroup structure and the jump target's boundary (whether the target is an intermediate node within the same subgroup vs a final exit) and set is_or_jump true only when the subgroup/target relation indicates an OR-style short-circuit jump, otherwise false; replace the fallback use of is_success_type at the is_or_jump assignment with this boundary/subgroup-derived check so mixed None-chains are handled correctly.
🧹 Nitpick comments (2)
pycrefine.py (1)
2289-2304: LegacyPOP_JUMP_IF_NONEbranch is now unreachable.Because Lines 2169-2174 already capture
POP_JUMP_IF_NONE/POP_JUMP_IF_NOT_NONE, this later branch no longer executes. Removing it will reduce confusion and maintenance risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 2289 - 2304, Remove the unreachable redundant branch that handles POP_JUMP_IF_NONE / POP_JUMP_IF_NOT_NONE: delete the entire elif block that checks '"POP_JUMP_IF_NONE" in opname or "POP_JUMP_IF_NOT_NONE" in opname' (the block that pops self.stack, sets is_not_none, calls self._append_reconstructed, increments self.indent_level, computes jump_target via self._get_jump_target(instr), and appends to self.blocks) because those opnames are already handled earlier; ensure no other code paths rely on this block before removing it.tests/test_pycrefine.py (1)
2800-2864: Prefer counting realifheaders instead ofout.count("if ").Using
count("if ")may overcount occurrences inside expressions/strings. A line-based header counter will make these tests steadier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_pycrefine.py` around lines 2800 - 2864, Replace fragile uses of out.count("if ") in the test methods (e.g. test_compound_or, test_compound_mixed_and_or, test_compound_none_and, test_compound_none_or, test_compound_complex_mixed, test_compound_short_circuit_with_call, test_compound_nested_if_merge_regression) with a line-based header counter that only counts actual `if` statement headers from the decompiled output; for example compute header_count = sum(1 for line in out.splitlines() if line.lstrip().startswith("if ")) and assert header_count == expected instead of using out.count("if "). Ensure you replace each self.assertEqual(out.count("if "), N) (or similar) with the header_count-based assertion using the same expected N.
🤖 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 961: The loop currently binds unused variables k and jump_idx in "for k,
(jump_idx, jinstr, expr_instrs, _) in enumerate(group):"; remove the enumerate
and replace with a tuple unpack that discards the first element so it reads "for
_, jinstr, expr_instrs, _ in group:" (or, if you still need the index, keep
enumerate but use "_" for k and jump_idx: "for _, (_, jinstr, expr_instrs, _) in
enumerate(group):") to silence linters and remove the unused bindings while
keeping jinstr and expr_instrs intact.
- Around line 1049-1054: The current parentheses check uses "(" not in
res/next_expr which fails for function calls; change the assembly to propagate
explicit top-level-or metadata instead of string-scanning—introduce and use
boolean flags (e.g., res_has_top_level_or, next_has_top_level_or) alongside the
expression strings created earlier, and replace the two checks in the block that
inspects conn == "and" (where res and next_expr are combined) to wrap with
parentheses only when the corresponding top-level-or flag is True; update the
code paths that build res and next_expr to set these flags when they contain a
top-level "or" so precedence is preserved even if function calls include
parentheses.
In `@tests/test_pycrefine.py`:
- Around line 2836-2839: The test's allowed decompilation variants include an
incorrect boolean grouping that changes semantics; in the assertion block
checking output strings (the four ORed literals) remove the variant "((x is not
None and x > 0) or y is None) and z == 1" and only allow forms that are
logically equivalent to (x is not None and x > 0) or (y is None and z == 1) —
e.g., keep the original unparenthesized form and variants that parenthesize each
conjunction together like "(x is not None and x > 0) or (y is None and z == 1)"
or "(x is not None and x > 0) or y is None and z == 1", but do not accept any
grouping where the final "and z == 1" binds outside the intended right-hand
conjunction.
- Around line 2824-2826: The test currently enforces only the unparenthesised
form via assert_contains(out, "if x is None or y is None:") which contradicts
the comment; update the assertion to accept either "if x is None or y is None:"
or "if (x is None or y is None):" (for example by replacing the single
assert_contains call with a check that either string is present or a regex
match), and keep the existing self.assertEqual(out.count("if "), 1) to ensure
exactly one if-statement is emitted.
---
Duplicate comments:
In `@pycrefine.py`:
- Around line 982-984: The assignment of is_or_jump currently uses
is_success_type as a fallback which misclassifies intermediate jump targets;
change the logic so is_or_jump is computed from the subgroup/target boundary
information instead: inspect the current subgroup structure and the jump
target's boundary (whether the target is an intermediate node within the same
subgroup vs a final exit) and set is_or_jump true only when the subgroup/target
relation indicates an OR-style short-circuit jump, otherwise false; replace the
fallback use of is_success_type at the is_or_jump assignment with this
boundary/subgroup-derived check so mixed None-chains are handled correctly.
---
Nitpick comments:
In `@pycrefine.py`:
- Around line 2289-2304: Remove the unreachable redundant branch that handles
POP_JUMP_IF_NONE / POP_JUMP_IF_NOT_NONE: delete the entire elif block that
checks '"POP_JUMP_IF_NONE" in opname or "POP_JUMP_IF_NOT_NONE" in opname' (the
block that pops self.stack, sets is_not_none, calls self._append_reconstructed,
increments self.indent_level, computes jump_target via
self._get_jump_target(instr), and appends to self.blocks) because those opnames
are already handled earlier; ensure no other code paths rely on this block
before removing it.
In `@tests/test_pycrefine.py`:
- Around line 2800-2864: Replace fragile uses of out.count("if ") in the test
methods (e.g. test_compound_or, test_compound_mixed_and_or,
test_compound_none_and, test_compound_none_or, test_compound_complex_mixed,
test_compound_short_circuit_with_call, test_compound_nested_if_merge_regression)
with a line-based header counter that only counts actual `if` statement headers
from the decompiled output; for example compute header_count = sum(1 for line in
out.splitlines() if line.lstrip().startswith("if ")) and assert header_count ==
expected instead of using out.count("if "). Ensure you replace each
self.assertEqual(out.count("if "), N) (or similar) with the header_count-based
assertion using the same expected N.
🪄 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: 171e97cc-6e5e-4175-bcc2-2cf868e18d6c
📒 Files selected for processing (2)
pycrefine.pytests/test_pycrefine.py
…t files for various language constructs and updates to the decompiler.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pycrefine.py (1)
2166-2171:⚠️ Potential issue | 🔴 CriticalPlease unify conditional jump matching with
_is_compound_cjumpto avoid missing Python 3.11+ opnames.Dear friend, this branch currently checks only
POP_JUMP_IF_*shapes.POP_JUMP_FORWARD_IF_*/POP_JUMP_BACKWARD_IF_*can slip past it, so modern bytecode may skipif/whileemission and compound-condition handling.Suggested fix
- elif ( - "POP_JUMP_IF_FALSE" in opname - or "POP_JUMP_IF_TRUE" in opname - or "POP_JUMP_IF_NONE" in opname - or "POP_JUMP_IF_NOT_NONE" in opname - ): + elif self._is_compound_cjump(opname):In CPython 3.11, 3.12, 3.13, and 3.14, what are the `dis` opcode names for conditional POP jumps (`IF_TRUE`, `IF_FALSE`, `IF_NONE`, `IF_NOT_NONE`)? Do they appear as `POP_JUMP_FORWARD_IF_*` / `POP_JUMP_BACKWARD_IF_*` rather than only `POP_JUMP_IF_*`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 2166 - 2171, Replace the ad-hoc opname checks for conditional POP jumps with the shared matcher `_is_compound_cjump` so forward/backward variants aren't missed; specifically, in the branch that currently quizzes `opname` for `"POP_JUMP_IF_FALSE" / "POP_JUMP_IF_TRUE" / "POP_JUMP_IF_NONE" / "POP_JUMP_IF_NOT_NONE"`, call `_is_compound_cjump(opname)` (or reuse its internal pattern) instead of the manual string checks so `POP_JUMP_FORWARD_IF_*` and `POP_JUMP_BACKWARD_IF_*` opnames from Python 3.11+ are handled consistently. Ensure you reference the existing `opname` variable and remove the duplicated string-list checks.
🧹 Nitpick comments (1)
pycrefine.py (1)
1086-1093: Consider removing or wiring_is_compound_or_jumpto active logic.Dear friend, this helper appears unused. Keeping unused branch-logic helpers in decompiler control flow increases drift risk; either use it in
_prescan_compound_conds()or delete it for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 1086 - 1093, The helper function _is_compound_or_jump is currently unused and should be either removed or wired into the prescan routine: either delete the _is_compound_or_jump definition to avoid dead code, or update _prescan_compound_conds to call _is_compound_or_jump(opname) where you currently check opcode names (replace any ad-hoc string checks like "IF_TRUE"/"IF_NONE" and AND/NOT exclusions with this helper) so the helper is actually exercised; ensure tests/flows that depend on _prescan_compound_conds still behave the same after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pycrefine.py`:
- Around line 2166-2171: Replace the ad-hoc opname checks for conditional POP
jumps with the shared matcher `_is_compound_cjump` so forward/backward variants
aren't missed; specifically, in the branch that currently quizzes `opname` for
`"POP_JUMP_IF_FALSE" / "POP_JUMP_IF_TRUE" / "POP_JUMP_IF_NONE" /
"POP_JUMP_IF_NOT_NONE"`, call `_is_compound_cjump(opname)` (or reuse its
internal pattern) instead of the manual string checks so `POP_JUMP_FORWARD_IF_*`
and `POP_JUMP_BACKWARD_IF_*` opnames from Python 3.11+ are handled consistently.
Ensure you reference the existing `opname` variable and remove the duplicated
string-list checks.
---
Nitpick comments:
In `@pycrefine.py`:
- Around line 1086-1093: The helper function _is_compound_or_jump is currently
unused and should be either removed or wired into the prescan routine: either
delete the _is_compound_or_jump definition to avoid dead code, or update
_prescan_compound_conds to call _is_compound_or_jump(opname) where you currently
check opcode names (replace any ad-hoc string checks like "IF_TRUE"/"IF_NONE"
and AND/NOT exclusions with this helper) so the helper is actually exercised;
ensure tests/flows that depend on _prescan_compound_conds still behave the same
after the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6d00932-eaf8-4636-b548-f857b9543922
📒 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
…of operator precedence, binary operations, and conditional jumps.
sahebbiswas
deleted the
27-compound-if-statements-are-incorrectly-placed-in-multiple-lines
branch
March 27, 2026 13:26
This was referenced Mar 28, 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