Skip to content

#[newtype_oparg] derive macro#7489

Open
ShaharNaveh wants to merge 14 commits intoRustPython:mainfrom
ShaharNaveh:macro-newtype-oparg
Open

#[newtype_oparg] derive macro#7489
ShaharNaveh wants to merge 14 commits intoRustPython:mainfrom
ShaharNaveh:macro-newtype-oparg

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Mar 23, 2026

Reason for creating a new crate for this is because rustpython-derive-impl has rustpython-compiler as a dependency, so it created a circular dependency:/

Summary by CodeRabbit

  • New Features

    • Added a procedural-macro crate to generate enum/newtype boilerplate.
  • Refactor

    • Converted many opcode/newtype declarations to use the new macro-driven approach, standardizing displays and conversions and reducing manual boilerplate.
  • Bug Fixes

    • Fixed parsing of containment-related bytecode arguments to preserve full argument width.
  • Chores

    • Added workspace dependency to enable the new macro crate.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 23, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a new procedural-macro crate rustpython-macros providing #[newtype_oparg], updates workspace and compiler-core dependencies to use it, refactors many bytecode oparg enums/newtypes to use the macro-generated implementations, and adjusts VM frame handling for ContainsOp* invert parsing.

Changes

Cohort / File(s) Summary
Workspace Manifest
Cargo.toml
Add workspace dependency rustpython-macros = { path = "crates/macros", version = "0.5.0" }.
Compiler Core Dependencies
crates/compiler-core/Cargo.toml
Add rustpython-macros = { workspace = true } under [dependencies].
New Proc-macro Crate Manifest
crates/macros/Cargo.toml
New crate manifest for rustpython-macros configured as a procedural macro with workspace-sourced metadata and proc-macro deps (proc-macro2, quote, syn).
Proc-macro Entrypoint
crates/macros/src/lib.rs
Add #[proc_macro_attribute] pub fn newtype_oparg(...) that dispatches to enum/struct handlers and emits compile-time diagnostics.
Proc-macro Generator
crates/macros/src/newtype_oparg.rs
Implement macro logic: validate #[oparg(...)] attributes, generate tuple-newtype structs and enums, produce From/TryFrom conversions, Display impls, catch-all handling, and OpArgType impls with diagnostics.
Bytecode Oparg Refactor
crates/compiler-core/src/bytecode/oparg.rs
Replace local oparg_enum!/newtype_oparg! macros with #[newtype_oparg]-annotated enums/structs for many oparg types (e.g., ConvertValueOparg, ResumeType, RaiseKind, IntrinsicFunction1/2, BinaryOperator, Invert, SpecialMethod, CommonConstant, BuildSliceArgCount, and newtypes like ConstIdx, VarNum, VarNums, LoadAttr, LoadSuperAttr, Label); remove manual From/Display impls and adjust discriminants/representations (notably ResumeType::Other(u32) catch-all).
VM Instruction Handling
crates/vm/src/frame.rs
Parse invert flag for Instruction::ContainsOp* from full u32 instead of truncating to u8, preserving fallback to Invert::No on parse failure.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • youknowone
  • coolreader18

Poem

🐰 I hopped through crates with tokens in paw,
I stitched newtypes where old macros once saw,
Enums wear labels, catch-alls hum low,
Bytecode now leaps where the carrots grow,
Compile and hop — a rabbit's small-wow!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: introduction of a #[newtype_oparg] derive macro. It matches the substantial refactoring throughout the codebase where local macros are replaced with this new derive macro.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)

5467-5469: Extract invert once and reuse shared inversion logic.

These branches all re-parse invert and repeat the same boolean inversion pattern. Please compute invert once per opcode path and apply common logic once.

♻️ Proposed refactor
+                let invert_yes = matches!(
+                    bytecode::Invert::try_from(u32::from(arg)),
+                    Ok(bytecode::Invert::Yes)
+                );
                 if let Some(dict) = b.downcast_ref_if_exact::<PyDict>(vm) {
                     let a = self.nth_value(1); // needle
                     let found = dict.get_item_opt(a, vm)?.is_some();
                     self.pop_value();
                     self.pop_value();
-                    let invert =
-                        bytecode::Invert::try_from(u32::from(arg)).unwrap_or(bytecode::Invert::No);
-                    let value = match invert {
-                        bytecode::Invert::No => found,
-                        bytecode::Invert::Yes => !found,
-                    };
+                    let value = if invert_yes { !found } else { found };
                     self.push_value(vm.ctx.new_bool(value).into());
                     Ok(None)
                 } else {
                     let b = self.pop_value();
                     let a = self.pop_value();
-                    let invert =
-                        bytecode::Invert::try_from(u32::from(arg)).unwrap_or(bytecode::Invert::No);
-                    let value = match invert {
-                        bytecode::Invert::No => self._in(vm, &a, &b)?,
-                        bytecode::Invert::Yes => self._not_in(vm, &a, &b)?,
-                    };
+                    let found = self._in(vm, &a, &b)?;
+                    let value = if invert_yes { !found } else { found };
                     self.push_value(vm.ctx.new_bool(value).into());
                     Ok(None)
                 }

As per coding guidelines, “When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code.”

Also applies to: 5478-5480, 5497-5499, 5508-5510

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 5467 - 5469, Compute the
bytecode::Invert value once into a local named invert (e.g., let invert =
bytecode::Invert::try_from(u32::from(arg)).unwrap_or(bytecode::Invert::No));
then replace the duplicate try_from/unwrap_or calls in the branches that handle
inversion with that single variable and apply the shared boolean inversion logic
one time (use a single match or if let on invert to decide whether to flip the
computed boolean value). Update all locations that currently re-parse invert
(the occurrences around the current match/branches handling inversion) to use
this extracted invert variable and a common inversion step so the branches only
differ by the value used, not the inversion logic.
🤖 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/macros/Cargo.toml`:
- Around line 14-17: The syn dependency in Cargo.toml lacks the "full" feature
required by the procedural macro because the code uses syn types Item, ItemEnum,
ItemStruct and Spanned; update the syn dependency entry to enable the full
feature (i.e., change the syn = { workspace = true } line to include features =
["full"] ) so the procedural macro can compile with those types available.

In `@crates/macros/src/newtype_oparg.rs`:
- Around line 128-141: The code currently calls
attr.parse_nested_meta(...).unwrap(), which panics on bad attribute syntax;
change this to propagate a syn::Result error instead: make the closure passed to
attr.parse_nested_meta return syn::Result<()> (using ? for nested parsing so
unknown/invalid attributes produce a syn::Error), and replace the surrounding
.map/.unwrap pattern with a fallible loop or an iterator collected into Result
(e.g., iterate variants, parse attributes with ? and push VariantInfo into
variants_info), so any parse error from parse_nested_meta bubbles up as a
compile-time error rather than panicking; update handling of display and
catch_all inside the closure to use ? for meta.value().parse and return
Err(meta.error(...)) as syn::Error.

---

Nitpick comments:
In `@crates/vm/src/frame.rs`:
- Around line 5467-5469: Compute the bytecode::Invert value once into a local
named invert (e.g., let invert =
bytecode::Invert::try_from(u32::from(arg)).unwrap_or(bytecode::Invert::No));
then replace the duplicate try_from/unwrap_or calls in the branches that handle
inversion with that single variable and apply the shared boolean inversion logic
one time (use a single match or if let on invert to decide whether to flip the
computed boolean value). Update all locations that currently re-parse invert
(the occurrences around the current match/branches handling inversion) to use
this extracted invert variable and a common inversion step so the branches only
differ by the value used, not the inversion logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: a3f37a06-ba28-4da8-a664-be77f97960bc

📥 Commits

Reviewing files that changed from the base of the PR and between 8c01615 and f5c2f9b.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • Cargo.toml
  • crates/compiler-core/Cargo.toml
  • crates/compiler-core/src/bytecode/oparg.rs
  • crates/macros/Cargo.toml
  • crates/macros/src/lib.rs
  • crates/macros/src/newtype_oparg.rs
  • crates/vm/src/frame.rs

@ShaharNaveh ShaharNaveh requested a review from youknowone March 24, 2026 09:55
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (1)
crates/macros/src/newtype_oparg.rs (1)

30-33: Consider adding #[repr(transparent)] for ABI-safe newtype.

For newtypes that wrap a primitive and implement bidirectional From conversions, adding #[repr(transparent)] guarantees the wrapper has the same memory layout as the inner u32. This is a best practice for FFI safety and ensures the compiler cannot add padding.

♻️ Proposed fix
     let output = quote! {
         #(`#attrs`)*
+        #[repr(transparent)]
         #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
         `#vis` `#struct_token` `#ident`(u32)#semi_token
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/macros/src/newtype_oparg.rs` around lines 30 - 33, The generated
newtype currently emits a wrapper struct (`#ident`) without a representation
attribute; modify the generator in newtype_oparg.rs (where `let output = quote!
{ ... }` is built) to insert `#[repr(transparent)]` on the struct before the
`#[derive(...)]` so the emitted type has ABI-compatible layout with the inner
`u32`; update the quote! invocation that produces `#(`#attrs`)* #[derive(...)]
`#vis` `#struct_token` `#ident`(u32)#semi_token` to include the `#[repr(transparent)]`
attribute (keeping `#(`#attrs`)*` intact) so the produced wrapper benefits from
FFI/ABI safety.
🤖 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/macros/src/newtype_oparg.rs`:
- Around line 171-179: The current use of Vec::pop_if only checks/removes the
last variant so a #[oparg(catch_all)] not at the end remains in variants_info
and later produces invalid match arms; locate the catch_all by searching
variants_info for vinfo.catch_all (e.g., using iter().position or partition),
remove it explicitly (variants_info.remove(idx) or partition into (catch_all,
others)), assign it to catch_all, and then keep the existing duplicate-check
(ensure no other vinfo.catch_all remains) before proceeding; this fixes the
issue where vinfo.discriminant is None for a non-removed catch_all and prevents
generating bad match arms.

---

Nitpick comments:
In `@crates/macros/src/newtype_oparg.rs`:
- Around line 30-33: The generated newtype currently emits a wrapper struct
(`#ident`) without a representation attribute; modify the generator in
newtype_oparg.rs (where `let output = quote! { ... }` is built) to insert
`#[repr(transparent)]` on the struct before the `#[derive(...)]` so the emitted
type has ABI-compatible layout with the inner `u32`; update the quote!
invocation that produces `#(`#attrs`)* #[derive(...)] `#vis` `#struct_token`
`#ident`(u32)#semi_token` to include the `#[repr(transparent)]` attribute (keeping
`#(`#attrs`)*` intact) so the produced wrapper benefits from FFI/ABI safety.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 526bc196-ee3f-44c6-b7f8-ce17a51066a5

📥 Commits

Reviewing files that changed from the base of the PR and between 9fbc6d3 and 77eae15.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • crates/compiler-core/Cargo.toml
  • crates/macros/Cargo.toml
  • crates/macros/src/newtype_oparg.rs
✅ Files skipped from review due to trivial changes (2)
  • crates/compiler-core/Cargo.toml
  • crates/macros/Cargo.toml

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (1)
crates/compiler-core/src/bytecode/oparg.rs (1)

119-573: Run a clean bytecode build sweep for this PR before merge.

This PR modifies bytecode oparg encoding/decoding definitions, so stale build artifacts can mask regressions.

As per coding guidelines, when modifying bytecode instructions, perform a clean build by running: rm -r target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 119 - 573, This
change touches bytecode oparg encodings (e.g., ConvertValueOparg,
MakeFunctionFlag, ComparisonOperator, UnpackExArgs), so perform a clean build
sweep to avoid stale artifacts: delete previous build artifacts for the
rustpython crates and any Python .pyc files, then rebuild (so the new oparg
encoding/decoding is recompiled and tested); after cleaning, run the full
bytecode build/test to verify instructions like Instruction::ConvertValue and
Decode/Encode paths pick up the updated enums and TryFrom/From impls.
🤖 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/oparg.rs`:
- Around line 124-133: The ConvertValueOparg enum now treats None = 0 which
breaks deserialization of legacy .pyc bytes where None was stored as 255; update
the enum deserialization to accept the legacy discriminant by adding a catch-all
variant or mapping for 255 so TryFrom<u32> no longer returns InvalidBytecode for
old files: modify the ConvertValueOparg definition (the enum with
#[newtype_oparg]) to include a #[oparg(catch_all)] variant that converts legacy
value 255 to the current None case (or otherwise upgrade it), ensuring
TryFrom<u32> will map legacy discriminants instead of erroring; alternatively,
if you prefer a breaking change, bump FORMAT_VERSION and add version-gated
deserialization in the TryFrom/u32 logic to reject or migrate old bytecode, but
do not leave TryFrom returning InvalidBytecode for 255.

---

Nitpick comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 119-573: This change touches bytecode oparg encodings (e.g.,
ConvertValueOparg, MakeFunctionFlag, ComparisonOperator, UnpackExArgs), so
perform a clean build sweep to avoid stale artifacts: delete previous build
artifacts for the rustpython crates and any Python .pyc files, then rebuild (so
the new oparg encoding/decoding is recompiled and tested); after cleaning, run
the full bytecode build/test to verify instructions like
Instruction::ConvertValue and Decode/Encode paths pick up the updated enums and
TryFrom/From impls.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: afc4b789-bd38-42dd-bc2f-a83bde9781a6

📥 Commits

Reviewing files that changed from the base of the PR and between 77eae15 and 4d6c725.

📒 Files selected for processing (1)
  • crates/compiler-core/src/bytecode/oparg.rs

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 `@crates/macros/src/newtype_oparg.rs`:
- Around line 96-144: In TryFrom<syn::Variant> for VariantInfo, add validation
so when catch_all is true the variant has exactly one unnamed (tuple) field;
inspect variant.fields and if it's not Fields::Unnamed with exactly one field
return a syn::Error using ident.span() with a clear message (e.g.,
"`#[oparg(catch_all)]` must be a single unnamed field like `Variant(T)`"); keep
this check alongside the existing discriminant/display checks so the macro only
accepts catch_all variants of the form Variant(T).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: af2c53f0-86e7-4456-b59f-3e55baaa4442

📥 Commits

Reviewing files that changed from the base of the PR and between 4d6c725 and c702fbe.

📒 Files selected for processing (1)
  • crates/macros/src/newtype_oparg.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant