diff --git a/src/install/PackageManager/install_with_manager.rs b/src/install/PackageManager/install_with_manager.rs index 1fe8942c32eb..4ec4b1e0423d 100644 --- a/src/install/PackageManager/install_with_manager.rs +++ b/src/install/PackageManager/install_with_manager.rs @@ -90,8 +90,9 @@ pub fn install_with_manager( // Snapshot the loaded-from-lockfile package count so // `Lockfile::get_package_id` can tell loaded pins apart from packages - // appended by manifest fetches in this resolve session. - manager.lockfile.mark_loaded_packages(); + // appended by manifest fetches in this resolve session, and which loaded + // packages a non-peer dependency holds, for `clean_with_logger`. + manager.lockfile.mark_loaded_packages()?; let (config_version, changed_config_version) = load_result.choose_config_version(); manager.options.config_version = Some(config_version); diff --git a/src/install/dedupe.rs b/src/install/dedupe.rs index 32ed09e28e02..e524e2b6e54a 100644 --- a/src/install/dedupe.rs +++ b/src/install/dedupe.rs @@ -605,7 +605,8 @@ pub(crate) fn effective_version( Some(version) } -// Optional-peer edges are followed too: with an in-sync package.json `clean` runs with `keep_optional_peer_targets`. +// Optional-peer edges are followed too: `clean` keeps a target the loaded lockfile held through them alone +// (`Lockfile::held_at_load`), and a version that survives here keeps every edge it had, so anything else they reach stays held. fn reachable(lockfile: &Lockfile, resolutions: &[PackageID]) -> DynamicBitSet { crate::lockfile::reachable::packages( lockfile, diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index da5691de7ee9..74005e34aaf3 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -203,6 +203,19 @@ pub struct Lockfile { /// Runtime-only — never serialised. pub(crate) loaded_package_count: PackageID, + /// `bit[id] == true` ⇔ a dependency other than an optional peer resolved + /// to package `id` in the lockfile as loaded. An optional peer slot never + /// keeps such a package alive through `clean_with_logger` (it is bound in + /// `Cloner::flush` if something else still reaches it), so a package whose + /// last real dependent leaves package.json is dropped. A loaded package + /// with the bit unset was held by optional peers alone; 1.3.x wrote such + /// entries and they are kept, otherwise every resolve would prune them and + /// rewrite the file on any unrelated change. Sized to + /// `loaded_package_count`, so packages appended during this resolve read + /// as held (a fresh resolve never creates peer-only entries). Set by + /// `mark_loaded_packages`; runtime-only — never serialised. + pub(crate) held_at_load: DynamicBitSet, + /// `bit[id] == true` ⇔ package `id` was appended for a dependency whose /// version range was an exact `=X.Y.Z` (i.e. the user — root or workspace /// — pinned this exact version somewhere in the tree). `get_package_id`'s @@ -969,9 +982,6 @@ impl Lockfile { let mut package_id_mapping = vec![invalid_package_id; old.packages.len()]; let clone_queue_ = PendingResolutions::new(); - // A frozen install never saves, so dropping peer-held targets could only fail its check. - let keep_optional_peer_targets = - manager.options.enable.frozen_lockfile() || !manager.summary.changes_resolutions(); // Explicit `&mut *` reborrows so `old`/`manager`/`new` are // released back to this scope once `cloner` is dropped. let mut cloner = Cloner { @@ -980,7 +990,6 @@ impl Lockfile { mapping: &mut package_id_mapping, clone_queue: clone_queue_, optional_peers: PendingResolutions::new(), - keep_optional_peer_targets, log, old_preinstall_state, manager: &mut *manager, @@ -1207,7 +1216,6 @@ pub struct Cloner<'a> { pub(crate) clone_queue: PendingResolutions, /// Bound in `flush`, once `clone_queue` has decided which targets survive. pub(crate) optional_peers: PendingResolutions, - pub(crate) keep_optional_peer_targets: bool, pub lockfile: &'a mut Lockfile, pub(crate) old: &'a mut Lockfile, pub(crate) mapping: &'a mut [PackageID], @@ -1986,16 +1994,32 @@ impl Lockfile { // session-appended, so the order-independence guard in // `get_package_id` applies from id 0. loaded_package_count: 0, + held_at_load: DynamicBitSet::default(), exact_pinned: DynamicBitSet::default(), } } - /// Snapshot `packages.len()` as the "loaded from lockfile" watermark. - /// Call exactly once after `load_from_cwd` (including npm/pnpm/yarn - /// migration) before any manifest-driven `append_package`. - #[inline] - pub(crate) fn mark_loaded_packages(&mut self) { - self.loaded_package_count = self.packages.len() as PackageID; + /// Snapshot `packages.len()` as the "loaded from lockfile" watermark and + /// which of those packages a non-peer dependency resolves to + /// (`held_at_load`). Call exactly once after `load_from_cwd` (including + /// npm/pnpm/yarn migration), before the differ, an update or a dedupe + /// re-points any resolution and before any manifest-driven + /// `append_package`. + pub(crate) fn mark_loaded_packages(&mut self) -> Result<(), AllocError> { + let packages_len = self.packages.len(); + self.loaded_package_count = packages_len as PackageID; + let mut held = DynamicBitSet::init_empty(packages_len)?; + // A load that failed partway leaves `resolutions` shorter than `dependencies`; + // that lockfile is replaced by `init_empty` before anything reads the set. + let dependencies = self.buffers.dependencies.as_slice(); + let resolutions = self.buffers.resolutions.as_slice(); + for (dependency, &package_id) in dependencies.iter().zip(resolutions) { + if !dependency.behavior.is_optional_peer() && (package_id as usize) < packages_len { + held.set(package_id as usize); + } + } + self.held_at_load = held; + Ok(()) } /// Record that package `id` was appended via an exact-version dependency diff --git a/src/install/lockfile/Package.rs b/src/install/lockfile/Package.rs index fcd5b545bd4d..767c6aa80c1f 100644 --- a/src/install/lockfile/Package.rs +++ b/src/install/lockfile/Package.rs @@ -616,8 +616,14 @@ impl Package { resolve_id: new_package.resolutions.off + PackageID::try_from(i).expect("int cast"), }; - // Peer slots must not keep their target alive; bound in `Cloner::flush`. - if old_dependencies[i].behavior.is_optional_peer() && !cloner.keep_optional_peer_targets + // A peer slot must not keep a target alive that something else held + // when the lockfile was loaded; it is bound in `Cloner::flush` if that + // holder survived. A target the loaded lockfile held through optional + // peers alone is cloned like a dependency (see `Lockfile::held_at_load`). + if old_dependencies[i].behavior.is_optional_peer() + && old + .held_at_load + .is_set_allow_out_of_bound(*old_resolution as usize, true) { cloner.optional_peers.push(pending); continue; diff --git a/test/cli/install/bun-lock.test.ts b/test/cli/install/bun-lock.test.ts index c8ba27d17e5f..8b837a06d3c7 100644 --- a/test/cli/install/bun-lock.test.ts +++ b/test/cli/install/bun-lock.test.ts @@ -1450,6 +1450,51 @@ it("--frozen-lockfile keeps a package that an older lockfile lists only as an op expect(await exists(join(packageDir, "node_modules", "no-deps", "package.json"))).toBeTrue(); }); +// Same entries, but the install that re-resolves and saves: a package.json change +// unrelated to them (here a dependency bun.lock already has) must not prune them +// either. Only a package whose real dependent leaves is dropped (the tests above). +it("a re-resolving install keeps the packages an older lockfile holds through optional peers alone", async () => { + const { packageDir, packageJson } = await registry.createTestDir({ + bunfigOpts: { saveTextLockfile: true, linker: "hoisted" }, + }); + const run = makeInstallRunner(packageDir); + const noDepsEntry = '"no-deps": ["no-deps@1.0.0"'; + const locked = { "optional-peer-deps": "1.0.0", "uses-a-dep-1": "1.0.0" }; + await Promise.all([ + // a-dep is already in bun.lock through uses-a-dep-1, so nothing resolves differently. + write(packageJson, JSON.stringify({ name: "foo", dependencies: { ...locked, "a-dep": "1.0.1" } })), + write( + join(packageDir, "bun.lock"), + JSON.stringify({ + lockfileVersion: 1, + configVersion: 1, + workspaces: { "": { name: "foo", dependencies: locked } }, + packages: { + "a-dep": ["a-dep@1.0.1", "", {}, ""], + // only optional-peer-deps's optional peer refers to this entry + "no-deps": ["no-deps@1.0.0", "", {}, ""], + "optional-peer-deps": [ + "optional-peer-deps@1.0.0", + "", + { peerDependencies: { "no-deps": "*" }, optionalPeers: ["no-deps"] }, + "", + ], + "uses-a-dep-1": ["uses-a-dep-1@1.0.0", "", { dependencies: { "a-dep": "1.0.1" } }, ""], + }, + }), + ), + ]); + + await run(["install"]); + const saved = await file(join(packageDir, "bun.lock")).text(); + expect(saved).toContain('"a-dep": "1.0.1"'); + expect(saved).toContain(noDepsEntry); + expect(await exists(join(packageDir, "node_modules", "no-deps", "package.json"))).toBeTrue(); + + await run(["install", "--frozen-lockfile"]); + expect(await file(join(packageDir, "bun.lock")).text()).toBe(saved); +}); + // The optional-peer-hoist-* fixtures are described in // registry/packages/create-optional-peer-hoist-packages.ts. In short: consumer // has an optional peer on target, and deep -> deep-child reaches target@1.0.0