Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new procedural-macro crate Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
5467-5469: Extractinvertonce and reuse shared inversion logic.These branches all re-parse
invertand repeat the same boolean inversion pattern. Please computeinvertonce 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/compiler-core/Cargo.tomlcrates/compiler-core/src/bytecode/oparg.rscrates/macros/Cargo.tomlcrates/macros/src/lib.rscrates/macros/src/newtype_oparg.rscrates/vm/src/frame.rs
There was a problem hiding this comment.
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
Fromconversions, adding#[repr(transparent)]guarantees the wrapper has the same memory layout as the inneru32. 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/compiler-core/Cargo.tomlcrates/macros/Cargo.tomlcrates/macros/src/newtype_oparg.rs
✅ Files skipped from review due to trivial changes (2)
- crates/compiler-core/Cargo.toml
- crates/macros/Cargo.toml
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
crates/compiler-core/src/bytecode/oparg.rs
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
crates/macros/src/newtype_oparg.rs
Reason for creating a new crate for this is because
rustpython-derive-implhasrustpython-compileras a dependency, so it created a circular dependency:/Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores