Enforce strict clippy lint policy - #71
Conversation
📝 WalkthroughWalkthroughThe workspace now enables broad Clippy and Rust lint warnings. Crates inherit these settings, while engine, server, client, benchmark, and test code use documented ChangesLint enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/omnikv-engine/src/lib.rs (1)
7-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftBroad crate-level
#[expect]for ~50 Clippy lints may mask new violations.While the reason string documents this as legacy debt to burn down,
#[expect]satisfies silently when the lint fires — meaning new code that introduces, e.g.,cast_possible_truncationorcast_sign_losswill produce no warning. Consider narrowing the most security-sensitive lints (casts,unwrap-adjacent) to item-level expectations so new violations in those categories are caught by CI.🤖 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/omnikv-engine/src/lib.rs` around lines 7 - 59, Replace the broad crate-level #[expect] list in the crate root with item-level expectations for the legacy Clippy findings, prioritizing security-sensitive cast and unwrap-adjacent lints so new violations remain visible to CI. Remove those sensitive lints from the crate-level attribute while preserving expectations only on the specific existing items that require them.crates/omnikv-engine/tests/raft_cluster.rs (1)
5-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffScope these
#[expect]attributes to the smallest test/benchmark items
These file-level expectations blanket-suppress the listed clippy lints for the whole crate, so unrelated future violations in the same test/bench file can slip through. Move them onto the specific helper/test/benchmark items that actually need each lint.🤖 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/omnikv-engine/tests/raft_cluster.rs` around lines 5 - 15, Replace the file-level Clippy #[expect] blocks with item-level attributes on only the specific helpers, tests, or benchmarks that trigger each lint, preserving their existing reasons and removing blanket suppression. Apply this scoping change in crates/omnikv-engine/tests/raft_cluster.rs (lines 5-15), crates/omnikv-engine/benches/omni_bench.rs (lines 10-19), crates/omnikv-engine/benches/scan_buffer_pool.rs (lines 11-16), crates/omnikv-engine/benches/volcano_dispatch.rs (lines 10-16), crates/omnikv-engine/tests/anomaly_demos.rs (lines 7-13), crates/omnikv-engine/tests/sql_layer.rs (lines 5-10), crates/omnikv-engine/tests/sql_v3_features.rs (lines 5-9), and crates/omnikv-engine/tests/storage_correctness.rs (lines 1-9).
🤖 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/omnikv-engine/tests/durability_evidence.rs`:
- Around line 1-7: Move each file-level #![expect] lint declaration onto the
specific test function or local block that triggers it, preserving the existing
lint list and reason. Apply this in
crates/omnikv-engine/tests/durability_evidence.rs (lines 1-7),
debug_compaction.rs (lines 1-5), debug_compaction_200.rs (lines 1-5),
debug_reopen.rs (lines 1-5), operations.rs (lines 1-7), ops_maturity.rs (lines
6-12), and panic_policy_audit.rs (lines 6-14); remove the crate-level
expectations so unrelated occurrences remain linted.
In `@crates/omnikv-engine/tests/storage_tests.rs`:
- Around line 4-11: Remove the file-level clippy expectations and relocate each
lint allowance to the smallest test, function, or module that actually triggers
it. Apply this in crates/omnikv-engine/tests/storage_tests.rs (4-11),
api_error_sanitization_tests.rs (1-6), benchmarks.rs (14-22),
concurrent_stress.rs (6-11), config_tests.rs (1-5), crash_consistency.rs (1-4),
storage_engine.rs (5-10), and storage_perf.rs (4-11); retain only narrowly
scoped expectations required by the corresponding test code.
---
Nitpick comments:
In `@crates/omnikv-engine/src/lib.rs`:
- Around line 7-59: Replace the broad crate-level #[expect] list in the crate
root with item-level expectations for the legacy Clippy findings, prioritizing
security-sensitive cast and unwrap-adjacent lints so new violations remain
visible to CI. Remove those sensitive lints from the crate-level attribute while
preserving expectations only on the specific existing items that require them.
In `@crates/omnikv-engine/tests/raft_cluster.rs`:
- Around line 5-15: Replace the file-level Clippy #[expect] blocks with
item-level attributes on only the specific helpers, tests, or benchmarks that
trigger each lint, preserving their existing reasons and removing blanket
suppression. Apply this scoping change in
crates/omnikv-engine/tests/raft_cluster.rs (lines 5-15),
crates/omnikv-engine/benches/omni_bench.rs (lines 10-19),
crates/omnikv-engine/benches/scan_buffer_pool.rs (lines 11-16),
crates/omnikv-engine/benches/volcano_dispatch.rs (lines 10-16),
crates/omnikv-engine/tests/anomaly_demos.rs (lines 7-13),
crates/omnikv-engine/tests/sql_layer.rs (lines 5-10),
crates/omnikv-engine/tests/sql_v3_features.rs (lines 5-9), and
crates/omnikv-engine/tests/storage_correctness.rs (lines 1-9).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfc6b970-6d77-4f1b-924c-902c08d51d3e
📒 Files selected for processing (39)
Cargo.tomlcrates/omnikv-engine/Cargo.tomlcrates/omnikv-engine/benches/omni_bench.rscrates/omnikv-engine/benches/scan_buffer_pool.rscrates/omnikv-engine/benches/volcano_dispatch.rscrates/omnikv-engine/src/lib.rscrates/omnikv-engine/src/query/optimizer.rscrates/omnikv-engine/src/query/plan_exec.rscrates/omnikv-engine/src/query/sql.rscrates/omnikv-engine/src/query/sql_exec.rscrates/omnikv-engine/src/query/volcano.rscrates/omnikv-engine/src/runtime/dist_txn.rscrates/omnikv-engine/src/storage/core.rscrates/omnikv-engine/tests/anomaly_demos.rscrates/omnikv-engine/tests/api_error_sanitization_tests.rscrates/omnikv-engine/tests/benchmarks.rscrates/omnikv-engine/tests/concurrent_stress.rscrates/omnikv-engine/tests/config_tests.rscrates/omnikv-engine/tests/crash_consistency.rscrates/omnikv-engine/tests/debug_compaction.rscrates/omnikv-engine/tests/debug_compaction_200.rscrates/omnikv-engine/tests/debug_reopen.rscrates/omnikv-engine/tests/durability_evidence.rscrates/omnikv-engine/tests/operations.rscrates/omnikv-engine/tests/ops_maturity.rscrates/omnikv-engine/tests/panic_policy_audit.rscrates/omnikv-engine/tests/raft_cluster.rscrates/omnikv-engine/tests/sql_layer.rscrates/omnikv-engine/tests/sql_v3_features.rscrates/omnikv-engine/tests/storage_correctness.rscrates/omnikv-engine/tests/storage_engine.rscrates/omnikv-engine/tests/storage_perf.rscrates/omnikv-engine/tests/storage_tests.rscrates/omnikv-server/Cargo.tomlcrates/omnikv-server/src/main.rscrates/omnikv-server/src/raft_routes.rsdocs/PANIC_POLICY.mdomni-client/Cargo.tomlomni-client/src/lib.rs
| #![expect( | ||
| clippy::cast_possible_truncation, | ||
| clippy::cast_sign_loss, | ||
| clippy::doc_markdown, | ||
| clippy::uninlined_format_args, | ||
| reason = "Durability evidence tests use generated crash-case data and compact diagnostic output to keep failure scenarios auditable." | ||
| )] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files crates/omnikv-engine/tests
printf '\n== Lint attribute locations ==\n'
rg -n '#!\[expect|#\[expect' crates/omnikv-engine/tests
printf '\n== Relevant file excerpts ==\n'
for f in \
crates/omnikv-engine/tests/durability_evidence.rs \
crates/omnikv-engine/tests/debug_compaction.rs \
crates/omnikv-engine/tests/debug_compaction_200.rs \
crates/omnikv-engine/tests/debug_reopen.rs \
crates/omnikv-engine/tests/operations.rs \
crates/omnikv-engine/tests/ops_maturity.rs \
crates/omnikv-engine/tests/panic_policy_audit.rs
do
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
doneRepository: SBALAVIGNESH123/OmniKV
Length of output: 32228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Rust lint-expect docs from installed toolchain ==\n'
rustc -W help | rg -n 'expect|lint'Repository: SBALAVIGNESH123/OmniKV
Length of output: 461
🌐 Web query:
Rust #[expect] lint attribute multiple occurrences same scope suppresses all occurrences documentation
💡 Result:
In Rust, the #[expect(...)] attribute creates a specific expectation for a lint. Each occurrence of an #[expect] attribute creates its own, independent lint expectation [1][2]. When you use multiple #[expect] attributes for the same lint in the same scope, they do not collectively suppress all instances of that lint; rather, each individual #[expect] attribute requires its own corresponding lint emission to be "fulfilled" [1][2]. Key aspects of this behavior include: 1. Independent Fulfillment: Each #[expect] attribute maintains its own state [1][2]. If you apply #[expect(lint_a)] twice in the same scope, the compiler expects lint_a to be emitted twice [1][2]. If the lint is only emitted once, one of the expectations will remain unfulfilled [1][2]. 2. Unfulfilled Expectations: If an expectation is not met because the expected lint is not triggered, the compiler emits the unfulfilled_lint_expectations diagnostic at the location of the #[expect] attribute [1][3]. 3. Multiple Lints: If a single #[expect] attribute contains multiple lints (e.g., #[expect(lint_a, lint_b)]), it creates separate expectations for each lint [1][2]. Each of these must be fulfilled individually [1][2]. 4. Lint Groups: If you expect a lint group (e.g., #[expect(unused)]), the expectation is fulfilled if at least one lint within that group is emitted [1][2]. In summary, because each #[expect] attribute creates a unique expectation, providing multiple attributes for the same lint essentially requires that lint to be triggered multiple times to satisfy all expectations [1][2]. If the code does not trigger the lint as many times as you have expectations, you will receive warnings for the unfulfilled expectations [1][3].
Citations:
- 1: https://doc.rust-lang.org/reference/attributes/diagnostics.html
- 2: https://doc.rust-lang.org/stable/reference/attributes/diagnostics.html
- 3: https://dev-doc.rust-lang.org/stable/rustc/lints/levels.html
🌐 Web query:
site:doc.rust-lang.org #[expect] lint occurrences scope
💡 Result:
In Rust, the #[expect(lint_name)] attribute functions as a lint level that suppresses a specific lint emission while simultaneously requiring that the lint actually be triggered within the associated scope [1][2][3]. Key details regarding its scope and behavior: 1. Scope: The #[expect] attribute applies to the entity it is attached to (e.g., a function, block, or statement), similar to other lint attributes like #[allow] or #[deny] [2][3]. It creates a lint expectation specifically for that location [3][4]. 2. Fulfillment: The expectation is considered fulfilled if the specified lint would have been emitted at that exact location (i.e., if it would have been triggered under a #[warn(lint_name)] setting) [3][4]. 3. Unfulfilled Expectations: If the expected lint is not triggered within the scope where the attribute is applied, the compiler emits the unfulfilled_lint_expectations lint [2][3][5]. This serves as a safeguard to ensure that you are not suppressing lints that are no longer relevant to your code [2]. 4. Interaction with Other Levels: If the lint level is modified within the same scope using other attributes like #[allow] or #[warn], the lint emission is handled according to those attributes, and the expectation typically remains unfulfilled [3][4]. 5. Independence: If an #[expect] attribute contains multiple lints, each lint is evaluated independently [3][4]. For lint groups, the expectation is fulfilled if at least one lint within that group is triggered [3][4]. Essentially, #[expect] is a safer alternative to #[allow] for scenarios where you want to suppress a warning but want to be alerted if the underlying issue being suppressed is no longer present [2][6].
Citations:
- 1: https://doc.rust-lang.org/reference/attributes/diagnostics.html
- 2: https://doc.rust-lang.org/nightly/rustc/lints/levels.html
- 3: https://doc.rust-lang.org/stable/reference/attributes/diagnostics.html
- 4: https://doc.rust-lang.org/nightly/reference/attributes/diagnostics.html
- 5: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/builtin/static.UNFULFILLED_LINT_EXPECTATIONS.html
- 6: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/enum.Level.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp --suffix=.rs)"
cat >"$tmp" <<'RS'
#![deny(unused_variables)]
#![expect(unused_variables)]
fn main() {
let a = 1;
let b = 2;
}
RS
rustc "$tmp" -Z unstable-options --pretty=expanded 2>/dev/null || true
rustc "$tmp" 2>&1 || true
rm -f "$tmp"Repository: SBALAVIGNESH123/OmniKV
Length of output: 407
Scope these #![expect] attributes to the specific tests. File-level expectations still suppress the listed lints anywhere else in the integration-test crate, so a new occurrence in the same file can slip past the gate as long as one existing occurrence remains. Move each expectation onto the affected function or block in crates/omnikv-engine/tests/durability_evidence.rs, debug_compaction.rs, debug_compaction_200.rs, debug_reopen.rs, operations.rs, ops_maturity.rs, and panic_policy_audit.rs.
📍 Affects 7 files
crates/omnikv-engine/tests/durability_evidence.rs#L1-L7(this comment)crates/omnikv-engine/tests/debug_compaction.rs#L1-L5crates/omnikv-engine/tests/debug_compaction_200.rs#L1-L5crates/omnikv-engine/tests/debug_reopen.rs#L1-L5crates/omnikv-engine/tests/operations.rs#L1-L7crates/omnikv-engine/tests/ops_maturity.rs#L6-L12crates/omnikv-engine/tests/panic_policy_audit.rs#L6-L14
🤖 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/omnikv-engine/tests/durability_evidence.rs` around lines 1 - 7, Move
each file-level #![expect] lint declaration onto the specific test function or
local block that triggers it, preserving the existing lint list and reason.
Apply this in crates/omnikv-engine/tests/durability_evidence.rs (lines 1-7),
debug_compaction.rs (lines 1-5), debug_compaction_200.rs (lines 1-5),
debug_reopen.rs (lines 1-5), operations.rs (lines 1-7), ops_maturity.rs (lines
6-12), and panic_policy_audit.rs (lines 6-14); remove the crate-level
expectations so unrelated occurrences remain linted.
| #![expect( | ||
| clippy::doc_markdown, | ||
| clippy::needless_collect, | ||
| clippy::redundant_clone, | ||
| clippy::uninlined_format_args, | ||
| reason = "Large storage integration suite favors explicit scenario setup and readable failure output; strict style cleanup is tracked separately." | ||
| )] | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the affected test files and any local policy references.
for f in \
crates/omnikv-engine/tests/storage_tests.rs \
crates/omnikv-engine/tests/api_error_sanitization_tests.rs \
crates/omnikv-engine/tests/benchmarks.rs \
crates/omnikv-engine/tests/concurrent_stress.rs \
crates/omnikv-engine/tests/config_tests.rs \
crates/omnikv-engine/tests/crash_consistency.rs \
crates/omnikv-engine/tests/storage_engine.rs \
crates/omnikv-engine/tests/storage_perf.rs
do
echo "==== $f ===="
wc -l "$f"
sed -n '1,40p' "$f"
echo
done
echo "==== Search for narrow-exception policy / expect usage ===="
rg -n "narrow-exception|#!\\[expect\\(|#\\[expect\\(" crates/omnikv-engine -SRepository: SBALAVIGNESH123/OmniKV
Length of output: 14405
🌐 Web query:
Rust #[expect] lint semantics multiple occurrences scope suppress all or one official docs
💡 Result:
The #[expect(lint)] attribute in Rust is designed to suppress a specific lint while simultaneously ensuring that the lint is actually triggered in the affected code [1][2]. If the expected lint is not emitted, the compiler issues an unfulfilled_lint_expectations warning [1][3]. Regarding your specific questions on semantics, scope, and multiple occurrences: 1. Suppression Mechanism: The #[expect] attribute suppresses the specified lint only if it occurs within the scope it decorates [4][2]. It behaves similarly to #[allow], but with the additional requirement that the lint must be generated [4][2]. 2. Multiple Occurrences and Scope: When multiple #[expect] attributes (or other lint-level attributes) are used, the most specific (inner) attribute takes precedence [1][3]. - An #[expect] attribute only considers lint emissions that are not already suppressed by other attributes (like #[allow] or #[warn]) in the same or more specific scopes [2]. - If a lint is suppressed by an inner #[allow], that lint emission is consumed by the #[allow] and cannot fulfill an outer #[expect] [2]. Similarly, if an inner #[expect] suppresses the lint, the outer #[expect] remains unfulfilled [2]. 3. Official Documentation: Detailed information can be found in the Rust Reference under Diagnostic Attributes and the rustc book under Lint Levels [1][3][2]. The feature was stabilized via RFC 2383 [4][5]. In summary, #[expect] does not "suppress all" lints globally; it strictly monitors for the presence of the specific lint it expects within its localized scope, and its fulfillment is sensitive to any shadowing or suppression attributes applied in nested or overlapping scopes [2].
Citations:
- 1: https://dev-doc.rust-lang.org/stable/rustc/lints/levels.html
- 2: https://doc.rust-lang.org/stable/reference/attributes/diagnostics.html
- 3: https://doc.rust-lang.org/nightly/rustc/lints/levels.html
- 4: https://github.com/rust-lang/rfcs/blob/master/text/2383-lint-reasons.md
- 5: Let's
#[expect]some lints: Stabilizelint_reasons(RFC 2383) rust-lang/rust#120924
Narrow these #![expect] scopes
These file-level expectations apply to the whole test target, so any matching lint anywhere in the file can satisfy them and later violations stay hidden. Move each one down to the smallest test, function, or module that actually needs it.
📍 Affects 8 files
crates/omnikv-engine/tests/storage_tests.rs#L4-L11(this comment)crates/omnikv-engine/tests/api_error_sanitization_tests.rs#L1-L6crates/omnikv-engine/tests/benchmarks.rs#L14-L22crates/omnikv-engine/tests/concurrent_stress.rs#L6-L11crates/omnikv-engine/tests/config_tests.rs#L1-L5crates/omnikv-engine/tests/crash_consistency.rs#L1-L4crates/omnikv-engine/tests/storage_engine.rs#L5-L10crates/omnikv-engine/tests/storage_perf.rs#L4-L11
🤖 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/omnikv-engine/tests/storage_tests.rs` around lines 4 - 11, Remove the
file-level clippy expectations and relocate each lint allowance to the smallest
test, function, or module that actually triggers it. Apply this in
crates/omnikv-engine/tests/storage_tests.rs (4-11),
api_error_sanitization_tests.rs (1-6), benchmarks.rs (14-22),
concurrent_stress.rs (6-11), config_tests.rs (1-5), crash_consistency.rs (1-4),
storage_engine.rs (5-10), and storage_perf.rs (4-11); retain only narrowly
scoped expectations required by the corresponding test code.
1 participant
Summary
Closes #64.
This PR turns OmniKV's clippy policy into an explicit production-readiness gate instead of relying on default lint coverage.
What changed
clippy::all,clippy::pedantic, andclippy::nurserywarning groups.#[allow(...)]suppressions with documented#[expect(..., reason = "...")]exceptions.docs/PANIC_POLICY.mdso future suppressions need a reason and a narrow scope.Why
OmniKV is being hardened toward database-grade engineering standards. This makes lint debt visible, prevents silent broad suppressions, and gives reviewers a clear reason for every remaining exception.
Validation
git diff --checkcargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo build --workspace --all-targets -j 1cargo test -p omnikv-engine --test panic_policy_audit -- --test-threads=1cargo test -p omnikv-engine --test config_tests -- --test-threads=1cargo test -p omnikv-engine --test storage_tests -- --test-threads=1rg -n "#!?\\[allow\\(" crates omni-client -g "*.rs"returned no matches.Summary by CodeRabbit
Chores
Documentation
Tests