more clippy rules by ShaharNaveh · Pull Request #8050 · RustPython/RustPython
PR changed again? Review this PR in Change Stack to compare snapshots and stay oriented.
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5e8c61b5-4d14-4150-a18e-457ba02954cd
📒 Files selected for processing (17)
Cargo.tomlcrates/capi/src/traceback.rscrates/common/src/boxvec.rscrates/common/src/cformat.rscrates/derive-impl/src/pyclass.rscrates/jit/src/instructions.rscrates/sre_engine/src/string.rscrates/stdlib/src/_asyncio.rscrates/vm/src/builtins/dict.rscrates/vm/src/builtins/getset.rscrates/vm/src/builtins/memory.rscrates/vm/src/builtins/str.rscrates/vm/src/bytes_inner.rscrates/vm/src/frame.rscrates/vm/src/stdlib/sys.rscrates/vm/src/types/slot_defs.rscrates/vm/src/vm/interpreter.rs
📝 Walkthrough
Walkthrough
This PR updates RustPython's code quality and build configuration by extending clippy lint rules, modernizing unsafe pointer arithmetic to use add/sub instead of offset, consolidating single-element iterator patterns, adding #[must_use] annotations to builder and constructor methods, and refactoring several control flow patterns for clarity.
Changes
Code Quality, Lint Configuration, and Pattern Modernization
| Layer / File(s) | Summary |
|---|---|
Workspace clippy lint configuration Cargo.toml |
Workspace lint section expanded with new nursery lints (iter_on_empty_collections, iter_on_single_items, literal_string_with_formatting_args, needless_collect) and extended pedantic lints (checked_conversions, cloned_instead_of_copied, collapsible_else_if, comparison_chain, duration_suboptimal_units, and others). |
Unsafe pointer arithmetic modernization crates/common/src/boxvec.rs, crates/sre_engine/src/string.rs |
Replaced ptr::offset(±1) calls with explicit ptr::add(1) and ptr::sub(1) in BoxVec::try_insert, StringCursor::back_peek, and UTF-8 decoding routines (next_code_point, next_code_point_reverse) for clearer intent and alignment with modern Rust idioms. |
Iterator pattern and collection refactoring crates/capi/src/traceback.rs, crates/stdlib/src/_asyncio.rs, crates/vm/src/stdlib/sys.rs, crates/derive-impl/src/pyclass.rs, crates/vm/src/bytes_inner.rs, crates/vm/src/types/slot_defs.rs |
Consolidated single-element keyword argument construction to use core::iter::once() instead of array iterators; refactored capitalize to use explicit for loop; replaced collection+length pattern with direct count() in test assertions; updated pyslot attribute parsing to use if let pattern. |
Builder and return-value #[must_use] annotations crates/vm/src/builtins/getset.rs, crates/vm/src/builtins/memory.rs, crates/vm/src/vm/interpreter.rs, crates/vm/src/builtins/dict.rs, crates/common/src/cformat.rs |
Added #[must_use] to builder methods (PyGetSet::new, with_get, with_set; InterpreterBuilder::init_hook, add_frozen_modules), public constructors (PyMemoryView::new_view), and value-returning methods (FormatBuf::concat, dict.setdefault). |
Targeted control flow refactorings crates/vm/src/builtins/dict.rs, crates/vm/src/builtins/str.rs, crates/vm/src/frame.rs, crates/jit/src/instructions.rs |
Refactored dict.get to use unwrap_or_else for default fallback; updated char/CodePoint conversion to use u8::try_from().map_or_else() pattern; strengthened ForIterGen specialization guard with i16::try_from() bounds check; added clippy mut_mut expectation attributes to store_variable and get_or_create_block methods. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- RustPython/RustPython#7875: Both PRs modify
Cargo.toml's[workspace.lints.clippy]configuration by reorganizing/adding clippy "nursery" and "pedantic"warnlints. - RustPython/RustPython#7764: Both PRs extend the same
Cargo.tomlworkspace Clippy lint configuration with expanded pedantic/warn lint rules, includingcloned_instead_of_copied. - RustPython/RustPython#7830: Both PRs modify
Cargo.toml's pedantic lint configuration, adding clippy rules likecollapsible_else_ifandcomparison_chain.
Suggested reviewers
- youknowone
- fanninpm
- coolreader18
Poem
🐰 A rabbit hops through clippy's garden bright,
With safer pointers, add and sub take flight,
Builder patterns marked "don't ignore my way!"
Once() for single items—cleaner code today,
Modern Rust idioms, refactored with care. 🌟
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title 'more clippy rules' is vague and generic, using non-descriptive phrasing that doesn't convey meaningful information about the specific changes in the changeset. | Consider a more descriptive title that specifies the main clippy rules being added or the primary refactoring effort (e.g., 'Add clippy pedantic and nursery lints to workspace' or 'Apply clippy suggestions across codebase'). |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 @coderabbitai help to get the list of available commands and usage tips.