install: bind unbound required peers while hoisting, like optional peers

src/install/lockfile.rs

@@ -1269,7 +1269,7 @@ impl<'a> Cloner<'a> {
 // ────────────────────────────────────────────────────────────────────────────
 
 impl Lockfile {
-    /// Re-hoists while a pass bound an optional peer late; a reload has that binding up front.
+    /// Re-hoists while a pass bound a peer late; a reload has that binding up front.
     pub(crate) fn resolve(&mut self, log: &mut bun_ast::Log) -> Result<(), tree::SubtreeError> {
         while self.hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)? {}
         Ok(())
@@ -1293,7 +1293,7 @@ impl Lockfile {
         Ok(())
     }
 
-    /// Sets `buffers.trees`/`hoisted_dependencies`; returns `Builder::late_bound_optional_peer`.
+    /// Sets `buffers.trees`/`hoisted_dependencies`; returns `Builder::late_bound_peer`.
     pub(crate) fn hoist<const METHOD: tree::BuilderMethod>(
         &mut self,
         log: &mut bun_ast::Log,
@@ -1324,8 +1324,8 @@ impl Lockfile {
             install_root_dependencies,
             workspace_filters,
             packages_to_install,
-            pending_optional_peers: Default::default(),
-            late_bound_optional_peer: false,
+            pending_peers: Default::default(),
+            late_bound_peer: false,
             list: Default::default(),
             sort_buf: Default::default(),
         };
@@ -1344,10 +1344,10 @@ impl Lockfile {
         }
 
         let cleaned = builder.clean()?;
-        let late_bound_optional_peer = builder.late_bound_optional_peer;
+        let late_bound_peer = builder.late_bound_peer;
         self.buffers.trees = cleaned.trees;
         self.buffers.hoisted_dependencies = cleaned.dep_ids;
-        Ok(late_bound_optional_peer)
+        Ok(late_bound_peer)
     }
 }
 

src/install/lockfile/Tree.rs

@@ -436,14 +436,13 @@ pub struct Builder<'a, const METHOD: BuilderMethod> {
     /// overlap; reads go through [`Builder::lockfile()`] which never touches
     /// `buffers.resolutions`.
     pub lockfile: bun_ptr::ParentRef<Lockfile>,
-    // Unresolved optional peers that might resolve later. if they do we will want to assign
+    // Unresolved peers that might resolve later. if they do we will want to assign
     // builder.resolutions[peer.dep_id] to the resolved pkg_id. A dependency ID set is used because there
     // can be multiple instances of the same package in the tree, so the same unresolved dependency ID
     // could be visited multiple times before it's resolved.
-    pub(crate) pending_optional_peers:
-        ArrayHashMap<PackageNameHash, ArrayHashMap<DependencyID, ()>>,
-    /// An optional peer got bound after its dependent was placed; see `Lockfile::resolve`.
-    pub(crate) late_bound_optional_peer: bool,
+    pub(crate) pending_peers: ArrayHashMap<PackageNameHash, ArrayHashMap<DependencyID, ()>>,
+    /// A peer got bound after its dependent was placed; see `Lockfile::resolve`.
+    pub(crate) late_bound_peer: bool,
     pub(crate) manager: Option<&'a PackageManager>,
     pub(crate) sort_buf: Vec<DependencyID>,
     pub(crate) workspace_filters: &'a [WorkspaceFilter],
@@ -518,7 +517,7 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> {
             for &dep_id in child.iter() {
                 let pkg_id = self.resolutions[dep_id as usize];
                 if pkg_id == invalid_package_id {
-                    // optional peers that never resolved
+                    // peers that never resolved
                     continue;
                 }
 
@@ -530,7 +529,7 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> {
             tree.dependencies.len = len;
         }
 
-        // queue / sort_buf / pending_optional_peers freed by Drop; explicit deinit removed.
+        // queue / sort_buf / pending_peers freed by Drop; explicit deinit removed.
         // The sole caller (`Lockfile::hoist`) drops the Builder immediately after clean().
 
         slice.deinit_owned();
@@ -768,7 +767,11 @@ impl Tree {
                 }
 
                 if pkg_id == invalid_package_id {
-                    if dependency.behavior.is_optional_peer() {
+                    // An unbound peer (optional, or one a migration had nothing recorded for)
+                    // binds to the copy next to or above its dependent, which is where loading
+                    // the saved bun.lock binds it (`PkgMap::find_resolution`); the isolated
+                    // store keys the dependent by that binding.
+                    if dependency.behavior.is_peer() {
                         break 'hoisted Tree::hoist_dependency::<true, METHOD>(
                             next_id,
                             hoist_root_id,
@@ -821,14 +824,10 @@ impl Tree {
                     debug_assert!(pkg_id == invalid_package_id);
                     debug_assert!(res_id != invalid_package_id);
                     builder.resolutions[dep_id as usize] = res_id;
-                    debug_assert!(
-                        !builder
-                            .pending_optional_peers
-                            .contains_key(&dependency.name_hash)
-                    );
+                    debug_assert!(!builder.pending_peers.contains_key(&dependency.name_hash));
 
                     if let Some(entry) = builder
-                        .pending_optional_peers
+                        .pending_peers
                         .fetch_swap_remove(&dependency.name_hash)
                     {
                         let peers = entry.1;
@@ -846,10 +845,10 @@ impl Tree {
                 }
                 HoistDependencyResult::ResolveReplace(replace) => {
                     debug_assert!(pkg_id != invalid_package_id);
-                    builder.late_bound_optional_peer = true;
+                    builder.late_bound_peer = true;
                     builder.resolutions[replace.dep_id as usize] = pkg_id;
                     if let Some(entry) = builder
-                        .pending_optional_peers
+                        .pending_peers
                         .fetch_swap_remove(&dependency.name_hash)
                     {
                         let peers = entry.1;
@@ -887,12 +886,10 @@ impl Tree {
                     builder.resolutions[dep_id as usize] = res_id;
                 }
                 HoistDependencyResult::ResolveLater => {
-                    // `dep_id` is an unresolved optional peer. while hoisting it deduplicated
-                    // with another unresolved optional peer. save it so we remember resolve it
+                    // `dep_id` is an unresolved peer. while hoisting it deduplicated
+                    // with another unresolved peer. save it so we remember resolve it
                     // later if it's possible to resolve it.
-                    let entry = builder
-                        .pending_optional_peers
-                        .get_or_put(dependency.name_hash)?;
+                    let entry = builder.pending_peers.get_or_put(dependency.name_hash)?;
                     if !entry.found_existing {
                         *entry.value_ptr = ArrayHashMap::default();
                     }
@@ -974,25 +971,25 @@ impl Tree {
             let res_id = builder.resolutions[dep_id as usize];
 
             if res_id == invalid_package_id && package_id == invalid_package_id {
-                debug_assert!(dep.behavior.is_optional_peer());
-                debug_assert!(dependency.behavior.is_optional_peer());
-                // both optional peers will need to be resolved if they can resolve later.
+                debug_assert!(dep.behavior.is_peer());
+                debug_assert!(dependency.behavior.is_peer());
+                // both peers will need to be resolved if they can resolve later.
                 // remember input package_id and dependency for later
                 return HoistDependencyResult::ResolveLater;
             }
 
             if res_id == invalid_package_id {
-                debug_assert!(dep.behavior.is_optional_peer());
+                debug_assert!(dep.behavior.is_peer());
                 return HoistDependencyResult::ResolveReplace(ResolveReplace {
                     id: this.id,
                     dep_id,
                 });
             }
 
             if package_id == invalid_package_id {
-                debug_assert!(dependency.behavior.is_optional_peer());
+                debug_assert!(dependency.behavior.is_peer());
                 debug_assert!(res_id != invalid_package_id);
-                // resolve optional peer to `builder.resolutions[dep_id]`
+                // resolve peer to `builder.resolutions[dep_id]`
                 return HoistDependencyResult::Resolve(res_id); // 1
             }
 

test/cli/install/migration/pnpm-lock-v9.test.ts

@@ -1663,6 +1663,205 @@ ${variants}`;
       ).toBeTrue();
     });
 
+    describe("a peer pnpm left unmet is bound by the install that migrates", () => {
+      // pnpm records a met peer as a suffix on the dependent's snapshot key; an unmet one (autoInstallPeers
+      // off) has no suffix, so the migration has nothing to bind peer-deps-too's `no-deps` edge to. Loading
+      // the migrated bun.lock binds it to the no-deps hoisted to the root, and the isolated store keys
+      // peer-deps-too by that binding, so the migrating install has to bind it the same way or the next
+      // install re-keys the entry (bun.lock itself is identical either way).
+      const unmetPeerProject = (importer: string, packageJson: Record<string, unknown>) => ({
+        "package.json": JSON.stringify({ name: "unmet-peer", ...packageJson }),
+        "pnpm-lock.yaml": `lockfileVersion: '9.0'
+
+settings:
+  autoInstallPeers: false
+
+importers:
+
+  .:
+${importer}
+
+packages:
+
+  no-deps@1.0.1:
+    resolution: {integrity: ${NO_DEPS_1_0_1_INTEGRITY}}
+
+  one-dep@1.0.0:
+    resolution: {integrity: ${ONE_DEP_1_0_0_INTEGRITY}}
+
+  peer-deps-too@1.0.0:
+    resolution: {integrity: ${PEER_DEPS_TOO_1_0_0_INTEGRITY}}
+    peerDependencies:
+      no-deps: '*'
+
+snapshots:
+
+  no-deps@1.0.1: {}
+
+  one-dep@1.0.0:
+    dependencies:
+      no-deps: 1.0.1
+
+  peer-deps-too@1.0.0: {}
+`,
+      });
+
+      const storeEntries = (dir: string) =>
+        readdirSync(join(dir, "node_modules", ".bun"))
+          .filter(name => name !== "node_modules")
+          .sort();
+
+      // Hoisting places a package's dependencies breadth-first in dependency-group order. With both in
+      // `dependencies`, one-dep's no-deps reaches the root before peer-deps-too's peer is looked at; as a
+      // devDependency peer-deps-too is processed first, and the peer is bound when no-deps arrives later.
+      test.concurrent.each([
+        {
+          group: "dependencies",
+          packageJson: { dependencies: { "one-dep": "1.0.0", "peer-deps-too": "1.0.0" } },
+          importer: `    dependencies:
+      one-dep:
+        specifier: 1.0.0
+        version: 1.0.0
+      peer-deps-too:
+        specifier: 1.0.0
+        version: 1.0.0`,
+        },
+        {
+          group: "devDependencies",
+          packageJson: { dependencies: { "one-dep": "1.0.0" }, devDependencies: { "peer-deps-too": "1.0.0" } },
+          importer: `    dependencies:
+      one-dep:
+        specifier: 1.0.0
+        version: 1.0.0
+    devDependencies:
+      peer-deps-too:
+        specifier: 1.0.0
+        version: 1.0.0`,
+        },
+      ])(
+        "peer-deps-too in $group keys its isolated store entry like a reinstall and like a fresh install",
+        async ({ packageJson, importer }) => {
+          const files = unmetPeerProject(importer, packageJson);
+          const { packageDir } = await verdaccio.createTestDir({ bunfigOpts: { linker: "isolated" }, files });
+
+          const install = await run(packageDir, "install");
+
+          expect(install.stderr).toContain("migrated lockfile from pnpm-lock.yaml");
+          expect(install.exitCode).toBe(0);
+          const migrated = await bunLockOf(packageDir);
+          expect(migrated).toContain(`{ "peerDependencies": { "no-deps": "*" } }`);
+          const store = storeEntries(packageDir);
+          expect(store).toEqual([
+            "no-deps@1.0.1",
+            "one-dep@1.0.0",
+            expect.stringMatching(/^peer-deps-too@1\.0\.0\+[0-9a-f]{16}$/),
+          ]);
+
+          const reinstall = await run(packageDir, "install");
+
+          expect(reinstall.stdout).toContain("(no changes)");
+          expect(reinstall.exitCode).toBe(0);
+          expect(await bunLockOf(packageDir)).toBe(migrated);
+          expect(storeEntries(packageDir)).toEqual(store);
+
+          const { packageDir: fresh } = await verdaccio.createTestDir({
+            bunfigOpts: { linker: "isolated" },
+            files: { "package.json": files["package.json"] },
+          });
+
+          const freshInstall = await run(fresh, "install");
+
+          expect(freshInstall.stderr).toContain("Saved lockfile");
+          expect(freshInstall.exitCode).toBe(0);
+          expect(storeEntries(fresh)).toEqual(store);
+        },
+      );
+
+      // The root's and a workspace's own peers come from package.json and are unbound the same way; the
+      // isolated linker links a bound peer into the importer's node_modules.
+      test.concurrent("the root's and a workspace's own peers are linked by the migrating install", async () => {
+        const manifests = {
+          "package.json": JSON.stringify({
+            name: "importer-peers",
+            workspaces: ["apps/*"],
+            dependencies: { "one-dep": "1.0.0" },
+            peerDependencies: { "no-deps": "*" },
+          }),
+          "apps/a/package.json": JSON.stringify({ name: "a", peerDependencies: { "no-deps": "*" } }),
+        };
+        const { packageDir } = await verdaccio.createTestDir({
+          bunfigOpts: { linker: "isolated" },
+          files: {
+            ...manifests,
+            "pnpm-lock.yaml": `lockfileVersion: '9.0'
+
+settings:
+  autoInstallPeers: false
+
+importers:
+
+  .:
+    dependencies:
+      one-dep:
+        specifier: 1.0.0
+        version: 1.0.0
+
+  apps/a: {}
+
+packages:
+
+  no-deps@1.0.1:
+    resolution: {integrity: ${NO_DEPS_1_0_1_INTEGRITY}}
+
+  one-dep@1.0.0:
+    resolution: {integrity: ${ONE_DEP_1_0_0_INTEGRITY}}
+
+snapshots:
+
+  no-deps@1.0.1: {}
+
+  one-dep@1.0.0:
+    dependencies:
+      no-deps: 1.0.1
+`,
+          },
+        });
+        const linked = (dir: string) => ({
+          root: readdirSync(join(dir, "node_modules"))
+            .filter(name => name !== ".bun")
+            .sort(),
+          a: existsSync(join(dir, "apps/a/node_modules")) ? readdirSync(join(dir, "apps/a/node_modules")).sort() : [],
+        });
+
+        const install = await run(packageDir, "install");
+
+        expect(install.stderr).toContain('skipped peer "no-deps" of the root package');
+        expect(install.stderr).toContain('skipped peer "no-deps" of workspace "apps/a"');
+        expect(install.stderr).toContain("migrated lockfile from pnpm-lock.yaml");
+        expect(install.exitCode).toBe(0);
+        const migrated = await bunLockOf(packageDir);
+        expect(linked(packageDir)).toEqual({ root: ["no-deps", "one-dep"], a: ["no-deps"] });
+
+        const reinstall = await run(packageDir, "install");
+
+        expect(reinstall.stdout).toContain("(no changes)");
+        expect(reinstall.exitCode).toBe(0);
+        expect(await bunLockOf(packageDir)).toBe(migrated);
+        expect(linked(packageDir)).toEqual({ root: ["no-deps", "one-dep"], a: ["no-deps"] });
+
+        const { packageDir: fresh } = await verdaccio.createTestDir({
+          bunfigOpts: { linker: "isolated" },
+          files: manifests,
+        });
+
+        const freshInstall = await run(fresh, "install");
+
+        expect(freshInstall.stderr).toContain("Saved lockfile");
+        expect(freshInstall.exitCode).toBe(0);
+        expect(linked(fresh)).toEqual({ root: ["no-deps", "one-dep"], a: ["no-deps"] });
+      });
+    });
+
     const linkedPeerFiles = {
       "package.json": JSON.stringify({
         name: "v9-linked-peer",