Autogen opcodes metadata#7983
Conversation
📝 WalkthroughWalkthroughThis PR refactors RustPython's opcode metadata generation infrastructure. The changes migrate from a monolithic legacy approach (single ChangesOpcode Metadata Generation Refactor
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/opcode.py dependencies:
dependent tests: (47 tests)
Legend:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
tools/opcode_metadata/cpython.py (1)
5-8: ⚡ Quick winChain the exception in the
except KeyErrorpath.Use
raise ... from errto preserve the original failure context and satisfy B904.Suggested fix
-try: - CPYTHON_ROOT = pathlib.Path(os.environ["CPYTHON_ROOT"]).expanduser().resolve() -except KeyError: - raise ValueError("Missing environment variable 'CPYTHON_ROOT'") +try: + CPYTHON_ROOT = pathlib.Path(os.environ["CPYTHON_ROOT"]).expanduser().resolve() +except KeyError as err: + raise ValueError("Missing environment variable 'CPYTHON_ROOT'") from errAs per coding guidelines, "Use ruff for linting Python code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/opcode_metadata/cpython.py` around lines 5 - 8, The except KeyError block that raises ValueError for missing CPYTHON_ROOT should chain the original KeyError so the original context is preserved; modify the except clause to capture the exception (e.g., except KeyError as err) and re-raise the ValueError using "raise ValueError(\"Missing environment variable 'CPYTHON_ROOT'\") from err" so the CPYTHON_ROOT resolution code (pathlib.Path(os.environ["CPYTHON_ROOT"]).expanduser().resolve()) preserves the original error context for lint B904.tools/opcode_metadata/opcodes.py (1)
95-98: ⚡ Quick winSet
stacklevelonwarnings.warnfor correct caller attribution.This warning should point to the caller site, not inside helper internals.
Suggested fix
- warnings.warn( + warnings.warn( f"Could not get instruction metadata for {rust_name}" - " from CPython or override conf" + " from CPython or override conf", + stacklevel=2, )As per coding guidelines, "Use ruff for linting Python code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/opcode_metadata/opcodes.py` around lines 95 - 98, The warnings.warn call that emits "Could not get instruction metadata for {rust_name} from CPython or override conf" should include a stacklevel so the warning points at the caller, not this helper; update the warnings.warn call to pass stacklevel=2 (e.g., warnings.warn(..., stacklevel=2)) referencing the existing rust_name variable and the warnings.warn invocation so linting (ruff) and caller attribution are correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 46-47: The hook `files` regex currently uses
`tools/opcode_metadata/*` which doesn't match files like
`tools/opcode_metadata/opcodes.py`; update both occurrences of the `files`
pattern in the pre-commit config (the lines with `files:` that currently contain
`tools/opcode_metadata/*`) to use `tools/opcode_metadata/.*` so the
opcode-metadata hooks trigger on changes under that directory (keep
`pass_filenames: false` as-is).
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 213-233: The macro arm that detects label fields only matches bare
"Label" but opcode variants use "oparg::Label", so add explicit macro arms to
match oparg::Label (mirror the existing Label arms) — e.g. add (`@match`
$self:expr, $name:ident, [$variant:ident { $field:ident : oparg::Label } ,
$($rest:tt)*]) and the final-arm variant (`@match` $self:expr, $name:ident,
[$variant:ident { $field:ident : oparg::Label }]) with the same bodies that
return Some(*$field); this ensures define_opcodes! macro recognizes oparg::Label
fields and preserves CodeObject::label_targets().
In `@tools/opcode_metadata/cpython.py`:
- Around line 17-18: The module currently imports SKIP_PROPERTIES, Analysis,
Family, Properties, analyze_files and get_stack_effect only for downstream
re-export but lint flags them as unused; make this explicit by adding an __all__
=
("SKIP_PROPERTIES","Analysis","Family","Properties","analyze_files","get_stack_effect")
at top (or alternatively append targeted "# noqa: F401" to those import lines)
so the re-exports are intentional and ruff/CI no longer reports F401. Ensure the
symbol names match the imported identifiers exactly (SKIP_PROPERTIES, Analysis,
Family, Properties, analyze_files, get_stack_effect).
In `@tools/opcode_metadata/generate_py_opcode_metadata.py`:
- Around line 12-15: Remove the unused imports that trigger F401 by deleting
"typing" from the top-level imports and removing "to_pascal_case" from the
from-utils import list in generate_py_opcode_metadata.py; update the import line
to only keep used symbols (OpcodeInfo, DEFAULT_INPUT, ROOT, get_conf,
to_upper_snake_case) so the module passes ruff/flake8 linting.
- Around line 84-85: Replace the lambda assigned to key with a small local def
to satisfy E731: create a function (e.g., def sort_key(op): return
(op.cpython_name not in PRIORITY_OPMAP, op.id)) and pass sort_key into
sorted(opcodes, key=sort_key) instead of using key = lambda ...; keep references
to PRIORITY_OPMAP and op.id/op.cpython_name unchanged.
In `@tools/opcode_metadata/generate_rs_opcode_metadata.py`:
- Line 23: The name OpcodeDef used as a type annotation is undefined; either
import the correct type that represents entries returned by
OpcodeInfo.iter_infos (e.g., add an import for OpcodeDef from the module that
defines it) or change the annotation to the actual type produced by
OpcodeInfo.iter_infos (for example Iterator[OpcodeInfo] or list[OpcodeInfo]) and
add the necessary typing imports (from typing import Iterator, List) so
ruff/F821 stops complaining; update the annotation on the variable/parameter
named info (or wherever OpcodeDef is used) accordingly.
- Around line 4-16: Remove the unused imports in the top import block: delete
"collections", "os", "sys", "typing", "tomllib" (if not used elsewhere), and the
unused names from the cpython from-import ("Analysis", "get_analysis",
"get_stack_effect"), and remove "to_pascal_case" from the utils import; keep
only the actually used symbols (e.g., dataclasses, io, pathlib, subprocess,
OpcodeInfo, DEFAULT_INPUT, ROOT, get_conf). Update the import lines in
generate_rs_opcode_metadata.py accordingly and run ruff to verify no F401s
remain.
In `@tools/opcode_metadata/opcodes.py`:
- Around line 35-37: The comparison that skips the family root uses raw
member.name against the normalized family_name, causing mismatches; update the
filter to compare the normalized member_name (the result of
to_pascal_case(member.name)) to family_name (or normalize family_name with
to_pascal_case) so the family opcode is correctly excluded—adjust the logic
around member_name, member.name, family_name and the call to to_pascal_case in
opcodes.py.
In `@tools/opcode_metadata/utils.py`:
- Line 25: Replace the PEP 695 type-alias statement so it parses on Python 3.11:
change the declaration "type OverrideConfs = dict[str, Override]" to an
assignment-style alias "OverrideConfs = dict[str, Override]" (keep the same name
OverrideConfs and the referenced Override type), ensuring any necessary imports
for Override remain unchanged.
---
Nitpick comments:
In `@tools/opcode_metadata/cpython.py`:
- Around line 5-8: The except KeyError block that raises ValueError for missing
CPYTHON_ROOT should chain the original KeyError so the original context is
preserved; modify the except clause to capture the exception (e.g., except
KeyError as err) and re-raise the ValueError using "raise ValueError(\"Missing
environment variable 'CPYTHON_ROOT'\") from err" so the CPYTHON_ROOT resolution
code (pathlib.Path(os.environ["CPYTHON_ROOT"]).expanduser().resolve()) preserves
the original error context for lint B904.
In `@tools/opcode_metadata/opcodes.py`:
- Around line 95-98: The warnings.warn call that emits "Could not get
instruction metadata for {rust_name} from CPython or override conf" should
include a stacklevel so the warning points at the caller, not this helper;
update the warnings.warn call to pass stacklevel=2 (e.g., warnings.warn(...,
stacklevel=2)) referencing the existing rust_name variable and the warnings.warn
invocation so linting (ruff) and caller attribution are correct.
🪄 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: 3d449e36-7fc4-4c30-a9b0-afbda43241c9
⛔ Files ignored due to path filters (1)
Lib/_opcode_metadata.pyis excluded by!Lib/**
📒 Files selected for processing (18)
.gitattributes.github/workflows/ci.yaml.pre-commit-config.yamlcrates/compiler-core/generate.pycrates/compiler-core/opcode.tomlcrates/compiler-core/src/bytecode.rscrates/compiler-core/src/bytecode/instruction.rscrates/compiler-core/src/bytecode/instructions.rscrates/compiler-core/src/bytecode/oparg.rscrates/compiler-core/src/bytecode/opcode_metadata.rscrates/compiler-core/src/lib.rsscripts/generate_opcode_metadata.pytools/opcode_metadata/conf.tomltools/opcode_metadata/cpython.pytools/opcode_metadata/generate_py_opcode_metadata.pytools/opcode_metadata/generate_rs_opcode_metadata.pytools/opcode_metadata/opcodes.pytools/opcode_metadata/utils.py
💤 Files with no reviewable changes (3)
- scripts/generate_opcode_metadata.py
- crates/compiler-core/opcode.toml
- crates/compiler-core/generate.py
Sorry, something went wrong.
f95b746
into
RustPython:main
May 27, 2026
Inspired by #7797 (comment)
Summary by CodeRabbit