Conversation
📝 WalkthroughWalkthroughThe changes refactor Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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 |
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 `@scripts/generate_opcode_metadata.py`:
- Around line 94-109: The current brace-scan can leave block_start or block_end
as None and then silently slice raw_body[block_start:block_end]; update the scan
logic (using variables brace_depth, block_start, block_end, match_start) to
validate after the loop that both block_start and block_end were found and
raise/throw a clear parsing error (including context like match_start or a
snippet of raw_body) if either is None so the script fails fast instead of
producing incomplete match_body/_specializations.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9108f4ed-8656-4d4a-b924-04e0a75cd111
📒 Files selected for processing (2)
crates/compiler-core/src/bytecode/instruction.rsscripts/generate_opcode_metadata.py
| brace_depth = 0 | ||
| block_start = None | ||
| block_end = None | ||
|
|
||
| if depth == 0 and (char in ("}", ",")): | ||
| arm = "".join(buf).strip() | ||
| arms.append(arm) | ||
| buf = [] | ||
| else: | ||
| buf.append(char) | ||
| for i, ch in enumerate(raw_body[match_start:], match_start): | ||
| if ch == "{": | ||
| brace_depth += 1 | ||
| if block_start is None: | ||
| block_start = i + 1 | ||
| elif ch == "}": | ||
| brace_depth -= 1 | ||
| if brace_depth == 0: | ||
| block_end = i | ||
| break | ||
|
|
||
| match_body = raw_body[block_start:block_end] |
There was a problem hiding this comment.
Fail fast if the brace scan never closes the match block.
raw_body[block_start:block_end] accepts None, so a parse miss here degrades into slicing to EOF instead of surfacing an error. That makes it possible to emit incomplete _specializations data silently if a valid future edit in deopt() throws off the brace counter.
Suggested guard
for i, ch in enumerate(raw_body[match_start:], match_start):
if ch == "{":
brace_depth += 1
if block_start is None:
block_start = i + 1
elif ch == "}":
brace_depth -= 1
if brace_depth == 0:
block_end = i
break
+ if block_start is None or block_end is None:
+ raise ValueError("Could not isolate deopt match block")
+
match_body = raw_body[block_start:block_end]📝 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.
| brace_depth = 0 | |
| block_start = None | |
| block_end = None | |
| if depth == 0 and (char in ("}", ",")): | |
| arm = "".join(buf).strip() | |
| arms.append(arm) | |
| buf = [] | |
| else: | |
| buf.append(char) | |
| for i, ch in enumerate(raw_body[match_start:], match_start): | |
| if ch == "{": | |
| brace_depth += 1 | |
| if block_start is None: | |
| block_start = i + 1 | |
| elif ch == "}": | |
| brace_depth -= 1 | |
| if brace_depth == 0: | |
| block_end = i | |
| break | |
| match_body = raw_body[block_start:block_end] | |
| brace_depth = 0 | |
| block_start = None | |
| block_end = None | |
| for i, ch in enumerate(raw_body[match_start:], match_start): | |
| if ch == "{": | |
| brace_depth += 1 | |
| if block_start is None: | |
| block_start = i + 1 | |
| elif ch == "}": | |
| brace_depth -= 1 | |
| if brace_depth == 0: | |
| block_end = i | |
| break | |
| if block_start is None or block_end is None: | |
| raise ValueError("Could not isolate deopt match block") | |
| match_body = raw_body[block_start:block_end] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate_opcode_metadata.py` around lines 94 - 109, The current
brace-scan can leave block_start or block_end as None and then silently slice
raw_body[block_start:block_end]; update the scan logic (using variables
brace_depth, block_start, block_end, match_start) to validate after the loop
that both block_start and block_end were found and raise/throw a clear parsing
error (including context like match_start or a snippet of raw_body) if either is
None so the script fails fast instead of producing incomplete
match_body/_specializations.
There was a problem hiding this comment.
This is fine, we don't care about the closing the match block, we care only about the match arms
There was a problem hiding this comment.
@ShaharNaveh, that's a fair point. Since the arm-extraction regex (Self::\w+ => Opcode::\w+) is specific enough that it won't produce false positives from unrelated code even if match_body happens to extend to EOF, the tight bounding of the block isn't strictly required for correctness. I'll withdraw the suggestion.
🧠 Learnings used
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7573
File: scripts/generate_opcode_metadata.py:69-85
Timestamp: 2026-04-15T08:58:35.342Z
Learning: In `scripts/generate_opcode_metadata.py` (RustPython repo), `extract_enum_body` intentionally returns `None` when the enum is not found or the closing brace is missing. The author wants the caller to crash with a `TypeError` in that case, rather than raising an explicit exception. Do not suggest converting `return None` to `raise ValueError(...)` in this function.
Summary by CodeRabbit