install: collapse a workspace's same-name dependency slots into one entry so --frozen-lockfile is stable

src/install/PackageManager/add_catalog.rs

@@ -618,7 +618,7 @@ fn keys_named(package_json: &Expr, dependency_list: &[u8], name: &[u8]) -> usize
         })
 }
 
-/// Runs between resolution and `clean_with_logger`, which would otherwise report a nameless positional colliding with the target's own row for the same name as a dependency loop.
+/// Runs between resolution and `clean_with_logger`, whose hoist would otherwise silently collapse a nameless positional onto the target's own row for the same name, keeping whichever of the two it places first.
 pub(crate) fn refuse_declared_positionals(manager: &PackageManager) {
     let Some(flag) = manager.options.add_catalog else {
         return;

src/install/error.rs

@@ -200,8 +200,6 @@ pub enum Error {
     LockfileValidationFailedInvalidPackageScripts,
     #[error("InvalidNPMLockfile")]
     InvalidNPMLockfile,
-    #[error("DependencyLoop")]
-    DependencyLoop,
     #[error("NotSupported")]
     NotSupported,
     #[error("Unexpected")]
@@ -371,7 +369,6 @@ impl Error {
                 "Lockfile validation failed: invalid package scripts"
             }
             Self::InvalidNPMLockfile => "InvalidNPMLockfile",
-            Self::DependencyLoop => "DependencyLoop",
             Self::NotSupported => "NotSupported",
             Self::Unexpected => "Unexpected",
             Self::NotSameFileSystem => "NotSameFileSystem",
@@ -431,7 +428,6 @@ impl From<crate::lockfile_real::tree::SubtreeError> for Error {
         use crate::lockfile_real::tree::SubtreeError as E;
         match e {
             E::OutOfMemory => Self::Alloc(bun_alloc::AllocError),
-            E::DependencyLoop => Self::DependencyLoop,
         }
     }
 }
@@ -452,7 +448,6 @@ impl From<crate::pnpm::MigratePnpmLockfileError> for Error {
         use crate::pnpm::MigratePnpmLockfileError as E;
         match e {
             E::OutOfMemory => Self::Alloc(bun_alloc::AllocError),
-            E::DependencyLoop => Self::DependencyLoop,
             _ => Self::InvalidLockfile,
         }
     }

src/install/lockfile/Tree.rs

@@ -155,8 +155,6 @@ pub(crate) struct Placement {
 pub enum SubtreeError {
     #[error("OutOfMemory")]
     OutOfMemory,
-    #[error("DependencyLoop")]
-    DependencyLoop,
 }
 
 bun_core::oom_from_alloc!(SubtreeError);
@@ -776,8 +774,9 @@ impl Tree {
                             hoist_root_id,
                             pkg_id,
                             dep_id,
+                            resolution_list,
                             builder,
-                        )?;
+                        );
                     }
 
                     // skip unresolvable dependencies
@@ -810,8 +809,9 @@ impl Tree {
                     hoist_root_id,
                     pkg_id,
                     dep_id,
+                    resolution_list,
                     builder,
-                )?
+                )
             };
 
             match hoisted {
@@ -949,30 +949,22 @@ impl Tree {
         hoist_root_id: Id,
         package_id: PackageID,
         input_dep_id: DependencyID,
+        input_dep_range: DependencyIDSlice,
         builder: &mut Builder<'_, METHOD>,
-    ) -> Result<HoistDependencyResult, SubtreeError> {
+    ) -> HoistDependencyResult {
         // Copy the slice ref out of `builder` so subsequent `&mut builder` does not conflict.
         let deps: &[Dependency] = builder.dependencies;
         let dependency: &Dependency = &deps[input_dep_id as usize];
 
         // Tree is Copy — snapshot the fields we need so we don't hold a borrow of builder.list.
         let this: Tree = builder.list.items_tree()[self_id as usize];
-        // Hoist the dep-id slice once.
-        // `builder.list` is not mutated for the duration of this loop (the recursive call happens
-        // *after* it), so the slice is stable; detach to raw ptr/len so the loop body can freely
-        // take `&builder` / `&mut builder.log` without borrowck re-deriving the view per iteration.
-        let (this_deps_ptr, this_deps_len): (*const DependencyID, usize) = {
-            let s = this
-                .dependencies
-                .get(builder.list.items_dependencies()[self_id as usize].as_slice());
-            (s.as_ptr(), s.len())
-        };
+        // The loop body only reads through `builder`; the recursive call comes after the loop.
+        let this_deps: &[DependencyID] = this
+            .dependencies
+            .get(builder.list.items_dependencies()[self_id as usize].as_slice());
         // Keep the comparand in a register; `deps.get_unchecked` may alias `dependency`.
         let target_name_hash = dependency.name_hash;
-        for i in 0..this_deps_len {
-            // SAFETY: `i < this_deps_len` and `builder.list` is not mutated until after this loop
-            // (see invariant above), so `this_deps_ptr[0..this_deps_len)` remains valid.
-            let dep_id: DependencyID = unsafe { *this_deps_ptr.add(i) };
+        for &dep_id in this_deps {
             // SAFETY: `dep_id` was produced by the same lockfile that produced `deps`,
             // so it is always in bounds.
             let dep = unsafe { deps.get_unchecked(dep_id as usize) };
@@ -987,36 +979,32 @@ impl Tree {
                 debug_assert!(dependency.behavior.is_optional_peer());
                 // both optional peers will need to be resolved if they can resolve later.
                 // remember input package_id and dependency for later
-                return Ok(HoistDependencyResult::ResolveLater);
+                return HoistDependencyResult::ResolveLater;
             }
 
             if res_id == invalid_package_id {
                 debug_assert!(dep.behavior.is_optional_peer());
-                return Ok(HoistDependencyResult::ResolveReplace(ResolveReplace {
+                return HoistDependencyResult::ResolveReplace(ResolveReplace {
                     id: this.id,
                     dep_id,
-                }));
+                });
             }
 
             if package_id == invalid_package_id {
                 debug_assert!(dependency.behavior.is_optional_peer());
                 debug_assert!(res_id != invalid_package_id);
                 // resolve optional peer to `builder.resolutions[dep_id]`
-                return Ok(HoistDependencyResult::Resolve(res_id)); // 1
+                return HoistDependencyResult::Resolve(res_id); // 1
             }
 
             if res_id == package_id {
                 // this dependency is the same package as the other, hoist
-                return Ok(HoistDependencyResult::Hoisted); // 1
+                return HoistDependencyResult::Hoisted; // 1
             }
 
-            if AS_DEFINED {
-                if dep.behavior.is_dev() != dependency.behavior.is_dev() {
-                    // will only happen in workspaces and root package because
-                    // dev dependencies won't be included in other types of
-                    // dependencies
-                    return Ok(HoistDependencyResult::Hoisted); // 1
-                }
+            if input_dep_range.contains(dep_id) {
+                // same package lists this name in another dependency group
+                return HoistDependencyResult::Hoisted; // 1
             }
 
             // now we either keep the dependency at this place in the tree,
@@ -1044,74 +1032,41 @@ impl Tree {
                     if resolution.tag == crate::resolution::Tag::Npm
                         && version.satisfies(resolution.npm().version, builder.buf(), builder.buf())
                     {
-                        return Ok(dedupe()); // 1
+                        return dedupe(); // 1
                     }
                 }
 
                 // Root dependencies are manually chosen by the user. Allow them
                 // to hoist other peers even if they don't satisfy the version
                 if builder.lockfile().is_workspace_root_dependency(dep_id) {
                     // TODO: warning about peer dependency version mismatch
-                    return Ok(dedupe()); // 1
+                    return dedupe(); // 1
                 }
             }
 
-            if AS_DEFINED && !dep.behavior.is_peer() {
-                // reshaped for borrowck — `maybe_report_error` takes
-                // `&mut self` but the format args borrow `&self` (via
-                // `package_name`/`package_version`/`buf`). Inline against split
-                // field borrows: copy the `ParentRef` out so the `&Lockfile` is
-                // not tied to `&builder`, then write to `builder.log`.
-                let lockfile_ref = builder.lockfile;
-                let lockfile: &Lockfile = lockfile_ref.get();
-                let buf = lockfile.buffers.string_bytes.as_slice();
-                let names = lockfile.packages.items_name();
-                let resolutions = lockfile.packages.items_resolution();
-                let _ = builder.log.add_error_fmt(
-                    None,
-                    bun_ast::Loc::EMPTY,
-                    format_args!(
-                        "Package \"{}@{}\" has a dependency loop\n  Resolution: \"{}@{}\"\n  Dependency: \"{}@{}\"",
-                        names[package_id as usize].fmt(buf),
-                        resolutions[package_id as usize].fmt(buf, bun_core::fmt::PathSep::Auto),
-                        names[res_id as usize].fmt(buf),
-                        resolutions[res_id as usize].fmt(buf, bun_core::fmt::PathSep::Auto),
-                        dependency.name.fmt(buf),
-                        dependency.version.literal.fmt(buf),
-                    ),
-                );
-                return Err(SubtreeError::DependencyLoop);
-            }
-
-            return Ok(HoistDependencyResult::DependencyLoop); // 3
+            return HoistDependencyResult::DependencyLoop; // 3
         }
 
         // this dependency was not found in this tree, try hoisting or placing in the next parent
         if this.parent != INVALID_ID && this.id != hoist_root_id {
-            let id = match Tree::hoist_dependency::<false, METHOD>(
+            let id = Tree::hoist_dependency::<false, METHOD>(
                 this.parent,
                 hoist_root_id,
                 package_id,
                 input_dep_id,
+                input_dep_range,
                 builder,
-            ) {
-                Ok(id) => id,
-                // SAFETY: `hoist_dependency::<false, _>` never returns `Err` —
-                // the only `Err(SubtreeError::DependencyLoop)` site above is
-                // gated on `AS_DEFINED`. Avoids faulting panic-format pages on
-                // the per-dependency recursion.
-                Err(_) => unsafe { core::hint::unreachable_unchecked() },
-            };
+            );
             if !AS_DEFINED || !matches!(id, HoistDependencyResult::DependencyLoop) {
-                return Ok(id); // 1 or 2
+                return id; // 1 or 2
             }
         }
 
         // place the dependency in the current tree
-        Ok(HoistDependencyResult::Placement(Placement {
+        HoistDependencyResult::Placement(Placement {
             id: this.id,
             bundled: false,
-        })) // 2
+        }) // 2
     }
 }
 

src/install/lockfile/bun.lock.rs

@@ -3287,11 +3287,8 @@ pub(crate) fn parse_into_binary_lockfile(
             }
         }
 
-        if let Err(err) = lockfile.resolve(log) {
-            return Err(match err {
-                tree::SubtreeError::OutOfMemory => ParseError::OutOfMemory,
-                tree::SubtreeError::DependencyLoop => ParseError::InvalidPackagesObject,
-            });
+        if let Err(tree::SubtreeError::OutOfMemory) = lockfile.resolve(log) {
+            return Err(ParseError::OutOfMemory);
         }
     }
 

src/install/pnpm.rs

@@ -366,8 +366,6 @@ pub enum MigratePnpmLockfileError {
     RelativeLinkDependency,
     #[error("WorkspaceNameMissing")]
     WorkspaceNameMissing,
-    #[error("DependencyLoop")]
-    DependencyLoop,
     #[error("PnpmLockfileNotObject")]
     PnpmLockfileNotObject,
     #[error("PnpmLockfileMissingVersion")]
@@ -398,7 +396,6 @@ impl From<crate::Error> for MigratePnpmLockfileError {
         // tags to InvalidPnpmLockfile.
         match e {
             crate::Error::Alloc(bun_alloc::AllocError) => Self::OutOfMemory,
-            crate::Error::DependencyLoop => Self::DependencyLoop,
             _ => Self::InvalidPnpmLockfile,
         }
     }
@@ -409,7 +406,6 @@ impl From<crate::lockfile_real::tree::SubtreeError> for MigratePnpmLockfileError
         use crate::lockfile_real::tree::SubtreeError as E;
         match e {
             E::OutOfMemory => Self::OutOfMemory,
-            E::DependencyLoop => Self::DependencyLoop,
         }
     }
 }

test/cli/install/bun-add-filter.test.ts

@@ -2443,7 +2443,6 @@ test.concurrent.each([
     expect(stderr).toContain(
       `error: --catalog cannot add "${url}": pkg-b already declares no-deps\n  bun add no-deps@${url} --catalog\n`,
     );
-    expect(stderr).not.toContain("dependency loop");
     expect(stderr).not.toContain("returned error");
     expect(exitCode).toBe(1);
 
@@ -2466,7 +2465,6 @@ test.concurrent("add <tarball> --catalog --filter with an existing entry is refu
   expect(stderr).toContain(
     `error: --catalog cannot add "${url}": pkg-b already declares no-deps\n  bun add no-deps@${url} --catalog\n`,
   );
-  expect(stderr).not.toContain("dependency loop");
   expect(exitCode).toBe(1);
 
   expect(await allPackageJsonTexts(dir)).toStrictEqual(before);

test/cli/install/bun-install.test.ts

@@ -7015,6 +7015,102 @@ describe.concurrent("bun-install", () => {
     });
   });
 
+  // https://github.com/oven-sh/bun/issues/19088
+  //
+  // Workspace package.jsons are parsed without the root's duplicate check, so a name listed in
+  // two dependency groups yields two dependency slots. The hoister has to collapse them into one
+  // node_modules entry; the slot sorted first wins (dev, optional, prod, then peer), as it already
+  // did for the root package. `expected` is the `packages` section of bun.lock, name -> resolution.
+  it.each<{
+    name: string;
+    root?: Record<string, Record<string, string>>;
+    pkgA: Record<string, Record<string, string>>;
+    pkgB?: Record<string, Record<string, string>>;
+    expected: Record<string, string>;
+  }>([
+    {
+      name: "dependencies + devDependencies",
+      pkgA: { dependencies: { baz: "0.0.5" }, devDependencies: { baz: "0.0.3" } },
+      expected: { "baz": "baz@0.0.3", "pkg-a": "pkg-a@workspace:packages/pkg-a" },
+    },
+    {
+      name: "dependencies + optionalDependencies",
+      pkgA: { dependencies: { baz: "0.0.5" }, optionalDependencies: { baz: "0.0.3" } },
+      expected: { "baz": "baz@0.0.3", "pkg-a": "pkg-a@workspace:packages/pkg-a" },
+    },
+    {
+      // the root pin keeps both of pkg-a's slots out of the root folder, so they collide inside
+      // pkg-a's own node_modules instead of a parent's
+      name: "dependencies + optionalDependencies while the root pins a third version",
+      root: { dependencies: { baz: "0.0.7" } },
+      pkgA: { dependencies: { baz: "0.0.5" }, optionalDependencies: { baz: "0.0.3" } },
+      expected: { "baz": "baz@0.0.7", "pkg-a": "pkg-a@workspace:packages/pkg-a", "pkg-a/baz": "baz@0.0.3" },
+    },
+    {
+      // pkg-b makes the peer slot resolve to a different package than pkg-a's own dependencies slot
+      name: "dependencies + peerDependencies while a sibling workspace pins the peer's version",
+      pkgA: { dependencies: { baz: "0.0.5" }, peerDependencies: { baz: "0.0.3" } },
+      pkgB: { dependencies: { baz: "0.0.3" } },
+      expected: {
+        "baz": "baz@0.0.5",
+        "pkg-a": "pkg-a@workspace:packages/pkg-a",
+        "pkg-b": "pkg-b@workspace:packages/pkg-b",
+        "pkg-b/baz": "baz@0.0.3",
+      },
+    },
+  ])("--frozen-lockfile passes after a workspace lists a name in $name", async ({ root, pkgA, pkgB, expected }) => {
+    await withContext(defaultOpts, async ctx => {
+      setContextHandler(
+        ctx,
+        dummyRegistryForContext(ctx, [], {
+          "0.0.3": { as: "0.0.3" },
+          "0.0.5": { as: "0.0.5" },
+          // a third version only has to resolve; there is no baz-0.0.7.tgz fixture
+          "0.0.7": { as: "0.0.5" },
+        }),
+      );
+
+      const files: Record<string, object> = {
+        "bunfig.toml": { install: { cache: false, registry: ctx.registry_url, linker: "hoisted" } },
+        "package.json": { name: "root", private: true, workspaces: ["packages/*"], ...root },
+        "packages/pkg-a/package.json": { name: "pkg-a", version: "1.0.0", ...pkgA },
+      };
+      if (pkgB) files["packages/pkg-b/package.json"] = { name: "pkg-b", version: "1.0.0", ...pkgB };
+      await Promise.all(
+        Object.entries(files).map(([path, contents]) =>
+          write(
+            join(ctx.package_dir, path),
+            path.endsWith(".toml") ? Bun.TOML.stringify(contents) : JSON.stringify(contents),
+          ),
+        ),
+      );
+
+      async function install(...args: string[]) {
+        const proc = spawn({
+          cmd: [bunExe(), "install", ...args],
+          cwd: ctx.package_dir,
+          stdout: "ignore",
+          stdin: "ignore",
+          stderr: "pipe",
+          env,
+        });
+        const [err, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
+        expect(err).not.toContain("error:");
+        expect(exitCode).toBe(0);
+        return await file(join(ctx.package_dir, "bun.lock")).text();
+      }
+
+      const lockfile = await install();
+      const packages = Bun.JSONC.parse(lockfile).packages as Record<string, [string, ...unknown[]]>;
+      expect(Object.fromEntries(Object.entries(packages).map(([name, [resolution]]) => [name, resolution]))).toEqual(
+        expected,
+      );
+
+      expect(await install("--frozen-lockfile")).toBe(lockfile);
+      expect(await install()).toBe(lockfile);
+    });
+  });
+
   it("should handle --frozen-lockfile", async () => {
     await withContext(defaultOpts, async ctx => {
       let urls: string[] = [];