perfectionist's `cloning_getter` (KSXGitHub/perfectionist#410) and `overly_complex_condition` (KSXGitHub/perfectionist#409) both landed upstream, so the pinned revision moves to the one that carries them and the workspace comes under both rules at once. `TestRegistry::url` and `PnprBenchmarkRegistryOverride::resolve_registry` return `&str`, and the call sites that keep the value copy it there, where the reader can see the copy. Most do not: they interpolate the URL into a request or a fixture path. `EmulatedCancellation::receiver` becomes `to_receiver` instead, since its one caller stores the receiver in the run it builds, so the copy is the method's business rather than the call site's. The test helpers that only forwarded a registry URL into `config.registry` now take `&str` and copy once inside. Conditions are capped at five `&&`/`||` operators, the point past which a guard's clause list stops reading as one predicate. Five conditions were over it. Each one either names the concept its clauses decide (`is_valid_repository_path`, `snapshot_is_reusable`) or folds the clauses that share a shape into one pattern. `frozen_tree_up_to_date`'s eleven-operator `let` chain becomes a sequence of guard clauses, each carrying the comment that explains the gate it holds, with the build checks in a helper of their own. Both caps are documented in the Rust code style guide. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
49 KiB
Code Style Guide
Introduction
Clippy cannot yet detect all suboptimal code. This guide supplements that.
This guide is incomplete. More may be added as more pull requests are reviewed.
This is a guide, not a rule. Contributors may break them if they have a good reason to do so.
Terminology
Owned type
Doesn't have a lifetime, neither implicit nor explicit.
Examples: String, OsString, PathBuf, Vec<T>, etc.
Borrowed type
Has a lifetime, either implicit or explicit.
Examples: &str, &OsStr, &Path, &[T], etc.
Copying
The act of cloning or creating an owned data from another owned/borrowed data.
Examples:
owned_data.clone()borrowed_data.to_owned()OwnedType::from(borrowed_data)path.to_path_buf()str.to_string()- etc.
Guides
Function length
Keep production function bodies within 40 lines of code. Extract helpers around distinct responsibilities and reuse existing helpers where possible. perfectionist::overly_long_function enforces this across the Rust workspace through dylint.toml.
Tests are exempt, so a test can keep its setup, action, and assertions together.
Nesting depth
Keep function and method bodies within three levels of nesting, including tests. Prefer guard clauses, match guards, and helpers named for a distinct responsibility. perfectionist::excessive_nesting enforces this across the Rust workspace through dylint.toml.
Keep iterator closures simple. A single expression can make a chain easy to follow. When a closure needs several statements, prefer an explicit loop or a named helper. Preserve lazy evaluation, allocation behavior, and short-circuiting when choosing between them.
Condition complexity
Keep if, while, and match-guard conditions within five && or || operators, including tests. perfectionist::overly_complex_condition enforces this across the Rust workspace through dylint.toml.
A longer condition is two predicates the author did not name. Name the whole condition, extract a helper for the group of clauses that names a concept, or split the leading clauses into a guard clause. Keep a helper call inside the short-circuit chain rather than binding its result above the condition, so the clauses after it stay unevaluated until the condition reaches them, and keep a while condition's work on every iteration.
Method chains
Keep production method chains within nine calls. Consecutive calls to the same method count once; .await and ? do not add to the count. Tests are exempt.
perfectionist::overly_long_method_chain enforces this through dylint.toml. When a chain exceeds the limit, name an intermediate value or extract a helper for a distinct operation.
File length
Keep production Rust files within 400 lines of code and test-only Rust files within 800. Count nonblank lines containing code, including multiline string content, and exclude comment-only lines. Split files into modules around distinct responsibilities or test scenarios. Keep tests in their existing test binary.
perfectionist::overly_long_file enforces the 400-line production limit across the Rust workspace through dylint.toml. Test files are exempt from the rule, so their 800-line limit is on you to keep.
Struct fields
Keep structs within eight fields, including test structs. perfectionist::too_many_struct_fields enforces this across the Rust workspace through dylint.toml.
Group fields by a shared responsibility and reuse existing types. Pass a group directly to helpers that need it, or put its behavior on the group. Avoid repeating the group name in its fields: prefer store.dir and package.integrity.
A struct that must match a fixed external configuration, serialized document, or binding interface may use a scoped #[expect(perfectionist::too_many_struct_fields, reason = "...")], gated with cfg_attr(dylint_lib = "perfectionist", ...). Name the format or interface in the reason. Internal runtime state and options should be refactored.
Naming convention
Follow the Rust API guidelines. Specific naming conventions for generics, variables, and closure parameters are covered in the sections below.
Module Organization
- Use the flat file pattern (
module.rs) rather thanmodule/mod.rsfor submodules. Enforced byclippy::mod_module_files, which bansmod.rsfiles. (perfectionist::flat_module_patterncovered this previously; perfectionist has since removed it in favor of the Clippy rule, somod_module_filesis now the sole enforcer.) - List
pub moddeclarations first, thenpub usere-exports, then private imports and items. - Use
pub useto re-export key types at the module level for convenience.
pub mod install_package_by_snapshot;
pub mod install_package_from_registry;
pub mod install_without_lockfile;
pub use install_package_by_snapshot::InstallPackageBySnapshot;
pub use install_package_from_registry::InstallPackageFromRegistry;
Import Organization
Prefer merged imports. Combine multiple items from the same crate root into a single use statement with nested braces rather than separate use lines (the crate granularity). Import ordering is enforced by the pinned formatter; the granularity is enforced by perfectionist::import_granularity_mismatch (configured to crate in dylint.toml). Imports gated by a platform attribute such as #[cfg(unix)] go in a separate block after the main imports.
use crate::{
package_manager::PackageManager,
store::Store,
};
use pipe_trait::Pipe;
use std::{path::PathBuf, sync::Arc};
#[cfg(unix)]
use std::os::unix::fs::PermissionsExt;
No star imports
Avoid star (glob) imports inside the bodies of regular modules. Import items explicitly by name everywhere except the two cases noted below. The rule applies to production code, tests, integration tests, build scripts, and developer tooling under tasks/.
The two exceptions are:
- External-crate preludes, such as
use rayon::prelude::*;oruse assert_cmd::prelude::*;. The upstream crate has already curated which items are intended to be glob-imported, so listing them out by hand creates a maintenance burden the moment the upstream prelude changes. Use the prelude in the form the crate documents. - Re-exports at the root of a module or crate, such as
pub use submodule::*;inlib.rs. These are part of the public surface that the crate intentionally exposes, and listing the items individually duplicates information that already lives in the submodule.
Star imports inside a module body are the case worth banning. They make it hard to tell where a name comes from, they hide accidental shadowing, and use super::*; is especially harmful in tests. The form pulls every privately imported item from the outer module into scope, so an import the production code no longer uses can keep compiling indefinitely as long as some test still references it. Removing dead imports becomes guesswork.
// Bad
#[cfg(test)]
mod tests {
use super::*;
}
// Good
#[cfg(test)]
mod tests {
use super::{ParsedThing, parse_thing};
}
// Allowed (external-crate prelude)
use rayon::prelude::*;
use assert_cmd::prelude::*;
// Allowed (root-of-module re-export)
pub use comver::*;
pub use load_lockfile::*;
Generic Parameter Naming
Use descriptive names for type parameters (Size, Name, Manifest, Store, Reporter) and const generics instead of single letters. Enforced by perfectionist::single_letter_generic and perfectionist::single_letter_const_generic. The idiomatic T for a lone type parameter and N for a const-generic array length are accepted at the site with #[expect(perfectionist::single_letter_generic, …)] / #[expect(perfectionist::single_letter_const_generic, …)].
Variable and Closure Parameter Naming
Use descriptive names for variables and closure parameters, in test code as well as production code. Single letters are accepted only where the rules' default allowlists permit them: n/f/i/j/k for their conventional roles, the sort_by / sort_by_key / min_by / max_by / fold callback shape, and single-expression closure bodies. Multi-line closure bodies and every let binding (including in #[cfg(test)] code) are flagged. The same applies to single-letter const/static items. Enforced by:
perfectionist::single_letter_function_paramperfectionist::single_letter_closure_paramperfectionist::single_letter_let_bindingperfectionist::single_letter_const_itemperfectionist::single_letter_static_item
When to use owned parameter? When to use borrowed parameter?
This is a trade-off between API flexibility and performance.
If using an owned signature would reduce copying, one should use an owned signature.
Otherwise, use a borrowed signature to widen the API surface.
Example 1: Preferring owned signature.
fn push_path(list: &mut Vec<PathBuf>, item: &Path) {
list.push(item.to_path_buf());
}
push_path(&mut my_list, &my_path_buf);
push_path(&mut my_list, my_path_ref);
The above code is suboptimal because it forces the copying of my_path_buf even though the type of my_path_buf is already PathBuf.
Changing the signature of item to PathBuf would help remove .to_path_buf() inside the push_path function, eliminate the cloning of my_path_buf (the ownership of my_path_buf is transferred to push_path).
fn push_path(list: &mut Vec<PathBuf>, item: PathBuf) {
list.push(item);
}
push_path(&mut my_list, my_path_buf);
push_path(&mut my_list, my_path_ref.to_path_buf());
It does force my_path_ref to be explicitly copied, but since item is not copied, the total number of copying remains the same for my_path_ref.
Example 2: Preferring borrowed signature.
fn show_path(path: PathBuf) {
println!("The path is {path:?}");
}
show_path(my_path_buf);
show_path(my_path_ref.to_path_buf());
The above code is suboptimal because it forces the copying of my_path_ref even though a &Path is already compatible with the code inside the function.
Changing the signature of path to &Path would help remove .to_path_buf(), eliminating the unnecessary copying:
fn show_path(path: &Path) {
println!("The path is {path:?}");
}
show_path(my_path_buf);
show_path(my_path_ref);
Use the most encompassing type for function parameters
The goal is to allow the function to accept more types of parameters, reducing type conversion.
Example 1:
fn node_bin_dir(workspace: &PathBuf) -> PathBuf {
workspace.join("node_modules").join(".bin")
}
let a = node_bin_dir(&my_path_buf);
let b = node_bin_dir(&my_path_ref.to_path_buf());
The above code is suboptimal because it forces the copying of my_path_ref only to be used as a reference.
Changing the signature of workspace to &Path would help remove .to_path_buf(), eliminating the unnecessary copying:
fn node_bin_dir(workspace: &Path) -> PathBuf {
workspace.join("node_modules").join(".bin")
}
let a = node_bin_dir(&my_path_buf);
let b = node_bin_dir(my_path_ref);
Getters return the borrowed form
A getter — an inherent &self method named for the field it reads, or one named get_* — returns the borrowed form of that field: &str for a String, &Path for a PathBuf, &[T] for a Vec<T>, Option<&T> for an Option<T>.
A getter that copies decides for every caller that they wanted an owned value, and most did not: they compare, print, or pass it on. The borrowed form serves every caller and leaves the one that needs ownership to copy at the call site, where the reader can see it. Where every call site would copy the borrow straight back, name the method to_* instead, the prefix for a conversion that costs something.
perfectionist::cloning_getter enforces this across the Rust workspace through dylint.toml. An Rc or Arc field is exempt, since cloning one bumps a refcount rather than copying what it points at.
Trait Bounds
Prefer where clauses over inline bounds when there are multiple constraints:
impl<Store, Manifest, Reporter> InstallPackage<Store, Manifest, Reporter>
where
Store: PackageStore + Send + Sync,
Manifest: AsRef<PackageManifest> + Send,
Reporter: ProgressReporter + Sync + ?Sized,
{
/* ... */
}
Pattern Matching
When mapping enum variants to values, prefer the concise wrapping style:
ExitCode::from(match self {
InstallError::NetworkFailure(_) => 2,
InstallError::ManifestParseFailure(_) => 3,
})
Using pipe-trait
This codebase uses the pipe-trait crate. The Pipe trait enables method-chaining through unary functions, keeping code in a natural left-to-right reading order. Import it as use pipe_trait::Pipe;.
Any callable that takes a single argument works with .pipe(). This includes free functions, closures, newtype constructors, enum variant constructors, Some, Ok, Err, and trait methods such as From::from. The guidance below applies equally to all of them.
When to use pipe
Chaining through a unary function at the end of an expression chain:
// Good: pipe keeps the chain flowing left-to-right
manifest.dependencies().pipe(DependencyMap)
entries.into_iter().collect::<HashMap<_, _>>().pipe(Store)
Avoiding deeply nested function calls:
// Nested calls are harder to read
let parsed = serde_json::from_slice::<Manifest>(&bytes)?;
// Prefer piping instead
let parsed = bytes.as_slice().pipe(serde_json::from_slice::<Manifest>)?;
Chaining through multiple unary functions:
name.pipe(InstallError::MissingPackage).pipe(Err)
Continuing a method chain through a free function and back to methods:
url
.pipe(normalize_registry_url)
.map(Cow::Borrowed)
Using .pipe_as_ref() to pass a reference mid-chain. This avoids introducing a temporary variable when a free function takes &T:
// Good: pipe_as_ref calls .as_ref() then passes to the function
"node_modules"
.pipe(Path::new)
.join(package_name)
.pipe_as_ref(is_within_store)
.then_some(package_name)
When NOT to use pipe
Simple standalone function calls. Pipe adds noise with no readability benefit:
// Bad: unnecessary pipe
let result = value.pipe(foo);
// Good: just call the function directly
let result = foo(value);
This applies to any unary callable, such as Some, Ok, or constructors, when there is no preceding chain to continue:
// Bad: pipe adds nothing here
let result = value.pipe(Some);
// Good: direct call is clearer
let result = Some(value);
However, piping through any unary function is preferred when it continues an existing chain:
// Good: continues a chain
manifest.summarize().pipe(Some)
Doc comment intra-links
When a doc comment (/// or //! ) mentions an identifier (type, trait, function, method, module, constant, macro, etc.) that is intra-linkable from the current scope, write the mention as a rustdoc intra-doc link rather than as bare prose. Intra-doc links give readers one-click navigation and let rustdoc warn when a referenced item is renamed or removed, so the docs stay in sync with the code.
// Good
/// Installs the package described by [`PackageManifest`] into [`Store`].
pub fn install(manifest: &PackageManifest, store: &Store) { /* ... */ }
// Bad: identifiers are mentioned as bare prose
/// Installs the package described by `PackageManifest` into `Store`.
pub fn install(manifest: &PackageManifest, store: &Store) { /* ... */ }
If the identifier is not directly in scope, use a path link ([`crate::store::Store`]) or a disambiguated link ([`Store`](crate::store::Store)) rather than dropping back to bare prose. Reserve plain backticks for things that genuinely cannot be linked, such as identifiers from external code that rustdoc cannot resolve, shell commands, file paths, or literal values.
Documentation comments
A doc comment (/// or //!) is rendered by rustdoc as the documentation of the item it attaches to, and it is visible to every reader who can see that item. The doc comment of a pub item therefore reaches every downstream user of the crate, including users who never read the source. Do not reference items more private than the item being documented. Disallowed references include a private function named in the doc comment of a pub item, a pub(crate) type named in the doc comment of a pub item, and a private constant named in the doc comment of a pub(crate) item. A reader who only sees the rendered docs cannot follow such a reference, and intra-doc links to inaccessible items become broken links in cargo doc output.
If the explanation genuinely depends on that more private item, choose one of two fixes. The first option is to widen the visibility of the referenced item, adding a re-export when one fits the API. The second option is to move the explanation into a regular // comment on the implementation, where readers of the source can see it. Reserve /// and //! for things a downstream user of the item needs to know. Use // for notes useful only to someone reading the source.
// Bad: public doc references a private helper
/// Builds the lockfile by walking dependencies.
///
/// Internally calls [`walk_deps_inner`] to handle cycles.
pub fn build_lockfile(/* ... */) { /* ... */ }
fn walk_deps_inner(/* ... */) { /* ... */ }
// Good: public doc describes observable behavior
/// Builds the lockfile by walking dependencies.
///
/// Cycles in the dependency graph are reported as
/// [`LockfileError::CycleDetected`].
pub fn build_lockfile(/* ... */) { /* ... */ }
fn walk_deps_inner(/* ... */) { /* ... */ }
Serde Cow<'de, str> vs String source types
When wiring a type into serde with #[serde(try_from = "...")] or #[serde(from = "...")], pick the source according to whether the deserialized value retains the string.
Do not use &'de str. Text formats such as JSON, YAML, and TOML accept escape sequences (for example, JSON's "\u0061") that the deserializer must decode into a fresh buffer. Borrowed &'de str deserialization rejects every input that requires decoding, so the type fails on values the format itself accepts.
Prefer Cow<'de, str> when the conversion discards the string or splits it into pieces, for example when it parses into a number, an enum discriminant, or a struct whose fields are substrings of the input. The deserializer borrows from the input when no decoding is needed and allocates only when escapes force it.
Prefer String when the entire input is moved into the resulting value verbatim. Taking String lets the conversion store the buffer directly without re-cloning.
use std::borrow::Cow;
#[derive(serde::Deserialize)]
#[serde(try_from = "Cow<'de, str>")]
struct Port(u16);
impl<'a> TryFrom<Cow<'a, str>> for Port {
type Error = std::num::ParseIntError;
fn try_from(value: Cow<'a, str>) -> Result<Self, Self::Error> {
value.parse().map(Port)
}
}
#[derive(serde::Deserialize)]
#[serde(try_from = "String")]
struct PackageName(String);
impl TryFrom<String> for PackageName {
type Error = InvalidPackageName;
fn try_from(value: String) -> Result<Self, Self::Error> {
validate(&value)?;
Ok(PackageName(value))
}
}
The same trade-off applies to the infallible #[serde(from = "...")] form.
Error Handling
- Use
derive_morefor error types. Only derive the traits that are actually used:Display: derive when the type needs to be displayed, such as when it is printed to stderr or used in format strings.Error: derive when the type is used as astd::error::Error, such as the error type inResultor the source of another error. Not all types withDisplayneedError.- A type that only needs formatting and not error handling should derive
DisplaywithoutError.
- Minimize
unwrap()in non-test code; use proper error propagation.unwrap()is acceptable in tests, and is also acceptable for provably infallible operations when accompanied by a comment explaining the invariant. When deliberately ignoring an error, use.ok()and document the rationale.
#[derive(Debug, Display, Error)]
#[non_exhaustive]
pub enum InstallError {
#[display("NetworkFailure: {_0}")]
NetworkFailure(reqwest::Error),
}
Conditional Test Skipping: #[cfg] vs #[cfg_attr(..., ignore)]
When a test cannot run under certain conditions, such as on the wrong platform, prefer #[cfg_attr(..., ignore)] over #[cfg(...)] to skip it. The test still compiles on every configuration and is only skipped at runtime. This approach catches type errors and regressions that a #[cfg] skip would hide.
Use #[cfg] on tests only when the code cannot compile under the condition. An example is a test that references types, functions, or trait methods gated behind #[cfg] that do not exist on other platforms or feature sets.
Prefer including a reason string in the ignore attribute to explain why the test is skipped.
// Good: test compiles everywhere, skipped at runtime on non-unix
#[test]
#[cfg_attr(not(unix), ignore = "only one path separator style is tested")]
fn unix_path_logic() { /* uses hardcoded unix paths but no unix-only types */ }
// Good: test CANNOT compile on non-unix (uses unix-only types)
#[cfg(unix)]
#[test]
fn unix_permissions() { /* uses PermissionsExt which only exists on unix */ }
When or when not to log during tests? What to log? How to log?
The goal is to enable the programmer to quickly inspect the test subject should a test fail.
Logging is almost always necessary when the assertion is not assert_eq!. For example: assert!, assert_ne!, etc.
Logging is sometimes necessary when the assertion is assert_eq!.
If the values being compared with assert_eq! are simple scalar or single line strings, logging is almost never necessary. It is because assert_eq! should already show both values when assertion fails.
If the values being compared with assert_eq! are strings that may have many lines, they should be logged with eprintln! and {} format.
If the values being compared with assert_eq! have complex structures (such as a struct or an array), they should be logged with dbg!.
Example 1: Logging before assertion is necessary
let message = my_func().unwrap_err().to_string();
eprintln!("MESSAGE:\n{message}\n");
assert!(message.contains("expected segment"));
let output = execute_my_command();
let received = output.stdout.to_string_lossy(); // could have multiple lines
eprintln!("STDOUT:\n{received}\n");
assert_eq!(received, expected);
let hash_map = create_map(my_argument);
dbg!(&hash_map);
assert!(hash_map.contains_key("foo"));
assert!(hash_map.contains_key("bar"));
Example 2: Logging is unnecessary
let received = add(2, 3);
assert_eq!(received, 5);
If the assertion fails, the value of received will appear alongside the error message.
Unit test file layout
Always place unit tests in a dedicated external tests submodule (#[cfg(test)] mod tests;) rather than inline in the parent file — for src/foo.rs the tests live in src/foo/tests.rs. This keeps production and test code in separate files and avoids churning git blame. Enforced by perfectionist::excessive_inline_tests (configured to inline_style = "external_only"), which flags any inline test code so it must move to an external mod tests;.
Cloning Arc and Rc
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types. The qualified form makes the O(1) refcount bump visible at the call site and fails to compile if a refactor changes the binding's type to something whose Clone is an arbitrarily expensive deep copy. Enforced by clippy::clone_on_ref_ptr, wired into [workspace.lints.clippy] in the root Cargo.toml.
Reading process state
Library code should rarely call std::env::var, std::env::var_os, or std::env::current_dir directly. Process state belongs to the running process, not to the operation a function performs, so reaching for it couples the logic to whichever shell invoked pacquet and hides the dependency from the caller. Prefer one of:
- Take the value as a parameter. A function that needs a path declares
path: &Path; one that needs an env-var value takes it as a&strargument or anOption<String>. - Thread the read through the DI seam. Declare
Sys: EnvVar(orEnvVarOs,GetHomeDir,GetCurrentDir, …) and callSys::var(name)/Sys::var_os(name)/Sys::home_dir()/Sys::current_dir(). See Dependency injection for tests for the convention. This is howConfig::currentresolves the home dir and theNPM_CONFIG_WORKSPACE_DIRlookup, and howcrates/workspace'sfind_workspace_dir_from_env_withresolves its env-var lookup — both let tests drive the read without touching the process environment.
The narrow case where a direct call is acceptable is computing the default of a Config field that has no caller to take the value from — default_store_dir, default_modules_dir, and default_virtual_store_dir are the canonical examples. Even there, route the lookup through the DI seam: write the helper as default_store_dir<Sys: EnvVar + GetHomeDir + GetCurrentDir>() and inline the production composition at the SmartDefault site (#[default(_code = "default_store_dir::<Host>()")]), so the fake-Sys test path and the production Host path resolve through the same generic. The other accepted boundary reads are program-entry knobs (RAYON_NUM_THREADS in crates/cli, TRACE in crates/diagnostics) and the lifecycle-script env snapshot in crates/executor and crates/git-fetcher, which has to forward the parent environment to spawned children verbatim. If new library code seems to need env::var / env::current_dir for any other reason, the answer is almost always a &Path (or equivalent) parameter, not a process-state read.
Dependency injection for tests
Side-effecting code — filesystem access, environment variables, network calls, time, process state — has two testing routes. The default route is a real fixture: a tempfile::TempDir for filesystem work, the mocked registry (just registry-mock) for HTTP, an integration test that spawns the actual pacquet binary in a scratch directory for end-to-end flows. Real fixtures keep tests close to what users see and scale with the codebase without per-call-site plumbing; they are the right tool everywhere except the cases enumerated below.
The dependency-injection seam described below is the narrow second route. Reach for it only when one of the following applies:
- Filesystem error branches the host OS won't reproduce portably.
PermissionDenied,ENOSPC, a directory that disappears mid-walk, a chmod that fails after the file exists — provoking these on real disks is platform-specific, racy, or both. A fake that returns the exactio::ErrorKindis the only portable way to drive the branch. - Deterministic time. Asserting that
prunedAtequals a specific HTTP-date (RFC 7231 IMF-fixdate, whathttpdate::fmt_http_dateemits), or that a throttled emitter fires on the second sample, needs the clock to be a known value. The realSystemTime::nowmakes those assertions flaky. - Shared process-global state that tests would otherwise mutate. When a branch depends on a single per-process slot — environment variables, the current working directory, the umask, signal handlers, the global allocator — the only way to exercise it without DI is to write to that slot, and the write is observed by every other test in the same process. A serialisation lock (the
EnvGuardpattern that pnpm/pacquet#343 + pnpm/pnpm#11718 retired fromdefault_store_dir) restores correctness only by forcing the affected tests to run single-threaded, and leaves anunsafe { env::set_var(...) }block at the call site. A capability-trait fake keeps the read deterministic and the mutation contained to the test that needs it, so nextest's in-process parallelism stays useful andunsafestays out of test code. The reverse follows too: a real-fixture happy path should not be promoted to DI just because it crosses a process-global slot — only the mutation side of the slot is the problem; reading whatever the shell already has is fine. - External-service happy paths that can't be staged in CI. pnpm has features whose normal flow depends on real external systems —
pnpm login's 2FA prompt round-trip, the OIDC token exchange and provenance attestation inpnpm publish, and similar — where the happy path itself is what needs faking, not just the error path. When pacquet grows those features, DI is the right tool for their tests too. (As of writing, these are not yet implemented; this exception is documented so the convention is in place when they land.) - Unreachable-by-design preconditions. When a function declares a capability bound but a specific test exercises a branch that never reaches that capability, the fake satisfies the bound with
unreachable!and documents the precondition. See the worked example below.
A function that takes a <Sys> generic but is only ever exercised via real fixtures is a smell — either the DI branches are missing coverage, or the generic is over-design. Either add the tests that justify the seam, or drop the generic and let the real fixture cover everything.
The rest of this section is the convention that applies when DI is the right tool.
Names
- The generic type parameter is named
Sys— short for "system seam," the slot in the function signature that selects between the real OS and the test fake. A single short name makes a generic call site instantly recognisable as the DI seam. - The production provider struct is named
Host— unqualified, because the production implementation is the default. Fakes carry behaviour-based names that describe what they do (FailingRead,EmptyRead,PermissionDenied,FakeHostName), not what category of thing they are. - Capability traits use the form
<Domain><Action>: filesystem capabilities areFs*(FsReadToString,FsCreateDirAll,FsWrite,FsReadDir,FsWalkFiles,FsSetExecutable,FsEnsureExecutableBits); environment-variable lookup isEnvVar; clock reads areClock; hostname lookup isGetHostName. The domain prefix lets a reader of a generic bound see which side effect the function reaches for without chasing definitions. Method names mirror theirstdequivalents so the trait is a thin seam overstd::fs::*/std::env::var/SystemTime::now, not a re-imagining.
Eight principles
-
Single-purpose traits. Each capability gets its own trait — one method per side effect, no umbrella trait that bundles
read,write,create_dir_allinto one bag. A function then binds only the capabilities it actually consumes, and a test fake implements only the methods the function under test exercises. -
One generic parameter with multiple bounds. Compose bounds on a single
Sys, never introduce a second type parameter per capability:// Good: one parameter, composed bounds pub fn read_modules_manifest<Sys>(modules_dir: &Path) -> Result<...> where Sys: FsReadToString + Clock, { /* ... */ } // Bad: a parameter per capability — every call site has to satisfy two slots pub fn read_modules_manifest<Fs, C>(modules_dir: &Path) -> Result<...> where Fs: FsReadToString, C: Clock { /* ... */ }One parameter keeps turbofish call sites short (
read_modules_manifest::<Host>(dir)) and makes the fake's job obvious: implement every trait in the bound list, no more. -
Static methods, not
&self. Capability methods are associated functions (no&selfreceiver). The provider is a unit struct that carries no data:pub trait FsReadToString { fn read_to_string(path: &Path) -> io::Result<String>; } pub struct Host; impl FsReadToString for Host { fn read_to_string(path: &Path) -> io::Result<String> { fs::read_to_string(path) } }Stateful fakes (a fake clock that returns a fixed
SystemTime, a recording fake that captures every call) store their state in an interior-mutablestaticdeclared inside the#[test]body, so the trait shape doesn't have to change to accommodate state. This keeps the production impl free of&selfplumbing the test-only fake would otherwise force on it. -
Associated types for data operations. When a capability operates over a domain data type, expose the data type as an associated type rather than threading an instance through every call. The provider chooses the concrete type, and fakes can pick a stub-friendly stand-in.
-
Capability traits on the implementor.
impl FsReadToString for Hostlives on the provider, not on the data type. The data types stay free of test-shim conditional impls; the seam is the provider. -
Domain-neutral provider, domain-scoped traits. The generic is
Sys, the production type isHost(or whatever your crate exports as its provider), and the trait names carry the domain prefix (Fs*,Env*,Clock,GetHostName, …). A reader ofSys: FsReadToString + Clock + EnvVarknows immediately which side effects the function reaches for. -
Explicit turbofish in production. Production call sites name the provider:
read_modules_manifest::<Host>(modules_dir) write_modules_manifest::<Host>(modules_dir, manifest) Config::current::<Host, _, _, _, _>(env::current_dir, home::home_dir, Default::default)The turbofish makes the production choice visible at the call site instead of relying on type inference; if a future caller wants to swap in a different provider (a test driver, a dry-run shim), the spot to change is obvious.
-
Capabilities are primitives, not algorithms. Each trait names a leaf-level effect that maps to a single
stdfunction (read_to_string,create_dir_all,write,var, …). Higher-level guarantees — atomic write, retry loops, walk-with-options — become free functions composed on top of those primitives, not new trait methods. That keeps the fake surface dead-simple: a fake declares one method per capability, never a knob-laden builder.
Worked example: modules-yaml
crates/modules-yaml reads and writes node_modules/.modules.yaml. The read path is generic over FsReadToString + Clock because it needs to read the file and stamp prunedAt from the wall clock; the write path is generic over FsCreateDirAll + FsWrite because it needs to ensure the parent directory exists before writing the serialized manifest:
pub trait FsReadToString {
fn read_to_string(path: &Path) -> io::Result<String>;
}
pub trait FsCreateDirAll {
fn create_dir_all(path: &Path) -> io::Result<()>;
}
pub trait FsWrite {
fn write(path: &Path, contents: &[u8]) -> io::Result<()>;
}
pub trait Clock {
fn now() -> SystemTime;
}
pub struct Host;
impl FsReadToString for Host {
fn read_to_string(path: &Path) -> io::Result<String> { fs::read_to_string(path) }
}
impl FsCreateDirAll for Host {
fn create_dir_all(path: &Path) -> io::Result<()> { fs::create_dir_all(path) }
}
impl FsWrite for Host {
fn write(path: &Path, contents: &[u8]) -> io::Result<()> { fs::write(path, contents) }
}
impl Clock for Host {
fn now() -> SystemTime { SystemTime::now() }
}
pub fn read_modules_manifest<Sys>(modules_dir: &Path) -> Result<Option<Modules>, ReadModulesError>
where
Sys: FsReadToString + Clock,
{
let content = match Sys::read_to_string(&manifest_path) { /* ... */ };
// ...
manifest.pruned_at = httpdate::fmt_http_date(Sys::now());
Ok(Some(manifest))
}
A test that wants to drive the PermissionDenied branch declares a unit-struct fake inside the #[test] body, implementing only the capability the function touches:
#[test]
fn read_propagates_non_not_found_io_error() {
struct FailingRead;
impl FsReadToString for FailingRead {
fn read_to_string(_: &Path) -> io::Result<String> {
Err(io::Error::new(io::ErrorKind::PermissionDenied, "mocked"))
}
}
// `read_modules_manifest`'s bound list is `FsReadToString + Clock`,
// so every fake must satisfy both bounds at the type level — Rust
// doesn't know that the `prunedAt` branch is unreachable for this
// input. The convention for capabilities the test won't exercise
// is a trivial impl whose body is `unreachable!`: the bound is
// satisfied, and the panic message documents the precondition the
// test relies on.
impl Clock for FailingRead {
fn now() -> SystemTime {
unreachable!("clock must not be called when read_to_string fails");
}
}
let err = read_modules_manifest::<FailingRead>(Path::new("/")).unwrap_err();
assert!(matches!(err, ReadModulesError::ReadFile { .. }));
}
Stateful fakes (deterministic clock, recording reads) hold their state in a static inside the test fn:
#[test]
fn read_fills_in_pruned_at_when_missing() {
static FAKE_NOW: SystemTime = SystemTime::UNIX_EPOCH;
struct FakeClock;
impl Clock for FakeClock {
fn now() -> SystemTime { FAKE_NOW }
}
// The `static` lives in this fn's scope, so other tests get
// independent storage and never race on it. The provider type
// stays an empty unit struct; the state lives next to the test
// that needs it. Use `SystemTime::UNIX_EPOCH` (a `const`) for a
// const-initialised static, or `LazyLock` when the desired value
// needs a runtime constructor.
// ...
}
Cross-domain composition
When a function needs capabilities from more than one domain, list them inline on Sys:
fn write_shim<Sys>(target_path: &Path, shim_path: &Path) -> Result<(), LinkBinsError>
where
Sys: FsReadToString + FsReadHead + FsWrite + FsSetExecutable + FsEnsureExecutableBits,
{ /* ... */ }
The provider implements each trait independently, so adding a domain to an existing Sys is one more impl X for Host block — no churn on the production type beyond the new line, and no churn on existing tests beyond the ones whose fakes now need the new method.
Sharing a stateful fake across many tests
Reusing a DI fake across tests is optional — most tests read clearest with the fake declared inline in the #[test] body (principle 3). When a fake is shared across a whole module, a module-local macro_rules! that expands the fn-local state at the top of each test body saves repeating the same thread_local! / static block and capability impls in every test. login_fake! in crates/auth-commands/src/login/support.rs is the reference implementation; poll_fake!, browser_fake!, fake_env!, recording_reporter!, and recording_browser! across the test suites follow the same shape.
The rule bites only in that shared case, and only for items that not every test drives: do not emit them all and silence the unused ones with #[allow(dead_code)]. That suppression is noise, and it can mask an item that has become genuinely dead. Declare each optional item's existence through a named macro argument instead — emit only the always-used core (such as the state block and reset) unconditionally, and generate each optional item only when a test names it. What counts as optional is fake-specific: a set_* or query helper, a reporter, or even a capability-impl provider that some tests don't drive — web_auth_fake! gates its FakeHost this way. The entry arm takes the item names as a variadic list and forwards each to an internal (@helper $name) arm; each test invokes the macro naming exactly the items it drives, so every emitted item is used:
macro_rules! poll_fake {
($($helper:ident),* $(,)?) => {
// this fake's always-used core: fn-local state, capability impls, reset()
$( poll_fake!(@helper $helper); )*
};
(@helper set_sleep_behavior) => { /* fn set_sleep_behavior(...) { ... } */ };
(@helper recorded_sleeps) => { /* fn recorded_sleeps() -> Vec<u64> { ... } */ };
(@helper $unknown:ident) => {
compile_error!(concat!(
"unknown `poll_fake!` helper `", stringify!($unknown),
"`; expected one of: set_sleep_behavior, recorded_sleeps",
));
};
}
poll_fake!(); // core only
poll_fake!(recorded_sleeps); // + one helper
poll_fake!(set_sleep_behavior, recorded_sleeps); // + both
Always include the catch-all (@helper $unknown:ident) arm. Without it a mistyped helper name expands to an internal macro call with no matching arm, and the compiler points at the macro internals; the compile_error! turns the same typo into a clear message that lists the valid helper names.
Reporter / log events
Pacquet and the TypeScript pnpm CLI share one reporter wire protocol — the NDJSON stream that @pnpm/cli.default-reporter parses. Every channel one stack fires must fire from the corresponding site in the other, with the same payload shape and the same firing cadence. The reporter lives in crates/reporter (the Reporter capability trait, the LogEvent enum, the NdjsonReporter and SilentReporter sinks); this section is the convention for keeping pacquet's emissions in step with that shared contract.
Where the events come from
In the TypeScript stack, log events come from one of three call shapes — worth knowing when you cross-reference it to keep an emit in sync:
globalLogger.<channel>.<level>(...)— the ergonomic helpers incore/core-loggers/src/<channel>Logger.ts.logger.<level>({ name: 'pnpm:<channel>', ... })— the raw logger with a name discriminant.- Direct
streamParser.write(...)calls — only in the reporter internals; pacquet wouldn't write through this.
The emit usually sits immediately before or after the side effect the event describes — e.g., progressLogger.debug({ status: 'fetched' }) after the tarball finishes downloading, before the import step starts.
Channel mapping
The channels pacquet currently emits live in crates/reporter/src/lib.rs's LogEvent enum. Each variant pins #[serde(rename = "pnpm:<channel>")] so the wire string matches the protocol byte-for-byte. The enum is not an exhaustive list of every channel — variants are added as pacquet starts emitting them. When a behavior change needs a channel LogEvent doesn't yet have, add the variant first (see "To add a new channel" below). The Stage and Progress channels span the install lifecycle and fire from multiple sites; the rest each fire from a single canonical emit site.
To add a new channel: extend the enum with a #[serde(rename = "pnpm:<channel>")] variant whose payload matches the wire shape field-for-field — camelCase via #[serde(rename_all = "camelCase")] where applicable, preserving status-tagged-union shapes (see ProgressMessage for the pattern). Add a wire-shape unit test to crates/reporter/src/tests.rs that asserts the JSON renders exactly what the protocol requires, identical to the TypeScript emitter's output.
Threading the reporter
A function that emits is generic over R: Reporter. Inside, it calls R::emit(...) with a LogEvent whose variant matches the channel — R::emit(&LogEvent::Stage(...)), R::emit(&LogEvent::Context(...)), etc.
fn install_step<R: Reporter>(prefix: String) {
R::emit(&LogEvent::Stage(StageLog {
level: LogLevel::Debug,
prefix,
stage: Stage::ImportingStarted,
}));
// ...
}
Production callers turbofish at the entry point:
Install { /* ... */ }.run::<NdjsonReporter>().await
Tests use the no-op SilentReporter when they don't care about emits, or a recording fake when they do (see Testing below).
The generic monomorphises away — there's no runtime cost. The ergonomic cost is one <R: Reporter> per intermediate fn and one turbofish at the production entry point. When threading reaches into a struct, add <R: Reporter> to the impl method or carry an install-scoped state field that the relevant emit depends on (see link_file::log_method_once's &AtomicU8 parameter for an example of the latter — the function dedupes per-install rather than per-process).
Where to put the emit
Put the emit at the same position relative to side effects in both stacks. The reporter expects events in a specific order (resolved before fetched, importing_started before any per-package events, etc.). Emitting in the wrong order makes the reporter render the "X/Y resolved" counter incorrectly or skip animations entirely.
// `pnpm:context` carries the directories the reporter prints in
// the install header. Emitted once, before the per-package events.
R::emit(&LogEvent::Context(ContextLog {
level: LogLevel::Debug,
current_lockfile_exists: false,
store_dir: config.store_dir.display().to_string(),
virtual_store_dir: config.virtual_store_dir.to_string_lossy().into_owned(),
}));
Testing the emit
Use a recording-fake reporter: a unit-struct declared inside the #[test] body, recording into a static Mutex<Vec<LogEvent>> declared in the same body. Assert the captured sequence. The unit struct keeps the fake's reach narrow (one test fn) and the static mutex makes the recorded events available to the assertion without threading a handle through the fn under test.
#[tokio::test]
async fn install_emits_pnpm_event_sequence() {
static EVENTS: Mutex<Vec<LogEvent>> = Mutex::new(Vec::new());
EVENTS.lock().unwrap().clear();
struct RecordingReporter;
impl Reporter for RecordingReporter {
fn emit(event: &LogEvent) {
EVENTS.lock().unwrap().push(event.clone());
}
}
// ... drive the function under test with `::<RecordingReporter>()` ...
let captured = EVENTS.lock().unwrap();
assert!(matches!(
captured.as_slice(),
[
LogEvent::Context(_),
LogEvent::Stage(StageLog { stage: Stage::ImportingStarted, .. }),
LogEvent::Stage(StageLog { stage: Stage::ImportingDone, .. }),
LogEvent::Summary(_),
]
), "unexpected event sequence: {captured:?}");
}
The static lives in the test function's own scope, so other tests have independent buffers and never race on it. Reset it at the start of the test anyway, in case the same test is re-run within the same process (nextest does this on retry).
Verify the test catches a regression: temporarily comment out the emit, run the test, observe the sequence assertion fail, then restore the emit. Same discipline plans/TEST_PORTING.md calls for.
What not to do
- Don't reformat the wire messages. Field names and string values are part of the wire contract — change them and
@pnpm/cli.default-reportersilently drops the record. - Don't invent new channels. The channel set is shared; it expands only when both stacks add a channel together.
- Don't emit at higher granularity than the reporter expects. Throttling and size gates exist for a reason — see
pnpm-tarball'sfetch_and_extract_once, which gatespnpm:fetching-progress in_progresson a knownContent-Lengthand>= 5 MB(BIG_TARBALL_SIZE), then throttles to 500ms with leading and trailing edges. - Don't emit at lower granularity, either. Skipping events the consumer expects (
fetchedafter a download succeeds,importedaftercreate_cas_filesOk) breaks pnpm's reporter counters.
Worked example: pnpm:summary in Install::run
The Install::run function brackets the install with pnpm:stage events and emits pnpm:summary after import completes — once per importer, after its link phase finishes.
In pacquet, the same emit lives at the bottom of Install::run:
R::emit(&LogEvent::Stage(StageLog {
level: LogLevel::Debug,
prefix: prefix.clone(),
stage: Stage::ImportingDone,
}));
// `pnpm:summary` closes the install and lets the reporter render
// the accumulated `pnpm:root` events as a "+N -M" block. Must
// come after `importing_done`.
R::emit(&LogEvent::Summary(SummaryLog { level: LogLevel::Debug, prefix }));
The recording-fake test that pins the sequence lives in crates/package-manager/src/install/tests.rs::install_emits_pnpm_event_sequence. The same pattern carries over to every other channel — only the variant name and payload shape change.