diff --git a/src/install/PackageManager.rs b/src/install/PackageManager.rs index a23f5df14736..2224867ce286 100644 --- a/src/install/PackageManager.rs +++ b/src/install/PackageManager.rs @@ -161,7 +161,10 @@ Learn more about these at https://bun.com/docs/cli/pm. use crate::lockfile_real::package as Package; use crate::package_manager_task as Task; -use crate::resolvers::folder_resolver::{Entry as FolderResolutionEntry, FolderResolution}; +use crate::resolvers::folder_resolver::{ + Entry as FolderResolutionEntry, FolderResolution, Key as FolderResolutionKey, + Kind as FolderResolutionKind, +}; use bun_install::lockfile::{self, Lockfile}; use bun_install::{ Dependency, DependencyID, NetworkTask, PackageID, PackageManifestMap, @@ -252,8 +255,7 @@ type RepositoryMap = HashMap, 80 */> /// process. type AppendedTaskPackageMap = HashMap, 80 */>; -pub(crate) type FolderResolutionMap = - HashMap, 80 */>; +pub(crate) type FolderResolutionMap = HashMap; pub(crate) type NpmAliasMap = HashMap, 80 */>; @@ -2107,8 +2109,8 @@ pub fn init( { // make sure folder packages can find the root package without creating a new one // Posix-normalize the - // separators before hashing; `FolderResolution.hash` is always fed `/`-separated - // bytes by every resolver-side caller. On Windows `getFdPath` yields `\`, so + // separators before hashing; the folder resolver always builds its `Key` from + // `/`-separated bytes. On Windows `getFdPath` yields `\`, so // hashing the raw bytes would seed a key the resolver never looks up — copy into // a stack buffer and convert separators in place. // SAFETY: ROOT_PACKAGE_JSON_PATH set above on the main thread. @@ -2119,7 +2121,7 @@ pub fn init( resolve_path::dangerously_convert_path_to_posix_in_place::(normalized); // SAFETY: singleton fully initialized; main thread, no workers yet. unsafe { &mut *manager_ptr }.folders.put( - crate::resolvers::folder_resolver::hash(normalized), + FolderResolutionKey::new(FolderResolutionKind::Folder, normalized), FolderResolutionEntry { abs_path: Box::<[u8]>::from(&*normalized), resolution: FolderResolution::PackageId(0), diff --git a/src/install/resolvers/folder_resolver.rs b/src/install/resolvers/folder_resolver.rs index d75d0c9c96df..82a9ec508e84 100644 --- a/src/install/resolvers/folder_resolver.rs +++ b/src/install/resolvers/folder_resolver.rs @@ -80,8 +80,46 @@ impl<'a> fmt::Display for PackageWorkspaceSearchPathFormatter<'a> { } } +/// What a directory's `package.json` is read as. Each kind parses it with +/// different `Features` and produces a different `Resolution`, so one +/// directory is cached separately per kind: a `link:` and a `file:` pointing +/// at the same directory are two packages, and whichever resolves first must +/// not be handed out for the other. +#[derive(Copy, Clone, PartialEq, Eq, Hash)] +pub(crate) enum Kind { + /// A `file:` dependency or a workspace member (`Features::FOLDER` / + /// `Features::WORKSPACE`, dependencies installed). These share an entry + /// so a `file:` pointing at a workspace member, or at the root package + /// seeded by `PackageManager::init`, resolves to that existing package. + Folder, + /// A `link:` target (`Features::LINK`: its dependencies are not + /// installed, `Resolution::Symlink`). + Link, + /// An npm package already extracted into the cache (`Features::NPM`, + /// `Resolution::Npm`). + CacheFolder, +} + +/// Key of the folder-resolution map. +#[derive(Copy, Clone, PartialEq, Eq, Hash)] +pub(crate) struct Key { + kind: Kind, + /// Hash of the normalized absolute `package.json` path; `Entry::abs_path` + /// holds the bytes it was computed from. + abs_hash: u64, +} + +impl Key { + pub(crate) fn new(kind: Kind, normalized_abs_path: &[u8]) -> Key { + Key { + kind, + abs_hash: bun_wyhash::hash(normalized_abs_path), + } + } +} + /// Value stored in the folder-resolution map: the resolution plus the -/// normalized absolute `package.json` path the key hash was computed from. +/// normalized absolute `package.json` path `Key::abs_hash` was computed from. /// Lookups compare the path, since a different path whose hash collides must /// not reuse this resolution. pub struct Entry { @@ -96,10 +134,6 @@ fn normalize(path: &[u8]) -> &[u8] { FileSystem::instance().normalize(path) } -pub(crate) fn hash(normalized_path: &[u8]) -> u64 { - bun_wyhash::hash(normalized_path) -} - // ── NewResolver ─────────────────────────────────────────────────────────── // The const-generic tag requires `#[derive(ConstParamTy)]` (already on `Tag`). struct NewResolver<'a, const TAG: ResolutionTag> { @@ -377,6 +411,20 @@ pub enum GlobalOrRelative<'a> { CacheFolder(&'a [u8]), } +impl GlobalOrRelative<'_> { + /// Must agree with the resolver `get_or_put` picks for each variant. + fn kind(self) -> Kind { + match self { + GlobalOrRelative::Global(_) => Kind::Link, + GlobalOrRelative::Relative( + dependency::version::Tag::Folder | dependency::version::Tag::Workspace, + ) => Kind::Folder, + GlobalOrRelative::Relative(_) => unreachable!(), + GlobalOrRelative::CacheFolder(_) => Kind::CacheFolder, + } + } +} + pub(crate) fn get_or_put( global_or_relative: GlobalOrRelative<'_>, version: &dependency::Version, @@ -400,7 +448,7 @@ pub(crate) fn get_or_put( // `(&[u8]).as_ptr().cast_mut()` would be UB under Stacked/Tree // Borrows: those pointers carry read-only provenance, and the // optimizer may assume `abs`'s bytes are unchanged when computing - // `hash(abs.as_bytes())` below. + // the `Key` below. // // Instead: capture lengths, let the shared borrows of `joined` die, // then take a fresh `&mut joined[..abs_len]` (write provenance) and @@ -421,13 +469,13 @@ pub(crate) fn get_or_put( &rel_buf[..rel_len], ) }; - let abs_hash = hash(abs.as_bytes()); + let key = Key::new(global_or_relative.kind(), abs.as_bytes()); // Check first, compute, then insert, because read_package_json_from_disk // needs &mut manager. Compare the stored path, not just its hash: a // different path whose hash collides must not reuse this resolution. On a // collision, resolve fresh without caching so the first path's entry stays. - let hash_collision = match manager.folders.get(&abs_hash) { + let hash_collision = match manager.folders.get(&key) { Some(existing) if *existing.abs_path == *abs.as_bytes() => return existing.resolution, Some(_) => true, None => false, @@ -498,7 +546,7 @@ pub(crate) fn get_or_put( }; if !hash_collision { manager.folders.insert( - abs_hash, + key, Entry { abs_path: abs.as_bytes().into(), resolution: stored, @@ -511,7 +559,7 @@ pub(crate) fn get_or_put( if !hash_collision { manager.folders.insert( - abs_hash, + key, Entry { abs_path: abs.as_bytes().into(), resolution: FolderResolution::PackageId(package.meta.id), diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index e35753d1be20..6290fe654ab2 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -9917,6 +9917,52 @@ it("fails when a transitive file: dependency's folder does not exist", async () expect(exitCode).toBe(1); }); +// A `file:` dependency is a folder package whose dependencies get installed; a +// `link:` dependency on the same directory is a bare symlink with none. The +// folder resolver caches resolutions per directory, so whichever of the two +// resolved first used to be handed out for the other one as well. Dependencies +// resolve in alias order, so "a" is resolved before "z". +for (const [first, second] of [ + ["link", "file"], + ["file", "link"], +] as const) { + it(`resolves file: and link: to the same directory as separate packages (${first}: resolves first)`, async () => { + using dir = tempDir("file-and-link-to-same-dir", { + "shared/package.json": JSON.stringify({ + name: "shared", + version: "1.0.0", + dependencies: { extra: "file:../app/extra" }, + }), + "app/extra/package.json": JSON.stringify({ name: "extra", version: "1.0.0" }), + "app/package.json": JSON.stringify({ + name: "app", + dependencies: { a: `${first}:../shared`, z: `${second}:../shared` }, + }), + }); + const projectDir = join(String(dir), "app"); + const fileAlias = first === "file" ? "a" : "z"; + const linkAlias = first === "link" ? "a" : "z"; + + await using proc = spawn({ + cmd: [bunExe(), "install", "--lockfile-only"], + cwd: projectDir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [err, , exitCode] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]); + + expect(err).not.toContain("error:"); + expect(exitCode).toBe(0); + const lockfile = Bun.JSONC.parse(await file(join(projectDir, "bun.lock")).text()) as any; + expect(lockfile.packages).toEqual({ + [fileAlias]: ["shared@file:../shared", { dependencies: { extra: "file:../app/extra" } }], + [`${fileAlias}/extra`]: ["extra@file:extra", {}], + [linkAlias]: ["shared@link:../shared", {}], + }); + }); +} + it("does not extract a local file: tarball outside the temp dir for a dependency alias containing '..' path segments", async () => { // For `file:` tarball dependencies, the dependency alias (the key in // `dependencies`) is used to derive the temporary extraction folder name.