General code nits#8107
Conversation
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR applies broad code-quality improvements across the RustPython VM crate: eliminates intermediate string allocations in error paths by changing ChangesVM crate code quality and refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/anystr.rs`:
- Around line 414-419: The py_islower and py_isupper methods are using
filter().all() which returns vacuously true when no ASCII alphabetic characters
are present, causing empty strings and digit-only strings to incorrectly return
true. In both functions (py_islower at lines 414-419 and py_isupper at lines
423-428), you need to ensure at least one ASCII alphabetic character exists
before checking the case condition. Modify the logic to use both an any() check
to verify at least one ASCII alphabetic byte is present AND the all() check to
ensure all such bytes match the required case, combining them with AND logic so
both conditions must be true for the function to return true.
In `@crates/vm/src/cformat.rs`:
- Around line 361-379: The guard condition in the bytes formatting branch at
crates/vm/src/cformat.rs#L361-L379 currently uses OR logic which makes the
literal-only path unreachable. The condition should reject only when the value
is neither a mapping nor an empty tuple. Change the boolean operator from || to
&& in the condition checking !is_mapping and the tuple predicate to properly
accept mapping objects or empty tuples while rejecting other values. Apply the
identical fix to the string formatting branch at
crates/vm/src/cformat.rs#L458-L476, changing the same OR operator to AND in the
corresponding guard condition.
In `@crates/vm/src/compiler.rs`:
- Around line 1-2: The ToPyException impl blocks at lines 41-52 have feature
gates that don't match the VM method gates they depend on. Locate the two
ToPyException impl blocks that call the methods new_syntax_error and
new_syntax_error_maybe_incomplete, and change their cfg attribute from
any(feature = "parser", feature = "codegen") to any(feature = "parser", feature
= "compiler") to match the gate on those called VM methods, preventing linking
failures when building with --features=codegen alone.
- Around line 24-27: The module paths in the error enum variant definitions are
stale and no longer valid. The Codegen variant references
super::codegen::error::CodegenErrorType which should be replaced with the
absolute path rustpython_codegen::error::CodegenErrorType. The Parse variant
references super::parser::ParseErrorType which should be replaced with the local
parser alias that is already defined on line 12, changing it to simply
parser::ParseErrorType.
🪄 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: fa25c5e2-0970-4999-807c-f9e62d8ebbfc
📒 Files selected for processing (14)
crates/stdlib/src/pystruct.rscrates/vm/src/anystr.rscrates/vm/src/buffer.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/staticmethod.rscrates/vm/src/byte.rscrates/vm/src/cformat.rscrates/vm/src/class.rscrates/vm/src/codecs.rscrates/vm/src/compiler.rscrates/vm/src/convert/try_from.rscrates/vm/src/gc_state.rscrates/vm/src/recursion.rscrates/vm/src/suggestion.rs
| #[cfg(all(not(feature = "compiler"), feature = "parser", feature = "codegen",))] | ||
| compile_error!("Use --features=compiler to enable both parser and codegen"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the relevant feature declarations and cfg gates without building.
rg -n -C3 '\bcompiler\s*=|\bparser\s*=|\bcodegen\s*=' --iglob 'Cargo.toml'
rg -n -C3 'new_syntax_error(_maybe_incomplete)?|impl crate::convert::ToPyException for \(CompileError' --type=rustRepository: RustPython/RustPython
Length of output: 14894
Change the ToPyException impl gates to match the VM method gates.
The new compile-time guard only rejects not(compiler) && parser && codegen, but codegen alone is a valid feature combination. Building with --features=codegen without parser or compiler would compile the ToPyException impls (lines 41-52) while the VM methods they call—new_syntax_error and new_syntax_error_maybe_incomplete—remain gated on any(feature = "parser", feature = "compiler"), causing a linking failure.
Fix: change the cfg gates on both impls from #[cfg(any(feature = "parser", feature = "codegen"))] to #[cfg(any(feature = "parser", feature = "compiler"))] to match the availability of the called methods.
Suggested diff
-#[cfg(any(feature = "parser", feature = "codegen"))]
+#[cfg(any(feature = "parser", feature = "compiler"))]
impl crate::convert::ToPyException for (CompileError, Option<&str>) {
fn to_pyexception(&self, vm: &crate::VirtualMachine) -> crate::builtins::PyBaseExceptionRef {
vm.new_syntax_error(&self.0, self.1)
}
}
-#[cfg(any(feature = "parser", feature = "codegen"))]
+#[cfg(any(feature = "parser", feature = "compiler"))]
impl crate::convert::ToPyException for (CompileError, Option<&str>, bool) {
fn to_pyexception(&self, vm: &crate::VirtualMachine) -> crate::builtins::PyBaseExceptionRef {
vm.new_syntax_error_maybe_incomplete(&self.0, self.1, self.2)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/compiler.rs` around lines 1 - 2, The ToPyException impl blocks
at lines 41-52 have feature gates that don't match the VM method gates they
depend on. Locate the two ToPyException impl blocks that call the methods
new_syntax_error and new_syntax_error_maybe_incomplete, and change their cfg
attribute from any(feature = "parser", feature = "codegen") to any(feature =
"parser", feature = "compiler") to match the gate on those called VM methods,
preventing linking failures when building with --features=codegen alone.
| Codegen(#[from] super::codegen::error::CodegenErrorType), | ||
| #[cfg(feature = "parser")] | ||
| #[error(transparent)] | ||
| Parse(#[from] super::parser::ParseErrorType), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect whether `super::parser` / `super::codegen` can resolve from `crate::compiler`.
rg -n -C3 'super::(parser|codegen)::|pub use ruff_python_parser as parser|rustpython_codegen::' --type=rust
rg -n -C3 '^\s*(pub\s+)?mod\s+(parser|codegen)\b|^\s*pub\s+use\s+.*\s+as\s+(parser|codegen)\b' --type=rustRepository: RustPython/RustPython
Length of output: 4574
🏁 Script executed:
#!/bin/bash
# Check if these are truly at top level or in a nested module
head -50 crates/vm/src/compiler.rs | cat -nRepository: RustPython/RustPython
Length of output: 2043
🏁 Script executed:
#!/bin/bash
# Look for where codegen module might be defined or re-exported
rg -n 'mod codegen|pub use.*codegen' crates/vm/src/compiler.rsRepository: RustPython/RustPython
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Check rustpython_codegen crate structure for the error types
fd -t f '\.rs$' crates/codegen/src/ 2>/dev/null | head -20 || echo "codegen crate not found, checking if it's external"Repository: RustPython/RustPython
Length of output: 307
🏁 Script executed:
#!/bin/bash
# Verify what types are actually available from ruff_python_parser via the parser alias
rg -n 'ParseError' crates/compiler/src/ | head -10Repository: RustPython/RustPython
Length of output: 893
🏁 Script executed:
#!/bin/bash
# Check what error types rustpython_codegen exports from its error module
rg -n 'pub.*CodegenError' crates/codegen/src/error.rs | head -5Repository: RustPython/RustPython
Length of output: 260
🏁 Script executed:
#!/bin/bash
# Check what ParseErrorType is exported as in ruff_python_parser
# Since ruff_python_parser is external, check how it's used in crates/compiler
rg -n 'ParseErrorType|ParseError' crates/compiler/src/lib.rs | grep -E '(use|import|pub)'Repository: RustPython/RustPython
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Verify that super::parser and super::codegen don't resolve by checking if these modules exist in vm crate
ls -la crates/vm/src/ | grep -E 'codegen|parser' || echo "No codegen or parser directories found"Repository: RustPython/RustPython
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Check if the file actually compiles with these broken paths
# by looking for any build/compilation context
cd crates/vm && cargo check --no-default-features --features parser 2>&1 | head -50 || echo "Cannot build in sandbox"Repository: RustPython/RustPython
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Double-check the exact exports from rustpython_codegen error module
rg -n 'pub (enum|struct) (CodegenError|CodegenErrorType)' crates/codegen/src/error.rsRepository: RustPython/RustPython
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Verify the cfg_select conditions that guard the parser alias availability
rg -B5 -A10 'pub use ruff_python_parser as parser' crates/vm/src/compiler.rsRepository: RustPython/RustPython
Length of output: 449
Update stale super:: paths after promoting the error enums.
The super::codegen::error::* and super::parser::* paths are broken. These enums are at the top level of compiler.rs with no parent module containing codegen or parser subdirectories. Use the local parser alias (defined on line 12) and the absolute rustpython_codegen::error::* path instead.
Proposed fix
#[cfg(feature = "codegen")]
#[error(transparent)]
- Codegen(#[from] super::codegen::error::CodegenErrorType),
+ Codegen(#[from] rustpython_codegen::error::CodegenErrorType),
#[cfg(feature = "parser")]
#[error(transparent)]
- Parse(#[from] super::parser::ParseErrorType),
+ Parse(#[from] parser::ParseErrorType),
}
@@
#[cfg(feature = "codegen")]
#[error(transparent)]
- Codegen(#[from] super::codegen::error::CodegenError),
+ Codegen(#[from] rustpython_codegen::error::CodegenError),
#[cfg(feature = "parser")]
#[error(transparent)]
- Parse(#[from] super::parser::ParseError),
+ Parse(#[from] parser::ParseError),
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/compiler.rs` around lines 24 - 27, The module paths in the
error enum variant definitions are stale and no longer valid. The Codegen
variant references super::codegen::error::CodegenErrorType which should be
replaced with the absolute path rustpython_codegen::error::CodegenErrorType. The
Parse variant references super::parser::ParseErrorType which should be replaced
with the local parser alias that is already defined on line 12, changing it to
simply parser::ParseErrorType.
| let duration = cfg_select! { | ||
| target_arch = "wasm32" => 0.0, | ||
| _ => elapsed_secs(&start_time), | ||
| }; |
There was a problem hiding this comment.
what's the rational of this manual inlining?
There was a problem hiding this comment.
Looking at it again, it does look silly. I'll revert this part
Summary
Summary by CodeRabbit