enable clippy nursery mode#507
Merged
Merged
Conversation
github-actions
Bot
added
rust
📝 WalkthroughWalkthroughThis PR modernizes the codebase by enabling stricter Clippy linting, tightening async generic bounds across command-execution and client interfaces, expanding public APIs for command handling, and refactoring control flow to use contemporary Rust patterns like ChangesLint Hardening & Code Quality Modernization
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (3)
crates/core/src/commands/pty/mod.rs (1)
47-47: Reconsiderpubvisibility onInteractiveChildChip.
rgfinds no references toInteractiveChildChipoutsidecrates/core/**, so no external crate currently uses it. Unless this is intended as part of the public API, consider keeping itpub(crate).🤖 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/core/src/commands/pty/mod.rs` at line 47, InteractiveChildChip is declared pub but appears unused outside the crate; change its visibility to pub(crate) if it is not intended as part of the public API by updating the struct declaration for InteractiveChildChip to use pub(crate) instead of pub, and run a build/test to ensure no external code requires the public visibility.crates/cli/src/sigint.rs (1)
12-12: Reducehandle_signalsvisibility incrates/cli/src/sigint.rs
handle_signalsis only used fromcrates/cli/src/main.rsand has no usages outsidecrates/cli, sopubdoesn’t need to expose any external API (thecrates/clicrate is bin-only—nolib.rs).- Keep it accessible to
main.rs(it’s imported viamod sigint; use crate::sigint::handle_signals;), but preferpub(super)orpub(crate)instead ofpub.🤖 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/cli/src/sigint.rs` at line 12, The function handle_signals is unnecessarily public; change its visibility from pub to a restricted visibility so it remains accessible to main.rs but not exported outside the binary crate—replace the signature pub async fn handle_signals(...) with a narrower visibility like pub(crate) async fn handle_signals(...) (or pub(super) if you prefer module-level restriction) so the existing import pattern (mod sigint; use crate::sigint::handle_signals;) still works but the symbol is not exposed externally.crates/core/src/commands/noninteractive.rs (1)
25-25: MakeNonInteractiveChildChippub(crate)
Search forNonInteractiveChildChipoutsidecrates/corereturns no matches, so it isn’t referenced by other crates; it can likely be crate-private unless you intentionally want it to be part of the public API.🤖 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/core/src/commands/noninteractive.rs` at line 25, The struct NonInteractiveChildChip is exported publicly but not used outside the crate; change its visibility to crate-private by replacing its declaration modifier (pub) with pub(crate) for NonInteractiveChildChip so it remains accessible within the crate but not part of the public API; ensure any internal uses continue to compile and run tests to verify no external references break.
🤖 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/nix_client/src/lib.rs`:
- Around line 146-149: The impls currently force T and reader types to be Send
at the impl level (see impl<NixClient<UnixStream, UnixStream, T>> and impl<R, W,
T> NixClient) which unnecessarily tightens the API; instead, remove the + Send
bound from the impl-level where clauses (delete + Send from T and from R in the
generic impl) and only require Send on the individual async methods that
actually need multithreaded futures (the async read/write methods referenced
around lines ~174-178). Concretely: drop the Send requirement from the impl
declarations (NixClient<UnixStream,UnixStream,T> and impl<R,W,T>) and add + Send
to the specific method-level where clauses (the async reader/writer functions)
so callers with non-Send callbacks/readers can still use the sync parts while
preserving Send for methods that will be spawned.
---
Nitpick comments:
In `@crates/cli/src/sigint.rs`:
- Line 12: The function handle_signals is unnecessarily public; change its
visibility from pub to a restricted visibility so it remains accessible to
main.rs but not exported outside the binary crate—replace the signature pub
async fn handle_signals(...) with a narrower visibility like pub(crate) async fn
handle_signals(...) (or pub(super) if you prefer module-level restriction) so
the existing import pattern (mod sigint; use crate::sigint::handle_signals;)
still works but the symbol is not exposed externally.
In `@crates/core/src/commands/noninteractive.rs`:
- Line 25: The struct NonInteractiveChildChip is exported publicly but not used
outside the crate; change its visibility to crate-private by replacing its
declaration modifier (pub) with pub(crate) for NonInteractiveChildChip so it
remains accessible within the crate but not part of the public API; ensure any
internal uses continue to compile and run tests to verify no external references
break.
In `@crates/core/src/commands/pty/mod.rs`:
- Line 47: InteractiveChildChip is declared pub but appears unused outside the
crate; change its visibility to pub(crate) if it is not intended as part of the
public API by updating the struct declaration for InteractiveChildChip to use
pub(crate) instead of pub, and run a build/test to ensure no external code
requires the public visibility.
🪄 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: 9197e76e-1dac-445b-aeae-eaaee51d1361
📒 Files selected for processing (25)
Cargo.tomlcrates/cli/src/apply.rscrates/cli/src/cli.rscrates/cli/src/main.rscrates/cli/src/sigint.rscrates/cli/src/tracing_setup.rscrates/core/src/cache/mod.rscrates/core/src/commands/builder.rscrates/core/src/commands/common.rscrates/core/src/commands/mod.rscrates/core/src/commands/noninteractive.rscrates/core/src/commands/pty/logbuffer.rscrates/core/src/commands/pty/mod.rscrates/core/src/commands/pty/output.rscrates/core/src/errors.rscrates/core/src/hive/mod.rscrates/core/src/hive/node.rscrates/core/src/hive/plan.rscrates/core/src/hive/steps/activate.rscrates/core/src/hive/steps/evaluate.rscrates/core/src/hive/steps/keys.rscrates/core/src/lib.rscrates/core/src/status.rscrates/nix_client/src/lib.rscrates/nix_client/src/store_path.rs
Comment on lines
146
to
149
| impl<T> NixClient<UnixStream, UnixStream, T> | ||
| where | ||
| T: Fn(LogMessage, &Arc<Mutex<HashMap<u64, Arc<String>>>>, bool) -> Option<String>, | ||
| T: Fn(LogMessage, &Arc<Mutex<HashMap<u64, Arc<String>>>>, bool) -> Option<String> + Send, | ||
| { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find NixClient usage patterns and callback definitions
echo "=== NixClient::open_local calls ==="
rg -nP '\bNixClient::open_local\s*\(' --type=rust -C3
echo -e "\n=== NixClient::handshake calls ==="
rg -nP '\bNixClient::handshake\s*\(' --type=rust -C3
echo -e "\n=== Callback function definitions ==="
rg -nP 'trace_nix_log_message|trace_callback' --type=rust -C2Repository: forallsys/wire
Length of output: 10631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant sections around the reported lines.
echo "=== crates/nix_client/src/lib.rs: lines 120-220 ==="
nl -ba crates/nix_client/src/lib.rs | sed -n '110,230p'
echo "=== crates/nix_client/src/lib.rs: lines 230-320 ==="
nl -ba crates/nix_client/src/lib.rs | sed -n '230,330p'
# Search for all Send bounds in nix_client lib.
echo "=== Send bounds in crates/nix_client/src/lib.rs ==="
rg -n "Send\b" crates/nix_client/src/lib.rs
# Identify the NixClient struct and its generic parameters.
echo "=== NixClient struct/impl generics ==="
rg -n "struct NixClient|impl<|type NixClient" crates/nix_client/src/lib.rsRepository: forallsys/wire
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== crates/nix_client/src/lib.rs: lines 120-220 ==="
nl -ba crates/nix_client/src/lib.rs | sed -n '120,220p'
echo "=== crates/nix_client/src/lib.rs: lines 220-320 ==="
nl -ba crates/nix_client/src/lib.rs | sed -n '220,330p'
echo "=== Send bounds in crates/nix_client/src/lib.rs ==="
rg -n "Send\b" crates/nix_client/src/lib.rs
echo "=== NixClient struct and key impl blocks ==="
rg -n "struct NixClient|impl<" crates/nix_client/src/lib.rsRepository: forallsys/wire
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== crates/nix_client/src/lib.rs: lines 120-220 ==="
nl -ba crates/nix_client/src/lib.rs | sed -n '120,220p'
echo "=== crates/nix_client/src/lib.rs: lines 220-330 ==="
nl -ba crates/nix_client/src/lib.rs | sed -n '220,330p'
echo "=== Send bounds in crates/nix_client/src/lib.rs ==="
rg -n "Send\b" crates/nix_client/src/lib.rs
echo "=== NixClient struct and key impl blocks ==="
rg -n "struct NixClient|impl<" crates/nix_client/src/lib.rsRepository: forallsys/wire
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/nix_client/src/lib.rs"
echo "=== Print crates/nix_client/src/lib.rs with line numbers (120-220) ==="
awk 'NR>=120 && NR<=220 {printf "%6d\t%s\n", NR, $0}' "$FILE"
echo "=== Print crates/nix_client/src/lib.rs with line numbers (220-330) ==="
awk 'NR>=220 && NR<=330 {printf "%6d\t%s\n", NR, $0}' "$FILE"
echo "=== Search for Send bounds in crates/nix_client/src/lib.rs ==="
rg -n "Send\b" "$FILE"
echo "=== Search for reader generic bounds containing Send (best-effort) ==="
rg -n "R\s*:\s*[^,\n]*Send|reader[^,\n]*Send" "$FILE" || true
echo "=== Search for where-clauses containing Send (best-effort) ==="
rg -n "where[^{\n]*Send|where[^{]*\+ Send|where[^\n]*Send" "$FILE" || trueRepository: forallsys/wire
Length of output: 11662
Confirm Send bound tightening affects both callback (T) and reader (R) generics
crates/nix_client/src/lib.rs now requires the callback to be Send (T: ... + Send) in both the UnixStream-specific impl and the generic impl, and it also requires the reader to be Send (impl<R, W, T> ... where ... R: Send). The async methods further add R: ... + Send / W: ... + Send bounds, which tightens the contract for any users providing non-Send readers/callbacks and affects whether the resulting async futures can be used with multithreaded executors (e.g., when spawned). [lines 146-149, 174-178]
🤖 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/nix_client/src/lib.rs` around lines 146 - 149, The impls currently
force T and reader types to be Send at the impl level (see
impl<NixClient<UnixStream, UnixStream, T>> and impl<R, W, T> NixClient) which
unnecessarily tightens the API; instead, remove the + Send bound from the
impl-level where clauses (delete + Send from T and from R in the generic impl)
and only require Send on the individual async methods that actually need
multithreaded futures (the async read/write methods referenced around lines
~174-178). Concretely: drop the Send requirement from the impl declarations
(NixClient<UnixStream,UnixStream,T> and impl<R,W,T>) and add + Send to the
specific method-level where clauses (the async reader/writer functions) so
callers with non-Send callbacks/readers can still use the sync parts while
preserving Send for methods that will be spawned.
mrshmllow
force-pushed
the
marshmallow/p-uqouktvuoktx
branch
from
2aabd14 to
ceb9acb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Refactor
Chores