GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Simplify `Intruction.deopt()` by ShaharNaveh · Pull Request #7615 · RustPython/RustPython 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9108f4ed-8656-4d4a-b924-04e0a75cd111
crates/compiler-core/src/bytecode/instruction.rsscripts/generate_opcode_metadata.py
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
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]
‼️ 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] |
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is fine, we don't care about the closing the match block, we care only about the match arms
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.