Macro for defining opcode & instruction enums by ShaharNaveh · Pull Request #7573 · RustPython/RustPython

ShaharNaveh · 2026-04-09T16:51:56Z

This macro will generate an Instruction and a Opcode enum, all within rust, no external python scripts with custom config in toml or something.

I'd love to hear what you guys think:)

Summary by CodeRabbit

Refactor
- Reworked bytecode instruction definitions to use macro-driven generation for consistency and maintainability.
- Public API now additionally exposes opcode enumeration types (Opcode and PseudoOpcode).
- Preserved binary sizes and backward-compatible conversions so existing bytecode behavior remains unchanged.

coderabbitai · 2026-04-09T16:52:05Z

📝 Walkthrough

Walkthrough

Replaces hand-written Instruction/PseudoInstruction enums with a macro-generated definition and adds standalone Opcode and PseudoOpcode enums; updates public re-exports to expose the new opcode types. Size assertions and typed conversions were adjusted accordingly. (≤50 words)

Changes

Cohort / File(s)	Summary
Public API exports `crates/compiler-core/src/bytecode.rs`	Extended `pub use crate::bytecode::instruction::{...}` to also re-export `Opcode` and `PseudoOpcode`.
Macro-generated opcodes & instructions `crates/compiler-core/src/bytecode/instruction.rs`	Replaced hand-written `Instruction`/`PseudoInstruction` and `mem::transmute` conversions with a `define_opcodes!` macro that generates `Opcode`/`PseudoOpcode`, `Instruction`/`PseudoInstruction` enums, `TryFrom`/`From` conversions via `match`, `Display` impls for opcodes, and moved size assertions to the file end. Removed `core::mem` usage.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

[RFC] Split oparg type from the opcode enum #6746: Changes introduce distinct Opcode enums and regenerate opcode→instruction conversions, aligning with that issue's objective.

Poem

🐰
I hopped through enums, bytes, and code,
Macros stitched pathways down the road,
No transmute tricks, just matches bright,
Opcodes dancing in morning light,
A carrot-cheer for bytecode's delight! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% 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 clearly and accurately describes the main change: a macro for defining opcode and instruction enums, which is exactly what the changeset implements.

_{✏️ 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

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

ShaharNaveh · 2026-04-09T17:09:58Z

crates/compiler-core/src/bytecode/instruction.rs

+        }
+
+        #[derive(Clone, Copy, Debug)]
+        #[repr($typ)]


I wanted to remove this #[repr(...)], but when doing so I get the following error:

thread 'main' (3429) panicked at crates/vm/src/frame.rs:3790:33: index out of bounds: the len is 5 but the index is 22 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

RustPython/crates/vm/src/frame.rs

Lines 3787 to 3791 in d5a90e5

Instruction::LoadAttrMethodWithValues => {

let oparg = LoadAttr::from_u32(u32::from(arg));

let cache_base = self.lasti() as usize;

let attr_name = self.code.names[oparg.name_idx() as usize];

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/compiler-core/src/bytecode/instruction.rs (1)
32-35: Consider deriving PartialEq and Eq for the opcode enum.

The Opcode and PseudoOpcode enums only derive Clone, Copy, and Debug. Adding PartialEq and Eq would enable equality comparisons and pattern matching in match guards, which may be useful for consumers of these types.
💡 Suggested change
-        #[derive(Clone, Copy, Debug)]
+        #[derive(Clone, Copy, Debug, PartialEq, Eq)]
         pub enum $opcode_name {
             $($op_name),*
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 32 - 35, The
opcode enums only derive Clone, Copy, and Debug; add PartialEq and Eq to the
derive list so consumers can compare values and use match guards. Update the
macro-generated enum declarations (the $opcode_name expansion and the concrete
Opcode and PseudoOpcode enums) to include #[derive(Clone, Copy, Debug,
PartialEq, Eq)] so equality comparisons are available for those types.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 1303-1308: The StoreFastMaybeNull instruction currently declares
its oparg var_num as NameIdx but should use the local variable index type;
change the oparg type for StoreFastMaybeNull from NameIdx to oparg::VarNum so it
matches the local-store semantics (same as StoreFast) and uses the correct
symbol names var_num and oparg::VarNum in the instruction definition.

---

Nitpick comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 32-35: The opcode enums only derive Clone, Copy, and Debug; add
PartialEq and Eq to the derive list so consumers can compare values and use
match guards. Update the macro-generated enum declarations (the $opcode_name
expansion and the concrete Opcode and PseudoOpcode enums) to include
#[derive(Clone, Copy, Debug, PartialEq, Eq)] so equality comparisons are
available for those types.

🪄 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: 490f37ff-9d9d-4813-9ac5-a6ddc00d643f

📥 Commits

Reviewing files that changed from the base of the PR and between d5a90e5 and 3d5295d.

📒 Files selected for processing (2)

crates/compiler-core/src/bytecode.rs
crates/compiler-core/src/bytecode/instruction.rs

crates/compiler-core/src/bytecode/instruction.rs

…r-macro

coderabbitai

🧹 Nitpick comments (1)

crates/compiler-core/src/bytecode/instruction.rs (1)
32-35: Consider adding PartialEq, Eq, and Hash derives to opcode enums.

The generated opcode enums (Opcode, PseudoOpcode) only derive Clone, Copy, Debug. Adding PartialEq, Eq, and optionally Hash would enable their use in comparisons and as map/set keys, which could be useful for opcode analysis or caching scenarios.
♻️ Suggested derive expansion
-        #[derive(Clone, Copy, Debug)]
+        #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
         pub enum $opcode_name {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 32 - 35, The
generated opcode enums (the template enum named by $opcode_name, e.g., Opcode
and PseudoOpcode) only derive Clone, Copy, Debug; update the derive attribute to
also include PartialEq, Eq, and Hash so these enums can be compared and used as
map/set keys; modify the derive line in the enum generation template (the block
containing #[derive(Clone, Copy, Debug)] and pub enum $opcode_name { ... }) to
add PartialEq, Eq, Hash, and ensure both generated enums are adjusted
accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 32-35: The generated opcode enums (the template enum named by
$opcode_name, e.g., Opcode and PseudoOpcode) only derive Clone, Copy, Debug;
update the derive attribute to also include PartialEq, Eq, and Hash so these
enums can be compared and used as map/set keys; modify the derive line in the
enum generation template (the block containing #[derive(Clone, Copy, Debug)] and
pub enum $opcode_name { ... }) to add PartialEq, Eq, Hash, and ensure both
generated enums are adjusted accordingly.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 2966abcb-4020-4bbc-ba78-d346734c5fff

📥 Commits

Reviewing files that changed from the base of the PR and between 3d5295d and 9c3fd3e.

📒 Files selected for processing (1)

crates/compiler-core/src/bytecode/instruction.rs

…r-macro

ShaharNaveh · 2026-04-13T13:29:56Z

ping @youknowone

youknowone · 2026-04-13T13:53:35Z

This PR is splitting Instruction and Opcode, right? I wasn't sure what's the benefit based on this changes. Could you tell me more?

ShaharNaveh · 2026-04-13T15:10:58Z

This PR is splitting Instruction and Opcode, right? I wasn't sure what's the benefit based on this changes. Could you tell me more?

yes, exactly. If this approach is good in your opinion I'll make the rest of the code to use the Opcode enum

ShaharNaveh added 2 commits April 9, 2026 18:18

Macro for defining opcode & instruction enums

ba8da19

Convert Instruction as well

ab5cfda

ShaharNaveh requested review from coolreader18 and youknowone April 9, 2026 16:52

revert #[repr(...)] changes

3d5295d

ShaharNaveh commented Apr 9, 2026

View reviewed changes

ShaharNaveh marked this pull request as ready for review April 9, 2026 20:31

coderabbitai bot reviewed Apr 9, 2026

View reviewed changes

crates/compiler-core/src/bytecode/instruction.rs Show resolved Hide resolved

ShaharNaveh added 2 commits April 10, 2026 06:39

Merge remote-tracking branch 'upstream/main' into bytecode-instr-supe…

27cab7e

…r-macro

fmt

9c3fd3e

coderabbitai bot reviewed Apr 10, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main' into bytecode-instr-supe…

d657b74

…r-macro

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Macro for defining opcode & instruction enums#7573

Macro for defining opcode & instruction enums#7573
ShaharNaveh wants to merge 6 commits intoRustPython:mainfrom
ShaharNaveh:bytecode-instr-super-macro

ShaharNaveh commented Apr 9, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Apr 9, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Poem

❌ Failed checks (1 warning)

Uh oh!

ShaharNaveh Apr 9, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

ShaharNaveh commented Apr 13, 2026

Uh oh!

youknowone commented Apr 13, 2026

Uh oh!

ShaharNaveh commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	Instruction::LoadAttrMethodWithValues => {
	let oparg = LoadAttr::from_u32(u32::from(arg));
	let cache_base = self.lasti() as usize;
	let attr_name = self.code.names[oparg.name_idx() as usize];

Conversation

ShaharNaveh commented Apr 9, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Poem

❌ Failed checks (1 warning)

Uh oh!

ShaharNaveh Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh commented Apr 13, 2026

Uh oh!

youknowone commented Apr 13, 2026

Uh oh!

ShaharNaveh commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShaharNaveh commented Apr 9, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Apr 9, 2026 •

edited

Loading