Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 14 additions & 144 deletions CODE_STYLE_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,104 +112,24 @@ pub use comver::*;
pub use load_lockfile::*;
```

### Generic Parameter Naming
### Single-letter identifiers

Use **descriptive names** for type parameters, not single letters:
Prefer descriptive names for type parameters, function and method parameters, closure parameters, and `let` bindings. The four perfectionist rules below enforce the policy and document their built-in allowlists and exceptions; consult the rule docs for the exact set of identifiers and call shapes that bypass the lint.

- `Size`, `Name`, `Manifest`, `Store`, `Reporter`
- Type parameters — [`perfectionist::single_letter_generic`](https://github.com/KSXGitHub/perfectionist). Use `Size`, `Name`, `Manifest`, `Store`, `Reporter`, etc. Single-letter generics remain acceptable in short, self-contained trait impls.
- Function and method parameters — [`perfectionist::single_letter_function_param`](https://github.com/KSXGitHub/perfectionist).
- Closure parameters — [`perfectionist::single_letter_closure_param`](https://github.com/KSXGitHub/perfectionist). The trivial-callback allowlist already covers `sort_by(|a, b| ...)`, `.fold(acc, |acc, x| ...)`, and trivial wrappers such as `.pipe(|x| vec![x])`.
- `let` bindings — [`perfectionist::single_letter_let_binding`](https://github.com/KSXGitHub/perfectionist). The rule switches off entirely under `#[cfg(test)]`, so `let a = ...; let b = ...;` fixtures for interchangeable specimens stay allowed in tests.

Single-letter generics are acceptable only in very short, self-contained trait impls.
Beyond what the lints enforce: when a closure or `for`-loop uses an index variable, `i` / `j` / `k` are reserved for that single role. Anywhere else, including `let` bindings outside index-based loops, use `index` or a `*_index` suffix.

### Variable and Closure Parameter Naming

Use **descriptive names** for variables and closure parameters by default. Single-letter names are permitted only in the specific cases listed below.

#### When single-letter names are allowed

- **Comparison closures:** `|a, b|` in `sort_by`, `cmp`, or similar two-argument comparison callbacks. This is idiomatic Rust.

```rust
packages.sort_by(|a, b| a.name.cmp(&b.name));
```

- **Conventional single-letter names:** `n` for a natural number such as an unsigned integer or count, `f` for a `fmt::Formatter`, and similar well-established conventions from math or the Rust standard library. Note: for indices, use `index`, or `*_index` such as `row_index`, not `n`. For `i`/`j`/`k`, see the dedicated rule below.

```rust
fn with_capacity(n: usize) -> Self { todo!() }
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { todo!() }
```

- **Index variables (`i`, `j`, `k`):** These may only be used in two contexts: short closures, and index-based loops or iterations. The latter is rare in Rust. In all other cases, use `index` or `*_index`.

```rust
// OK: short closure
rows.zip(cols).map(|(i, j)| matrix[i][j])

// OK: index-based loop
for i in 0..len { /* ... */ }

// Bad: use a descriptive name instead
let i = items.iter().position(|item| item.is_active()).unwrap();
```

- **Trivial single-expression closures:** A closure whose body is a single field access, method call, or wrapper may use a single letter when the type and purpose are obvious from context.

```rust
.pipe(|x| vec![x])
```

- **Fold accumulators:** `acc` for the accumulator and a single letter for the element in trivial folds.

```rust
.fold(PathBuf::new(), |acc, x| acc.join(x))
```

- **Test fixtures:** `let a`, `let b`, `let c` for interchangeable specimens with identical roles in equality or comparison tests. Do not use single letters when the variables have distinct roles; use `actual`/`expected` or similar descriptive names instead.

```rust
let a = vec![3, 1, 2].into_iter().collect::<BTreeSet<_>>();
let b = vec![2, 3, 1].into_iter().collect::<BTreeSet<_>>();
assert_eq!(a, b);
```

#### When single-letter names are NOT allowed

- **Multi-line functions and closures:** Use a descriptive name when a function or closure body spans multiple lines. Examples include a body that contains a `let` binding followed by another expression, or a body with multiple chained operations.

```rust
// Good
.map(|package| {
let manifest = package.manifest()?;
install(&manifest)
})

// Bad
.map(|p| {
let manifest = p.manifest()?;
install(&manifest)
})
```

- **`let` bindings in non-test code:** Always use descriptive names.

```rust
// Good
let manifest = package.manifest()?;
// Bad
let m = package.manifest()?;
```

- **Function and method parameters:** Always use descriptive names, except for the conventional single-letter names listed above, such as `n` and `f`.

- **Closures with non-obvious context:** When the type or purpose is not immediately clear from the surrounding method chain, use a descriptive name.

```rust
// Good: descriptive name makes the closure self-documenting
.filter(|entry| entry.is_published())
```rust
// OK: index-based loop
for i in 0..len { /* ... */ }

// Bad: reader must look up what .filter receives
.filter(|x| x.is_published())
```
// Bad: use a descriptive name instead
let i = items.iter().position(|item| item.is_active()).unwrap();
```

### When to use [owned] parameter? When to use [borrowed] parameter?

Expand Down Expand Up @@ -589,57 +509,7 @@ Do not flatten the tests into a sibling file such as `src/foo_tests.rs`, and do

### Cloning `Arc` and `Rc`

Prefer using `Arc::clone` or `Rc::clone` to vague `.clone()` or `Clone::clone`.

**Error resistance:** Explicitly specifying the cloned type would avoid accidentally cloning the wrong type. As seen below:

```rust
fn my_function(value: Arc<Vec<u8>>) {
// ... do many things here
let value_clone = value.clone(); // inexpensive clone
tokio::task::spawn(async move {
// ... do stuff with value_clone
});
}
```

The above function could easily be refactored into the following code:

```rust
fn my_function(value: &Vec<u8>) {
// ... do many things here
let value_clone = value.clone(); // expensive clone, oops
tokio::task::spawn(async move {
// ... do stuff with value_clone
});
}
```

With an explicit `Arc::clone`, however, the performance characteristic will never be missed:

```rust
fn my_function(value: Arc<Vec<u8>>) {
// ... do many things here
let value_clone = Arc::clone(&value); // no compile error
tokio::task::spawn(async move {
// ... do stuff with value_clone
});
}
```

```rust
fn my_function(value: &Vec<u8>) {
// ... do many things here
let value_clone = Arc::clone(&value); // compile error
tokio::task::spawn(async move {
// ... do stuff with value_clone
});
}
```

The above code is still valid code, and the Rust compiler doesn't error, but it has a different performance characteristic now.

**Readability:** The generic `.clone()` or `Clone::clone` often implies an expensive operation (for example: cloning a `Vec`), but `Arc` and `Rc` are not as expensive as the generic `.clone()`. Explicitly marking the cloned type aids future refactoring.
Prefer the qualified `Arc::clone(&value)` / `Rc::clone(&value)` form over the method-call `value.clone()`. Enforced by [`perfectionist::arc_rc_clone`](https://github.com/KSXGitHub/perfectionist), whose rule docs cover the rationale (explicit cost, reader signal, refactor resistance) and the qualified shapes the lint accepts.

### Reporter / log events

Expand Down
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ The same procedure applies when a perfectionist rule itself is wrong — for exa

You can run the same check locally with `just dylint` (requires `cargo-dylint` and `dylint-link`; install with `cargo binstall cargo-dylint dylint-link`).

If you cannot run `cargo dylint` in your environment — for example, an AI agent in a sandbox without the dylint toolchain installed — read the rule descriptions out of [perfectionist](https://github.com/KSXGitHub/perfectionist)'s `rules/` directory at the same git ref that `dylint.toml` pins. The directory contains one Markdown file per implemented rule (auto-generated from the rule sources by `gen-docs`), so the documentation always matches the rule behavior at the pinned ref. Browse the directory at `https://github.com/KSXGitHub/perfectionist/tree/<tag>/rules` (substituting the tag from `dylint.toml`) to see what each rule does, when it fires, and how to configure it.

## Setup

### Prerequisites
Expand Down
4 changes: 2 additions & 2 deletions crates/cli/src/cli_args/add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ pub struct AddArgs {

impl AddArgs {
/// Execute the subcommand.
pub async fn run<R: Reporter>(self, mut state: State) -> miette::Result<()> {
pub async fn run<Reporter: self::Reporter>(self, mut state: State) -> miette::Result<()> {
// TODO: if a package already exists in another dependency group, don't remove the existing entry.

let State { tarball_mem_cache, http_client, config, manifest, lockfile, resolved_packages } =
Expand All @@ -116,7 +116,7 @@ impl AddArgs {
resolved_packages,
supported_architectures,
}
.run::<R>()
.run::<Reporter>()
.await
.wrap_err("adding a new package")
}
Expand Down
4 changes: 2 additions & 2 deletions crates/cli/src/cli_args/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ pub struct InstallArgs {
}

impl InstallArgs {
pub async fn run<R: Reporter>(self, state: State) -> miette::Result<()> {
pub async fn run<Reporter: self::Reporter>(self, state: State) -> miette::Result<()> {
let State { tarball_mem_cache, http_client, config, manifest, lockfile, resolved_packages } =
&state;
let InstallArgs {
Expand Down Expand Up @@ -178,7 +178,7 @@ impl InstallArgs {
supported_architectures,
node_linker,
}
.run::<R>()
.run::<Reporter>()
.await
.wrap_err("installing dependencies")?;

Expand Down
4 changes: 2 additions & 2 deletions crates/cli/tests/hoist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,15 +235,15 @@ fn modules_yaml_records_hoisted_dependencies() {
// doesn't drag in a YAML parser. Assert the presence rather than
// exact serialization to keep the test resilient to ordering.
assert!(
modules_yaml_text.contains("\"@pnpm.e2e/hello-world-js-bin@1.0.0\""),
modules_yaml_text.contains(r#""@pnpm.e2e/hello-world-js-bin@1.0.0""#),
"hoistedDependencies should record the transitive dep path; got:\n{modules_yaml_text}",
);
// Alias-as-stored is the full scoped name, since that's how the
// dep appears in `@pnpm.e2e/hello-world-js-bin-parent`'s
// `dependencies` map. Mirrors upstream's
// `hoistedDependencies[depPath][alias] = kind`.
assert!(
modules_yaml_text.contains("\"@pnpm.e2e/hello-world-js-bin\": \"private\""),
modules_yaml_text.contains(r#""@pnpm.e2e/hello-world-js-bin": "private""#),
"transitive should be marked as `private` hoist; got:\n{modules_yaml_text}",
);

Expand Down
6 changes: 3 additions & 3 deletions crates/cli/tests/pnpm_compatibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,16 @@ fn same_file_structure() {
.into_iter()
// Per-project metadata that pnpm 11 populates and pacquet doesn't.
// Doesn't affect the shared-cafs story.
.filter(|p| !p.starts_with("v11/projects/"))
.filter(|path| !path.starts_with("v11/projects/"))
// Hoisted-symlinks layout introduced in pnpm 11 — pnpm stores
// one `node_modules` tree per `<name>/<version>/<hash>/` under
// `v11/links/` and links the project's `node_modules/X` into there.
// Pacquet still uses the older per-project `.pnpm/` virtual store,
// so these paths exist only on the pnpm side.
.filter(|p| !p.starts_with("v11/links/"))
.filter(|path| !path.starts_with("v11/links/"))
// SQLite WAL sidecars exist only while a connection holds the
// journal open. Their presence at compare-time depends on timing.
.filter(|p| p != "v11/index.db-wal" && p != "v11/index.db-shm")
.filter(|path| path != "v11/index.db-wal" && path != "v11/index.db-shm")
.collect()
};

Expand Down
6 changes: 3 additions & 3 deletions crates/cmd-shim/src/bin_resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,9 @@ fn is_safe_bin_name(name: &str) -> bool {
if name.is_empty() {
return false;
}
name.bytes().all(|b| {
b.is_ascii_alphanumeric()
|| matches!(b, b'-' | b'_' | b'.' | b'!' | b'~' | b'*' | b'\'' | b'(' | b')')
name.bytes().all(|byte| {
byte.is_ascii_alphanumeric()
|| matches!(byte, b'-' | b'_' | b'.' | b'!' | b'~' | b'*' | b'\'' | b'(' | b')')
})
}

Expand Down
2 changes: 1 addition & 1 deletion crates/cmd-shim/src/bin_resolver/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ fn skip_dangerous_bin_names() {
"version": "1.0.0",
"bin": {
"../bad": "./bad",
"..\\bad": "./bad",
r"..\bad": "./bad",
"good": "./good",
"~/bad": "./bad",
},
Expand Down
6 changes: 3 additions & 3 deletions crates/cmd-shim/src/link_bins/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,11 @@ fn writes_all_three_shim_flavors_per_bin() {

let cmd_body = read_to_string(&cmd).unwrap();
assert!(cmd_body.starts_with("@SETLOCAL\r\n"), "cmd shim must use CRLF SETLOCAL");
assert!(cmd_body.contains("\"%~dp0\\..\\foo\\cli.js\""), "cmd target should be windows-style");
assert!(cmd_body.contains(r#""%~dp0\..\foo\cli.js""#), "cmd target should be windows-style");

let ps1_body = read_to_string(&ps1).unwrap();
assert!(ps1_body.starts_with("#!/usr/bin/env pwsh\n"));
assert!(ps1_body.contains("\"$basedir/../foo/cli.js\""));
assert!(ps1_body.contains(r#""$basedir/../foo/cli.js""#));
}

/// End-to-end exercise: a package with a `bin` field has a shim written
Expand Down Expand Up @@ -87,7 +87,7 @@ fn writes_shim_for_bin_string() {
assert!(shim_path.exists(), "shim should be created");

let body = read_to_string(&shim_path).unwrap();
assert!(body.contains("\"$basedir/../foo/bin/cli.js\""), "shim body: {body}");
assert!(body.contains(r#""$basedir/../foo/bin/cli.js""#), "shim body: {body}");
assert!(is_shim_pointing_at(&body, &pkg_dir.join("bin/cli.js")));

#[cfg(unix)]
Expand Down
8 changes: 4 additions & 4 deletions crates/cmd-shim/src/shim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ pub fn generate_sh_shim(
// emits `exit $?` on this branch for parity with non-execve POSIX
// shells.
runtime_opt => {
let args = runtime_opt.map(|r| r.args.as_str()).unwrap_or("");
let args = runtime_opt.map(|rt| rt.args.as_str()).unwrap_or("");
sh.push_str(&format!("{quoted_target} {args} \"$@\"\nexit $?\n"));
}
}
Expand Down Expand Up @@ -239,7 +239,7 @@ pub fn generate_cmd_shim(
));
}
runtime_opt => {
let args = runtime_opt.map(|r| r.args.as_str()).unwrap_or("");
let args = runtime_opt.map(|rt| rt.args.as_str()).unwrap_or("");
// No runtime detected, so exec the target directly.
cmd.push_str(&format!("@{quoted_target} {args} %*\r\n"));
}
Expand Down Expand Up @@ -294,7 +294,7 @@ pub fn generate_pwsh_shim(
writeln!(pwsh, "exit $ret").unwrap();
}
runtime_opt => {
let args = runtime_opt.map(|r| r.args.as_str()).unwrap_or("");
let args = runtime_opt.map(|rt| rt.args.as_str()).unwrap_or("");
writeln!(pwsh).unwrap();
writeln!(pwsh, "# Support pipeline input").unwrap();
writeln!(pwsh, "if ($MyInvocation.ExpectingInput) {{").unwrap();
Expand Down Expand Up @@ -329,7 +329,7 @@ if ($PSVersionTable.PSVersion -lt "6.0" -or $IsWindows) {
fn relative_target_windows(target_path: &Path, shim_path: &Path) -> String {
let shim_dir = shim_path.parent().unwrap_or_else(|| Path::new(""));
let rel = relative_path_from(shim_dir, target_path);
rel.to_string_lossy().replace('/', "\\")
rel.to_string_lossy().replace('/', r"\")
}

const SH_SHIM_HEADER: &str = r#"#!/bin/sh
Expand Down
10 changes: 5 additions & 5 deletions crates/cmd-shim/src/shim/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ fn generate_sh_shim_uses_absolute_target_when_no_common_prefix() {
let runtime = ScriptRuntime { prog: Some("node".into()), args: String::new() };
let body = generate_sh_shim(target, shim, Some(&runtime));
assert!(
body.contains("\"/abs/elsewhere/cli\""),
body.contains(r#""/abs/elsewhere/cli""#),
"absolute-target branch must skip $basedir prefix, body:\n{body}",
);
}
Expand Down Expand Up @@ -358,7 +358,7 @@ fn read_head_filled_real_fs_long_file_fills_buffer() {
use tempfile::tempdir;
let tmp = tempdir().unwrap();
let path = tmp.path().join("long");
let payload: Vec<u8> = (0..1024).map(|i| (i % 251) as u8).collect();
let payload: Vec<u8> = (0..1024).map(|idx| (idx % 251) as u8).collect();
std::fs::write(&path, &payload).unwrap();

let mut buf = [0u8; 256];
Expand Down Expand Up @@ -502,7 +502,7 @@ fn generate_cmd_shim_emits_direct_exec_when_no_runtime() {
let shim = Path::new("/p/.bin/cli.cmd");
let body = generate_cmd_shim(target, shim, None);
assert!(
body.contains("@\"%~dp0\\..\\cli\""),
body.contains(r#"@"%~dp0\..\cli""#),
"no-runtime arm must exec the target directly, body:\n{body}",
);
}
Expand All @@ -522,7 +522,7 @@ fn generate_pwsh_shim_matches_pnpm_template() {
body.contains("$basedir=Split-Path $MyInvocation.MyCommand.Definition -Parent"),
"must declare $basedir from MyInvocation",
);
assert!(body.contains("$exe=\".exe\""), "Windows-detection branch must set $exe to .exe");
assert!(body.contains(r#"$exe=".exe""#), "Windows-detection branch must set $exe to .exe");
assert!(
body.contains(
"if (Test-Path \"$basedir/node$exe\") {\n # Support pipeline input\n if ($MyInvocation.ExpectingInput) {\n $input | & \"$basedir/node$exe\" \"$basedir/../typescript/bin/tsc\" $args\n } else {\n & \"$basedir/node$exe\" \"$basedir/../typescript/bin/tsc\" $args\n }",
Expand All @@ -540,7 +540,7 @@ fn generate_pwsh_shim_emits_direct_exec_when_no_runtime() {
let shim = Path::new("/p/.bin/cli.ps1");
let body = generate_pwsh_shim(target, shim, None);
assert!(
body.contains("& \"$basedir/../cli\""),
body.contains(r#"& "$basedir/../cli""#),
"no-runtime arm must exec the target directly, body:\n{body}",
);
assert!(body.ends_with("exit $LASTEXITCODE\n"));
Expand Down
2 changes: 1 addition & 1 deletion crates/config/src/defaults.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ pub fn default_child_concurrency_with_parallelism(parallelism: u32) -> u32 {
/// [`getAvailableParallelism`](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/config/reader/src/concurrency.ts#L5-L13).
/// Floors at 1.
pub fn available_parallelism() -> u32 {
std::thread::available_parallelism().map(|n| n.get() as u32).unwrap_or(1).max(1)
std::thread::available_parallelism().map(|count| count.get() as u32).unwrap_or(1).max(1)
}

/// Resolve `childConcurrency` from a possibly-negative yaml value
Expand Down
Loading
Loading