feat(real-hoist): crate skeleton + lockfile-to-HoisterTree wrapper (#438 slice 3a)#448
Conversation
slice 3a) First sub-slice of the Slice 3 hoister port from the umbrella. Lands the IO surface and the pnpm-side wrapper; the inner `@yarnpkg/nm` algorithm is stubbed (returns the input tree unchanged) and replaced in 3b. New crate `pacquet-real-hoist`: - `HoisterTree` / `HoisterResult` / `HoisterDependencyKind` / `HoistingLimits` / `HoistOpts` / `HoistError`, mirroring upstream's `@yarnpkg/nm` types and the pnpm wrapper's option object. - `RcByPtr<T>` wrapper providing identity-hashed `Rc<T>` so children stored in `IndexSet` keep JS `Set<HoisterTree>` semantics (a node added via two parent paths stays shared) without paying the cost of structural hashing. - `hoist(&Lockfile, &HoistOpts)` — ports the wrapper at `installing/linking/real-hoist/src/index.ts`. Builds the HoisterTree from the lockfile (root importer's dependencies/devDependencies/optionalDependencies merged, then recursive descent through `snapshots`), adds workspace importer children, plus `link:` placeholders for `externalDependencies`, runs the inner stub, and post-filters `externalDependencies` out of the top-level result. - `LockfileMissingDependency` error surfaced as a `miette` diagnostic with code `ERR_PNPM_LOCKFILE_MISSING_DEPENDENCY`, matching the upstream error code at https://pnpm.io/errors. Five unit tests pin the wrapper's observable behaviour: - The "broken lockfile" error case is a direct port of pnpm's `installing/linking/real-hoist/test/index.ts` test. - An empty lockfile yields an empty root. - A `root → a → b` lockfile, under the stub, gives `root → a → b` (nothing hoisted). Pinning this means 3b's algorithm replacement is observably correct — the tree shape will change to `root → {a, b}` once real hoisting lands. - A diamond dep (`root → {a, c} → b`) keeps `b` as one identity through both parents (proves the wrapper's dedup cache). - `external_dependencies` are stripped from the result. Upstream references pin `pnpm/pnpm@94240bc0` and `yarnpkg/berry@4287909fa6`.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 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). (9)
📝 WalkthroughWalkthroughAdds a new crate Changespacquet-real-hoist crate implementation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Hoist as hoist()
participant Collector as collect_importer_deps()
participant Builder as build_dep_node()
participant Memo as memo (cache)
participant NM as nm_hoist()
Caller->>Hoist: call hoist(lockfile, opts)
Hoist->>Collector: collect importer deps & inject externals
Collector->>Builder: resolve alias -> build_dep_node()
Builder->>Memo: check/insert placeholder for cycles
Builder->>Builder: collect_snapshot_deps() recursively
Hoist->>NM: nm_hoist(HoisterTree)
NM->>Hoist: return HoisterResult
Hoist->>Caller: return Result (strip external placeholders)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #448 +/- ##
==========================================
+ Coverage 87.04% 87.21% +0.17%
==========================================
Files 96 101 +5
Lines 7246 8075 +829
==========================================
+ Hits 6307 7043 +736
- Misses 939 1032 +93 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Introduces the new pacquet-real-hoist crate as the (currently stubbed) “real hoist” wrapper for nodeLinker: hoisted, translating a Lockfile into a HoisterTree, running a placeholder hoist pass, and returning a HoisterResult. This is positioned as sub-slice 3a of the broader hoisted-linker effort (#438), focusing on IO types + lockfile-to-tree construction and the wrapper’s error surface.
Changes:
- Added new
pacquet-real-hoistcrate with public hoister IO types andhoist(&Lockfile, &HoistOpts)wrapper. - Implemented lockfile importer/snapshot traversal to build the hoister input tree, plus
external_dependenciesplaceholder reservation + stripping. - Added unit tests pinning wrapper behavior and identity-dedup semantics.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/real-hoist/src/lib.rs | New crate implementation: public hoister types, lockfile-to-tree builder, stub hoist conversion, and error type. |
| crates/real-hoist/src/tests.rs | Unit tests covering wrapper behavior, missing-snapshot error surfacing, identity dedup, and external dependency stripping. |
| crates/real-hoist/Cargo.toml | New crate manifest and dependencies. |
| Cargo.toml | Adds pacquet-real-hoist to workspace dependencies. |
| Cargo.lock | Records the new crate in the lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Micro-Benchmark ResultsLinux |
Slice and sub-slice numbering is PR-organisation scaffolding — it's useful in commit messages and PR descriptions but it rots inside committed code the moment the referenced slices land, get renumbered, or are abandoned. Rewrite the relevant doc-comments to describe what the code *is* today rather than where it sits in a multi-PR sequence: - Top-level module doc drops the "This is Sub-slice 3a of #438" intro. - `hoist` doc drops the "Sub-slice 3a pins / later sub-slices replace" framing. - `nm_hoist` stub doc drops "Sub-slice 3b replaces this". - `external_dependencies` / non-root importer comments drop their "umbrella's Slice 10 / Slice 9" pointers. - One test doc rewords "Sub-slice 3b's algorithm replacement is observably correct" into a description of what the test pins without naming the future slice.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/real-hoist/src/lib.rs (2)
334-334: ⚡ Quick winUse
Rc::clone(&existing)for visibility.The guidelines specify preferring
Rc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site. As per coding guidelines: "PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site."♻️ Suggested change
- return Ok(Rc::clone(existing)); + return Ok(Rc::clone(&existing));🤖 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/real-hoist/src/lib.rs` at line 334, The return currently calls Rc::clone(existing) which hides the clone cost; change it to call Rc::clone(&existing) so the reference-counted clone is explicit at the call site—update the return expression that uses Rc::clone and the local variable existing accordingly (i.e., pass &existing to Rc::clone).
497-497: ⚡ Quick winUse
Rc::clone(&existing)for visibility.Same issue as line 334: prefer
Rc::clone(&x)overx.clone()for reference-counted types per coding guidelines.♻️ Suggested change
- return Rc::clone(existing); + return Rc::clone(&existing);🤖 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/real-hoist/src/lib.rs` at line 497, The return statement currently calls Rc::clone(existing); change it to use a reference per project style by calling Rc::clone(&existing) instead; locate the return that returns the cloned Rc for the variable named existing (the same pattern as at the earlier occurrence around line 334) and update that call to pass a reference to existing.
🤖 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.
Nitpick comments:
In `@crates/real-hoist/src/lib.rs`:
- Line 334: The return currently calls Rc::clone(existing) which hides the clone
cost; change it to call Rc::clone(&existing) so the reference-counted clone is
explicit at the call site—update the return expression that uses Rc::clone and
the local variable existing accordingly (i.e., pass &existing to Rc::clone).
- Line 497: The return statement currently calls Rc::clone(existing); change it
to use a reference per project style by calling Rc::clone(&existing) instead;
locate the return that returns the cloned Rc for the variable named existing
(the same pattern as at the earlier occurrence around line 334) and update that
call to pass a reference to existing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e2a397f-1dca-4b14-b0fe-3e56c6a3933c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlcrates/real-hoist/Cargo.tomlcrates/real-hoist/src/lib.rscrates/real-hoist/src/tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/real-hoist/src/tests.rscrates/real-hoist/src/lib.rs
🧠 Learnings (3)
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/real-hoist/src/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In this repo’s Rust tests (run via `cargo nextest`), stdout/stderr from passing tests is captured and is only shown when a test fails. As a result, `eprintln!` (and similar stderr logging) in these tests should generally not be flagged as CI “noise” on the happy path; CI output should only appear for failing tests.
Applied to files:
crates/real-hoist/src/tests.rs
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/real-hoist/src/tests.rscrates/real-hoist/src/lib.rs
🔇 Additional comments (25)
crates/real-hoist/Cargo.toml (1)
1-22: LGTM!Cargo.toml (1)
34-34: LGTM!crates/real-hoist/src/lib.rs (16)
20-27: LGTM!
29-49: LGTM!
51-89: LGTM!
91-105: LGTM!
107-114: LGTM!
116-128: LGTM!
130-149: LGTM!
151-198: LGTM!
211-288: LGTM!
290-322: LGTM!
337-409: LGTM!
411-432: LGTM!
434-439: LGTM!
441-480: LGTM!
499-534: LGTM!
536-537: LGTM!crates/real-hoist/src/tests.rs (7)
1-7: LGTM!
9-38: LGTM!
47-69: LGTM!
74-81: LGTM!
90-127: LGTM!
134-179: LGTM!
186-215: LGTM!
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.46846971504,
"stddev": 0.08756429931353996,
"median": 2.45118976524,
"user": 2.6013509599999995,
"system": 3.398308159999999,
"min": 2.32747669974,
"max": 2.6230195487400003,
"times": [
2.45165180474,
2.37711763074,
2.6230195487400003,
2.53186183274,
2.44216262574,
2.5673126437400002,
2.48444720974,
2.42891942874,
2.32747669974,
2.45072772574
]
},
{
"command": "pacquet@main",
"mean": 2.42491967764,
"stddev": 0.07500880341391536,
"median": 2.43614090374,
"user": 2.5920244599999998,
"system": 3.4000415599999996,
"min": 2.33027050774,
"max": 2.59257235274,
"times": [
2.33027050774,
2.45034985474,
2.59257235274,
2.3777098257400002,
2.44755325474,
2.4484370487400002,
2.33224666074,
2.44130145874,
2.39777546374,
2.43098034874
]
},
{
"command": "pnpm",
"mean": 5.74589620494,
"stddev": 0.1095422989671101,
"median": 5.74484399424,
"user": 8.260028259999999,
"system": 4.267844859999999,
"min": 5.57465318974,
"max": 5.95667776174,
"times": [
5.77470164874,
5.66325593774,
5.57465318974,
5.77170584474,
5.95667776174,
5.87349349874,
5.74123683874,
5.66684232774,
5.74845114974,
5.68794385174
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.68212582268,
"stddev": 0.04910193651149939,
"median": 0.6648381758799999,
"user": 0.3401931,
"system": 1.49230928,
"min": 0.6379466338800001,
"max": 0.8098443508800001,
"times": [
0.8098443508800001,
0.66750053388,
0.65635761588,
0.70117992688,
0.6531812058800001,
0.66217581788,
0.67822766688,
0.69862345588,
0.65622101888,
0.6379466338800001
]
},
{
"command": "pacquet@main",
"mean": 0.68106939128,
"stddev": 0.026362252500029496,
"median": 0.66949407988,
"user": 0.34268,
"system": 1.50263188,
"min": 0.6524222388800001,
"max": 0.72450306588,
"times": [
0.72035244688,
0.66730836788,
0.65670315088,
0.70239793888,
0.66320788488,
0.68848994388,
0.67167979188,
0.72450306588,
0.66362908288,
0.6524222388800001
]
},
{
"command": "pnpm",
"mean": 2.4306578795799996,
"stddev": 0.1282440979739961,
"median": 2.37921015738,
"user": 2.7939405999999996,
"system": 2.1550779799999997,
"min": 2.32425675288,
"max": 2.68230174988,
"times": [
2.55697234688,
2.32578541188,
2.37429146788,
2.68230174988,
2.3435663738800003,
2.32425675288,
2.39830762788,
2.33419144788,
2.58277676988,
2.38412884688
]
}
]
} |
…_exhaustive HoistError (#448 review) Address four Copilot review findings on the new crate: 1. `collect_snapshot_deps` looked up `SnapshotDepRef::Alias` deps under the alias name instead of the resolved target name. The snapshot key for an npm-alias is `<target>@<ver>`, not `<alias>@<ver>`, so any real aliased transitive would have surfaced as `LockfileMissingDependency`. `build_dep_node` now takes the resolved `&PkgNameVerPeer` directly and the caller builds it via `dep_ref.resolve(alias)` for snapshot deps or `PkgNameVerPeer::new(alias, spec.version)` for importer deps. 2. `build_dep_node`'s cycle handling re-allocated the node on the way out (placeholder Rc, recurse, *new* finished Rc) which left any cycle-visiting Rc pointing at the empty placeholder. `HoisterTree::dependencies` is now a `RefCell<IndexSet<...>>` and the construction populates the cell in place — the placeholder and the populated node are the same allocation, so a back-edge reads the eventually-populated set. Same JS `Set<HoisterTree>` mutation semantics the real hoist algorithm needs. 3. `convert` had the same bug and gets the same fix on `HoisterResult::dependencies` and `HoisterResult::references`. The stub's traversal now collects the input children into a Vec before recursing so the borrow on the input cell drops, and populates the output cell in place. 4. `HoistError` is now `#[non_exhaustive]`, matching the rest of pacquet's public error enums. A new regression test (`transitive_npm_alias_resolves_target_snapshot`) pins the fix for (1): the snapshot key the wrapper looks up matches the target package, the node's exposed `name` stays the alias, and `ident_name` / `reference` carry the resolved target's identity. Existing tests updated to take `Ref<'_, …>` via `.borrow()`. All six pass; `just ready`, `cargo doc --document-private-items`, `taplo`, and `just dylint` clean.
| let mut out = String::with_capacity(s.len()); | ||
| for ch in s.chars() { | ||
| match ch { | ||
| 'A'..='Z' | ||
| | 'a'..='z' | ||
| | '0'..='9' | ||
| | '-' | ||
| | '_' | ||
| | '.' | ||
| | '!' | ||
| | '~' | ||
| | '*' | ||
| | '\'' | ||
| | '(' | ||
| | ')' => out.push(ch), | ||
| '/' => out.push_str("%2F"), | ||
| other => { | ||
| // Best-effort %xx encode for the ASCII subset we | ||
| // expect in importer ids. Anything else is left | ||
| // verbatim — pacquet's lockfile doesn't currently | ||
| // hand the wrapper non-ASCII paths. | ||
| if (other as u32) < 0x80 { | ||
| out.push_str(&format!("%{:02X}", other as u32)); | ||
| } else { | ||
| out.push(other); | ||
| } |
After rebasing on top of #448 (new `pacquet-real-hoist` crate), the lockfile-to-HoisterTree wrapper consumed `spec.version` expecting a `PkgVerPeer` — but this PR widens `ResolvedDependencySpec.version` to `ImporterDepVersion` so `link:` (cross-importer `workspace:*`) deps can be represented. `link:` deps don't live in the virtual store, so they have no snapshot to hoist; mirror the same skip that `pacquet-package-manager`'s `build_sequence::collect_root_dep_paths` already does and `continue` past `ImporterDepVersion::Link` values. The matching `resolved_dep` test helper takes a `&str` literal and is unit-test infrastructure, so a `.into()` from `PkgVerPeer` to `ImporterDepVersion::Regular` is enough — no functional change. --- Written by an agent (Claude Code, claude-opus-4-7).
After rebasing on top of #448 (new `pacquet-real-hoist` crate), the lockfile-to-HoisterTree wrapper consumed `spec.version` expecting a `PkgVerPeer` — but this PR widens `ResolvedDependencySpec.version` to `ImporterDepVersion` so `link:` (cross-importer `workspace:*`) deps can be represented. `link:` deps don't live in the virtual store, so they have no snapshot to hoist; mirror the same skip that `pacquet-package-manager`'s `build_sequence::collect_root_dep_paths` already does and `continue` past `ImporterDepVersion::Link` values. The matching `resolved_dep` test helper takes a `&str` literal and is unit-test infrastructure, so a `.into()` from `PkgVerPeer` to `ImporterDepVersion::Regular` is enough — no functional change. --- Written by an agent (Claude Code, claude-opus-4-7).
After rebasing on top of #448 (new `pacquet-real-hoist` crate), the lockfile-to-HoisterTree wrapper consumed `spec.version` expecting a `PkgVerPeer` — but this PR widens `ResolvedDependencySpec.version` to `ImporterDepVersion` so `link:` (cross-importer `workspace:*`) deps can be represented. `link:` deps don't live in the virtual store, so they have no snapshot to hoist; mirror the same skip that `pacquet-package-manager`'s `build_sequence::collect_root_dep_paths` already does and `continue` past `ImporterDepVersion::Link` values. The matching `resolved_dep` test helper takes a `&str` literal and is unit-test infrastructure, so a `.into()` from `PkgVerPeer` to `ImporterDepVersion::Regular` is enough — no functional change. --- Written by an agent (Claude Code, claude-opus-4-7).
After rebasing on top of #448 (new `pacquet-real-hoist` crate), the lockfile-to-HoisterTree wrapper consumed `spec.version` expecting a `PkgVerPeer` — but this PR widens `ResolvedDependencySpec.version` to `ImporterDepVersion` so `link:` (cross-importer `workspace:*`) deps can be represented. `link:` deps don't live in the virtual store, so they have no snapshot to hoist; mirror the same skip that `pacquet-package-manager`'s `build_sequence::collect_root_dep_paths` already does and `continue` past `ImporterDepVersion::Link` values. The matching `resolved_dep` test helper takes a `&str` literal and is unit-test infrastructure, so a `.into()` from `PkgVerPeer` to `ImporterDepVersion::Regular` is enough — no functional change. --- Written by an agent (Claude Code, claude-opus-4-7).
## Summary Replaces the stub `nm_hoist` (landed in #448) with a working hoister and the surrounding guardrails: BFS over the result graph pulling every eligible descendant up to the root, plus upfront refusal of inputs the algorithm doesn't yet model. ### What the algorithm models - **Free hoist.** A transitive dep with no name collision at the root surfaces at the root. - **Identity dedup.** A dep reachable through multiple parents shares one `Rc` thanks to the wrapper's cache; the hoist preserves that identity and strips the duplicate reference at the other parent path. - **Parent-wins on version conflict.** When two distinct deps share an alias but resolve to different snapshot keys, the first BFS visitor takes the root slot and the other stays under its declaring parent. Visit order matches the wrapper's alias-sorted insertion order, so the outcome is deterministic. - **Deep chains flatten in one pass.** `root → a → b → c → d` becomes `root → {a, b, c, d}` — each node, once hoisted, is queued for further descent; its own children evaluate against root's slots rather than against the (now-empty) intermediate parent. - **O(1) root-slot lookup.** A side `HashMap<String, RcByPtr<HoisterResult>>` mirrors root's direct deps, so the per-edge "is this name taken?" check doesn't degrade to O(N²) `IndexSet` scans on flat graphs. ### Fail-fast guards The algorithm doesn't yet model peers, hoisting limits, or multi-importer (workspace) hoist trees. Rather than emit a layout pnpm would reject, the wrapper refuses these inputs upfront with three new `HoistError` variants: - `UnsupportedPeerDependency { ident, peers }` — fires when scanning the constructed `HoisterTree` finds any node with non-empty `peer_names` (either `peerDependencies` from the `packages:` map or `transitive_peer_dependencies` on a `snapshots:` entry). - `UnsupportedHoistingLimits { len }` — non-empty `opts.hoisting_limits`. - `UnsupportedWorkspace { extra_importers }` — any importer beyond the root `.`. Each carries enough context for an operator to identify what triggered it. The `UnsupportedWorkspace` help points at `SymlinkDirectDependencies` — workspaces *do* work in pacquet's wider install path (workspace support landed in #443 for the isolated linker); only the hoister is restricted. ### Rebase pickup `collect_importer_deps` carries the `Link`-variant skip introduced by #443 (workspace support) — `spec.version.as_regular()` extracts the snapshot key for `Regular` deps and `continue`s past `Link` deps, since workspace siblings are direct symlinks materialised by `SymlinkDirectDependencies` and have no snapshot to hoist. ### Performance Nothing in this PR is reachable from `pacquet install` today (`crates/package-manager/src/install*.rs` doesn't import `pacquet-real-hoist` — the crate is dead code from the install pipeline's perspective until the hoisted-linker wiring lands in later slices). The benchmark results bear that out: cold Frozen Lockfile is within CI variance (+3.2% mean, +3.5% median, driven by one outlier), Hot Cache is 6% *faster* (same `stat → lstat` improvement that shipped in slice 1's `import_indexed_dir`), micro is identical. No code path explains a real regression. ## What's deferred - Peer-dependency-aware hoisting (`peer_names` constraints, peer-promise satisfaction). - Multi-round convergence — the BFS handles deep chains in one pass, so the cases requiring true multi-round are limited to peer interactions. - `hoistingLimits` runtime enforcement. - `dependencyKind` distinctions for workspaces and external soft links (today only `ExternalSoftLink` placeholders are added by the wrapper and stripped post-hoist; `Workspace`-kind nodes are blocked by the `UnsupportedWorkspace` guard upfront). ## Upstream reference - [`hoist.ts` algorithm overview](https://github.com/yarnpkg/berry/blob/4287909fa6a0a1ec976a55776bff606864b31990/packages/yarnpkg-nm/sources/hoist.ts#L1-L51) — the recipe pacquet's single-pass BFS approximates. - [`hoistTo` main loop](https://github.com/yarnpkg/berry/blob/4287909fa6a0a1ec976a55776bff606864b31990/packages/yarnpkg-nm/sources/hoist.ts#L329) — the structural intent the port mirrors for the subset above.
Summary
First sub-slice of the Slice 3 hoister port from #438. Lands the IO
surface and the pnpm-side wrapper around the hoist algorithm; the
inner
@yarnpkg/nmalgorithm is stubbed (returns the input treeunchanged) and replaced in 3b.
The umbrella's original Slice 3 — a faithful port of the full
@yarnpkg/nm/hoist.ts(1032 LOC) — is being split into fivesub-slices so each one is reviewable on its own. Sub-slice 3a covers
the wrapper's IO and the construction-time error surface; 3b drops in
the single-round hoist core; 3c adds peer-constraint handling; 3d adds
multi-round convergence + soft-link; 3e wires
hoistingLimits/externalDependenciesknobs through and ports the full upstream testfixture set.
New crate
pacquet-real-hoistMirrors upstream's
installing/linking/real-hoistworkspace package. Exposes:HoisterTree,HoisterResult,HoisterDependencyKind,HoistingLimits,HoistOpts,HoistError— the public surfacethe hoister contract speaks in.
RcByPtr<T>— identity-hashedRc<T>wrapper used as children ofIndexSets so a node added via two parent paths stays shared.Matches JS
Set<HoisterTree>semantics (object-identity dedup)without the cost of structural hashing on deep trees.
hoist(&Lockfile, &HoistOpts) -> Result<HoisterResult, HoistError>— the pnpm wrapper itself. Builds the HoisterTree (root importer's
merged dependencies + devDependencies + optionalDependencies,
recursive descent through
snapshots, pluslink:placeholdersfor
externalDependenciesand workspace-importer children), runsthe inner stub, then post-filters
externalDependenciesout ofthe top-level result.
HoistError::LockfileMissingDependencysurfaces with miette codeERR_PNPM_LOCKFILE_MISSING_DEPENDENCY(pnpm.io/errors) — same code upstream uses.Upstream reference
Ports/cites pinned at
pnpm/pnpm@94240bc0andyarnpkg/berry@4287909fa6:installing/linking/real-hoist/src/index.ts— the wrapper this PR ports.installing/linking/real-hoist/test/index.ts— the upstream test suite (one error-path test ported directly; the fixture-roundtrip test is deferred to 3e since it pins algorithm output).packages/yarnpkg-nm/sources/hoist.ts— the inner algorithm being stubbed for now.Test plan
Five unit tests covering the wrapper's observable behaviour:
hoist_throws_on_broken_lockfile— port of upstream's"hoist throws an error if the lockfile is broken"test.empty_lockfile_yields_empty_root— wrapper's "no rootimporter" branch.
one_transitive_dep_appears_under_its_parent_in_the_stub—pins the current shape so 3b's algorithm replacement is
observably correct (the tree will go from
root → a → btoroot → {a, b}once real hoisting lands).shared_dep_via_two_root_paths_is_one_node— pins the dedupcache; future refactors of the
nodesHashMap won't silentlylose the identity sharing the real algorithm relies on.
external_dependencies_are_stripped_from_the_result.just ready,cargo doc --document-private-items,taplo format --check,just dylintall clean.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Tests