diff --git a/mordant-baseline.toml b/mordant-baseline.toml index 753e3178cf44..814909d5a2a4 100644 --- a/mordant-baseline.toml +++ b/mordant-baseline.toml @@ -44,8 +44,6 @@ "reimplemented_helper:src/runtime/api/bun/Terminal.rs" = 1 "reimplemented_helper:src/runtime/hw_exports.rs" = 1 "same_match_twice:src/runtime/api/bun/h2_frame_parser.rs" = 1 -"same_match_twice:src/runtime/cli/pack_command.rs" = 1 -"same_match_twice:src/runtime/cli/update_interactive_command.rs" = 2 "same_match_twice:src/runtime/server/RequestContext.rs" = 4 "same_match_twice:src/runtime/shell/builtin/cat.rs" = 1 "same_match_twice:src/runtime/webcore/Blob.rs" = 2 diff --git a/src/install/PackageManager/WorkspacePackageJSONCache.rs b/src/install/PackageManager/WorkspacePackageJSONCache.rs index 3ef125dddb95..cef56492e5fc 100644 --- a/src/install/PackageManager/WorkspacePackageJSONCache.rs +++ b/src/install/PackageManager/WorkspacePackageJSONCache.rs @@ -114,14 +114,36 @@ pub enum GetResult<'a> { ParseErr(Error), } +/// The step of [`WorkspacePackageJSONCache::get_with_path`] that failed. +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum GetStep { + Read, + Parse, +} + +impl GetStep { + /// The word for the step in a "failed to ... package.json" message. + pub fn verb(self) -> &'static str { + match self { + GetStep::Read => "read", + GetStep::Parse => "parse", + } + } +} + impl<'a> GetResult<'a> { - pub(crate) fn unwrap(self) -> Result<&'a mut MapEntry, Error> { + /// The entry, or the step that failed and its error. + pub fn entry(self) -> Result<&'a mut MapEntry, (GetStep, Error)> { match self { GetResult::Entry(entry) => Ok(entry), - GetResult::ReadErr(err) => Err(err), - GetResult::ParseErr(err) => Err(err), + GetResult::ReadErr(err) => Err((GetStep::Read, err)), + GetResult::ParseErr(err) => Err((GetStep::Parse, err)), } } + + pub(crate) fn unwrap(self) -> Result<&'a mut MapEntry, Error> { + self.entry().map_err(|(_, err)| err) + } } #[derive(Default)] diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index da5691de7ee9..aca1e2114dab 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -324,7 +324,8 @@ pub enum LoadStep { } impl LoadStep { - pub(crate) fn verb(self) -> &'static str { + /// The word for the step in a "failed to ... lockfile" message. + pub fn verb(self) -> &'static str { match self { LoadStep::OpenFile => "open", LoadStep::ReadFile => "read", diff --git a/src/runtime/cli/outdated_command.rs b/src/runtime/cli/outdated_command.rs index 18757f7d0d32..86405fcf4a0a 100644 --- a/src/runtime/cli/outdated_command.rs +++ b/src/runtime/cli/outdated_command.rs @@ -7,8 +7,8 @@ use bun_core::strings; use bun_core::{Global, Output}; use bun_glob as glob; use bun_install::dependency::{self, Behavior}; +use bun_install::lockfile::LoadResult; use bun_install::lockfile::package::PackageColumns as _; -use bun_install::lockfile::{LoadResult, LoadStep}; use bun_install::package_manager::{ LogLevel, ManifestLoad, Subcommand, WorkspaceFilter, populate_manifest_cache, }; @@ -118,24 +118,10 @@ impl OutdatedCommand { if not_silent && !bun_install::migration::reported_unsupported_lockfile_version(&cause) { - match cause.step { - LoadStep::OpenFile => Output::err_generic( - "failed to open lockfile: {s}", - (cause.value.name(),), - ), - LoadStep::ParseFile => Output::err_generic( - "failed to parse lockfile: {s}", - (cause.value.name(),), - ), - LoadStep::ReadFile => Output::err_generic( - "failed to read lockfile: {s}", - (cause.value.name(),), - ), - LoadStep::Migrating => Output::err_generic( - "failed to migrate lockfile: {s}", - (cause.value.name(),), - ), - } + Output::err_generic( + "failed to {s} lockfile: {s}", + (cause.step.verb(), cause.value.name()), + ); if ctx.log_ref().has_errors() { // SAFETY: `log_ptr` aliases `manager.log` which is the // `*logger.Log` borrowed from `Command::Context`; no diff --git a/src/runtime/cli/pack_command.rs b/src/runtime/cli/pack_command.rs index aafee8e9db8c..12492645d01b 100644 --- a/src/runtime/cli/pack_command.rs +++ b/src/runtime/cli/pack_command.rs @@ -1890,46 +1890,54 @@ fn opt_pack_gzip_level(m: &PackageManager) -> Option<&[u8]> { // `Some` only when FOR_PUBLISH == true. pub(crate) type PackReturn<'a, const FOR_PUBLISH: bool> = Option>; -pub(crate) fn pack( - ctx: &mut Context<'_>, +/// The package.json being packed, through the manager's cache; a package.json +/// that cannot be read or parsed ends the command. +/// +/// `workspace_package_json_cache` and `log` are disjoint fields on +/// `PackageManager`; route through raw-pointer field projections so the two +/// `&mut` borrows don't conflict. +fn package_json_entry<'a>( + manager_ptr: *mut PackageManager, abs_package_json_path: &ZStr, -) -> Result, PackError> { - // Raw pointer for the `pm_workspace_cache`/`pm_log` disjoint-field - // projections and the `'static` lifetime extension when returning - // `Publish::Context`. - let manager_ptr: *mut PackageManager = &raw mut *ctx.manager; - let log_level = ctx.manager.options.log_level; - let bump = pack_bump(); - // Note: `workspace_package_json_cache` and `log` are disjoint fields on - // `PackageManager`; route through raw-pointer field projections so the - // two `&mut` borrows don't conflict. - let mut json = match pm_workspace_cache(manager_ptr).get_with_path( +) -> &'a mut WorkspacePackageJSONCache::MapEntry { + let result = pm_workspace_cache(manager_ptr).get_with_path( pm_log(manager_ptr), abs_package_json_path.as_bytes(), WorkspacePackageJSONCache::GetJSONOptions { guess_indentation: true, ..Default::default() }, - ) { - WorkspacePackageJSONCache::GetResult::ReadErr(err) => { - Output::err( - err, - "failed to read package.json: {}", - format_args!("{}", bstr::BStr::new(abs_package_json_path.as_bytes())), - ); - Global::crash(); - } - WorkspacePackageJSONCache::GetResult::ParseErr(err) => { + ); + match result.entry() { + Ok(entry) => entry, + Err((step, err)) => { Output::err( err, - "failed to parse package.json: {}", - format_args!("{}", bstr::BStr::new(abs_package_json_path.as_bytes())), + "failed to {} package.json: {}", + ( + step.verb(), + bstr::BStr::new(abs_package_json_path.as_bytes()), + ), ); - let _ = pm_log(manager_ptr).print(std::ptr::from_mut(Output::error_writer())); + if step == WorkspacePackageJSONCache::GetStep::Parse { + let _ = pm_log(manager_ptr).print(std::ptr::from_mut(Output::error_writer())); + } Global::crash(); } - WorkspacePackageJSONCache::GetResult::Entry(entry) => entry, - }; + } +} + +pub(crate) fn pack( + ctx: &mut Context<'_>, + abs_package_json_path: &ZStr, +) -> Result, PackError> { + // Raw pointer for the `pm_workspace_cache`/`pm_log` disjoint-field + // projections and the `'static` lifetime extension when returning + // `Publish::Context`. + let manager_ptr: *mut PackageManager = &raw mut *ctx.manager; + let log_level = ctx.manager.options.log_level; + let bump = pack_bump(); + let mut json = package_json_entry(manager_ptr, abs_package_json_path); if FOR_PUBLISH { if let Some(config) = json.root.get(b"publishConfig") { @@ -2156,34 +2164,7 @@ pub(crate) fn pack( let cache_key: &[u8] = abs_package_json_path.as_bytes(); let _ = pm_workspace_cache(manager_ptr).map.remove(cache_key); - // Re-read package.json from disk - json = match pm_workspace_cache(manager_ptr).get_with_path( - pm_log(manager_ptr), - abs_package_json_path.as_bytes(), - WorkspacePackageJSONCache::GetJSONOptions { - guess_indentation: true, - ..Default::default() - }, - ) { - WorkspacePackageJSONCache::GetResult::ReadErr(err) => { - Output::err( - err, - "failed to read package.json: {}", - format_args!("{}", bstr::BStr::new(abs_package_json_path.as_bytes())), - ); - Global::crash(); - } - WorkspacePackageJSONCache::GetResult::ParseErr(err) => { - Output::err( - err, - "failed to parse package.json: {}", - format_args!("{}", bstr::BStr::new(abs_package_json_path.as_bytes())), - ); - let _ = pm_log(manager_ptr).print(std::ptr::from_mut(Output::error_writer())); - Global::crash(); - } - WorkspacePackageJSONCache::GetResult::Entry(entry) => entry, - }; + json = package_json_entry(manager_ptr, abs_package_json_path); // Re-validate private flag after scripts may have modified it. if FOR_PUBLISH { diff --git a/src/runtime/cli/package_manager_command.rs b/src/runtime/cli/package_manager_command.rs index 62b21f210e1b..7c3603cdd8ca 100644 --- a/src/runtime/cli/package_manager_command.rs +++ b/src/runtime/cli/package_manager_command.rs @@ -6,7 +6,7 @@ use bun_core::fmt::PathSep; use bun_core::strings; use bun_core::{Global, Output, env_var, fmt as bun_fmt}; use bun_install::dependency::Dependency; -use bun_install::lockfile::{LoadResult, LoadStep, Lockfile, package::PackageColumns as _, tree}; +use bun_install::lockfile::{LoadResult, Lockfile, package::PackageColumns as _, tree}; use bun_install::npm as Npm; use bun_install::package_manager_real::{ CommandLineArguments, Subcommand, fetch_cache_directory_path, get_cache_directory, @@ -51,15 +51,6 @@ impl<'a> ByName<'a> { } } -fn load_step_verb(step: LoadStep) -> &'static str { - match step { - LoadStep::OpenFile => "open", - LoadStep::ReadFile => "read", - LoadStep::ParseFile => "parse", - LoadStep::Migrating => "migrate", - } -} - pub(crate) struct PackageManagerCommand; impl PackageManagerCommand { @@ -93,7 +84,7 @@ impl PackageManagerCommand { if not_silent && !migration::reported_unsupported_lockfile_version(err) { Output::err_generic( "failed to {s} lockfile: {s}", - (load_step_verb(err.step), err.value.name()), + (err.step.verb(), err.value.name()), ); } Global::exit(1); diff --git a/src/runtime/cli/update_interactive_command.rs b/src/runtime/cli/update_interactive_command.rs index cbae1c6acf25..8dbf622fccee 100644 --- a/src/runtime/cli/update_interactive_command.rs +++ b/src/runtime/cli/update_interactive_command.rs @@ -9,8 +9,8 @@ use bun_alloc::Arena as Bump; use bun_collections::{StringHashMap, index_sort}; use bun_core::{Global, Output}; use bun_install::dependency::{self, Behavior}; +use bun_install::lockfile::LoadResult; use bun_install::lockfile::package::PackageColumns as _; -use bun_install::lockfile::{LoadResult, LoadStep}; use bun_install::package_manager::options::Do; use bun_install::package_manager::{ LogLevel, ManifestLoad, Subcommand, WorkspaceFilter, populate_manifest_cache, @@ -19,8 +19,8 @@ use bun_install::package_manager::{ use bun_install::package_manager_real::command_line_arguments::UpdateGroups; use bun_install::update_scope::selects; use bun_install::{ - CommandLineArguments, GetJsonOptions, GetJsonResult, INVALID_PACKAGE_ID, PackageID, - PackageManager, WorkspacePackageJsonCacheEntry, resolution, + CommandLineArguments, GetJsonOptions, INVALID_PACKAGE_ID, PackageID, PackageManager, + WorkspacePackageJsonCacheEntry, resolution, }; use bun_install_types::DependencyGroup; use bun_js_printer::{self as js_printer, BufferPrinter, BufferWriter, PrintJsonOptions}; @@ -181,6 +181,36 @@ impl UpdateInteractiveCommand { } } + /// A workspace's package.json through the manager's cache. One that cannot + /// be read or parsed is reported and returns `None`, so the caller skips + /// that workspace and carries on with the others. + fn load_package_json<'m>( + manager: &'m mut PackageManager, + package_json_path: &[u8], + ) -> Option<&'m mut WorkspacePackageJsonCacheEntry> { + // `log_mut()` returns a borrow decoupled from `&self`, so it can + // overlap the `workspace_package_json_cache` field borrow. + let log = manager.log_mut(); + let result = manager.workspace_package_json_cache.get_with_path( + log, + package_json_path, + GetJsonOptions { + guess_indentation: true, + ..Default::default() + }, + ); + match result.entry() { + Ok(entry) => Some(entry), + Err((step, err)) => { + Output::err_generic( + "Failed to {s} package.json at {s}: {s}", + (step.verb(), BStr::new(package_json_path), err.name()), + ); + None + } + } + } + // Helper to update a catalog entry at a specific path in the package.json AST // No `*PackageManager` parameter: there is no per-manager allocator, // and dropping it avoids overlapping `&mut PackageManager` with the live @@ -334,36 +364,9 @@ impl UpdateInteractiveCommand { let package_json_path = Self::build_package_json_path(root_dir, workspace_path, &mut path_buf); - // Load and parse the package.json - // Reshaped for borrowck — `log_mut()` returns a borrow - // decoupled from `&self`, so it can overlap the disjoint - // `workspace_package_json_cache` field borrow below. - let log = manager.log_mut(); - let package_json: &mut WorkspacePackageJsonCacheEntry = - match manager.workspace_package_json_cache.get_with_path( - log, - package_json_path, - GetJsonOptions { - guess_indentation: true, - ..Default::default() - }, - ) { - GetJsonResult::ParseErr(err) => { - Output::err_generic( - "Failed to parse package.json at {s}: {s}", - (BStr::new(package_json_path), err.name()), - ); - continue; - } - GetJsonResult::ReadErr(err) => { - Output::err_generic( - "Failed to read package.json at {s}: {s}", - (BStr::new(package_json_path), err.name()), - ); - continue; - } - GetJsonResult::Entry(entry) => entry, - }; + let Some(package_json) = Self::load_package_json(manager, package_json_path) else { + continue; + }; let mut modified = false; @@ -464,33 +467,9 @@ impl UpdateInteractiveCommand { let package_json_path = Self::build_package_json_path(root_dir, workspace_path, &mut path_buf); - // Load and parse the package.json properly - let log = manager.log_mut(); - let package_json: &mut WorkspacePackageJsonCacheEntry = - match manager.workspace_package_json_cache.get_with_path( - log, - package_json_path, - GetJsonOptions { - guess_indentation: true, - ..Default::default() - }, - ) { - GetJsonResult::ParseErr(err) => { - Output::err_generic( - "Failed to parse package.json at {s}: {s}", - (BStr::new(package_json_path), err.name()), - ); - continue; - } - GetJsonResult::ReadErr(err) => { - Output::err_generic( - "Failed to read package.json at {s}: {s}", - (BStr::new(package_json_path), err.name()), - ); - continue; - } - GetJsonResult::Entry(entry) => entry, - }; + let Some(package_json) = Self::load_package_json(manager, package_json_path) else { + continue; + }; edit_catalog_definitions( &mut updates_for_workspace[..], @@ -527,24 +506,10 @@ impl UpdateInteractiveCommand { if not_silent && !bun_install::migration::reported_unsupported_lockfile_version(&cause) { - match cause.step { - LoadStep::OpenFile => Output::err_generic( - "failed to open lockfile: {s}", - (cause.value.name(),), - ), - LoadStep::ParseFile => Output::err_generic( - "failed to parse lockfile: {s}", - (cause.value.name(),), - ), - LoadStep::ReadFile => Output::err_generic( - "failed to read lockfile: {s}", - (cause.value.name(),), - ), - LoadStep::Migrating => Output::err_generic( - "failed to migrate lockfile: {s}", - (cause.value.name(),), - ), - } + Output::err_generic( + "failed to {s} lockfile: {s}", + (cause.step.verb(), cause.value.name()), + ); // SAFETY: `ctx.log` is set by `Command::create_context_data` // for every subcommand and is non-null for the command's // lifetime. diff --git a/test/cli/install/bun-lock.test.ts b/test/cli/install/bun-lock.test.ts index c8ba27d17e5f..0b5ceb041f79 100644 --- a/test/cli/install/bun-lock.test.ts +++ b/test/cli/install/bun-lock.test.ts @@ -1267,6 +1267,61 @@ describe.concurrent("hand-edited bun.lock that lists workspaces but has no packa }); }); +// The commands that only read the lockfile name the step that failed and stop, without touching the registry. +describe.concurrent("a bun.lock that does not parse", () => { + const projectFiles = { + "package.json": JSON.stringify({ name: "unparsable-lockfile", dependencies: { "no-deps": "1.0.0" } }), + "bun.lock": "this is not a lockfile\n", + }; + + async function run(prefix: string, ...args: string[]) { + using dir = tempDir(prefix, projectFiles); + await using proc = spawn({ + cmd: [bunExe(), ...args], + cwd: String(dir), + env, + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { out: normalizeBunSnapshot(out, String(dir)), err: normalizeBunSnapshot(err, String(dir)), exitCode }; + } + + it("bun outdated", async () => { + const { out, err, exitCode } = await run("unparsable-lockfile-outdated", "outdated"); + expect(err).toMatchInlineSnapshot(` + "1 | this is not a lockfile + ^ + error: Unexpected this + at bun.lock:1:1 + error: failed to parse lockfile: ParserError" + `); + expect(out).toMatchInlineSnapshot(`"bun outdated ()"`); + expect(exitCode).toBe(1); + }); + + it("bun update --interactive", async () => { + const { out, err, exitCode } = await run("unparsable-lockfile-update-interactive", "update", "--interactive"); + expect(err).toMatchInlineSnapshot(` + "1 | this is not a lockfile + ^ + error: Unexpected this + at bun.lock:1:1 + error: failed to parse lockfile: ParserError" + `); + expect(out).toMatchInlineSnapshot(`"bun update --interactive ()"`); + expect(exitCode).toBe(1); + }); + + it("bun pm ls", async () => { + const { out, err, exitCode } = await run("unparsable-lockfile-pm-ls", "pm", "ls"); + expect(err).toMatchInlineSnapshot(`"error: failed to parse lockfile: ParserError"`); + expect(out).toMatchInlineSnapshot(`""`); + expect(exitCode).toBe(1); + }); +}); + const makeInstallRunner = (cwd: string) => async (args: string[]) => { await using proc = spawn({ cmd: [bunExe(), ...args], diff --git a/test/cli/install/bun-pack.test.ts b/test/cli/install/bun-pack.test.ts index 34f42ac8a360..57964bc3c85e 100644 --- a/test/cli/install/bun-pack.test.ts +++ b/test/cli/install/bun-pack.test.ts @@ -837,6 +837,50 @@ test("lifecycle script modifying version updates tarball filename (#17195)", asy ]); }); +describe.concurrent("package.json that cannot be loaded", () => { + test("unparsable package.json", async () => { + using dir = tempDir("pack-unparsable-package-json", { + "package.json": `{ "name": "pack-unparsable", "version": "1.0.0",`, + }); + + const { err } = await packExpectError(String(dir), bunEnv); + // the parser's own diagnostic is printed along with the error + expect(err).toMatch(/at .*package\.json:1:\d+/); + expect(err).toMatch(/^ParserError: failed to parse package\.json: .*package\.json$/m); + }); + + test("prepack script removes package.json before it is re-read", async () => { + using dir = tempDir("pack-prepack-removes-package-json", { + "package.json": JSON.stringify({ + name: "pack-prepack-removes", + version: "1.0.0", + scripts: { prepack: `${bunExe()} remove.js` }, + }), + "remove.js": `require("fs").unlinkSync("package.json");`, + }); + + const { err } = await packExpectError(String(dir), bunEnv); + expect(err).toMatch(/^ENOENT: failed to read package\.json: .*package\.json$/m); + expect(await exists(join(String(dir), "pack-prepack-removes-1.0.0.tgz"))).toBeFalse(); + }); + + test("prepack script leaves package.json unparsable before it is re-read", async () => { + using dir = tempDir("pack-prepack-breaks-package-json", { + "package.json": JSON.stringify({ + name: "pack-prepack-breaks", + version: "1.0.0", + scripts: { prepack: `${bunExe()} break.js` }, + }), + "break.js": `require("fs").writeFileSync("package.json", "{ broken\\n");`, + }); + + const { err } = await packExpectError(String(dir), bunEnv); + expect(err).toMatch(/at .*package\.json:1:\d+/); + expect(err).toMatch(/^ParserError: failed to parse package\.json: .*package\.json$/m); + expect(await exists(join(String(dir), "pack-prepack-breaks-1.0.0.tgz"))).toBeFalse(); + }); +}); + describe("bundledDependnecies", () => { for (const bundledDependencies of ["bundledDependencies", "bundleDependencies"]) { test(`basic (${bundledDependencies})`, async () => { diff --git a/test/cli/install/bun-update-transitive.test.ts b/test/cli/install/bun-update-transitive.test.ts index 43d7ae599a80..b29f32368601 100644 --- a/test/cli/install/bun-update-transitive.test.ts +++ b/test/cli/install/bun-update-transitive.test.ts @@ -2465,3 +2465,17 @@ test.concurrent("`bun update -i --latest` honours an entry toggled back to its i await frozen(dir); expect(exitCode).toBe(0); }); + +// A member whose package.json no longer parses is reported and skipped; the other members are still written. +test.concurrent( + "`bun update -i -r` reports a member whose package.json cannot be parsed and still updates the rest", + async () => { + const { dir } = await staleMembers("~1.0.0", "~1.0.0"); + await write(join(dir, "packages/pkg2/package.json"), "{ broken\n"); + const { stderr, exitCode } = await runInteractive(dir, "a\r", "-r"); + expect(stderr).toMatch(/^error: Failed to parse package\.json at .*pkg2[\\/]package\.json: ParserError$/m); + expect(await packageJsonOf(dir, "packages/pkg1")).toStrictEqual(member("pkg1", { "no-deps": "~1.0.1" })); + expect(await packageJsonText(dir, "packages/pkg2")).toBe("{ broken\n"); + expect(exitCode).toBe(1); + }, +);