GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Simplify `Intruction.deopt()` by ShaharNaveh · Pull Request #7615 · RustPython/RustPython

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Apr 16, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Refactor
    • Improved internal compiler infrastructure and code organization for better maintainability.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ShaharNaveh added 2 commits April 16, 2026 10:30
@ShaharNaveh
Simplify Instruction.deopt()
58419b1
@ShaharNaveh
Adjust scripts/generate_opcode_metadata.py
dfaa089
@coderabbitai

coderabbitai Bot commented Apr 16, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The changes refactor Instruction::deopt to delegate base opcode construction to Opcode::as_instruction() instead of inline argument handling. Correspondingly, the metadata generation script is rewritten to parse deopt match arms via brace-depth scanning and regex, replacing prior line-based filtering logic.

Changes

Cohort / File(s) Summary
Opcode Deoptimization Refactoring
crates/compiler-core/src/bytecode/instruction.rs
Restructured Instruction::deopt to map specialized opcodes (e.g., ResumeCheck, LoadConstMortal, ToBool*, BinaryOp*, StoreSubscr*, etc.) to corresponding base Opcode variants, delegating marker-field construction to Opcode::as_instruction().
Metadata Parsing Update
scripts/generate_opcode_metadata.py
Rewrote build_deopts to parse deopt implementation using brace-depth scanning for match block extraction and regex for arm extraction, replacing line-based filtering heuristics with explicit error handling for missing match self.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 With ops that hop through deopt's door,
We delegate to as_instruction more,
Brace-depths are scanned with careful care,
A refactor crisp floats through the air!
Bytecode now bounds with pattern so fair,
✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Simplify Intruction.deopt()' directly matches the main change: refactoring the deopt method to delegate argument construction to Opcode::as_instruction() rather than inlining it.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share
  • X
  • Mastodon
  • Reddit
  • LinkedIn

Comment @coderabbitai help to get the list of available commands and usage tips.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai[bot]
coderabbitai Bot reviewed Apr 16, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between aa12acc and dfaa089.

📒 Files selected for processing (2)
  • crates/compiler-core/src/bytecode/instruction.rs
  • scripts/generate_opcode_metadata.py

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread scripts/generate_opcode_metadata.py
Comment on lines +94 to +109
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]

@coderabbitai coderabbitai Bot Apr 16, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ShaharNaveh ShaharNaveh Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai[bot] reacted with eyes emoji
All reactions
  • 👀 1 reaction

@coderabbitai coderabbitai Bot Apr 16, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.


🧠 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
youknowone
youknowone approved these changes Apr 17, 2026
View reviewed changes
@youknowone
youknowone merged commit 2827eca into RustPython:main Apr 17, 2026
20 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@coderabbitai coderabbitai Bot mentioned this pull request May 1, 2026
AnyInstruction helpers + doc imrovments #7757
Merged
@coderabbitai coderabbitai Bot mentioned this pull request May 9, 2026
Autogen instructions & opcodes #7797
Merged
youknowone pushed a commit that referenced this pull request Sep 16, 2026
@ShaharNaveh
Simplify Intruction.deopt() (#7615)
8d1000d
* Simplify `Instruction.deopt()`

* Adjust `scripts/generate_opcode_metadata.py`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@youknowone youknowone youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@ShaharNaveh @youknowone
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.