Actionable comments posted: 2

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.

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.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

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

• Cargo.lock is excluded by !**/*.lock

• 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

Actionable comments posted: 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.

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.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

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

• Cargo.lock is excluded by !**/*.lock

• crates/compiler-core/Cargo.toml
• crates/macros/Cargo.toml
• crates/macros/src/newtype_oparg.rs

• crates/compiler-core/Cargo.toml
• crates/macros/Cargo.toml

Actionable comments posted: 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.

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.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

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

• crates/compiler-core/src/bytecode/oparg.rs

Actionable comments posted: 1

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).

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

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

• crates/macros/src/newtype_oparg.rs