Enable some pedantic clippy lints#7764
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 many #[must_use] annotations across crates, updates workspace Clippy lint entries in Cargo.toml, replaces a few .cloned() with .copied(), and performs small behavior-preserving control-flow simplifications and explicit discards of returned values. ChangesWorkspace lint and global config
Must-use annotations, iterator refinements, and small refactors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/cformat.rs (1)
422-424: ⚠️ Potential issue | 🔴 CriticalFix
usizeunderflow in padding calculation.Line 24 uses
cmp::max(0, width - bytes.len())where both areusize. When formatted bytes exceed field width, the subtraction underflows beforemax()is applied: this panics in debug builds and wraps to a huge value in release builds, leading to failed allocation or memory exhaustion. Usesaturating_sub()instead.Proposed fix
- let fill = cmp::max(0, width - bytes.len()); + let fill = width.saturating_sub(bytes.len());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/common/src/cformat.rs` around lines 422 - 424, The padding calculation in CFormatQuantity::Amount handling uses cmp::max(0, width - bytes.len()) which can underflow because width and bytes.len() are usize; change the subtraction to use saturating_sub (e.g., width.saturating_sub(bytes.len())) so fill is computed safely, then allocate Vec::with_capacity(bytes.len() + fill) as before; update the block that reads self.min_field_width and computes fill to use saturating_sub to prevent panic/overflow.
🧹 Nitpick comments (7)
crates/vm/src/builtins/set.rs (1)
111-113: ⚡ Quick winAdd
#[must_use]toPyFrozenSet::elements()for consistency.The
elements()method onPyFrozenSethas the same signature and behavior asPySet::elements()(which now has#[must_use]at line 49), but lacks the attribute. Since both methods allocate aVecwith no side effects, they should both carry the same annotation.🔧 Suggested fix
+ #[must_use] pub fn elements(&self) -> Vec<PyObjectRef> { self.inner.elements() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/set.rs` around lines 111 - 113, The PyFrozenSet::elements() method is missing the #[must_use] attribute like PySet::elements(); add #[must_use] above the pub fn elements(&self) -> Vec<PyObjectRef> in the PyFrozenSet impl so callers are warned to use the returned Vec, mirroring the annotation on PySet::elements().crates/vm/src/types/slot.rs (1)
40-41: ⚡ Quick win
get_mutis missing a symmetric#[must_use]
getat line 41 now carries#[must_use]but the siblingget_mutat line 50 does not, despite also returningOption<&mut T>. Withmust_use_candidateenabled as a workspace lint,get_mutwill now generate a Clippy warning (and fail CI if warnings are denied).🔧 Suggested fix
/// Get a mutable reference to the data if the type matches. + #[must_use] pub fn get_mut<T: Any + 'static>(&mut self) -> Option<&mut T> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/types/slot.rs` around lines 40 - 41, The sibling method get_mut is missing the #[must_use] attribute and will trigger must_use_candidate warnings; add #[must_use] above the pub fn get_mut<T: Any + 'static>(&mut self) -> Option<&mut T> declaration so it matches pub fn get<T...>() and avoids Clippy CI failures, keeping the return type Option<&mut T> unchanged.crates/vm/src/builtins/asyncgenerator.rs (1)
646-652: 💤 Low value
PyAnextAwaitable::newis missing#[must_use]
PyAnextAwaitable::newis another public constructor in this file that was not annotated whilePyAsyncGen::newwas. For consistency with the PR's intent:✏️ Proposed fix
+ #[must_use] pub fn new(wrapped: PyObjectRef, default_value: PyObjectRef) -> Self {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/asyncgenerator.rs` around lines 646 - 652, The constructor PyAnextAwaitable::new is a public factory that should be annotated with #[must_use] for consistency and to warn callers when the returned awaitable is ignored; add the #[must_use] attribute directly above the pub fn new definition for PyAnextAwaitable (the function named new with signature pub fn new(wrapped: PyObjectRef, default_value: PyObjectRef) -> Self) so the compiler will emit a warning if the returned value is not used.crates/host_env/src/posix.rs (1)
85-87: ⚡ Quick winMake
#[must_use]consistent across all cfg variants ofget_number_of_os_threads.Right now only the fallback implementation is annotated. Adding
#[must_use]to the macOS (Line 16) and Linux (Line 63) variants too will keep lint behavior uniform across targets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/host_env/src/posix.rs` around lines 85 - 87, The three platform-specific implementations of get_number_of_os_threads have inconsistent annotation: only the fallback variant has #[must_use]; add #[must_use] above the macOS and Linux cfg blocks (the functions that implement get_number_of_os_threads under cfg(target_os = "macos") and cfg(target_os = "linux")) so all cfg variants have the same must_use lint behavior.crates/common/src/hash.rs (1)
54-57: ⚡ Quick win
HashSecret::hash_valueis missing#[must_use].
hash_bytes,hash_str, andhash_bigint— all returningPyHash— were annotated, buthash_valuewas not. It would be flagged bymust_use_candidatejust like the others.✏️ Proposed fix
+ #[must_use] pub fn hash_value<T: Hash + ?Sized>(&self, data: &T) -> PyHash { fix_sentinel(mod_int(self.hash_one(data) as _)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/common/src/hash.rs` around lines 54 - 57, The method HashSecret::hash_value is missing the #[must_use] attribute like the other hash_* methods; add #[must_use] immediately above the impl method declaration for hash_value to match hash_bytes, hash_str, and hash_bigint so callers are warned when the returned PyHash is ignored (target the HashSecret::hash_value function identifier).crates/derive-impl/src/lib.rs (1)
77-83: ⚡ Quick win
py_compileandpy_freezeare missing#[must_use].Every other public
-> TokenStreamfunction in this file received the annotation, but these two did not. Withmust_use_candidatenow enabled as a workspace lint, Clippy will still warn on them.✏️ Proposed fix
+#[must_use] pub fn py_compile(input: TokenStream, compiler: &dyn Compiler) -> TokenStream { result_to_tokens(compile_bytecode::impl_py_compile(input, compiler)) } +#[must_use] pub fn py_freeze(input: TokenStream, compiler: &dyn Compiler) -> TokenStream { result_to_tokens(compile_bytecode::impl_py_freeze(input, compiler)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/derive-impl/src/lib.rs` around lines 77 - 83, Add the #[must_use] attribute to the public functions that return TokenStream so callers are warned if the result is ignored; specifically annotate py_compile and py_freeze with #[must_use] (matching the other public TokenStream-returning functions in this module) so Clippy's must_use_candidate lint no longer emits warnings for impl_py_compile/impl_py_freeze wrappers.crates/vm/src/vm/interpreter.rs (1)
229-256: ⚡ Quick win
init_hookandadd_frozen_modulesare missing#[must_use].Both are builder-chain methods returning
Self. All other builder methods in thisimplblock are annotated; these two would still triggermust_use_candidate.✏️ Proposed fix
+ #[must_use] pub fn init_hook<F>(mut self, init: F) -> Self where F: FnOnce(&mut VirtualMachine) + 'static, { self.init_hooks.push(Box::new(init)); self } + #[must_use] pub fn add_frozen_modules<I>(mut self, frozen: I) -> Self where I: IntoIterator<Item = (&'static str, FrozenModule)>, { self.frozen_modules.extend(frozen); self }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/vm/interpreter.rs` around lines 229 - 256, The builder methods init_hook and add_frozen_modules are missing #[must_use]; add the #[must_use] attribute above both pub fn init_hook<F>(...) -> Self and pub fn add_frozen_modules<I>(...) -> Self so callers get a compiler warning if the returned Self is discarded, matching the other builder methods in this impl block and resolving the must_use_candidate lint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 279-281: The comment header "# pendantic lints to enforce
gradually" contains a typo; change "pendantic" to "pedantic" (the comment above
the lint entries such as cloned_instead_of_copied and must_use_candidate) so the
section correctly reads "# pedantic lints to enforce gradually".
In `@crates/host_env/src/syslog.rs`:
- Around line 64-67: The log_mask function currently shifts the priority value
instead of creating a bit mask; update the pub const fn log_mask(pri: i32) ->
i32 to return a single-bit mask using 1 << pri (matching the semantics used by
log_upto), so that priority N maps to a mask with bit N set (e.g., priority 0 ->
1, priority 3 -> 8) while preserving the function signature and attributes.
In `@crates/vm/src/macros.rs`:
- Around line 70-72: Remove the duplicate import of PyPayload in the first
doctest: the doctest currently imports PyPayload twice (once in the grouped
import use rustpython_vm::{PyPayload, match_class}; and again in use
rustpython_vm::{PyPayload};), causing a duplicate-name compilation error; delete
the redundant use rustpython_vm::{PyPayload}; (or merge the imports) so
PyPayload is only imported once in that doctest.
---
Outside diff comments:
In `@crates/common/src/cformat.rs`:
- Around line 422-424: The padding calculation in CFormatQuantity::Amount
handling uses cmp::max(0, width - bytes.len()) which can underflow because width
and bytes.len() are usize; change the subtraction to use saturating_sub (e.g.,
width.saturating_sub(bytes.len())) so fill is computed safely, then allocate
Vec::with_capacity(bytes.len() + fill) as before; update the block that reads
self.min_field_width and computes fill to use saturating_sub to prevent
panic/overflow.
---
Nitpick comments:
In `@crates/common/src/hash.rs`:
- Around line 54-57: The method HashSecret::hash_value is missing the
#[must_use] attribute like the other hash_* methods; add #[must_use] immediately
above the impl method declaration for hash_value to match hash_bytes, hash_str,
and hash_bigint so callers are warned when the returned PyHash is ignored
(target the HashSecret::hash_value function identifier).
In `@crates/derive-impl/src/lib.rs`:
- Around line 77-83: Add the #[must_use] attribute to the public functions that
return TokenStream so callers are warned if the result is ignored; specifically
annotate py_compile and py_freeze with #[must_use] (matching the other public
TokenStream-returning functions in this module) so Clippy's must_use_candidate
lint no longer emits warnings for impl_py_compile/impl_py_freeze wrappers.
In `@crates/host_env/src/posix.rs`:
- Around line 85-87: The three platform-specific implementations of
get_number_of_os_threads have inconsistent annotation: only the fallback variant
has #[must_use]; add #[must_use] above the macOS and Linux cfg blocks (the
functions that implement get_number_of_os_threads under cfg(target_os = "macos")
and cfg(target_os = "linux")) so all cfg variants have the same must_use lint
behavior.
In `@crates/vm/src/builtins/asyncgenerator.rs`:
- Around line 646-652: The constructor PyAnextAwaitable::new is a public factory
that should be annotated with #[must_use] for consistency and to warn callers
when the returned awaitable is ignored; add the #[must_use] attribute directly
above the pub fn new definition for PyAnextAwaitable (the function named new
with signature pub fn new(wrapped: PyObjectRef, default_value: PyObjectRef) ->
Self) so the compiler will emit a warning if the returned value is not used.
In `@crates/vm/src/builtins/set.rs`:
- Around line 111-113: The PyFrozenSet::elements() method is missing the
#[must_use] attribute like PySet::elements(); add #[must_use] above the pub fn
elements(&self) -> Vec<PyObjectRef> in the PyFrozenSet impl so callers are
warned to use the returned Vec, mirroring the annotation on PySet::elements().
In `@crates/vm/src/types/slot.rs`:
- Around line 40-41: The sibling method get_mut is missing the #[must_use]
attribute and will trigger must_use_candidate warnings; add #[must_use] above
the pub fn get_mut<T: Any + 'static>(&mut self) -> Option<&mut T> declaration so
it matches pub fn get<T...>() and avoids Clippy CI failures, keeping the return
type Option<&mut T> unchanged.
In `@crates/vm/src/vm/interpreter.rs`:
- Around line 229-256: The builder methods init_hook and add_frozen_modules are
missing #[must_use]; add the #[must_use] attribute above both pub fn
init_hook<F>(...) -> Self and pub fn add_frozen_modules<I>(...) -> Self so
callers get a compiler warning if the returned Self is discarded, matching the
other builder methods in this impl block and resolving the must_use_candidate
lint.
🪄 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: 123fae55-9354-4efb-9690-1100108da36d
📒 Files selected for processing (90)
Cargo.tomlcrates/codegen/src/compile.rscrates/codegen/src/symboltable.rscrates/common/src/atomic.rscrates/common/src/boxvec.rscrates/common/src/cformat.rscrates/common/src/float_ops.rscrates/common/src/format.rscrates/common/src/hash.rscrates/common/src/int.rscrates/common/src/linked_list.rscrates/common/src/lock/thread_mutex.rscrates/common/src/rand.rscrates/common/src/refcount.rscrates/common/src/str.rscrates/compiler-core/src/bytecode.rscrates/compiler-core/src/bytecode/instruction.rscrates/compiler-core/src/bytecode/oparg.rscrates/compiler-core/src/marshal.rscrates/compiler/src/lib.rscrates/derive-impl/src/compile_bytecode.rscrates/derive-impl/src/lib.rscrates/derive-impl/src/pyclass.rscrates/derive-impl/src/pymodule.rscrates/derive-impl/src/pystructseq.rscrates/host_env/src/crt_fd.rscrates/host_env/src/os.rscrates/host_env/src/posix.rscrates/host_env/src/select.rscrates/host_env/src/signal.rscrates/host_env/src/syslog.rscrates/host_env/src/time.rscrates/sre_engine/src/engine.rscrates/sre_engine/src/string.rscrates/stdlib/src/compression.rscrates/stdlib/src/faulthandler.rscrates/stdlib/src/select.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rscrates/vm/src/buffer.rscrates/vm/src/builtins/asyncgenerator.rscrates/vm/src/builtins/bytes.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/coroutine.rscrates/vm/src/builtins/float.rscrates/vm/src/builtins/function.rscrates/vm/src/builtins/generator.rscrates/vm/src/builtins/int.rscrates/vm/src/builtins/iter.rscrates/vm/src/builtins/module.rscrates/vm/src/builtins/range.rscrates/vm/src/builtins/set.rscrates/vm/src/builtins/staticmethod.rscrates/vm/src/builtins/str.rscrates/vm/src/builtins/template.rscrates/vm/src/builtins/traceback.rscrates/vm/src/builtins/tuple.rscrates/vm/src/builtins/union.rscrates/vm/src/class.rscrates/vm/src/datastack.rscrates/vm/src/function/argument.rscrates/vm/src/function/buffer.rscrates/vm/src/function/fspath.rscrates/vm/src/function/method.rscrates/vm/src/function/number.rscrates/vm/src/function/protocol.rscrates/vm/src/function/time.rscrates/vm/src/gc_state.rscrates/vm/src/macros.rscrates/vm/src/object/core.rscrates/vm/src/object/ext.rscrates/vm/src/ospath.rscrates/vm/src/protocol/buffer.rscrates/vm/src/protocol/mapping.rscrates/vm/src/protocol/number.rscrates/vm/src/protocol/sequence.rscrates/vm/src/scope.rscrates/vm/src/signal.rscrates/vm/src/sliceable.rscrates/vm/src/stdlib/_ctypes.rscrates/vm/src/stdlib/_sre.rscrates/vm/src/stdlib/_thread.rscrates/vm/src/stdlib/builtins.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rscrates/vm/src/version.rscrates/vm/src/vm/interpreter.rscrates/vm/src/vm/mod.rscrates/vm/src/vm/setting.rscrates/vm/src/vm/thread.rs
Sorry, something went wrong.
408a498 to
37418c5
Compare
May 2, 2026 13:39
ee5e9d0
into
RustPython:main
May 4, 2026
Since the codebase is very large, enabling all the pedantic lints at once will be a impossible to review. I'll try to make relatively small incremental changes
Summary by CodeRabbit
Chores
Notes