#43 Improve bytecode pattern suppression and control flow handling - #44
Merged
sahebbiswas merged 6 commits intoApr 2, 2026
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change refines pycrefine's prescan and emission logic: collapses duplicated augmented assignments, makes ternary prescans aware of INPLACE/BINARY-derived augments, tightens while/try prescans and except/finally handling, reconstructs full function signatures (kw-only and varargs), enriches pyc magic/version errors, adjusts CLI empty-output behavior, adds tests, and introduces a verification fixture while pruning debug scripts. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 6
🧹 Nitpick comments (1)
tests/test_pycrefine.py (1)
3973-4015: A gentle refactor: extract duplicated_run39_fullhelper.The same helper appears in multiple new classes. Centralizing it will reduce drift and future maintenance effort.
Also applies to: 4129-4171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_pycrefine.py` around lines 3973 - 4015, The duplicated helper _run39_full should be extracted to a single shared implementation (e.g., a module-level function or a test utility class) and all copies replaced to call that single definition; locate the duplicated definitions named _run39_full in tests/test_pycrefine.py (and the other occurrence around the referenced block) and move the body into one place, preserve its signature (instructions), ensure it constructs BytecodeInstruction and runs the same prescans and main loop, then update each test class to call the shared _run39_full helper instead of keeping duplicated code.
🤖 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 2726-2744: The code incorrectly computes varargs/ varkw indices
using only inner_code.co_argcount; fix by including inner_code.co_kwonlyargcount
when computing positions: set varargs_idx = inner_code.co_argcount +
inner_code.co_kwonlyargcount and varkw_idx = inner_code.co_argcount +
inner_code.co_kwonlyargcount + int(has_varargs), then use those indices when
building varargs_name and varkw_name and keep the existing bounds checks; update
the same logic in both places where varargs/varkw are computed (the blocks
creating varargs_name and varkw_name).
- Around line 160-169: The augmented-assignment cleanup using m_double must run
before the generic paren-stripping that uses assignment_parens_re so patterns
like "x = (x += y)" are rewritten to "x += y" while the RHS still contains the
parentheses; move the m_double matching/rewrite block so it executes prior to
the code that applies assignment_parens_re (ensure you keep the existing regex
and the variables indent, var_name, op_sym, rhs and the replacement line =
f"{indent}{var_name} {op_sym}= {rhs}" unchanged).
- Around line 4486-4504: The current branch that classifies a
SETUP_FINALLY/SETUP_EXCEPT target only checks for a DUP_TOP handler entry (via
target_is_handler_entry) and misclassifies bare except: and nested try/finally;
update the logic in the except-handler branch (the block that currently computes
target_idx and target_is_handler_entry and either calls
super()._handle_instruction(instr) or appends ("exc_cleanup") to self.blocks) to
reuse the prescan rules: consult the existing _finally_targets set and the
POP_TOP–POP_TOP–POP_TOP pattern recognized by _prescan_try_structure() in
addition to DUP_TOP before deciding it is a real nested handler; if any of those
conditions match treat it as a real nested try (clear self._except_header_indent
and call super()._handle_instruction(instr)), otherwise append (jump_target,
"exc_cleanup") as now.
In `@test_files/verify_scenes.py`:
- Around line 4-7: Add a file-level Ruff suppression by inserting a ruff noqa
directive at the top of the file (next to the existing "# pylint: disable=all"
comment) so Ruff ignores this intentional fixture; update the header to include
"# ruff: noqa" (or "# ruff: noqa: ALL") while keeping the existing pylint
disable line and comments intact to silence Ruff for the whole file during CI.
In `@tests/test_pycrefine.py`:
- Line 4043: Rename one-letter ambiguous variables that trigger Ruff E741:
change the alias I = BytecodeInstruction to a descriptive name like Instr or
BytecodeInstr, and similarly replace single-letter variables such as l with
clearer names (e.g., line_no, idx, or length) in occurrences tied to bytecode
handling; update all references to the old single-letter identifiers (for
example any uses of I and l) throughout the test module so they match the new
names and preserve functionality.
- Around line 4397-4414: The test
test_augmented_add_ternary_keeps_augmented_form currently only checks for
variable and branch names; update it to assert the actual augmented-ternary
structure in the decompiler output by checking for the augmented operator and
conditional keywords (e.g., assert that out contains the substring "s += a if c
else b" or at minimum assertIn("+=", out) and assertIn("if", out) and
assertIn("else", out)); locate the assertions around the decompile output
variable out in test_augmented_add_ternary_keeps_augmented_form and add these
stronger assertions to prevent false positives.
---
Nitpick comments:
In `@tests/test_pycrefine.py`:
- Around line 3973-4015: The duplicated helper _run39_full should be extracted
to a single shared implementation (e.g., a module-level function or a test
utility class) and all copies replaced to call that single definition; locate
the duplicated definitions named _run39_full in tests/test_pycrefine.py (and the
other occurrence around the referenced block) and move the body into one place,
preserve its signature (instructions), ensure it constructs BytecodeInstruction
and runs the same prescans and main loop, then update each test class to call
the shared _run39_full helper instead of keeping duplicated code.
🪄 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: 6f1fdcf9-e250-4397-9753-1876f2b4e8d7
📒 Files selected for processing (3)
pycrefine.pytest_files/verify_scenes.pytests/test_pycrefine.py
…nt indexing while adding comprehensive test suite
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 849-866: The code currently only checks for legacy INPLACE_*
opcodes to set is_inplace before adjusting st_idx; update the detection to also
treat a BINARY_OP with an argument in the augmented-assignment range (13–25) as
an in-place operation: when inspecting self.instructions[st_idx].opname,
consider both "INPLACE_*" and opname == "BINARY_OP" with the BINARY_OP argument
in 13..25, then set is_inplace = True and increment st_idx accordingly so the
subsequent else_end/else_instrs logic excludes the merge opcode; update the
conditional around is_inplace/st_idx (the block that modifies st_idx and sets
is_inplace) that currently references self.instructions[st_idx].opname to
include this BINARY_OP check.
- Around line 2726-2744: The reconstructed signature is missing keyword-only
parameters because the code only uses inner_code.co_kwonlyargcount for indexing
*args/**kwargs but never reads the kw-only names; update the logic around
inner_code to slice inner_code.co_varnames from inner_code.co_argcount to
inner_code.co_argcount + inner_code.co_kwonlyargcount to extract the kw-only
parameter names, format them as plain names (and if needed prefix with defaults
or annotations like the surrounding signature code does), and insert them into
the parameter list immediately after positional parameters and before
varargs/varkw handling (retain existing varargs/varkw computation using
varargs_idx/varkw_idx). Ensure you reference inner_code.co_kwonlyargcount,
inner_code.co_argcount, inner_code.co_varnames, varargs_idx, and varkw_idx when
making the change so kw-only names are preserved in the final signature.
In `@tests/test_pycrefine.py`:
- Around line 4314-4328: The test
test_augmented_add_ternary_keeps_augmented_form fails because pycrefine.py
currently decompiles "s += a if c else b" into a plain assignment instead of
preserving the augmented operator; update the decompilation logic (the handler
that formats augmented assignments / the code path that checks for IfExp) to
recognize an ast.IfExp on the right-hand side and emit the augmented form using
the INPLACE_* tokens (e.g., INPLACE_ADD) rather than falling back to a normal
'=' assignment; locate the code that maps AugAssign/AugAssign-like bytecode to
output (search for symbols like INPLACE_ADD, INPLACE_*, handle_AugAssign or the
assignment formatting function) and adjust it so when op is an INPLACE_* and
value is an IfExp the output prints "target <INPLACE_TOKEN> <body> if <test>
else <orelse>" preserving the augmented syntax.
- Around line 157-161: Remove the unreachable post-return assertion block that
references fragments and output: delete the for loop that iterates over
fragments and the accompanying assert (the copied block containing "for frag in
fragments: assert frag not in output, ...") so the test no longer contains code
after the function's return that refers to the undefined symbols fragments and
output.
- Around line 113-156: The helper _run39_full_impl currently skips the real
decompile path steps (docstring pre-pass, _prescan_compound_conds(), and
post_process_source()) and returns unprocessed reconstructed text; update
_run39_full_impl to run the same sequence as DecompilerGeneric.decompile():
perform the docstring pre-pass (the same pre-pass used by Decompiler39), call
dec._prescan_compound_conds() before iterating instructions, and after finishing
and calling dec._close_blocks(...) call dec.post_process_source() (and any
docstring handling required) so the helper produces the same post-processed
output as Decompiler39.decompile().
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7783b7a-7a54-4091-a813-825207f8ff6e
📒 Files selected for processing (3)
pycrefine.pytest_files/verify_scenes.pytests/test_pycrefine.py
Comment on lines
+157
to
+161
| """Assert that none of the fragments appear in *output*.""" | ||
| for frag in fragments: | ||
| assert frag not in output, ( | ||
| f"Unexpected fragment {frag!r} found in decompiled output:\n{output}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Please remove the unreachable copied block after the return.
With respect, Lines 157-160 still reference fragments and output, so Ruff reports F821 here even though the block can never execute.
✂️ Minimal cleanup
- """Assert that none of the fragments appear in *output*."""
- for frag in fragments:
- assert frag not in output, (
- f"Unexpected fragment {frag!r} found in decompiled output:\n{output}"
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """Assert that none of the fragments appear in *output*.""" | |
| for frag in fragments: | |
| assert frag not in output, ( | |
| f"Unexpected fragment {frag!r} found in decompiled output:\n{output}" | |
| ) |
🧰 Tools
🪛 Ruff (0.15.7)
[error] 158-158: Undefined name fragments
(F821)
[error] 159-159: Undefined name output
(F821)
[error] 160-160: Undefined name output
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_pycrefine.py` around lines 157 - 161, Remove the unreachable
post-return assertion block that references fragments and output: delete the for
loop that iterates over fragments and the accompanying assert (the copied block
containing "for frag in fragments: assert frag not in output, ...") so the test
no longer contains code after the function's return that refers to the undefined
symbols fragments and output.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Request timed out after 1200000ms (requestId=7bf97a0a-483e-42be-b2c5-4962dcc2e027) |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 21 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 21 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_pycrefine.py (1)
4348-4349: Let this assertion be firm, but not brittle.Line 4349 hard-codes the fully formatted expression. That can fail on harmless parenthesisation or spacing changes, even though this suite otherwise tries to tolerate them. It is steadier to assert the augmented operator, the ternary keywords, and both branch values separately.
Keep the regression focused on structure
- # The output must include the variable and the augmented-ternary structure - self.assertIn("s += a if c else b", out, f"Augmented-ternary structure missing: {out}") + # Keep this resilient to harmless formatting differences. + self.assertIn("s", out, f"Target missing:\n{out}") + self.assertIn("+=", out, f"Augmented operator missing:\n{out}") + self.assertIn(" if ", out, f"Ternary condition missing:\n{out}") + self.assertIn(" else ", out, f"Ternary else branch missing:\n{out}") + self.assertIn("a", out, f"Then-branch value missing:\n{out}") + self.assertIn("b", out, f"Else-branch value missing:\n{out}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_pycrefine.py` around lines 4348 - 4349, Replace the brittle single assertIn that checks the entire formatted expression "s += a if c else b" with separate, tolerant assertions: ensure the augmented operator token "+=" appears in out, that the ternary keywords " if " and " else " appear, and that both branch values "a" and "b" appear (use self.assertIn for each with clear failure messages). This keeps the regression focused on structure (augmented operator, ternary keywords, branch values) while tolerating harmless spacing/parenthesisation changes.
🤖 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 156-160: The doubled-assignment cleanup regex used in the re.match
call (assigned to m_double) omits the matrix-multiply augmented assignment
operator "@=", so add "@" to the operator alternation in the pattern (i.e.,
include @ in the group that currently lists +|-|*|/|//|%|&|\||\^|<<|>>|**) so
expressions like x = (x @= y) are matched and converted; update the regex in the
re.match(...) call accordingly while preserving existing escaping and the
trailing "=" part of the alternation.
- Around line 2783-2792: The parameter reconstruction currently appends
keyword-only params before varargs, producing invalid signatures; update the
logic around params, positional, kwonly_params, varargs_name, and varkw_name so
that if varargs_name exists you append varargs_name first, then append
kwonly_params; if varargs_name is absent but kwonly_params exists, append a bare
"*" before adding kwonly_params; finally handle varkw_name as before—this
ensures the order positional → *args (or bare *) → keyword-only → **kwargs.
In `@tests/test_pycrefine.py`:
- Around line 4187-4190: The test currently allows >=2 "try:" blocks which masks
stray try blocks; change the assertion in tests/test_pycrefine.py to require
exactly two occurrences by asserting try_count == 2 (replace
self.assertGreaterEqual(try_count, 2, ...) with an equality assertion), keeping
the diagnostic message that includes out and using the existing try_count and
out variables to report failures.
- Around line 4073-4093: The test's synthetic ternary isn't a real diamond
because the JUMP_FORWARD (Instr with opcode 110) targets the wrong offset, so
_prescan_ternaries() may skip the pattern; update that Instr(110,
"JUMP_FORWARD", ...) so its target jumps to the STORE_FAST instruction's offset
(so the true branch skips over the else branch and both branches converge at
STORE_FAST), and ensure the post-store statement (the RETURN_VALUE at offset 12)
remains as the real instruction after the ternary boundary so _ternary_suppress
(used by _prescan_ternaries) can be correctly exercised.
---
Nitpick comments:
In `@tests/test_pycrefine.py`:
- Around line 4348-4349: Replace the brittle single assertIn that checks the
entire formatted expression "s += a if c else b" with separate, tolerant
assertions: ensure the augmented operator token "+=" appears in out, that the
ternary keywords " if " and " else " appear, and that both branch values "a" and
"b" appear (use self.assertIn for each with clear failure messages). This keeps
the regression focused on structure (augmented operator, ternary keywords,
branch values) while tolerating harmless spacing/parenthesisation changes.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d058ce7f-b558-4a9e-92a2-442c5c5e1538
📒 Files selected for processing (15)
debug/custom_scan.pydebug/deep_scan.pydebug/detailed_inspect.pydebug/dump_313.pydebug/dump_inst.pydebug/dump_raw.pydebug/dump_raw2.pydebug/dump_std.pydebug/inspect_code.pydebug/scan_all.pydebug/scan_marshal.pydebug/test_marshal.pydebug/test_simple.pypycrefine.pytests/test_pycrefine.py
✅ Files skipped from review due to trivial changes (13)
- debug/dump_inst.py
- debug/custom_scan.py
- debug/test_marshal.py
- debug/dump_raw.py
- debug/dump_313.py
- debug/detailed_inspect.py
- debug/scan_all.py
- debug/dump_std.py
- debug/inspect_code.py
- debug/test_simple.py
- debug/dump_raw2.py
- debug/deep_scan.py
- debug/scan_marshal.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 21 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 21 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/test_pycrefine.py (2)
4336-4350:⚠️ Potential issue | 🟠 MajorThis regression remains red until the emit path preserves
+=.CI is already failing on Python 3.12 and 3.14 here: the current decompiler output is still
s = a if c else b. Kindly land the corresponding augmented-ternary emit fix inpycrefine.pywith this test, otherwise the branch cannot go green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_pycrefine.py` around lines 4336 - 4350, The decompiler is dropping the augmented operator for augmented-ternary expressions; update the emitter in pycrefine.py (the code path that handles AugAssign / visit_AugAssign or the emit function that formats assignment expressions) to detect when node.value is an IfExp and emit "target <op>= <body> if <test> else <orelse>" using the actual operator symbol from node.op (e.g., Add -> "+") instead of lowering to "target = ..."; ensure the emitted string preserves the augmented token (+=) and the conditional expression structure so the test test_augmented_add_ternary_keeps_augmented_form passes.
4073-4093:⚠️ Potential issue | 🟡 MinorKindly make this a positive suppression check as well.
This still passes when
_prescan_ternaries()fails to recognise the diamond entirely, because an empty_ternary_suppressalready satisfies12 not in suppress. Please also assert that one or both ternary branch offsets are present in the suppress set, and ideally keep a real post-store statement after the sharedSTORE_FAST.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_pycrefine.py` around lines 4073 - 4093, Update the test_ternary_suppression_does_not_leak_into_next_statement to include a positive assertion that _prescan_ternaries() actually detected the ternary: after calling dec._prescan_ternaries(), assert that dec._ternary_suppress contains at least one of the branch offsets (the two branch target offsets present in dec.instructions for the ternary branches) so the test fails if the diamond wasn't recognized; additionally, replace or append a real post-store instruction after the shared STORE_FAST entry in dec.instructions so there is a concrete statement to verify is not suppressed by checking that its offset (previously 12 for RETURN_VALUE) is not in _ternary_suppress.pycrefine.py (1)
849-873:⚠️ Potential issue | 🔴 CriticalThe augmented ternary still loses its
+=form.A quiet gap remains here: this only recognizes the in-place op when it sits at the merge target. On 3.12/3.14, the
BINARY_OP 13..25/INPLACE_*can sit at the tail of each branch with only the finalSTORE_*shared, so those ops are absorbed intothen/else_instrs, suppressed, and the shared store emitss = ...—which matches the failing pipeline case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pycrefine.py` around lines 849 - 873, When determining is_inplace around the shared store (st_idx) also check for an in-place/BINARY_OP augmented operation sitting at the tail of each branch (not only at the merge target): scan the instruction immediately before st_idx (and if not found, the last instruction in the candidate branch slice t_idx:st_idx) for opname starting with "INPLACE_" or opname == "BINARY_OP" with arg in 13..25; if found set is_inplace = True, advance st_idx past the STORE as you do now, and remove that trailing inplace/BINARY_OP from the then/else instruction slices (else_raw/then_raw) so the shared STORE will be emitted as an inplace assignment (e.g. +=). Reference symbols: st_idx, is_inplace, t_idx, self.instructions, BINARY_OP, INPLACE_*, _TERNARY_STORES, else_raw, else_end.
🤖 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 2783-2798: The reconstructed signature currently drops the
positional-only marker; use inner_code.co_posonlyargcount to preserve it by
inserting a '/' separator into the params list after the first
co_posonlyargcount entries (e.g. params.insert(posonly_count, "/") when
posonly_count > 0) so signatures like def f(a, /, b) are emitted correctly;
ensure this insertion happens before adding varargs/kw-only handling so it works
whether or not varargs_name, kwonly_params or varkw_name are present and handle
the edge case where all parameters are positional-only by still emitting the
trailing '/'.
- Around line 2775-2781: The kw-only defaults from kw_defs are not applied when
building kwonly_params; update the block that computes kwonly_params (using
inner_code.co_kwonlyargcount and inner_code.co_varnames) to look up each kw-only
name in the popped kw_defs mapping and attach its default (e.g., produce a
representation like "name=default" or an AST default node) when present;
reference inner_code, kw_defs, and kwonly_params so you locate and modify the
signature construction logic and ensure functions like def f(*, x=1) render with
their default values.
In `@tests/test_pycrefine.py`:
- Around line 179-183: The helper _run39_full_impl() currently builds raw_source
from dec.reconstructed and calls post_process_source() but skips the
module-level compiler-generated "return None" suppression that
Decompiler39.decompile() performs; update _run39_full_impl() to mirror that
cleanup by detecting and removing a trailing top-level "return None" (the same
suppression logic Decompiler39.decompile() uses) from raw_source (or from
dec.reconstructed) before calling post_process_source(), so tests follow the
identical output path as Decompiler39.decompile().
---
Duplicate comments:
In `@pycrefine.py`:
- Around line 849-873: When determining is_inplace around the shared store
(st_idx) also check for an in-place/BINARY_OP augmented operation sitting at the
tail of each branch (not only at the merge target): scan the instruction
immediately before st_idx (and if not found, the last instruction in the
candidate branch slice t_idx:st_idx) for opname starting with "INPLACE_" or
opname == "BINARY_OP" with arg in 13..25; if found set is_inplace = True,
advance st_idx past the STORE as you do now, and remove that trailing
inplace/BINARY_OP from the then/else instruction slices (else_raw/then_raw) so
the shared STORE will be emitted as an inplace assignment (e.g. +=). Reference
symbols: st_idx, is_inplace, t_idx, self.instructions, BINARY_OP, INPLACE_*,
_TERNARY_STORES, else_raw, else_end.
In `@tests/test_pycrefine.py`:
- Around line 4336-4350: The decompiler is dropping the augmented operator for
augmented-ternary expressions; update the emitter in pycrefine.py (the code path
that handles AugAssign / visit_AugAssign or the emit function that formats
assignment expressions) to detect when node.value is an IfExp and emit "target
<op>= <body> if <test> else <orelse>" using the actual operator symbol from
node.op (e.g., Add -> "+") instead of lowering to "target = ..."; ensure the
emitted string preserves the augmented token (+=) and the conditional expression
structure so the test test_augmented_add_ternary_keeps_augmented_form passes.
- Around line 4073-4093: Update the
test_ternary_suppression_does_not_leak_into_next_statement to include a positive
assertion that _prescan_ternaries() actually detected the ternary: after calling
dec._prescan_ternaries(), assert that dec._ternary_suppress contains at least
one of the branch offsets (the two branch target offsets present in
dec.instructions for the ternary branches) so the test fails if the diamond
wasn't recognized; additionally, replace or append a real post-store instruction
after the shared STORE_FAST entry in dec.instructions so there is a concrete
statement to verify is not suppressed by checking that its offset (previously 12
for RETURN_VALUE) is not in _ternary_suppress.
🪄 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: aaa271df-6fdb-45dc-a7e5-dbc7dc7072bf
📒 Files selected for processing (2)
pycrefine.pytests/test_pycrefine.py
…s and enhance function signature parsing for keyword-only and positional-only arguments.
sahebbiswas
deleted the
43-bug-python-39-decompilation-produces-faulty-control-flow-argument-and-syntax-changes
branch
April 2, 2026 02:35
This was referenced Apr 2, 2026
This was referenced Apr 18, 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
Bug Fixes
New Features
Tests
Chores