#16 Improve generated code cleanliness - #17
Merged
Conversation
Closed
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWeights adjusted and a token-level Hamming scorer added to the coherency checker; cleanliness detection rewritten to ignore string literals and use regex-anchored artefact counts. Decompiler output post-processing (imports, parentheses, blank-line compaction, tuple→expression rendering) implemented and test coverage expanded, hmmm. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@debug/check_coherency.py`:
- Around line 603-610: The regex _TUPLE_LEAK_RE is too narrow because it only
matches ('func'/'class',...) in statement-start positions; relax it to detect
raw tuple literals anywhere by removing the statement/assignment/return/yield
anchoring and just search for a parenthesized tuple whose first element is
'func' or 'class' (e.g. use r"\(\s*['\"](?:func|class)['\"]\s*,") with the same
flags; make the same change to the second tuple-leak regex referenced around
lines 633-637 so both patterns catch tuples in call arguments, lists, or nested
expressions.
- Around line 717-756: Replace the current use of dec_set with a consumed-count
approach so repeated decompiled lines are not reused: build a
collections.Counter (e.g. dec_counts) from dec_lines (instead of dec_set) and
check/decrement counts when you claim a match. In the loop over orig_lines, when
you detect an exact match use dec_counts[ol] > 0 and decrement it after adding
to total_match; for fuzzy matches, only accept a best_line if
dec_counts[best_line] > 0 and then decrement that count (or otherwise treat as
no-match), and keep using dec_tok_cache, _best_match and dec_index as before;
this reserves each decompiled line once for exact or fuzzy matching so
total_match no longer overstates due to reused lines.
- Around line 538-563: The string-stripper loop in debug/check_coherency.py
misinterprets quotes inside comments because it never checks for '#' before
handling quotes; update the while i < n loop to detect a comment start (text[i]
== '#') and skip to the end of line (append the comment content to out or
advance i to the next newline) before attempting triple-quote or single-quote
handling so that comments like "# don't touch this" don't open a string; ensure
this change is applied around the existing variables and logic (text, i, n, out,
q3, q, j) so subsequent string-detection code only runs when not inside a
comment.
In `@tests/test_pycrefine.py`:
- Line 1370: Multiple occurrences create a single-letter alias "I =
BytecodeInstruction" which triggers Ruff E741; replace each alias with a clearer
name (e.g., "Instr = BytecodeInstruction" or "Insn = BytecodeInstruction") and
then update all uses of the alias in the file to the new name (search for "I"
aliases and references near the existing BytecodeInstruction usages such as in
tests/test_pycrefine.py). Ensure you change every instance listed (the repeated
alias definitions) so imports and type references remain correct and the linter
no longer flags E741.
🪄 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: 2a2dc5d5-5043-48a1-88e3-4101c54628e5
📒 Files selected for processing (3)
debug/check_coherency.pypycrefine.pytests/test_pycrefine.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pycrefine.py
Comment on lines
+538
to
+563
| while i < n: | ||
| # Triple-quoted strings (""" or ''') | ||
| for q3 in ('"""', "'''"): | ||
| if text[i:i+3] == q3: | ||
| end = text.find(q3, i + 3) | ||
| if end != -1: | ||
| out.append(q3 + q3) # replace content with empty triple-quote | ||
| i = end + 3 | ||
| break | ||
| else: | ||
| # Single-quoted strings (" or ') | ||
| if text[i] in ('"', "'"): | ||
| q = text[i] | ||
| j = i + 1 | ||
| while j < n: | ||
| if text[j] == '\\': | ||
| j += 2 | ||
| continue | ||
| if text[j] == q: | ||
| break | ||
| j += 1 | ||
| out.append(q + q) # replace content with empty string | ||
| i = j + 1 | ||
| else: | ||
| out.append(text[i]) | ||
| i += 1 |
Contributor
There was a problem hiding this comment.
Quotes inside comments, this stripper misreads.
Line 549 opens a string even after #. A comment like # don't touch this will scan until the next ' and can hide later artefacts from score_cleanliness.
Patch suggestion
while i < n:
+ if text[i] == '#':
+ nl = text.find('\n', i)
+ if nl == -1:
+ out.append(text[i:])
+ break
+ out.append(text[i:nl + 1])
+ i = nl + 1
+ continue
# Triple-quoted strings (""" or ''')
for q3 in ('"""', "'''"):
if text[i:i+3] == q3:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@debug/check_coherency.py` around lines 538 - 563, The string-stripper loop in
debug/check_coherency.py misinterprets quotes inside comments because it never
checks for '#' before handling quotes; update the while i < n loop to detect a
comment start (text[i] == '#') and skip to the end of line (append the comment
content to out or advance i to the next newline) before attempting triple-quote
or single-quote handling so that comments like "# don't touch this" don't open a
string; ensure this change is applied around the existing variables and logic
(text, i, n, out, q3, q, j) so subsequent string-detection code only runs when
not inside a comment.
Comment on lines
+603
to
+610
| # Assignment/return-anchored regex for raw ('func',...) / ('class',...) leakage. | ||
| # Applied to original dec_text because _strip_string_literals blanks the key strings. | ||
| _TUPLE_LEAK_RE = re.compile( | ||
| r"(?:^|\n)[ \t]*(?:[A-Za-z_][A-Za-z0-9_.]*[ \t]*=[ \t]*" | ||
| r"|return[ \t]+|yield[ \t]+)" | ||
| r"\([ \t]*['\"](?:func|class)['\"][ \t]*,", | ||
| re.MULTILINE, | ||
| ) |
Contributor
There was a problem hiding this comment.
Statement-anchored tuple leak check, too narrow this is.
Lines 606-608 only detect raw tuples in assignment, return, or yield positions. Because pycrefine.py:221-235 only rewrites statement-position leaks too, raw ('func', ...) / ('class', ...) inside call arguments, list literals, or nested expressions can survive and still score as clean.
Also applies to: 633-637
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@debug/check_coherency.py` around lines 603 - 610, The regex _TUPLE_LEAK_RE is
too narrow because it only matches ('func'/'class',...) in statement-start
positions; relax it to detect raw tuple literals anywhere by removing the
statement/assignment/return/yield anchoring and just search for a parenthesized
tuple whose first element is 'func' or 'class' (e.g. use
r"\(\s*['\"](?:func|class)['\"]\s*,") with the same flags; make the same change
to the second tuple-leak regex referenced around lines 633-637 so both patterns
catch tuples in call arguments, lists, or nested expressions.
Comment on lines
+717
to
+756
| dec_set = set(dec_lines) | ||
| dec_index = _build_trigram_index(dec_lines) | ||
|
|
||
| # Cache tokenised dec lines so we only tokenise each unique dec line once | ||
| dec_tok_cache: Dict[str, List[str]] = {} | ||
|
|
||
| total_orig = 0 | ||
| total_match = 0 | ||
| flip_sample: List[str] = [] # collect up to ~500 flipped tokens for detail | ||
|
|
||
| for ol in orig_lines: | ||
| o_toks = _line_tokenise(ol) | ||
| if not o_toks: | ||
| continue | ||
| total_orig += len(o_toks) | ||
|
|
||
| if ol in dec_set: | ||
| # Exact line match -- every token agrees | ||
| total_match += len(o_toks) | ||
| else: | ||
| best_line, _ = _best_match(ol, dec_lines, dec_index, cutoff=0.30) | ||
| if best_line is not None: | ||
| if best_line not in dec_tok_cache: | ||
| dec_tok_cache[best_line] = _line_tokenise(best_line) | ||
| d_toks = dec_tok_cache[best_line] | ||
| sm = difflib.SequenceMatcher(None, o_toks, d_toks, autojunk=False) | ||
| matched = sum(b.size for b in sm.get_matching_blocks()) | ||
| total_match += matched | ||
| # Collect flipped tokens from this line for the detail string | ||
| if len(flip_sample) < 500: | ||
| for tag, i1, i2, _, _ in sm.get_opcodes(): | ||
| if tag in ('replace', 'delete'): | ||
| flip_sample.extend(o_toks[i1:i2]) | ||
| else: | ||
| # No close match at all -- every token in this line is a flip | ||
| flip_sample.extend(o_toks) | ||
|
|
||
| flips = total_orig - total_match | ||
| score = total_match / total_orig if total_orig > 0 else 1.0 | ||
| return score, total_match, total_orig, flips, flip_sample |
Contributor
There was a problem hiding this comment.
Reusable matches, the Hamming score overstates.
Lines 733-744 never consume a matched decompiled line, so duplicate source lines are over-scored. ['x = 1', 'x = 1'] versus ['x = 1'] returns perfect agreement today. Track remaining exact counts and reserve fuzzy-match indices once used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@debug/check_coherency.py` around lines 717 - 756, Replace the current use of
dec_set with a consumed-count approach so repeated decompiled lines are not
reused: build a collections.Counter (e.g. dec_counts) from dec_lines (instead of
dec_set) and check/decrement counts when you claim a match. In the loop over
orig_lines, when you detect an exact match use dec_counts[ol] > 0 and decrement
it after adding to total_match; for fuzzy matches, only accept a best_line if
dec_counts[best_line] > 0 and then decrement that count (or otherwise treat as
no-match), and keep using dec_tok_cache, _best_match and dec_index as before;
this reserves each decompiled line once for exact or fuzzy matching so
total_match no longer overstates due to reused lines.
| 14 RETURN_VALUE <- loop exit (is_jump_target=True) | ||
| """ | ||
| Instr = BytecodeInstruction | ||
| I = BytecodeInstruction |
Contributor
There was a problem hiding this comment.
I alias, rename you should.
Line 1370 and the repeated alias definitions below trip Ruff E741, so lint pass this file will not. Instr or Insn instead, use.
Also applies to: 1390-1390, 1418-1418, 1463-1463, 1506-1506, 1536-1536, 1561-1561, 1592-1592, 1622-1622, 1637-1637, 1649-1649, 1661-1661, 1677-1677, 1709-1709, 1735-1735
🧰 Tools
🪛 Ruff (0.15.6)
[error] 1370-1370: Ambiguous variable name: I
(E741)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_pycrefine.py` at line 1370, Multiple occurrences create a
single-letter alias "I = BytecodeInstruction" which triggers Ruff E741; replace
each alias with a clearer name (e.g., "Instr = BytecodeInstruction" or "Insn =
BytecodeInstruction") and then update all uses of the alias in the file to the
new name (search for "I" aliases and references near the existing
BytecodeInstruction usages such as in tests/test_pycrefine.py). Ensure you
change every instance listed (the repeated alias definitions) so imports and
type references remain correct and the linter no longer flags E741.
This was referenced Mar 25, 2026
This was referenced Apr 3, 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
Documentation