Autogen opcodes metadata#7983
Open
ShaharNaveh wants to merge 4 commits into
Open
Conversation
Contributor
📝 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 |
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/opcode.py dependencies:
dependent tests: (47 tests)
Legend:
|
Contributor
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
Comment on lines
+46
to
47
| files: '^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/*)$' | ||
| pass_filenames: false |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import re
pat = re.compile(r'^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/*)$')
samples = [
"crates/compiler-core/src/bytecode/instruction.rs",
"tools/opcode_metadata/opcodes.py",
"tools/opcode_metadata/utils.py",
]
print("Sample matches:")
for s in samples:
print(f"{s}: {bool(pat.search(s))}")
PY
python - <<'PY'
import re, subprocess
pat = re.compile(r'^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/*)$')
files = subprocess.check_output(["git", "ls-files"], text=True).splitlines()
matches = [f for f in files if pat.search(f)]
print(f"repo_match_count={len(matches)}")
for f in matches[:20]:
print(f)
PYRepository: RustPython/RustPython
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact current lines around the two reported occurrences
sed -n '35,70p' .pre-commit-config.yaml | nl -ba | sed -n '30,55p'
python3 - <<'PY'
import re, subprocess
pat = re.compile(r'^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/*)$')
files = subprocess.check_output(["git", "ls-files"], text=True).splitlines()
matches = [f for f in files if pat.search(f)]
print(f"repo_match_count={len(matches)}")
for f in matches:
print(f)
PY
python3 - <<'PY'
import re, subprocess
pat = re.compile(r'^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/.*)$')
files = subprocess.check_output(["git", "ls-files"], text=True).splitlines()
matches = [f for f in files if pat.search(f)]
print(f"repo_match_count_with_fixed_regex={len(matches)}")
for f in matches[:30]:
print(f)
PYRepository: RustPython/RustPython
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Display relevant section with line numbers (cat -n is widely available)
cat -n .pre-commit-config.yaml | sed -n '35,70p'
python3 - <<'PY'
import re, subprocess
pat = re.compile(r'^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/*)$')
files = subprocess.check_output(["git", "ls-files"], text=True).splitlines()
matches = [f for f in files if pat.search(f)]
print("repo_match_count=", len(matches))
for f in matches:
print(f)
PY
python3 - <<'PY'
import re, subprocess
pat = re.compile(r'^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/.*)$')
files = subprocess.check_output(["git", "ls-files"], text=True).splitlines()
matches = [f for f in files if pat.search(f)]
print("repo_match_count_with_fixed_regex=", len(matches))
for f in matches[:50]:
print(f)
PYRepository: RustPython/RustPython
Length of output: 1850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Display relevant section with line numbers (cat -n is widely available)
cat -n .pre-commit-config.yaml | sed -n '35,70p'
python3 - <<'PY'
import re, subprocess
pat = re.compile(r'^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/*)$')
files = subprocess.check_output(["git", "ls-files"], text=True).splitlines()
matches = [f for f in files if pat.search(f)]
print("repo_match_count=", len(matches))
for f in matches:
print(f)
PY
python3 - <<'PY'
import re, subprocess
pat = re.compile(r'^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/.*)$')
files = subprocess.check_output(["git", "ls-files"], text=True).splitlines()
matches = [f for f in files if pat.search(f)]
print("repo_match_count_with_fixed_regex=", len(matches))
for f in matches[:50]:
print(f)
PYRepository: RustPython/RustPython
Length of output: 1850
Fix files regex so opcode-metadata hooks trigger on tools/opcode_metadata/* changes
The current regex tools/opcode_metadata/* doesn’t match real files like tools/opcode_metadata/opcodes.py, so both manual hooks only match crates/compiler-core/src/bytecode/instruction.rs. Update both hook files patterns to tools/opcode_metadata/.*.
💡 Proposed fix
- files: '^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/*)$'
+ files: '^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/.*)$'
...
- files: '^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/*)$'
+ files: '^(crates/compiler-core/src/bytecode/instruction\.rs|tools/opcode_metadata/.*)$'🤖 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 @.pre-commit-config.yaml around lines 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).
Comment on lines
+213
to
+233
| (@match $self:expr, $name:ident, [$variant:ident { $field:ident : Label } , $($rest:tt)*]) => { | ||
| match $self { | ||
| $name::$variant { $field } => Some(*$field), | ||
| other => define_opcodes!(@match other, $name, [$($rest)*]), | ||
| } | ||
| }; | ||
|
|
||
| // Label field variant (last in list) | ||
| (@match $self:expr, $name:ident, [$variant:ident { $field:ident : Label }]) => { | ||
| match $self { | ||
| $name::$variant { $field } => Some(*$field), | ||
| other => define_opcodes!(@match other, $name, []), | ||
| } | ||
| }; | ||
|
|
||
| // Non-Label field variant (with trailing variants) | ||
| (@match $self:expr, $name:ident, [$variant:ident { $field:ident : $type:ty } , $($rest:tt)*]) => { | ||
| match $self { | ||
| $name::$variant { .. } => None, | ||
| other => define_opcodes!(@match other, $name, [$($rest)*]), | ||
| } |
Contributor
There was a problem hiding this comment.
label_arg() macro arm does not match oparg::Label typed fields.
The label-detection arm matches ... : Label, but opcode declarations pass oparg::Label. Those variants fall into the non-label arm and return None, which can break CodeObject::label_targets().
Targeted fix
- (`@match` $self:expr, $name:ident, [$variant:ident { $field:ident : Label } , $($rest:tt)*]) => {
+ (`@match` $self:expr, $name:ident, [$variant:ident { $field:ident : oparg::Label } , $($rest:tt)*]) => {
match $self {
$name::$variant { $field } => Some(*$field),
other => define_opcodes!(`@match` other, $name, [$($rest)*]),
}
};
@@
- (`@match` $self:expr, $name:ident, [$variant:ident { $field:ident : Label }]) => {
+ (`@match` $self:expr, $name:ident, [$variant:ident { $field:ident : oparg::Label }]) => {
match $self {
$name::$variant { $field } => Some(*$field),
other => define_opcodes!(`@match` other, $name, []),
}
};📝 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
| (@match $self:expr, $name:ident, [$variant:ident { $field:ident : Label } , $($rest:tt)*]) => { | |
| match $self { | |
| $name::$variant { $field } => Some(*$field), | |
| other => define_opcodes!(@match other, $name, [$($rest)*]), | |
| } | |
| }; | |
| // Label field variant (last in list) | |
| (@match $self:expr, $name:ident, [$variant:ident { $field:ident : Label }]) => { | |
| match $self { | |
| $name::$variant { $field } => Some(*$field), | |
| other => define_opcodes!(@match other, $name, []), | |
| } | |
| }; | |
| // Non-Label field variant (with trailing variants) | |
| (@match $self:expr, $name:ident, [$variant:ident { $field:ident : $type:ty } , $($rest:tt)*]) => { | |
| match $self { | |
| $name::$variant { .. } => None, | |
| other => define_opcodes!(@match other, $name, [$($rest)*]), | |
| } | |
| (`@match` $self:expr, $name:ident, [$variant:ident { $field:ident : oparg::Label } , $($rest:tt)*]) => { | |
| match $self { | |
| $name::$variant { $field } => Some(*$field), | |
| other => define_opcodes!(`@match` other, $name, [$($rest)*]), | |
| } | |
| }; | |
| // Label field variant (last in list) | |
| (`@match` $self:expr, $name:ident, [$variant:ident { $field:ident : oparg::Label }]) => { | |
| match $self { | |
| $name::$variant { $field } => Some(*$field), | |
| other => define_opcodes!(`@match` other, $name, []), | |
| } | |
| }; | |
| // Non-Label field variant (with trailing variants) | |
| (`@match` $self:expr, $name:ident, [$variant:ident { $field:ident : $type:ty } , $($rest:tt)*]) => { | |
| match $self { | |
| $name::$variant { .. } => None, | |
| other => define_opcodes!(`@match` other, $name, [$($rest)*]), | |
| } |
🤖 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 `@crates/compiler-core/src/bytecode/instruction.rs` around lines 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().
Comment on lines
+17
to
+18
| from analyzer import SKIP_PROPERTIES, Analysis, Family, Properties, analyze_files | ||
| from stack import get_stack_effect |
Contributor
There was a problem hiding this comment.
Make intentional re-exports explicit to clear F401 lint failures.
These names are imported for downstream use (opcodes.py), but lint treats them as unused. Add __all__ (or targeted # noqa: F401) so the module intent is explicit and CI lint won’t fail.
Suggested fix
from analyzer import SKIP_PROPERTIES, Analysis, Family, Properties, analyze_files
from stack import get_stack_effect
+
+__all__ = [
+ "SKIP_PROPERTIES",
+ "Analysis",
+ "Family",
+ "Properties",
+ "analyze_files",
+ "get_stack_effect",
+ "get_analysis",
+]As per coding guidelines, "Use ruff for linting Python code".
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 17-17: 'analyzer.SKIP_PROPERTIES' imported but unused
(F401)
[error] 17-17: 'analyzer.Family' imported but unused
(F401)
[error] 17-17: 'analyzer.Properties' imported but unused
(F401)
[error] 18-18: 'stack.get_stack_effect' imported but unused
(F401)
🤖 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 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).
Comment on lines
+12
to
+15
| import typing | ||
|
|
||
| from opcodes import OpcodeInfo | ||
| from utils import DEFAULT_INPUT, ROOT, get_conf, to_pascal_case, to_upper_snake_case |
Contributor
There was a problem hiding this comment.
Remove unused imports to satisfy lint checks (Line 12, Line 15).
typing and to_pascal_case are unused and trigger F401. Please drop both imports to keep ruff/flake8 clean.
As per coding guidelines **/*.py: "Follow PEP 8 style for custom Python code" and "Use ruff for linting Python code".
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 12-12: 'typing' imported but unused
(F401)
[error] 15-15: 'utils.to_pascal_case' imported but unused
(F401)
🤖 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/generate_py_opcode_metadata.py` around lines 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.
Comment on lines
+84
to
+85
| key = lambda op: (op.cpython_name not in PRIORITY_OPMAP, op.id) | ||
| for opcode in sorted(opcodes, key=key): |
Contributor
There was a problem hiding this comment.
Replace the lambda sort key with a named function (E731)
Ruff reports E731 Do not assign a lambda expression, use a def for key = lambda .... Replace it with a small local def.
Suggested fix
- key = lambda op: (op.cpython_name not in PRIORITY_OPMAP, op.id)
- for opcode in sorted(opcodes, key=key):
+ def sort_key(opcode):
+ return (opcode.cpython_name not in PRIORITY_OPMAP, opcode.id)
+
+ for opcode in sorted(opcodes, key=sort_key):🧰 Tools
🪛 Flake8 (7.3.0)
[error] 84-84: do not assign a lambda expression, use a def
(E731)
🪛 Ruff (0.15.14)
[error] 84-84: Do not assign a lambda expression, use a def
Rewrite key as a def
(E731)
🤖 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/generate_py_opcode_metadata.py` around lines 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.
Comment on lines
+4
to
+16
| import collections | ||
| import dataclasses | ||
| import io | ||
| import os | ||
| import pathlib | ||
| import subprocess | ||
| import sys | ||
| import typing | ||
|
|
||
| import tomllib | ||
| from cpython import Analysis, get_analysis, get_stack_effect | ||
| from opcodes import OpcodeInfo | ||
| from utils import DEFAULT_INPUT, ROOT, get_conf, to_pascal_case |
Contributor
There was a problem hiding this comment.
Remove unused imports to keep the generator Ruff-clean.
This block contains multiple unused imports (collections, os, sys, typing, tomllib, Analysis/get_analysis/get_stack_effect, to_pascal_case), which fail lint (F401).
Suggested cleanup
-import collections
import dataclasses
import io
-import os
import pathlib
import subprocess
-import sys
-import typing
-import tomllib
-from cpython import Analysis, get_analysis, get_stack_effect
+from cpython import Analysis, get_analysis, get_stack_effect
from opcodes import OpcodeInfo
-from utils import DEFAULT_INPUT, ROOT, get_conf, to_pascal_case
+from utils import DEFAULT_INPUT, ROOT, get_confAs per coding guidelines "**/*.py: Use ruff for linting Python code".
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 4-4: 'collections' imported but unused
(F401)
[error] 7-7: 'os' imported but unused
(F401)
[error] 10-10: 'sys' imported but unused
(F401)
[error] 11-11: 'typing' imported but unused
(F401)
[error] 13-13: 'tomllib' imported but unused
(F401)
[error] 14-14: 'cpython.Analysis' imported but unused
(F401)
[error] 14-14: 'cpython.get_analysis' imported but unused
(F401)
[error] 14-14: 'cpython.get_stack_effect' imported but unused
(F401)
[error] 16-16: 'utils.to_pascal_case' imported but unused
(F401)
🤖 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/generate_rs_opcode_metadata.py` around lines 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.
|
|
||
| @dataclasses.dataclass(frozen=True, slots=True) | ||
| class OpcodeGen: | ||
| info: OpcodeDef |
Contributor
There was a problem hiding this comment.
Fix unresolved type annotation (OpcodeDef) before merge.
OpcodeDef is undefined in this module, so linting fails (F821). Import the right type or update the annotation to the actual symbol returned by OpcodeInfo.iter_infos(...).
As per coding guidelines "**/*.py: Use ruff for linting Python code".
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 23-23: undefined name 'OpcodeDef'
(F821)
🪛 Ruff (0.15.14)
[error] 23-23: Undefined name OpcodeDef
(F821)
🤖 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/generate_rs_opcode_metadata.py` at 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.
Comment on lines
+35
to
+37
| member_name = to_pascal_case(member.name) | ||
| if member.name == family_name: | ||
| continue |
Contributor
There was a problem hiding this comment.
Fix mismatched-name comparison when filtering family root members.
You normalize both names to PascalCase, but compare raw member.name to normalized family_name. This can incorrectly include the family opcode in its own deopt list.
Suggested fix
- if member.name == family_name:
+ if member_name == family_name:
continue🤖 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 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.
| stack_effect: StackEffect = dataclasses.field(default_factory=StackEffect) | ||
|
|
||
|
|
||
| type OverrideConfs = dict[str, Override] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Python-version expectations used by CI/tooling and where this alias appears.
fd -HI "pyproject.toml|setup.cfg|tox.ini|.python-version|ci.yaml|*.yml|*.yaml" .github workflows . 2>/dev/null | xargs -r rg -n "python-version|requires-python|3\\.(1[01]|12|13)"
rg -n "type\\s+OverrideConfs\\s*=" tools/opcode_metadata/utils.pyRepository: RustPython/RustPython
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Python version constraints (best-effort) =="
rg -n --hidden --no-ignore \
"requires-python|python-version|target-version|minimum-python-version|Programming Language :: Python" \
. \
|| true
echo
echo "== Ruff target-version (if any) =="
rg -n --hidden --no-ignore \
"target-version" \
pyproject.toml setup.cfg tox.ini .ruff.toml ruff.toml 2>/dev/null || true
echo
echo "== Inspect tools/opcode_metadata/utils.py around line 25 =="
python3 - <<'PY'
from pathlib import Path
p = Path("tools/opcode_metadata/utils.py")
if p.exists():
lines = p.read_text().splitlines()
for i in range(18, 33):
if 0 <= i-1 < len(lines):
print(f"{i}:{lines[i-1]}")
else:
print("File not found:", p)
PYRepository: RustPython/RustPython
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .python-version =="
if [ -f .python-version ]; then cat .python-version; else echo "missing"; fi
echo
echo "== requires-python in packaging configs (limited) =="
rg -n --hidden --no-ignore "requires-python" pyproject.toml setup.cfg tox.ini . 2>/dev/null || true
rg -n --hidden --no-ignore "Programming Language :: Python" pyproject.toml setup.cfg setup.* 2>/dev/null || true
echo
echo "== ruff target-version (limited) =="
rg -n --hidden --no-ignore "target-version" pyproject.toml ruff.toml .ruff.toml setup.cfg tox.ini 2>/dev/null || true
echo
echo "== Inspect tools/opcode_metadata/utils.py lines 1-80 (for context around line 25) =="
nl -ba tools/opcode_metadata/utils.py | sed -n '1,80p'Repository: RustPython/RustPython
Length of output: 50440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Ruff config files =="
ls -la .ruff.toml ruff.toml pyproject.toml setup.cfg 2>/dev/null || true
echo
echo "== Search ruff target-version =="
rg -n --hidden --no-ignore "target-version|python-version|requires-python" .ruff.toml ruff.toml pyproject.toml setup.cfg 2>/dev/null || true
echo
echo "== Search ruff target-version in workflow (limited) =="
rg -n --hidden --no-ignore "target-version|requires-python|python-version" .github/workflows 2>/dev/null | head -n 50 || true
echo
echo "== tools/opcode_metadata/utils.py around line 25 =="
python3 - <<'PY'
from pathlib import Path
p = Path("tools/opcode_metadata/utils.py")
lines = p.read_text().splitlines()
start, end = 18, 32
for i in range(start, end+1):
if i-1 < len(lines):
print(f"{i}:{lines[i-1]}")
PYRepository: RustPython/RustPython
Length of output: 2817
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sandbox python3 version =="
python3 --version || true
echo
echo "== ruff.toml (first 120 lines) =="
sed -n '1,120p' ruff.toml
echo
echo "== ruff.toml Python-version related keys =="
rg -n "target-version|python-version|requires-python" ruff.toml || true
echo
echo "== ruff usage in CI workflows (limited context) =="
rg -n "ruff" .github/workflows/ci.yaml .github/workflows/*.yml .github/workflows/*.yaml 2>/dev/null | head -n 80 || true
echo
echo "== python-version in CI workflow where ruff runs (best-effort) =="
rg -n "python-version" .github/workflows/ci.yaml 2>/dev/null | head -n 80 || trueRepository: RustPython/RustPython
Length of output: 1567
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ruff references in ci.yaml =="
rg -n "ruff" .github/workflows/ci.yaml || true
echo
echo "== setup-python / python-version in ci.yaml (context) =="
rg -n "setup-python|python-version:" .github/workflows/ci.yaml || true
echo
echo "== Lint job section (best-effort) =="
rg -n "lint|format|check" .github/workflows/ci.yaml | head -n 80 || true
echo
echo "== show around first ruff match (if any) =="
ruff_line=$(rg -n "ruff" .github/workflows/ci.yaml | head -n 1 | cut -d: -f1 || true)
if [ -n "${ruff_line:-}" ]; then
start=$((ruff_line-25)); end=$((ruff_line+25))
sed -n "${start},${end}p" .github/workflows/ci.yaml
else
echo "No ruff match found."
fiRepository: RustPython/RustPython
Length of output: 2502
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== py_compile with sandbox python =="
python3 - <<'PY'
import py_compile, sys
try:
py_compile.compile("tools/opcode_metadata/utils.py", doraise=True)
print("py_compile: OK")
except Exception as e:
print("py_compile: FAIL")
print(type(e).__name__ + ":", e)
PY
echo
echo "== Inspect CI lint job section in .github/workflows/ci.yaml =="
sed -n '500,620p' .github/workflows/ci.yaml
echo
echo "== Search for ruff usage in workflows =="
rg -n "ruff" .github/workflows || true
echo
echo "== Current ruff version (if installed) =="
ruff --version || trueRepository: RustPython/RustPython
Length of output: 4287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate ruff in .pre-commit-config.yaml =="
rg -n "ruff" .pre-commit-config.yaml || true
echo
echo "== Show ruff-related hook blocks (around matches) =="
rg -n -C 3 "ruff" .pre-commit-config.yaml || true
echo
echo "== Show full .pre-commit-config.yaml (limited) if small =="
wc -l .pre-commit-config.yaml || trueRepository: RustPython/RustPython
Length of output: 721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ruff check tools/opcode_metadata/utils.py -v || trueRepository: RustPython/RustPython
Length of output: 632
Replace the PEP 695 type alias to avoid a Python 3.11 parse-time SyntaxError.
tools/opcode_metadata/utils.py line 25 uses type OverrideConfs = dict[str, Override] (PEP 695), which fails to parse under Python 3.11 (py_compile reports SyntaxError: invalid syntax). Use the assignment alias form instead:
Suggested fix
- type OverrideConfs = dict[str, Override]
+OverrideConfs = dict[str, Override]📝 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
| type OverrideConfs = dict[str, Override] | |
| OverrideConfs = dict[str, Override] |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 25-25: SyntaxError: invalid syntax
(E999)
🤖 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/utils.py` at 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.
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.
Inspired by #7797 (comment)
Summary by CodeRabbit