Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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 |
Contributor
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
Comment on lines
+14
to
+17
| [dependencies] | ||
| proc-macro2 = { workspace = true } | ||
| quote = { workspace = true } | ||
| syn = { workspace = true } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how syn is configured in the workspace and what features the macro implementation requires
echo "=== Workspace syn configuration ==="
rg -n 'syn\s*=' Cargo.toml
echo -e "\n=== Macro implementation usage ==="
# Check what syn types/functions are used in the macro crate
fd -e rs . crates/macros/src --exec cat {} \; 2>/dev/null | head -100Repository: RustPython/RustPython
Length of output: 2835
🏁 Script executed:
#!/bin/bash
# Check workspace Cargo.toml for syn definition with features
echo "=== Workspace syn definition ==="
rg -A 3 'syn = ' Cargo.toml | head -20
# Also check if there are any feature specifications
echo -e "\n=== Checking for syn features in workspace ==="
rg 'syn.*features' Cargo.tomlRepository: RustPython/RustPython
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Check if workspace Cargo.toml has a [dependencies] section with syn features
echo "=== Workspace Cargo.toml content around syn ==="
rg -B 5 -A 5 'syn = "2"' Cargo.toml
# Also check the exact structure of how dependencies are defined in workspace
echo -e "\n=== Workspace Cargo.toml dependencies section ==="
sed -n '/\[workspace.dependencies\]/,/^\[/p' Cargo.toml | head -50Repository: RustPython/RustPython
Length of output: 2976
🏁 Script executed:
#!/bin/bash
# Check if any other crates in the repo specify features for syn
echo "=== How syn is used in other Cargo.toml files ==="
fd -name Cargo.toml -type f . | xargs rg -l 'syn' | head -10
# Check specific crates to see if they specify features
echo -e "\n=== Syn usage in other crates ==="
fd -name Cargo.toml -type f . | xargs grep -A 2 'syn.*=' | grep -v "^--$" | head -30Repository: RustPython/RustPython
Length of output: 853
🏁 Script executed:
#!/bin/bash
# Find all Cargo.toml files and check how syn is specified
echo "=== All syn dependencies in Cargo.toml files ==="
find . -name Cargo.toml -type f -exec grep -H 'syn' {} \; | grep -v '.pyc' | head -20
# Also check the macros crate Cargo.toml directly
echo -e "\n=== crates/macros/Cargo.toml full content ==="
cat crates/macros/Cargo.tomlRepository: RustPython/RustPython
Length of output: 841
Specify required syn features for the procedural macro.
The macro code uses Item, ItemEnum, ItemStruct, and Spanned from the syn crate, all of which require the "full" feature to be enabled. Update the dependency to:
syn = { workspace = true, features = ["full"] }This aligns with how crates/derive-impl specifies syn features for its procedural macro implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/macros/Cargo.toml` around lines 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.
Comment on lines
+128
to
+141
| attr.parse_nested_meta(|meta| { | ||
| if meta.path.is_ident("display") { | ||
| let value = meta.value()?.parse::<syn::LitStr>()?; | ||
| display = Some(value.value()); | ||
| Ok(()) | ||
| } else if meta.path.is_ident("catch_all") { | ||
| catch_all = true; | ||
| Ok(()) | ||
| } else { | ||
| Err(meta.error("unknown oparg attribute")) | ||
| } | ||
| }) | ||
| .unwrap(); | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate parse error instead of panicking.
Using .unwrap() on the parse_nested_meta result will cause a panic if the attribute syntax is invalid (e.g., #[oparg(display)] without = "value"). This should return a proper compile-time error.
🐛 Proposed fix
attr.parse_nested_meta(|meta| {
if meta.path.is_ident("display") {
let value = meta.value()?.parse::<syn::LitStr>()?;
display = Some(value.value());
Ok(())
} else if meta.path.is_ident("catch_all") {
catch_all = true;
Ok(())
} else {
Err(meta.error("unknown oparg attribute"))
}
- })
- .unwrap();
+ })?;Note: This requires changing the closure to return syn::Result and adjusting the outer map to collect errors properly, or restructuring the iteration to use a fallible pattern:
let mut variants_info = Vec::with_capacity(variants.len());
for variant in variants.iter() {
// ... parsing logic with ? operator ...
variants_info.push(VariantInfo { ... });
}🤖 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 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.
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.
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