install: stop hoisting a dependency cycle forever

src/install/lockfile/Tree.rs

@@ -649,6 +649,29 @@
             return Ok(());
         }
 
+        // A copy of a package nested somewhere below another copy of itself only exists
+        // because a different version of its name shadows the copy above; its
+        // dependencies were all placed when that copy was processed. Laying them out again
+        // here would nest whichever of them the levels in between shadow, and in a cycle
+        // through two versions of the same names (a@1 -> b@1 -> a@2 -> b@2 -> a@1 ...)
+        // that shadows the next one in turn, so the copies never end. This is the point at
+        // which npm links back to the copy above; the copy stays as is here, and its
+        // dependencies resolve to whatever is on the path. bun.lock skips these copies when
+        // it binds dependencies from the tree, so this check and that one have to agree.
+        {
+            let trees = builder.list.items_tree();
+            let mut ancestor_id = self.id;
+            while ancestor_id != INVALID_ID {
+                let ancestor = &trees[ancestor_id as usize];
+                if (ancestor.dependency_id as usize) < builder.resolutions.len()
+                    && builder.resolutions[ancestor.dependency_id as usize] == parent_pkg_id
+                {
+                    return Ok(());
+                }
+                ancestor_id = ancestor.parent;
+            }
+        }
+
         builder.list.append(BuilderEntry {
             tree: Tree {
                 parent: self.id,
@@ -1070,7 +1093,7 @@
                     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),
@@ -1106,6 +1129,18 @@
             }
         }
 
+        // The package this node_modules belongs to is itself resolvable from everything
+        // inside it. The walk stops at a bundled root before reaching the folder that holds
+        // it, so a bundled dependency depending back on the package bundling it would copy
+        // that package into its own node_modules, and processing the copy bundles the
+        // dependency again, without end.
+        if (this.dependency_id as usize) < deps.len()
+            && builder.resolutions[this.dependency_id as usize] == package_id
+            && deps[this.dependency_id as usize].name_hash == target_name_hash
+        {
+            return Ok(HoistDependencyResult::Hoisted); // 1
+        }
+
         // place the dependency in the current tree
         Ok(HoistDependencyResult::Placement(Placement {
             id: this.id,

src/install/lockfile/bun.lock.rs

@@ -1817,6 +1817,23 @@ impl<T> PkgMap<T> {
     }
 }
 
+impl PkgMap<PackageID> {
+    /// Whether one of the folders `pkg_path` is nested in holds `pkg_id` as well.
+    /// Every folder on the way down to a package is a key of its own; a prefix cut
+    /// inside a scoped name is not a key and does not match anything.
+    fn is_below_copy_of(&self, pkg_path: &[u8], pkg_id: PackageID) -> bool {
+        let mut end: usize = 0;
+        while let Some(i) = strings::index_of_char_usize(&pkg_path[end..], b'/') {
+            end += i;
+            if self.get(&pkg_path[..end]) == Some(&pkg_id) {
+                return true;
+            }
+            end += 1;
+        }
+        false
+    }
+}
+
 // const PkgMap = struct {};
 
 fn object_rows(expr: &Expr) -> &[JSON::E::PropertyJSON] {
@@ -3230,6 +3247,14 @@ pub(crate) fn parse_into_binary_lockfile(
                 continue;
             }
 
+            // `Tree::process_subtree` gives a copy nested below another copy of the same
+            // package no node_modules of its own, so from this path its dependencies walk
+            // to whatever versions the copy happens to sit under. The copy above, which the
+            // tree was built from, binds them; this row is only a folder.
+            if pkg_map.is_below_copy_of(pkg_path, pkg_id) {
+                continue;
+            }
+
             // find resolutions. iterate up to root through the pkg path.
             let deps = pkg_deps[pkg_id as usize];
             'deps: for _dep_id in deps.begin()..deps.end() {

test/cli/install/hoist.test.ts

@@ -1,5 +1,6 @@
-import { afterAll, beforeAll, test } from "bun:test";
-import { VerdaccioRegistry, bunEnv, runBunInstall } from "harness";
+import { afterAll, beforeAll, expect, test } from "bun:test";
+import { VerdaccioRegistry, bunEnv, bunExe, runBunInstall } from "harness";
+import { join } from "node:path";
 
 const registry = new VerdaccioRegistry();
 
@@ -28,3 +29,188 @@
   // this shouldn't hit an assertion
   await runBunInstall(bunEnv, packageDir);
 });
+
+// The hoist-*-cycle-* fixtures are described in registry/packages/create-hoist-cycle-packages.ts.
+//
+// These graphs can only be laid out by nesting: every package in the cycle conflicts with the
+// version of its name one level up, so each copy used to get another copy nested below it and
+// `bun install` never finished. The tree now ends at the first copy of a package that is nested
+// below another copy of itself; that copy gets no node_modules of its own, so its dependency in
+// the cycle resolves to the conflicting version next to it (no finite layout of these graphs avoids
+// that). The trees below are what that produces, keyed the way bun.lock keys `packages`.
+//
+// bun.lock has to load back into the same tree. That copy's row resolves its dependencies to the
+// wrong versions by path, so the loader has to bind the package from the copy above it instead; the
+// reload half of each test fails if it does not, without --frozen-lockfile noticing.
+
+type Linker = "hoisted" | "isolated";
+
+async function install(cwd: string, ...args: string[]) {
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "install", ...args],
+    cwd,
+    env: bunEnv,
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  expect({ args, stdout, stderr, exitCode }).toMatchObject({
+    stderr: expect.not.stringContaining("error:"),
+    exitCode: 0,
+  });
+}
+
+async function lockfileTree(dir: string) {
+  const { packages } = Bun.JSONC.parse(await Bun.file(join(dir, "bun.lock")).text()) as {
+    packages: Record<string, [string, ...unknown[]]>;
+  };
+  return Object.fromEntries(Object.entries(packages).map(([path, [resolution]]) => [path, resolution]));
+}
+
+async function installedPackageJsons(dir: string) {
+  const nodeModules = join(dir, "node_modules");
+  return (await Array.fromAsync(new Bun.Glob("**/package.json").scan({ cwd: nodeModules, dot: true }))).sort();
+}
+
+// Installs `files` from scratch, then again in a new directory from the bun.lock that produced,
+// and returns the first install's directory.
+async function installFreshAndFromLockfile(
+  files: Record<string, string>,
+  linker: Linker,
+  expectedTree: Record<string, string>,
+) {
+  const { packageDir } = await registry.createTestDir({ bunfigOpts: { linker }, files });
+  await install(packageDir);
+  const lockfile = await Bun.file(join(packageDir, "bun.lock")).text();
+  expect(await lockfileTree(packageDir)).toEqual(expectedTree);
+
+  const { packageDir: reloadDir } = await registry.createTestDir({
+    bunfigOpts: { linker },
+    files: { ...files, "bun.lock": lockfile },
+  });
+  await install(reloadDir, "--frozen-lockfile");
+  expect(await installedPackageJsons(reloadDir)).toEqual(await installedPackageJsons(packageDir));
+
+  // --lockfile-only always writes, so this is the tree a reload builds, printed back.
+  await install(reloadDir, "--lockfile-only");
+  expect(await Bun.file(join(reloadDir, "bun.lock")).text()).toBe(lockfile);
+
+  return packageDir;
+}
+
+test.each(["hoisted", "isolated"] as const)(
+  "a dependency cycle through two versions of the same packages is installed (%s linker)",
+  async linker => {
+    const packageDir = await installFreshAndFromLockfile(
+      { "package.json": JSON.stringify({ name: "pkg", dependencies: { "hoist-cycle-x": "1.0.0" } }) },
+      linker,
+      {
+        "hoist-cycle-x": "hoist-cycle-x@1.0.0",
+        "hoist-cycle-y": "hoist-cycle-y@1.0.0",
+        "hoist-cycle-y/hoist-cycle-x": "hoist-cycle-x@2.0.0",
+        "hoist-cycle-y/hoist-cycle-x/hoist-cycle-y": "hoist-cycle-y@2.0.0",
+        "hoist-cycle-y/hoist-cycle-x/hoist-cycle-y/hoist-cycle-x": "hoist-cycle-x@1.0.0",
+        // This y@1.0.0 is below the y@1.0.0 at the root, so it gets no node_modules of its own.
+        "hoist-cycle-y/hoist-cycle-x/hoist-cycle-y/hoist-cycle-x/hoist-cycle-y": "hoist-cycle-y@1.0.0",
+      },
+    );
+
+    if (linker === "hoisted") {
+      expect(await installedPackageJsons(packageDir)).toEqual([
+        "hoist-cycle-x/package.json",
+        "hoist-cycle-y/node_modules/hoist-cycle-x/node_modules/hoist-cycle-y/node_modules/hoist-cycle-x/node_modules/hoist-cycle-y/package.json",
+        "hoist-cycle-y/node_modules/hoist-cycle-x/node_modules/hoist-cycle-y/node_modules/hoist-cycle-x/package.json",
+        "hoist-cycle-y/node_modules/hoist-cycle-x/node_modules/hoist-cycle-y/package.json",
+        "hoist-cycle-y/node_modules/hoist-cycle-x/package.json",
+        "hoist-cycle-y/package.json",
+      ]);
+    }
+  },
+);
+
+test("a cycle entered at both of its versions, from the root and from a workspace, is installed", async () => {
+  await installFreshAndFromLockfile(
+    {
+      "package.json": JSON.stringify({
+        name: "pkg",
+        workspaces: ["packages/*"],
+        dependencies: { "hoist-cycle-x": "1.0.0" },
+      }),
+      "packages/app/package.json": JSON.stringify({
+        name: "app",
+        version: "1.0.0",
+        dependencies: { "hoist-cycle-x": "2.0.0" },
+      }),
+    },
+    "hoisted",
+    {
+      "app": "app@workspace:packages/app",
+      "hoist-cycle-x": "hoist-cycle-x@1.0.0",
+      "hoist-cycle-y": "hoist-cycle-y@1.0.0",
+      "hoist-cycle-y/hoist-cycle-x": "hoist-cycle-x@2.0.0",
+      "hoist-cycle-y/hoist-cycle-x/hoist-cycle-y": "hoist-cycle-y@2.0.0",
+      "hoist-cycle-y/hoist-cycle-x/hoist-cycle-y/hoist-cycle-x": "hoist-cycle-x@1.0.0",
+      "hoist-cycle-y/hoist-cycle-x/hoist-cycle-y/hoist-cycle-x/hoist-cycle-y": "hoist-cycle-y@1.0.0",
+      // The workspace's branch enters the cycle at x@2.0.0, so it is x@2.0.0's copy that ends it.
+      "app/hoist-cycle-x": "hoist-cycle-x@2.0.0",
+      "app/hoist-cycle-x/hoist-cycle-y": "hoist-cycle-y@2.0.0",
+      "app/hoist-cycle-x/hoist-cycle-y/hoist-cycle-x": "hoist-cycle-x@1.0.0",
+      "app/hoist-cycle-x/hoist-cycle-y/hoist-cycle-x/hoist-cycle-y": "hoist-cycle-y@1.0.0",
+      "app/hoist-cycle-x/hoist-cycle-y/hoist-cycle-x/hoist-cycle-y/hoist-cycle-x": "hoist-cycle-x@2.0.0",
+    },
+  );
+});
+
+test.each(["hoisted", "isolated"] as const)(
+  "a bundled dependency with a peer on the package bundling it is installed (%s linker)",
+  async linker => {
+    // The plugin's peer is satisfied by the host it is bundled in. The host used to be copied into
+    // its own node_modules instead, and the copy bundled the plugin again below it, and so on.
+    await installFreshAndFromLockfile(
+      { "package.json": JSON.stringify({ name: "pkg", dependencies: { "hoist-bundled-cycle-host": "1.0.0" } }) },
+      linker,
+      {
+        "hoist-bundled-cycle-host": "hoist-bundled-cycle-host@1.0.0",
+        "hoist-bundled-cycle-host/hoist-bundled-cycle-plugin": "hoist-bundled-cycle-plugin@1.0.0",
+      },
+    );
+  },
+);
+
+test("a cycle closed through an optional peer is installed", async () => {
+  await installFreshAndFromLockfile(
+    {
+      "package.json": JSON.stringify({
+        name: "pkg",
+        dependencies: { "hoist-optional-peer-cycle-x": "2.0.0" },
+        devDependencies: { "hoist-optional-peer-cycle-entry": "1.0.0" },
+      }),
+    },
+    "hoisted",
+    {
+      "hoist-optional-peer-cycle-entry": "hoist-optional-peer-cycle-entry@1.0.0",
+      "hoist-optional-peer-cycle-x": "hoist-optional-peer-cycle-x@2.0.0",
+      "hoist-optional-peer-cycle-y": "hoist-optional-peer-cycle-y@2.0.0",
+      "hoist-optional-peer-cycle-z": "hoist-optional-peer-cycle-z@2.0.0",
+      "hoist-optional-peer-cycle-entry/hoist-optional-peer-cycle-x": "hoist-optional-peer-cycle-x@1.0.0",
+      "hoist-optional-peer-cycle-entry/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-y":
+        "hoist-optional-peer-cycle-y@1.0.0",
+      "hoist-optional-peer-cycle-entry/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-z":
+        "hoist-optional-peer-cycle-z@1.0.0",
+      // y@1.0.0's peer on x@2.0.0, nested because x@1.0.0 is above it.
+      "hoist-optional-peer-cycle-entry/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-y/hoist-optional-peer-cycle-x":
+        "hoist-optional-peer-cycle-x@2.0.0",
+      // z@1.0.0's y@2.0.0, whose optional peer binds to the x@1.0.0 above it here.
+      "hoist-optional-peer-cycle-entry/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-z/hoist-optional-peer-cycle-y":
+        "hoist-optional-peer-cycle-y@2.0.0",
+      "hoist-optional-peer-cycle-entry/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-y/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-y":
+        "hoist-optional-peer-cycle-y@2.0.0",
+      "hoist-optional-peer-cycle-entry/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-y/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-z":
+        "hoist-optional-peer-cycle-z@2.0.0",
+      // That y@2.0.0 sits below x@2.0.0, so the bound x@1.0.0 is nested under it; this x@1.0.0 is
+      // below the x@1.0.0 under `entry` and is where the tree ends.
+      "hoist-optional-peer-cycle-entry/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-y/hoist-optional-peer-cycle-x/hoist-optional-peer-cycle-y/hoist-optional-peer-cycle-x":
+        "hoist-optional-peer-cycle-x@1.0.0",
+    },
+  );
+});