Skip to content

Add more clippy rules#8183

Merged
youknowone merged 7 commits into
RustPython:mainfrom
ShaharNaveh:clippy-rules-6
Jun 30, 2026
Merged

Add more clippy rules#8183
youknowone merged 7 commits into
RustPython:mainfrom
ShaharNaveh:clippy-rules-6

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Summary by CodeRabbit

  • Chores
    • Expanded code quality checks with additional lint rules and several lint exceptions where needed.
    • Updated a few internal implementations to use clearer, equivalent patterns for string handling, cloning, and assertions.
  • Bug Fixes
    • Improved error handling and path setup behavior in a few runtime code paths.
    • Refined system load average handling to return a consistent error when values can’t be obtained.

@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 9bba8eba-344c-475a-849e-2f6d79dc5c69

📥 Commits

Reviewing files that changed from the base of the PR and between e1d1331 and 5bf2a0c.

📒 Files selected for processing (3)
  • Cargo.toml
  • crates/jit/src/instructions.rs
  • crates/vm/src/stdlib/os.rs
💤 Files with no reviewable changes (1)
  • Cargo.toml
✅ Files skipped from review due to trivial changes (1)
  • crates/jit/src/instructions.rs

📝 Walkthrough

Walkthrough

Adds four new Clippy warn lints (tuple_array_conversions, while_float, assigning_clones, manual_assert) to the workspace configuration, then updates all affected code sites to comply: replacing if/panic with assert!, .clone() assignments with clone_from, "...".to_owned() with String::from, making hash_float a const fn, and minor cleanups in os.rs and JIT.

Changes

Clippy lint enforcement and code fixes

Layer / File(s) Summary
Clippy lint configuration
Cargo.toml
Adds tuple_array_conversions, while_float, assigning_clones, and manual_assert as warn-level lints in the workspace Clippy configuration.
Float, hash, and numeric fixes
crates/common/src/float_ops.rs, crates/common/src/hash.rs
decompose_float uses canonical value == 0.0 comparison; hash_float promoted to const fn with #[expect(clippy::while_float)]; hash_bigint refactored from match/unreachable_unchecked to if let/unwrap_unchecked.
assert! and clone_from replacements
crates/vm/src/vm/mod.rs, crates/vm/src/types/slot.rs, crates/vm/src/function/builtin.rs, crates/vm/src/getpath.rs, crates/codegen/src/symboltable.rs
Manual if condition { panic!(...) } guards replaced with assert!; field clone assignments replaced with clone_from calls.
String::from and minor cleanups
crates/codegen/src/compile.rs, crates/vm/src/vm/vm_new.rs, crates/vm/src/stdlib/os.rs, crates/jit/src/instructions.rs
.to_owned() on literals replaced with String::from; getloadavg simplified via map(Into::into); #[expect(clippy::tuple_array_conversions)] added in JIT; statvfs doc comment removed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

🐇 A rabbit who lints with great care,
Replaced panics with assert! right there,
clone_from saves bytes,
const fn delights,
Now Clippy finds nothing to snare! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 69.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding additional Clippy lint rules.
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.
✨ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/stdlib/src/json.rs (1)

167-183: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Keep parse_constant callback arguments as strings.

Line 172 now forwards b"NaN"/b"Infinity"/b"-Infinity" into self.parse_constant.call(...), so this path passes Python bytes instead of the str tokens that call_scan_once still uses at Lines 594, 599, and 605. That breaks the callback contract for these constants depending on which scanner path is taken.

Proposed fix
             macro_rules! parse_constant {
-                ($s:literal) => {
-                    if rest.starts_with($s) {
+                ($bytes:literal, $token:literal) => {
+                    if rest.starts_with($bytes) {
                         return Ok(PyIterReturn::Return(
                             vm.new_tuple((
-                                self.parse_constant.call(($s,), vm)?,
-                                char_idx + $s.len(),
+                                self.parse_constant.call(($token,), vm)?,
+                                char_idx + $bytes.len(),
                             ))
                             .into(),
                         ));
                     }
                 };
             }

-            parse_constant!(b"NaN");
-            parse_constant!(b"Infinity");
-            parse_constant!(b"-Infinity");
+            parse_constant!(b"NaN", "NaN");
+            parse_constant!(b"Infinity", "Infinity");
+            parse_constant!(b"-Infinity", "-Infinity");
🤖 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/stdlib/src/json.rs` around lines 167 - 183, The parse_constant
callback in json scanning is receiving bytes for NaN/Infinity/-Infinity, which
breaks the existing string-based contract. Update the constant parsing path in
the macro in json.rs so `self.parse_constant.call(...)` is passed string tokens
instead of byte literals, matching the `call_scan_once` paths and keeping
`parse_constant` consistent across all scanner entry points.
🤖 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.

Outside diff comments:
In `@crates/stdlib/src/json.rs`:
- Around line 167-183: The parse_constant callback in json scanning is receiving
bytes for NaN/Infinity/-Infinity, which breaks the existing string-based
contract. Update the constant parsing path in the macro in json.rs so
`self.parse_constant.call(...)` is passed string tokens instead of byte
literals, matching the `call_scan_once` paths and keeping `parse_constant`
consistent across all scanner entry points.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 8424896a-357f-42eb-a11f-ce26e1fe854b

📥 Commits

Reviewing files that changed from the base of the PR and between 0009dd6 and e1d1331.

📒 Files selected for processing (13)
  • Cargo.toml
  • crates/codegen/src/compile.rs
  • crates/codegen/src/symboltable.rs
  • crates/common/src/float_ops.rs
  • crates/common/src/hash.rs
  • crates/common/src/int.rs
  • crates/stdlib/src/json.rs
  • crates/vm/src/builtins/bytes.rs
  • crates/vm/src/function/builtin.rs
  • crates/vm/src/getpath.rs
  • crates/vm/src/types/slot.rs
  • crates/vm/src/vm/mod.rs
  • crates/vm/src/vm/vm_new.rs

@youknowone youknowone merged commit 12b8305 into RustPython:main Jun 30, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants