chore(pacquet/lint): more clippy#11839
Conversation
Wire `clone_on_ref_ptr`, `if_then_some_else_none`, and
`unnecessary_lazy_evaluations` into `[workspace.lints.clippy]` so they
fire under the existing `just lint` (`--deny warnings`) gate. The first
of these makes `Arc::clone(&x)` / `Rc::clone(&x)` mandatory at the call
site, which is what `perfectionist::arc_rc_clone` was already enforcing
via Dylint — consolidate on clippy and disable the perfectionist rule in
`dylint.toml` so the requirement lives in one place and shows up under
any `cargo clippy` run, not just the Dylint job. Update the
`Cloning Arc and Rc` section of `CODE_STYLE_GUIDE.md` to point at the
clippy lint.
Turning on `if_then_some_else_none` surfaced seven pre-existing
`if x { Some(...) } else { None }` patterns across the workspace.
Rewrite each to `x.then(|| ...)` (or, for the registry deprecation
visitor, `value.then(String::new)` to also satisfy
`unnecessary_lazy_evaluations`). The rewrites are pure desugarings;
behavior is unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThis PR introduces workspace-level Clippy lints to enforce optional chaining patterns and Arc/Rc cloning idioms, then refactors eight implementation sites across the codebase to use ChangesClippy Lint Configuration and Codebase Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
Align the three lint keys to the longest key (`unnecessary_lazy_evaluations`), which is what `taplo format` produces under the workspace `.taplo.toml`. Caught by the `Format` CI job on the previous commit.
Per review on #11839: the `[workspace.lints.clippy]` comment in `Cargo.toml` and the "Cloning `Arc` and `Rc`" section in the style guide don't need to mention the dylint-side disable — that detail lives in `dylint.toml`, where it's locally relevant.
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11839 +/- ##
=======================================
Coverage 87.55% 87.55%
=======================================
Files 204 204
Lines 24394 24389 -5
=======================================
- Hits 21358 21355 -3
+ Misses 3036 3034 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR tightens pacquet’s Rust linting by enabling additional Clippy restriction lints at the workspace level, and then updating existing code patterns across multiple crates to comply with the newly enabled lints while keeping behavior unchanged. It also consolidates the “Arc/Rc clone” enforcement onto Clippy by disabling the equivalent Perfectionist/Dylint rule and updating the style guide accordingly.
Changes:
- Enabled
clone_on_ref_ptr,if_then_some_else_none, andunnecessary_lazy_evaluationsvia[workspace.lints.clippy]in the rootCargo.toml. - Rewrote several
if cond { Some(...) } else { None }patterns tocond.then(...)across affected crates. - Disabled
perfectionist::arc_rc_cloneindylint.tomland updatedCODE_STYLE_GUIDE.mdto referenceclippy::clone_on_ref_ptr.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pacquet/crates/resolving-npm-resolver/src/resolve_from_workspace.rs | Replaces an if ... Some/None branch with .then(...) for Clippy compliance. |
| pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs | Uses .then(...) for conditional alias propagation in peer resolution logic. |
| pacquet/crates/registry/src/package_version.rs | Uses bool::then to encode deprecated boolean → Option<String> conversion. |
| pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs | Gates lockfile construction with `.then( |
| pacquet/crates/package-manager/src/create_virtual_store.rs | Refactors hoisted-mode optional CAS index assembly to `.then( |
| pacquet/crates/executor/src/lifecycle.rs | Refactors conditional fallback install script selection to `.then( |
| pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs | Uses `.then( |
| pacquet/crates/cmd-shim/src/link_bins.rs | Uses `cfg!(windows).then( |
| pacquet/CODE_STYLE_GUIDE.md | Updates guidance to reference the Clippy lint for Arc/Rc cloning. |
| dylint.toml | Disables Perfectionist’s arc_rc_clone since Clippy now enforces it. |
| Cargo.toml | Adds workspace-wide Clippy lint configuration for the newly enabled lints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.46115845218,
"stddev": 0.09555106799677077,
"median": 2.47295701858,
"user": 2.7105048199999997,
"system": 3.7475204600000005,
"min": 2.3229786930799996,
"max": 2.61937797908,
"times": [
2.47971264008,
2.4374201520799996,
2.61937797908,
2.56070228708,
2.53211611508,
2.4676834340799996,
2.3603065320799996,
2.3530560860799996,
2.4782306030799996,
2.3229786930799996
]
},
{
"command": "pacquet@main",
"mean": 2.4337455323799992,
"stddev": 0.07650717361742444,
"median": 2.4363993320799997,
"user": 2.7823518199999997,
"system": 3.683980160000001,
"min": 2.3139261300799996,
"max": 2.5202091480799997,
"times": [
2.3641351330799996,
2.4385420490799996,
2.36864299308,
2.51378347108,
2.3139261300799996,
2.5202091480799997,
2.5097085840799997,
2.5061942610799997,
2.43425661508,
2.3680569390799997
]
},
{
"command": "pnpm",
"mean": 4.82857822758,
"stddev": 0.053746075677015386,
"median": 4.83869240358,
"user": 8.181519219999998,
"system": 4.148016159999999,
"min": 4.72913486008,
"max": 4.89692366208,
"times": [
4.88135070008,
4.84386708208,
4.83351772508,
4.84898585708,
4.88133413708,
4.72913486008,
4.77520458308,
4.81261805908,
4.89692366208,
4.78284561008
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.69454086166,
"stddev": 0.02574221285905309,
"median": 0.68899090776,
"user": 0.4059614,
"system": 1.55201028,
"min": 0.67571437576,
"max": 0.76566319676,
"times": [
0.76566319676,
0.6904387777600001,
0.6816029037600001,
0.6824062107600001,
0.69188370876,
0.6948419107600001,
0.67571437576,
0.6935212927600001,
0.6875430377600001,
0.6817932017600001
]
},
{
"command": "pacquet@main",
"mean": 0.7183406753600001,
"stddev": 0.05167648093835655,
"median": 0.6940961487600001,
"user": 0.3912659,
"system": 1.56262048,
"min": 0.68543115476,
"max": 0.8540975907600001,
"times": [
0.7260963147600001,
0.6946236537600001,
0.69356864376,
0.7488235707600001,
0.7073911637600001,
0.6926858927600001,
0.68543115476,
0.8540975907600001,
0.6881083507600001,
0.6925804177600001
]
},
{
"command": "pnpm",
"mean": 2.6283746474600003,
"stddev": 0.18100141326716326,
"median": 2.54607030626,
"user": 3.2167562999999992,
"system": 2.1746878799999996,
"min": 2.4382442007600003,
"max": 2.95632711176,
"times": [
2.7999464887600003,
2.53258608276,
2.55284624976,
2.8836934637600002,
2.4941708837600003,
2.4382442007600003,
2.5681349357600003,
2.51850269476,
2.95632711176,
2.53929436276
]
}
]
} |
Resolved two conflicts against upstream changes in `perf(pacquet): close the warm-cache resolve gap to pnpm CLI` (#11837) and `fix(installing.deps-resolver): deterministically order cyclic peer suffixes` (#11826): - `package-manager/src/install_with_fresh_lockfile.rs`: main dropped the `enable_global_virtual_store` conditional and now builds the lockfile unconditionally (renamed `layout_lockfile` → `built_lockfile`, added a tracing span). The clippy `if_then_some_else_none` rewrite this branch added to that block is no longer applicable — took main's version. - `resolving-deps-resolver/src/resolve_peers.rs` (two sites): main changed `NodeId` from a `Copy` newtype to an `enum { Counter(u64), Leaf(Arc<str>) }`, so `Some(node_id)` had to become `Some(node_id.clone())`. Combined with this branch's `if_then_some_else_none` rewrite of the sibling `alias` field. The new `node_id.clone()` does not trip `clippy::clone_on_ref_ptr` because `NodeId` is an enum, not an `Arc`/`Rc`/`Weak`. `engine-runtime-node-resolver/src/node_resolver.rs` and `resolving-npm-resolver/src/resolve_from_workspace.rs` auto-merged; the clippy fixes on those lines survived intact. Verified locally: `cargo clippy --locked --workspace --all-targets -- --deny warnings`, `cargo fmt --check`, `taplo format --check`.
Enable three clippy restriction lints workspace-wide via
[workspace.lints.clippy]in the rootCargo.toml:clone_on_ref_ptr— makesArc::clone(&x)/Rc::clone(&x)mandatory at the call site. Same requirementperfectionist::arc_rc_clonewas already enforcing via Dylint, so disable the perfectionist rule indylint.tomland consolidate on clippy. The requirement now shows up under anycargo clippyrun, not just the Dylint job. The "CloningArcandRc" section ofCODE_STYLE_GUIDE.mdis updated to point at the clippy lint.if_then_some_else_none— preferx.then(|| ...)overif x { Some(...) } else { None }.unnecessary_lazy_evaluations— pair with the above; use.then_some(...)/.then(fn_ref)when the value is trivial.Turning on
if_then_some_else_nonesurfaced seven pre-existing patterns across the workspace (cmd-shim,engine-runtime-node-resolver,executor,package-manager×2,registry,resolving-deps-resolver×2,resolving-npm-resolver). Each is rewritten to the lint-compliant form; all rewrites are pure desugarings, behavior unchanged.Verified locally:
cargo clippy --locked --workspace --all-targets -- --deny warnings,cargo fmt --check,taplo format --check, andcargo check --locked --workspace --all-targetsall pass.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Release Notes