lint: enable unused_qualifications workspace-wide#783
Conversation
- Enable `unused_qualifications = "warn"` in `[workspace.lints.rust]` in the root `Cargo.toml`. - Run `cargo clippy --fix` to resolve redundant fully-qualified paths and other stylistic warnings across the workspace. - Affected crates: logfwd, logfwd-arrow, logfwd-bench, logfwd-competitive-bench, logfwd-config, logfwd-core, logfwd-io, logfwd-output, logfwd-transform. Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
WalkthroughThis PR enables the Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 passed)
Comment |
- Enable `unused_qualifications = "warn"` in `[workspace.lints.rust]` in the root `Cargo.toml`. - Run `cargo clippy --fix` to resolve redundant fully-qualified paths and other stylistic warnings across the workspace. - Run `cargo fmt --all` to ensure the workspace-wide changes adhere to the project's formatting standards. - Affected crates: logfwd, logfwd-arrow, logfwd-bench, logfwd-competitive-bench, logfwd-config, logfwd-core, logfwd-io, logfwd-output, logfwd-transform. Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd-core/tests/scanner_conformance.rs`:
- Line 87: The loop changed to "for (key_str, val) in obj" which fails with
sonic-rs 0.5.0 because Object does not implement IntoIterator for &Object;
revert to iterating via the provided method by using obj.iter() in the loop
header (i.e., change the loop in scanner_conformance.rs that currently uses "for
(key_str, val) in obj" back to using "for (key_str, val) in obj.iter()" so it
compiles with sonic-rs 0.5.0).
In `@crates/logfwd-io/src/generator.rs`:
- Around line 437-440: The test silently ignores non-Data InputEvent variants;
replace the `if let InputEvent::Data { bytes } = event { ... }` with explicit
handling so unexpected variants fail clearly — either use a `match event {
InputEvent::Data { bytes } => { /* existing logic updating total_lines */ },
other => panic!("expected Data event, got {:?}", other) }` or add a short
comment asserting the GeneratorInput invariant; reference the `InputEvent::Data`
pattern, the `event` variable, `GeneratorInput`, and the `total_lines` update
when locating the change.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a88d9cce-254a-4ae3-91df-746cfc716938
📒 Files selected for processing (40)
Cargo.tomlcrates/logfwd-arrow/src/streaming_builder.rscrates/logfwd-arrow/tests/allocation_regression.rscrates/logfwd-bench/benches/elasticsearch_arrow.rscrates/logfwd-bench/benches/pipeline.rscrates/logfwd-competitive-bench/src/main.rscrates/logfwd-competitive-bench/src/rate_bench.rscrates/logfwd-competitive-bench/src/runner.rscrates/logfwd-config/src/lib.rscrates/logfwd-core/benches/scanner.rscrates/logfwd-core/examples/arrow_ipc_roundtrip.rscrates/logfwd-core/src/json_scanner.rscrates/logfwd-core/src/pipeline/lifecycle.rscrates/logfwd-core/tests/compliance_data.rscrates/logfwd-core/tests/scanner_conformance.rscrates/logfwd-io/src/diagnostics.rscrates/logfwd-io/src/enrichment.rscrates/logfwd-io/src/generator.rscrates/logfwd-io/src/metric_history.rscrates/logfwd-io/src/otlp_receiver.rscrates/logfwd-io/src/span_exporter.rscrates/logfwd-io/src/tail.rscrates/logfwd-io/src/tcp_input.rscrates/logfwd-io/tests/transport_e2e.rscrates/logfwd-output/src/elasticsearch.rscrates/logfwd-output/src/lib.rscrates/logfwd-output/src/loki.rscrates/logfwd-output/src/null.rscrates/logfwd-output/src/otlp_sink.rscrates/logfwd-output/tests/elasticsearch_arrow_ipc.rscrates/logfwd-transform/src/lib.rscrates/logfwd-transform/src/udf/grok.rscrates/logfwd/src/main.rscrates/logfwd/src/pipeline.rscrates/logfwd/src/worker_pool.rscrates/logfwd/tests/allocation_e2e.rscrates/logfwd/tests/allocation_scaling.rscrates/logfwd/tests/compliance.rscrates/logfwd/tests/compliance_file.rscrates/logfwd/tests/integration.rs
| // Check each field from the sonic-rs parse | ||
| let mut seen_keys = std::collections::HashSet::new(); | ||
| for (key_str, val) in obj.iter() { | ||
| for (key_str, val) in obj { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify sonic-rs Object supports direct iteration with same semantics as .iter()
rg -A 10 'impl.*IntoIterator.*for.*Object' --type rustRepository: strawgate/memagent
Length of output: 44
🌐 Web query:
Does sonic-rs 0.5.0 Object type implement IntoIterator for &Object, and does iterating directly produce the same iterator as calling .iter()?
💡 Result:
No, the sonic-rs 0.5.0 Object type does not implement IntoIterator for &Object. There is a value::object::Iter type listed in the crate documentation, suggesting iteration is provided via an .iter method (similar to standard Rust collections), but no explicit evidence of IntoIterator impl for &Object. Direct iteration (for obj in &object {...}) would not work without that impl, and thus would not produce the same iterator as .iter. The presence of value::object::Iter and value::array::IntoIter (noted separately for arrays) supports that Object follows the pattern of providing .iter returning Iter, without blanket IntoIterator for &T.
Citations:
Revert iteration style: sonic-rs 0.5.0 Object does not implement IntoIterator for &Object.
The change from obj.iter() to direct iteration (for (key_str, val) in obj) is incompatible with sonic-rs 0.5.0. The Object type provides iteration only via the .iter() method, not through IntoIterator for borrowed references. This change will fail to compile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-core/tests/scanner_conformance.rs` at line 87, The loop changed
to "for (key_str, val) in obj" which fails with sonic-rs 0.5.0 because Object
does not implement IntoIterator for &Object; revert to iterating via the
provided method by using obj.iter() in the loop header (i.e., change the loop in
scanner_conformance.rs that currently uses "for (key_str, val) in obj" back to
using "for (key_str, val) in obj.iter()" so it compiles with sonic-rs 0.5.0).
| if let InputEvent::Data { bytes } = event { | ||
| let text = String::from_utf8_lossy(bytes); | ||
| total_lines += text.trim().lines().count() as u64; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider explicit handling of unexpected event types for test robustness.
The if let pattern silently ignores Rotated and Truncated events. While GeneratorInput only emits Data events by design, other tests in this file use explicit else { panic!("expected Data event") } branches (lines 212, 249) for clarity and early failure. This test would still catch issues (via the assertion at line 444) but with a less informative error message.
🛡️ Proposed fix for explicit event type checking
if let InputEvent::Data { bytes } = event {
let text = String::from_utf8_lossy(bytes);
total_lines += text.trim().lines().count() as u64;
+ } else {
+ panic!("GeneratorInput must only emit Data events, got {event:?}");
}Alternatively, add a comment explaining the invariant:
+ // GeneratorInput only emits Data events by design (see poll() line 186)
if let InputEvent::Data { bytes } = event {
let text = String::from_utf8_lossy(bytes);
total_lines += text.trim().lines().count() as u64;
}Based on learnings: The relevant code snippet notes that although GeneratorInput never emits Rotated/Truncated variants, the if let pattern is fragile to future changes in InputEvent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd-io/src/generator.rs` around lines 437 - 440, The test silently
ignores non-Data InputEvent variants; replace the `if let InputEvent::Data {
bytes } = event { ... }` with explicit handling so unexpected variants fail
clearly — either use a `match event { InputEvent::Data { bytes } => { /*
existing logic updating total_lines */ }, other => panic!("expected Data event,
got {:?}", other) }` or add a short comment asserting the GeneratorInput
invariant; reference the `InputEvent::Data` pattern, the `event` variable,
`GeneratorInput`, and the `total_lines` update when locating the change.
|
Reviewed: mechanical clippy --fix for unused_qualifications. All changes are removing redundant qualifiers, raw string literals, and static ref borrows. CI fully green. Ready to merge. |
Acknowledged. Thank you for the review. |
This PR enables the
unused_qualificationslint at the workspace level and applies automated fixes usingcargo clippy --fix. This reduces code noise by removing redundant fully-qualified paths (e.g.,std::io::ErrorwhenErroris already in scope).Steps taken:
RUSTC_WRAPPER="" cargo clippy --fix --allow-dirty --all-targets -- -W unused_qualificationsto apply fixes.RUSTC_WRAPPER="" cargo clippy --all-targets -- -D warnings.unused_qualifications = "warn"to[workspace.lints.rust]inCargo.toml.cargo test --lib --binsto ensure no regressions were introduced.Fixes #762
PR created automatically by Jules for task 8720157761045900515 started by @strawgate