diff --git a/src/install/PackageManager/add_catalog.rs b/src/install/PackageManager/add_catalog.rs index 3ad4cc3065c1..0dede5332a5c 100644 --- a/src/install/PackageManager/add_catalog.rs +++ b/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; diff --git a/src/install/error.rs b/src/install/error.rs index 87ce091d1bca..6219d1ed0dc8 100644 --- a/src/install/error.rs +++ b/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 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 for Error { use crate::pnpm::MigratePnpmLockfileError as E; match e { E::OutOfMemory => Self::Alloc(bun_alloc::AllocError), - E::DependencyLoop => Self::DependencyLoop, _ => Self::InvalidLockfile, } } diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index a22e23128ffc..cf3f9726c3dc 100644 --- a/src/install/lockfile/Tree.rs +++ b/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 { // 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,7 +1032,7 @@ 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 } } @@ -1052,66 +1040,33 @@ impl Tree { // 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::( + let id = Tree::hoist_dependency::( this.parent, hoist_root_id, package_id, input_dep_id, + input_dep_range, builder, - ) { - Ok(id) => id, - // SAFETY: `hoist_dependency::` 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 } } diff --git a/src/install/lockfile/bun.lock.rs b/src/install/lockfile/bun.lock.rs index 963a06ebbcb6..b06df838c9b5 100644 --- a/src/install/lockfile/bun.lock.rs +++ b/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); } } diff --git a/src/install/pnpm.rs b/src/install/pnpm.rs index abf7916c98f8..e14532accf02 100644 --- a/src/install/pnpm.rs +++ b/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 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 for MigratePnpmLockfileError use crate::lockfile_real::tree::SubtreeError as E; match e { E::OutOfMemory => Self::OutOfMemory, - E::DependencyLoop => Self::DependencyLoop, } } } diff --git a/test/cli/install/bun-add-filter.test.ts b/test/cli/install/bun-add-filter.test.ts index 80d66f7ecdc8..42b3f00aa574 100644 --- a/test/cli/install/bun-add-filter.test.ts +++ b/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 --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); diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index 279851e5dd27..dbfb3f229e6e 100644 --- a/test/cli/install/bun-install.test.ts +++ b/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>; + pkgA: Record>; + pkgB?: Record>; + expected: Record; + }>([ + { + 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 = { + "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; + 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[] = [];