install: only re-resolve rows a package still owns when overrides or catalogs change

src/install/PackageManager/install_with_manager.rs

@@ -523,70 +523,26 @@ pub fn install_with_manager(
                         pinned_rows = enqueue_transitive(manager, &transitive, invalidates_rows)?;
                     }
 
-                    // `enqueueDependencyWithMain` can reach `Lockfile.Package.fromNPM`,
-                    // which grows `buffers.dependencies` and may reallocate it.
-                    // Iterate by index against a snapshot of the original length and
-                    // copy each entry to the stack so neither the loop nor the callee
-                    // ever reads through a pointer into the old backing storage.
-                    if manager.summary.overrides_changed && !all_name_hashes.is_empty() {
-                        let dependencies_len = manager.lockfile.buffers.dependencies.len();
-                        for dependency_i in 0..dependencies_len {
-                            if pinned_rows.is_set_allow_out_of_bound(dependency_i, false) {
-                                continue;
-                            }
-                            let dependency =
-                                manager.lockfile.buffers.dependencies[dependency_i].clone();
-                            if all_name_hashes.binary_search(&dependency.name_hash).is_ok() {
-                                manager.lockfile.buffers.resolutions[dependency_i] =
-                                    invalid_package_id;
-                                if let Err(err) = enqueue_dependency_with_main(
-                                    manager,
-                                    dependency_i as u32,
-                                    &dependency,
-                                    invalid_package_id,
-                                    false,
-                                ) {
-                                    add_dependency_error(manager, &dependency, err);
-                                }
-                            }
-                        }
-                    }
-
-                    if manager.summary.catalogs_changed {
+                    if invalidates_rows {
+                        let catalogs_changed = manager.summary.catalogs_changed;
                         let mut catalog_overridden: Vec<PackageNameHash> = Vec::new();
-                        manager
-                            .lockfile
-                            .overrides
-                            .append_catalog_valued_name_hashes(&mut catalog_overridden);
-                        catalog_overridden.sort_unstable();
-                        catalog_overridden.dedup();
-                        let dependencies_len = manager.lockfile.buffers.dependencies.len();
-                        for _dep_id in 0..dependencies_len {
-                            let dep_id: DependencyID = u32::try_from(_dep_id).expect("int cast");
-                            if pinned_rows.is_set_allow_out_of_bound(_dep_id, false) {
-                                continue;
-                            }
-                            let dep =
-                                manager.lockfile.buffers.dependencies[dep_id as usize].clone();
-                            if dep.version.tag != DependencyVersionTag::Catalog
-                                && (catalog_overridden.is_empty()
-                                    || catalog_overridden.binary_search(&dep.name_hash).is_err())
-                            {
-                                continue;
-                            }
-
-                            manager.lockfile.buffers.resolutions[dep_id as usize] =
-                                invalid_package_id;
-                            if let Err(err) = enqueue_dependency_with_main(
-                                manager,
-                                dep_id,
-                                &dep,
-                                invalid_package_id,
-                                false,
-                            ) {
-                                add_dependency_error(manager, &dep, err);
-                            }
+                        if catalogs_changed {
+                            manager
+                                .lockfile
+                                .overrides
+                                .append_catalog_valued_name_hashes(&mut catalog_overridden);
+                            catalog_overridden.sort_unstable();
+                            catalog_overridden.dedup();
                         }
+                        // `all_name_hashes` is empty unless the overrides changed.
+                        reresolve_owned_rows(manager, &pinned_rows, |dependency| {
+                            all_name_hashes.binary_search(&dependency.name_hash).is_ok()
+                                || (catalogs_changed
+                                    && dependency.version.tag == DependencyVersionTag::Catalog)
+                                || catalog_overridden
+                                    .binary_search(&dependency.name_hash)
+                                    .is_ok()
+                        });
                     }
 
                     // Split this into two passes because the below may allocate memory or invalidate pointers
@@ -1577,7 +1533,7 @@ fn report_lockfile_load_error(
     Ok(())
 }
 
-/// Returns the rows the plan re-resolved so the overrides/catalogs invalidation loops that follow leave them pinned; only tracked when those loops will run.
+/// Returns the rows the plan re-resolved so the overrides/catalogs invalidation pass that follows leaves them pinned; only tracked when that pass will run.
 fn enqueue_transitive(
     manager: &mut PackageManager,
     transitive: &TransitiveUpdate,
@@ -1590,6 +1546,42 @@ fn enqueue_transitive(
     transitive.enqueue_tracked(manager)
 }
 
+/// Re-resolves the rows `selects`, walking each package's current dependency list in package order. The root's
+/// list (just rebuilt by the differ) goes first because a later row dedupes onto what an earlier one appended
+/// (`Lockfile::get_package_id`); the root's loaded rows, now in no list, would resolve as nobody's and are not
+/// walked, nor are `pinned_rows`, which the update plan just resolved. A workspace the add/update pass is about
+/// to re-read still holds its loaded list here and is walked like any other package.
+fn reresolve_owned_rows(
+    manager: &mut PackageManager,
+    pinned_rows: &DynamicBitSet,
+    selects: impl Fn(&Dependency) -> bool,
+) {
+    // Resolving appends packages and rows; only the lists present now are walked.
+    let lists: Vec<lockfile::DependencySlice> =
+        manager.lockfile.packages.items_dependencies().to_vec();
+    for list in lists {
+        for dep_id in list.begin()..list.end() {
+            if pinned_rows.is_set_allow_out_of_bound(dep_id as usize, false) {
+                continue;
+            }
+            let dependency = manager.lockfile.buffers.dependencies[dep_id as usize].clone();
+            if !selects(&dependency) {
+                continue;
+            }
+            manager.lockfile.buffers.resolutions[dep_id as usize] = invalid_package_id;
+            if let Err(err) = enqueue_dependency_with_main(
+                manager,
+                dep_id,
+                &dependency,
+                invalid_package_id,
+                false,
+            ) {
+                add_dependency_error(manager, &dependency, err);
+            }
+        }
+    }
+}
+
 #[derive(Default)]
 struct NamedUpdates {
     /// Invalidated rows paired with the package they resolved to, for redirect_moved_edges.

test/cli/install/catalogs.test.ts

@@ -259,6 +259,54 @@ describe("basic", () => {
     await runBunInstall(bunEnv, packageDir, { savesLockfile: false });
   });
 
+  // A file: path outside the project is only accepted on a row the root (or a workspace) owns. A catalog change
+  // re-resolves every catalog: row; the rows the root was loaded with have been replaced by then and belong to
+  // nobody, so re-resolving them fails the way an escaping transitive file: dependency does.
+  test.concurrent("changing the entry of a catalog: dependency pointing outside the project", async () => {
+    const packageJson = (xPath: string) =>
+      JSON.stringify({
+        name: "catalog-file-dep",
+        workspaces: { packages: [], catalog: { x: xPath } },
+        dependencies: { x: "catalog:" },
+      });
+    using dir = tempDir("catalog-file-dep", {
+      "x/package.json": JSON.stringify({ name: "x", version: "1.0.0" }),
+      "x2/package.json": JSON.stringify({ name: "x", version: "2.0.0" }),
+      "project/package.json": packageJson("file:../x"),
+    });
+    const packageDir = join(String(dir), "project");
+    const installedVersion = async () =>
+      (await file(join(packageDir, "node_modules", "x", "package.json")).json()).version;
+
+    await runBunInstall(bunEnv, packageDir);
+    expect(await installedVersion()).toBe("1.0.0");
+
+    await write(join(packageDir, "package.json"), packageJson("file:../x2"));
+    await runBunInstall(bunEnv, packageDir);
+    expect(await installedVersion()).toBe("2.0.0");
+    expect(normalizeBunSnapshot(await file(join(packageDir, "bun.lock")).text(), packageDir)).toMatchInlineSnapshot(`
+      "{
+        "lockfileVersion": 2,
+        "configVersion": 1,
+        "workspaces": {
+          "": {
+            "name": "catalog-file-dep",
+            "dependencies": {
+              "x": "catalog:",
+            },
+          },
+        },
+        "catalog": {
+          "x": "file:../x2",
+        },
+        "packages": {
+          "x": ["x@file:../x2", {}],
+        }
+      }"
+    `);
+    await runBunInstall(bunEnv, packageDir, { frozenLockfile: true });
+  });
+
   test.concurrent("catalog and catalogs.default may split different packages between them", async () => {
     const { packageDir } = await registry.createTestDir({
       files: {
@@ -1274,6 +1322,65 @@ describe("peer dependencies", () => {
     expect(await packageKeys(dir)).toStrictEqual(dedupedKeys);
   });
 
+  // When the catalog changes, every row declared through it is re-resolved. The root's rows from bun.lock have
+  // been replaced by then and belong to no package; re-resolving them too bound the peer a second time and
+  // repeated its warning.
+  describe("a root peer whose catalog range stops matching the installed version is checked once", () => {
+    const peerWarning = 'warn: incorrect peer dependency "no-deps@1.0.0"';
+    const peerWarnings = (err: string) => err.split(peerWarning).length - 1;
+
+    function rootWithPeer(peerSpec: string, fields: Record<string, unknown> = {}) {
+      return JSON.stringify({ name: "root", peerDependencies: { "no-deps": peerSpec }, ...fields });
+    }
+
+    async function installedAlone(packageJson: string) {
+      const { packageDir } = await registry.createTestDir({
+        bunfigOpts: { linker: "hoisted" },
+        files: { "package.json": packageJson },
+      });
+      const { err } = await install(packageDir, "hoisted");
+      expect(peerWarnings(err)).toBe(0);
+      expect((await Bun.file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).version).toBe(
+        "1.0.0",
+      );
+      return packageDir;
+    }
+
+    async function reinstall(dir: string, packageJson: string) {
+      await Bun.write(join(dir, "package.json"), packageJson);
+      const { err } = await install(dir, "hoisted");
+      expect(err).toContain("Saved lockfile");
+      return peerWarnings(err);
+    }
+
+    // The inline row is the baseline: it changes itself and is only re-enqueued by the add/update pass.
+    test.concurrent.each([
+      [
+        "through the catalog",
+        (range: string) => rootWithPeer("catalog:", { workspaces: { catalog: { "no-deps": range } } }),
+      ],
+      ["inline", (range: string) => rootWithPeer(range)],
+    ])("declared %s", async (_, packageJson) => {
+      const dir = await installedAlone(packageJson("1.0.0"));
+      expect(await reinstall(dir, packageJson("^1.0.1"))).toBe(1);
+    });
+
+    test.concurrent("overridden to catalog:", async () => {
+      const packageJson = (range: string) =>
+        rootWithPeer("1.0.0", { overrides: { "no-deps": "catalog:" }, workspaces: { catalog: { "no-deps": range } } });
+      const dir = await installedAlone(packageJson("1.0.0"));
+      expect(await reinstall(dir, packageJson("^1.0.1"))).toBe(1);
+    });
+
+    // Selected both as an overridden name and as a catalog: row; one pass handles both.
+    test.concurrent("declared through the catalog while an override of the same name changes too", async () => {
+      const packageJson = (range: string) =>
+        rootWithPeer("catalog:", { overrides: { "no-deps": range }, workspaces: { catalog: { "no-deps": range } } });
+      const dir = await installedAlone(packageJson("1.0.0"));
+      expect(await reinstall(dir, packageJson("^1.0.1"))).toBe(1);
+    });
+  });
+
   // pnpm: deps-installer/test/catalogs.ts "frozen lockfile error is thrown if catalog config changes"
   test.concurrent("--frozen-lockfile fails when only a peer's catalog range changed", async () => {
     const dir = await makeRepo({ catalog: { "no-deps": ">=1.0.0" }, peerSpec: "catalog:", linker: "hoisted" });

test/cli/install/nested-overrides.test.ts

@@ -2,7 +2,7 @@ import { file, write } from "bun";
 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
 import { existsSync, realpathSync } from "fs";
 import { rm } from "fs/promises";
-import { VerdaccioRegistry, bunEnv, bunExe } from "harness";
+import { VerdaccioRegistry, bunEnv, bunExe, normalizeBunSnapshot, tempDir } from "harness";
 import { join } from "path";
 
 const registry = new VerdaccioRegistry();
@@ -1290,6 +1290,125 @@ describe.concurrent("lockfile", () => {
     await installOk(dir, "--frozen-lockfile");
   });
 
+  // An overrides change re-resolves every row naming a previously or newly overridden package. By then the root's
+  // rows from bun.lock have been replaced by freshly parsed ones and belong to no package; they must not be
+  // re-resolved as well: a range that is no longer in package.json would add a package the current row then dedupes
+  // onto, and a file: path outside the project is only accepted on a row the root (or a workspace) owns. The
+  // root's current rows also have to be resolved before everybody else's, as on a fresh install, since a row
+  // resolved later dedupes onto a same-major package an earlier one added.
+  describe.concurrent("removing a flat rule re-resolves only the root's current rows", () => {
+    test("the root's row resolves before a dependency's row of the same name", async () => {
+      const deps = { "no-deps": "^1.0.0", "one-dep": "1.0.0" }; // one-dep@1.0.0 depends on no-deps@1.0.1
+      const [dir, fresh] = await Promise.all([
+        project({ dependencies: deps, overrides: { "no-deps": "2.0.0" } }),
+        project({ dependencies: deps }),
+      ]);
+      await Promise.all([installOk(dir), installOk(fresh)]);
+      expect(await lock(dir)).toContain("no-deps@2.0.0");
+
+      await write(join(dir, "package.json"), JSON.stringify({ name: "nested-overrides", dependencies: deps }));
+      const { err } = await installOk(dir);
+      expect(err).toContain("Saved lockfile");
+      expect(await versionSeenBy(dir, undefined, "no-deps")).toBe("1.1.0");
+      expect(await versionSeenBy(dir, "one-dep", "no-deps")).toBe("1.0.1");
+      // Same packages as installing this package.json from scratch.
+      expect(await lock(dir)).toBe(await lock(fresh));
+    });
+
+    test("a range changed in the same edit resolves on its own", async () => {
+      const dir = await project({ dependencies: { "no-deps": "~1.0.0" }, overrides: { "no-deps": "2.0.0" } });
+      await installOk(dir);
+      expect(await versionSeenBy(dir, undefined, "no-deps")).toBe("2.0.0");
+
+      await write(
+        join(dir, "package.json"),
+        JSON.stringify({ name: "nested-overrides", dependencies: { "no-deps": "^1.0.0" } }),
+      );
+      const { err } = await installOk(dir);
+      expect(err).toContain("Saved lockfile");
+      // 1.0.1 is what the dropped ~1.0.0 range would pick.
+      expect(await versionSeenBy(dir, undefined, "no-deps")).toBe("1.1.0");
+      const after = await lock(dir);
+      expect(after).not.toContain('"overrides"');
+      expect(after).not.toContain("no-deps@1.0.1");
+      expect(after).not.toContain("no-deps@2.0.0");
+      await installOk(dir, "--frozen-lockfile");
+    });
+
+    const outside = {
+      "x/package.json": JSON.stringify({ name: "x", version: "1.0.0" }),
+      "x2/package.json": JSON.stringify({ name: "x", version: "2.0.0" }),
+      "project/y/package.json": JSON.stringify({ name: "y", version: "1.0.0" }),
+    };
+    const rootPackageJson = (pkg: Record<string, unknown>) => JSON.stringify({ name: "nested-overrides", ...pkg });
+    const before = rootPackageJson({
+      dependencies: { x: "file:../x", y: "file:./y" },
+      overrides: { x: "file:../x2" },
+    });
+
+    test("a file: dependency outside the project re-resolves to its own path", async () => {
+      using root = tempDir("override-removed-file-dep", { ...outside, "project/package.json": before });
+      const dir = join(String(root), "project");
+      await installOk(dir);
+      expect(await versionSeenBy(dir, undefined, "x")).toBe("2.0.0");
+
+      await write(join(dir, "package.json"), rootPackageJson({ dependencies: { x: "file:../x", y: "file:./y" } }));
+      const { err } = await installOk(dir);
+      expect(err).toContain("Saved lockfile");
+      expect(await versionSeenBy(dir, undefined, "x")).toBe("1.0.0");
+      expect(normalizeBunSnapshot(await lock(dir), dir)).toMatchInlineSnapshot(`
+        "{
+          "lockfileVersion": 2,
+          "configVersion": 1,
+          "workspaces": {
+            "": {
+              "name": "nested-overrides",
+              "dependencies": {
+                "x": "file:../x",
+                "y": "file:./y",
+              },
+            },
+          },
+          "packages": {
+            "x": ["x@file:../x", {}],
+
+            "y": ["y@file:y", {}],
+          }
+        }"
+      `);
+      await installOk(dir, "--frozen-lockfile");
+    });
+
+    test("a file: dependency outside the project removed together with its rule", async () => {
+      using root = tempDir("override-and-file-dep-removed", { ...outside, "project/package.json": before });
+      const dir = join(String(root), "project");
+      await installOk(dir);
+      expect(await versionSeenBy(dir, undefined, "x")).toBe("2.0.0");
+
+      await write(join(dir, "package.json"), rootPackageJson({ dependencies: { y: "file:./y" } }));
+      const { err } = await installOk(dir);
+      expect(err).toContain("Saved lockfile");
+      expect(normalizeBunSnapshot(await lock(dir), dir)).toMatchInlineSnapshot(`
+        "{
+          "lockfileVersion": 2,
+          "configVersion": 1,
+          "workspaces": {
+            "": {
+              "name": "nested-overrides",
+              "dependencies": {
+                "y": "file:./y",
+              },
+            },
+          },
+          "packages": {
+            "y": ["y@file:y", {}],
+          }
+        }"
+      `);
+      await installOk(dir, "--frozen-lockfile");
+    });
+  });
+
   test("changing only the parent's range text is a frozen-lockfile change", async () => {
     const dir = await project({ dependencies: twoParents, overrides: { "one-fixed-dep@1": { "no-deps": "1.1.0" } } });
     await installOk(dir);