Align _opcode_metadata.py to 3.14.3#7456
Open
ShaharNaveh wants to merge 8 commits intoRustPython:mainfrom
Open
Align _opcode_metadata.py to 3.14.3#7456ShaharNaveh wants to merge 8 commits intoRustPython:mainfrom
_opcode_metadata.py to 3.14.3#7456ShaharNaveh wants to merge 8 commits intoRustPython:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Python setup step to CI; converts multiple Instruction helpers to const, centralizes deoptimization via a new Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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)
📝 Coding Plan
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 |
ShaharNaveh
commented
Mar 17, 2026
Contributor
Author
There was a problem hiding this comment.
Changes to this file are purely "cosmetic". Altering the arms order (and variants order) so the generated output will match CPython
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/opcode.py dependencies:
dependent tests: (44 tests)
[x] lib: cpython/Lib/dis.py dependencies:
dependent tests: (70 tests)
Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-format.yaml (1)
55-61:⚠️ Potential issue | 🟡 MinorMention opcode metadata regeneration in the failure hint.
Line 61 still tells contributors to rerun only
cargo fmt/ruff, but this job now also rewritesLib/_opcode_metadata.py. Ifgenerate_opcode_metadata.pyis the only source of diff, the current message sends people to the wrong fix.Suggested tweak
- echo "::error::Formatting changes detected. Please run 'cargo fmt --all', 'ruff format', and 'ruff check --select I --fix' locally." + echo "::error::Formatting or generated-file changes detected. Please run 'cargo fmt --all', 'ruff format', 'ruff check --select I --fix', and 'python scripts/generate_opcode_metadata.py' locally."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-format.yaml around lines 55 - 61, Update the failure hint in the "Check for formatting changes" job so it mentions regenerating opcode metadata; specifically, when the git diff check fails after running scripts/generate_opcode_metadata.py, include guidance to run python scripts/generate_opcode_metadata.py (and commit Lib/_opcode_metadata.py) in addition to 'cargo fmt --all', 'ruff format', and 'ruff check --select I --fix' so contributors know to regenerate Lib/_opcode_metadata.py if that is the only change.
🤖 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 75-78: The current build_deopts function assumes re.search found a
match and calls .group(1) which raises an opaque AttributeError if the
deoptimize() signature isn't present; update build_deopts to check the result of
re.search(r"fn deoptimize\(self\) -> Self(.*)", contents, re.DOTALL) (the match
object used to produce raw_body) and if it's None raise a ValueError with a
clear message like "deoptimize() signature not found in input contents" so CI
failures are immediately actionable.
- Around line 133-136: The generated file banner stored in the output variable
in generate_opcode_metadata.py still says "CPython 3.13 compatible opcode
numbers"; update that string to reference "CPython 3.14 compatible opcode
numbers" (and adjust any surrounding version numbers in the same output
variable) so regenerated artifacts advertise the correct 3.14 version; locate
the output variable definition near the top of the script and change the
embedded version text accordingly.
---
Outside diff comments:
In @.github/workflows/pr-format.yaml:
- Around line 55-61: Update the failure hint in the "Check for formatting
changes" job so it mentions regenerating opcode metadata; specifically, when the
git diff check fails after running scripts/generate_opcode_metadata.py, include
guidance to run python scripts/generate_opcode_metadata.py (and commit
Lib/_opcode_metadata.py) in addition to 'cargo fmt --all', 'ruff format', and
'ruff check --select I --fix' so contributors know to regenerate
Lib/_opcode_metadata.py if that is the only change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d00895b0-bbe5-4a1f-95f1-f0e288b1617c
⛔ Files ignored due to path filters (2)
Lib/_opcode_metadata.pyis excluded by!Lib/**Lib/test/test_dis.pyis excluded by!Lib/**
📒 Files selected for processing (3)
.github/workflows/pr-format.yamlcrates/compiler-core/src/bytecode/instruction.rsscripts/generate_opcode_metadata.py
Comment on lines
+75
to
+78
| def build_deopts(contents: str) -> dict[str, list[str]]: | ||
| raw_body = re.search( | ||
| r"fn deoptimize\(self\) -> Self(.*)", contents, re.DOTALL | ||
| ).group(1) |
Contributor
There was a problem hiding this comment.
Fail with a clear error when deoptimize() is not found.
If the Rust signature or surrounding text changes, this becomes an opaque AttributeError from .group(1). Raising a ValueError here makes CI failures much easier to diagnose.
Suggested tweak
def build_deopts(contents: str) -> dict[str, list[str]]:
- raw_body = re.search(
- r"fn deoptimize\(self\) -> Self(.*)", contents, re.DOTALL
- ).group(1)
+ deopt_match = re.search(
+ r"fn deoptimize\(self\) -> Self(.*)", contents, re.DOTALL
+ )
+ if not deopt_match:
+ raise ValueError("Could not find Instruction.deoptimize()")
+ raw_body = deopt_match.group(1)📝 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
| def build_deopts(contents: str) -> dict[str, list[str]]: | |
| raw_body = re.search( | |
| r"fn deoptimize\(self\) -> Self(.*)", contents, re.DOTALL | |
| ).group(1) | |
| def build_deopts(contents: str) -> dict[str, list[str]]: | |
| deopt_match = re.search( | |
| r"fn deoptimize\(self\) -> Self(.*)", contents, re.DOTALL | |
| ) | |
| if not deopt_match: | |
| raise ValueError("Could not find Instruction.deoptimize()") | |
| raw_body = deopt_match.group(1) |
🤖 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 75 - 78, The current
build_deopts function assumes re.search found a match and calls .group(1) which
raises an opaque AttributeError if the deoptimize() signature isn't present;
update build_deopts to check the result of re.search(r"fn deoptimize\(self\) ->
Self(.*)", contents, re.DOTALL) (the match object used to produce raw_body) and
if it's None raise a ValueError with a clear message like "deoptimize()
signature not found in input contents" so CI failures are immediately
actionable.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/generate_opcode_metadata.py (1)
1-5:⚠️ Potential issue | 🟡 MinorUpdate module docstring to reference CPython 3.14.
The docstring on line 4 still mentions "CPython 3.13" while the generated banner at line 134 correctly references 3.14.
Proposed fix
""" Generate Lib/_opcode_metadata.py for RustPython bytecode. -This file generates opcode metadata that is compatible with CPython 3.13. +This file generates opcode metadata that is compatible with CPython 3.14. """🤖 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 1 - 5, The module docstring at the top of scripts/generate_opcode_metadata.py still says "CPython 3.13"; update that string to "CPython 3.14" so the header matches the generated banner (the triple-quoted module docstring near the top of the file and the banner referenced around line 134). Keep the rest of the docstring text unchanged—only change the version token from "3.13" to "3.14".
♻️ Duplicate comments (1)
scripts/generate_opcode_metadata.py (1)
75-78:⚠️ Potential issue | 🟡 MinorAdd error handling when
deoptimize()signature is not found.The
re.search(...).group(1)call will raise an opaqueAttributeErrorif the Rust signature changes or isn't found. This makes CI failures difficult to diagnose.Proposed fix
def build_deopts(contents: str) -> dict[str, list[str]]: - raw_body = re.search( + deopt_match = re.search( r"fn deoptimize\(self\) -> Self(.*)", contents, re.DOTALL - ).group(1) + ) + if not deopt_match: + raise ValueError("Could not find Instruction.deoptimize() in source") + raw_body = deopt_match.group(1)🤖 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 75 - 78, The code in build_deopts uses re.search(...).group(1) which will raise an AttributeError if the deoptimize() signature isn't found; change it to first assign the result of re.search to a variable (e.g., match = re.search(r"fn deoptimize\(self\) -> Self(.*)", contents, re.DOTALL)), check if match is None, and if so raise a clear RuntimeError or ValueError with a descriptive message that includes enough context (e.g., mention build_deopts, deoptimize() not found and a short snippet or length of contents) so CI failures are actionable; otherwise use match.group(1) to set raw_body and continue.
🧹 Nitpick comments (1)
scripts/generate_opcode_metadata.py (1)
81-86: Rename ambiguous lambda parameterltoline.Flake8 E741 flags
las ambiguous (easily confused with1orI). Rename for clarity.Proposed fix
body = "\n".join( itertools.takewhile( - lambda l: not l.startswith("_ =>"), # Take until reaching fallback + lambda line: not line.startswith("_ =>"), # Take until reaching fallback filter( - lambda l: ( - not l.startswith(("//", "match")) # Skip comments or start of match + lambda line: ( + not line.startswith(("//", "match")) # Skip comments or start of match ), map(str.strip, raw_body.splitlines()), ), ) ).removeprefix("{")🤖 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 81 - 86, Rename the ambiguous lambda parameter `l` to `line` in the two lambdas used when processing raw_body (the lambda used in the take-until condition not l.startswith("_ =>") and the lambda inside filter that checks not l.startswith(("//", "match"))), i.e., update both anonymous functions to use `line` and adjust their bodies to call line.startswith(...) so Flake8 E741 is resolved while keeping behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/generate_opcode_metadata.py`:
- Around line 1-5: The module docstring at the top of
scripts/generate_opcode_metadata.py still says "CPython 3.13"; update that
string to "CPython 3.14" so the header matches the generated banner (the
triple-quoted module docstring near the top of the file and the banner
referenced around line 134). Keep the rest of the docstring text unchanged—only
change the version token from "3.13" to "3.14".
---
Duplicate comments:
In `@scripts/generate_opcode_metadata.py`:
- Around line 75-78: The code in build_deopts uses re.search(...).group(1) which
will raise an AttributeError if the deoptimize() signature isn't found; change
it to first assign the result of re.search to a variable (e.g., match =
re.search(r"fn deoptimize\(self\) -> Self(.*)", contents, re.DOTALL)), check if
match is None, and if so raise a clear RuntimeError or ValueError with a
descriptive message that includes enough context (e.g., mention build_deopts,
deoptimize() not found and a short snippet or length of contents) so CI failures
are actionable; otherwise use match.group(1) to set raw_body and continue.
---
Nitpick comments:
In `@scripts/generate_opcode_metadata.py`:
- Around line 81-86: Rename the ambiguous lambda parameter `l` to `line` in the
two lambdas used when processing raw_body (the lambda used in the take-until
condition not l.startswith("_ =>") and the lambda inside filter that checks not
l.startswith(("//", "match"))), i.e., update both anonymous functions to use
`line` and adjust their bodies to call line.startswith(...) so Flake8 E741 is
resolved while keeping behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 35f211ac-982d-48f9-ae6d-dae272ba7f40
⛔ Files ignored due to path filters (2)
Lib/_opcode_metadata.pyis excluded by!Lib/**Lib/test/test_dis.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/compiler-core/src/bytecode/instruction.rsscripts/generate_opcode_metadata.py
ShaharNaveh
commented
Mar 17, 2026
| | Self::CallTuple1 | ||
| | Self::CallType1 => Self::Call { | ||
| argc: Arg::marker(), | ||
| pub const fn deopt(self) -> Option<Self> { |
Contributor
Author
There was a problem hiding this comment.
I have extracted the specializations depot part of the deoptimize method into a separate one called deopt.
The behavior of deoptimize did not change
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/generate_opcode_metadata.py (1)
75-78:⚠️ Potential issue | 🟡 MinorHandle missing
deopt()match explicitly before.group(1).Line 76-Line 78 can fail with a non-actionable
AttributeErrorif the signature changes; raise a clearValueErrorinstead.Suggested fix
def build_deopts(contents: str) -> dict[str, list[str]]: - raw_body = re.search( - r"fn deopt\(self\) -> Option<Self>(.*)", contents, re.DOTALL - ).group(1) + deopt_match = re.search( + r"fn deopt\(self\) -> Option<Self>(.*)", contents, re.DOTALL + ) + if not deopt_match: + raise ValueError("Could not find Instruction.deopt()") + raw_body = deopt_match.group(1)🤖 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 75 - 78, The regex search in build_deopts currently calls .group(1) on the result of re.search which will raise an AttributeError if no match is found; instead, capture the match into a variable, check if it is None, and if so raise a descriptive ValueError (e.g., "deopt() signature not found in contents") before accessing group(1); update the code in build_deopts to use the match variable and raise the clear error to make failures actionable.
🤖 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 81-83: The lambdas using the ambiguous parameter name "l" (in the
first lambda used with take-until comment "Take until reaching fallback" and the
second lambda passed into filter) should be renamed to a descriptive parameter
like "line" to satisfy Flake8 E741; update both occurrences (the lambda in the
take/until expression and the lambda passed to filter) to use "line" and adjust
any internal references accordingly so behavior is unchanged.
---
Duplicate comments:
In `@scripts/generate_opcode_metadata.py`:
- Around line 75-78: The regex search in build_deopts currently calls .group(1)
on the result of re.search which will raise an AttributeError if no match is
found; instead, capture the match into a variable, check if it is None, and if
so raise a descriptive ValueError (e.g., "deopt() signature not found in
contents") before accessing group(1); update the code in build_deopts to use the
match variable and raise the clear error to make failures actionable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 357b7761-b806-459d-be4b-25ddced83286
⛔ Files ignored due to path filters (1)
Lib/test/test__opcode.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/compiler-core/src/bytecode/instruction.rscrates/stdlib/src/_opcode.rsscripts/generate_opcode_metadata.py
Comment on lines
+81
to
+83
| lambda l: not l.startswith("_ =>"), # Take until reaching fallback | ||
| filter( | ||
| lambda l: ( |
Contributor
There was a problem hiding this comment.
Rename ambiguous lambda variable l to satisfy lint.
Line 81 and Line 83 trigger Flake8 E741 (ambiguous variable name 'l').
Suggested fix
body = "\n".join(
itertools.takewhile(
- lambda l: not l.startswith("_ =>"), # Take until reaching fallback
+ lambda line: not line.startswith("_ =>"), # Take until reaching fallback
filter(
- lambda l: (
- not l.startswith(
+ lambda line: (
+ not line.startswith(
("//", "Some(match")
) # Skip comments or start of match
),📝 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
| lambda l: not l.startswith("_ =>"), # Take until reaching fallback | |
| filter( | |
| lambda l: ( | |
| body = "\n".join( | |
| itertools.takewhile( | |
| lambda line: not line.startswith("_ =>"), # Take until reaching fallback | |
| filter( | |
| lambda line: ( | |
| not line.startswith( | |
| ("//", "Some(match") | |
| ) # Skip comments or start of match | |
| ), |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 81-81: ambiguous variable name 'l'
(E741)
[error] 83-83: ambiguous variable name 'l'
(E741)
🤖 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 81 - 83, The lambdas using
the ambiguous parameter name "l" (in the first lambda used with take-until
comment "Take until reaching fallback" and the second lambda passed into filter)
should be renamed to a descriptive parameter like "line" to satisfy Flake8 E741;
update both occurrences (the lambda in the take/until expression and the lambda
passed to filter) to use "line" and adjust any internal references accordingly
so behavior is unchanged.
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
Chores