diff --git a/src/jsc/JSGlobalObject.rs b/src/jsc/JSGlobalObject.rs index 804bdabdaa80..6033ffe24728 100644 --- a/src/jsc/JSGlobalObject.rs +++ b/src/jsc/JSGlobalObject.rs @@ -541,6 +541,7 @@ impl JSGlobalObject { } /// "The {argname} argument must be of type {typename}. Received {value}" + /// (a dotted `argname` is a "property", see `InvalidArgTypeName`). /// /// Accepts `&str`, `&[u8]`, or `b"..."` for `argname`/`typename`. pub fn throw_invalid_argument_type_value( @@ -556,8 +557,8 @@ impl JSGlobalObject { self.err( JscError::INVALID_ARG_TYPE, format_args!( - "The \"{}\" argument must be of type {}. Received {}", - bstr::BStr::new(argname.as_ref()), + "The {} must be of type {}. Received {}", + InvalidArgTypeName(argname.as_ref()), bstr::BStr::new(typename.as_ref()), actual_string_value ), @@ -565,6 +566,8 @@ impl JSGlobalObject { .throw() } + /// Like [`Self::throw_invalid_argument_type_value`], but `typename` is the whole + /// phrase after "must be" (e.g. `"an instance of Array"`). pub fn throw_invalid_argument_type_value2( &self, argname: impl AsRef<[u8]>, @@ -578,8 +581,8 @@ impl JSGlobalObject { self.err( JscError::INVALID_ARG_TYPE, format_args!( - "The \"{}\" argument must be {}. Received {}", - bstr::BStr::new(argname.as_ref()), + "The {} must be {}. Received {}", + InvalidArgTypeName(argname.as_ref()), bstr::BStr::new(typename.as_ref()), actual_string_value ), @@ -627,8 +630,8 @@ impl JSGlobalObject { self.err( JscError::INVALID_ARG_TYPE, format_args!( - "The \"{}\" argument must be one of type {}. Received {}", - bstr::BStr::new(argname.as_ref()), + "The {} must be one of type {}. Received {}", + InvalidArgTypeName(argname.as_ref()), bstr::BStr::new(typename.as_ref()), actual_string_value ), @@ -1380,6 +1383,26 @@ pub struct SysErrOptions { pub name: Option<&'static [u8]>, } +/// The subject of Node's `ERR_INVALID_ARG_TYPE` message: `"name" argument`, +/// `"options.name" property` for a dotted name, or the name verbatim when it +/// already ends in ` argument` ("first argument"). Same rule as +/// `Bun::Message::addParameter` in ErrorCode.cpp. +/// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/errors.js#L1407-L1414 +struct InvalidArgTypeName<'a>(&'a [u8]); + +impl core::fmt::Display for InvalidArgTypeName<'_> { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + let name = bstr::BStr::new(self.0); + if self.0.ends_with(b" argument") { + write!(f, "{name}") + } else if strings::contains_char(self.0, b'.') { + write!(f, "\"{name}\" property") + } else { + write!(f, "\"{name}\" argument") + } + } +} + // Unified with the crate-root definitions (lib.rs) — re-exported here so // `bun_jsc::js_global_object::{IntegerRange, ValidateObjectOpts}` keep // resolving for any caller that named them via this path. The previous local diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index dd6773bdc442..45983eaed688 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -3220,6 +3220,17 @@ pub mod args { Ok(encoding) } + /// Node's `validateBoolean(value, name)`, where `name` is the dotted path + /// node reports (`"options.recursive"`). `validators::validate_boolean` is + /// not used here because it still renders Bun's older `..., got ` + /// wording and node:fs errors have to match node's text. + fn validate_boolean_option(ctx: &JSGlobalObject, value: JSValue, name: &str) -> JsResult { + if value.is_boolean() { + return Ok(value.as_boolean()); + } + Err(ctx.throw_invalid_argument_type_value(name, "boolean", value)) + } + pub struct Unlink { pub path: PathLike, } @@ -3304,23 +3315,13 @@ pub mod args { val.get(ctx, name) } }; - if let Some(boolean) = get_option("recursive")? { - if boolean.is_boolean() { - recursive = boolean.to_boolean(); - } else { - return Err(ctx.throw_invalid_arguments(format_args!( - "The \"options.recursive\" property must be of type boolean." - ))); - } + // Validated in the order node's `validateRmOptions` uses, so an + // options bag with several bad values reports the same one. + if let Some(force_) = get_option("force")? { + force = validate_boolean_option(ctx, force_, "options.force")?; } - if let Some(boolean) = get_option("force")? { - if boolean.is_boolean() { - force = boolean.to_boolean(); - } else { - return Err(ctx.throw_invalid_arguments(format_args!( - "The \"options.force\" property must be of type boolean." - ))); - } + if let Some(recursive_) = get_option("recursive")? { + recursive = validate_boolean_option(ctx, recursive_, "options.recursive")?; } if let Some(delay) = get_option("retryDelay")? { retry_delay = c_uint::try_from(validators::validate_integer( @@ -3343,9 +3344,7 @@ pub mod args { .expect("infallible: validated range"); } } else if !val.is_undefined() { - return Err(ctx.throw_invalid_arguments(format_args!( - "The \"options\" argument must be of type object." - ))); + return Err(ctx.throw_invalid_argument_type_value(b"options", b"object", val)); } } Ok(RmDir { @@ -3392,8 +3391,8 @@ pub mod args { if let Some(val) = arguments.next() { arguments.eat(); if val.is_object() { - if let Some(b) = val.get_boolean_strict(ctx, "recursive")? { - recursive = b; + if let Some(recursive_) = val.get(ctx, "recursive")? { + recursive = validate_boolean_option(ctx, recursive_, "options.recursive")?; } if let Some(mode_) = val.get(ctx, "mode")? { mode = node::mode_from_js(ctx, mode_)?.unwrap_or(mode); @@ -3485,11 +3484,16 @@ pub mod args { _ => { if val.is_object() { encoding = get_encoding(val, ctx, encoding)?; - if let Some(r) = val.get_boolean_strict(ctx, "recursive")? { - recursive = r; + if let Some(recursive_) = val.get(ctx, "recursive")? { + recursive = + validate_boolean_option(ctx, recursive_, "options.recursive")?; } - if let Some(w) = val.get_boolean_strict(ctx, "withFileTypes")? { - with_file_types = w; + if let Some(with_file_types_) = val.get(ctx, "withFileTypes")? { + with_file_types = validate_boolean_option( + ctx, + with_file_types_, + "options.withFileTypes", + )?; } } } diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 7fd0be69245b..e3432057fa61 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -1034,7 +1034,179 @@ describe("mkdirSync", () => { // @ts-expect-error { recursive: "lalala" }, ), - ).toThrow('The "recursive" property must be of type boolean, got string'); + ).toThrow(`The "options.recursive" property must be of type boolean. Received type string ('lalala')`); + }); +}); + +// Node validates these option-bag booleans with `validateBoolean(value, 'options.')`, +// so the message names the option as a property and ends with node's description of +// the value that was received. Every message below is node v26.3.0's text. +describe("fs boolean options report ERR_INVALID_ARG_TYPE like node", () => { + type Thrown = { name: string; code: string; message: string }; + const pick = (e: any): Thrown => ({ name: e.name, code: e.code, message: e.message }); + const invalidArgType = (message: string): Thrown => ({ name: "TypeError", code: "ERR_INVALID_ARG_TYPE", message }); + const recursiveMessage = (received: string) => + `The "options.recursive" property must be of type boolean. Received ${received}`; + + function thrownBy(fn: () => unknown): Thrown { + try { + fn(); + } catch (e) { + return pick(e); + } + throw new Error("expected the call to throw"); + } + async function rejectedBy(promise: Promise): Promise { + return promise.then( + () => { + throw new Error("expected the promise to reject"); + }, + e => pick(e), + ); + } + // The callback APIs hand the error to the callback or, like node for these + // options, throw it synchronously; either way it is the native parser's error. + function failedCallback(call: (callback: (err: any) => void) => void): Promise { + const { promise, resolve, reject } = Promise.withResolvers(); + try { + call(err => (err ? resolve(pick(err)) : reject(new Error("expected the callback to receive an error")))); + } catch (e) { + resolve(pick(e)); + } + return promise; + } + + describe("mkdir options.recursive", () => { + const cases: [value: unknown, received: string][] = [ + ["x", "type string ('x')"], + [1, "type number (1)"], + [null, "null"], + [[], "an instance of Array"], + ]; + + it.each(cases)("mkdirSync(path, { recursive: %p })", (recursive, received) => { + using dir = tempDir("fs-mkdir-recursive-option", {}); + const target = join(String(dir), "a", "b"); + expect(thrownBy(() => mkdirSync(target, { recursive } as any))).toEqual( + invalidArgType(recursiveMessage(received)), + ); + expect(existsSync(target)).toBe(false); + }); + + it("mkdir (callback) and promises.mkdir use the same message", async () => { + using dir = tempDir("fs-mkdir-recursive-option-async", {}); + const target = join(String(dir), "a", "b"); + expect(await failedCallback(cb => fs.mkdir(target, { recursive: "x" } as any, cb))).toEqual( + invalidArgType(recursiveMessage("type string ('x')")), + ); + expect(await rejectedBy(promises.mkdir(target, { recursive: 1 } as any))).toEqual( + invalidArgType(recursiveMessage("type number (1)")), + ); + expect(existsSync(target)).toBe(false); + }); + }); + + describe("readdir options.recursive / options.withFileTypes", () => { + const withFileTypesMessage = (received: string) => + `The "options.withFileTypes" property must be of type boolean. Received ${received}`; + + it("readdirSync", () => { + using dir = tempDir("fs-readdir-boolean-options", { "file.txt": "" }); + const d = String(dir); + expect(thrownBy(() => readdirSync(d, { recursive: "x" } as any))).toEqual( + invalidArgType(recursiveMessage("type string ('x')")), + ); + expect(thrownBy(() => readdirSync(d, { recursive: {} } as any))).toEqual( + invalidArgType(recursiveMessage("an instance of Object")), + ); + // node does not validate withFileTypes (it coerces it); Bun keeps rejecting + // a non-boolean here, using the same wording it would have in node. + expect(thrownBy(() => readdirSync(d, { withFileTypes: "x" } as any))).toEqual( + invalidArgType(withFileTypesMessage("type string ('x')")), + ); + // recursive is validated first, as in node. + expect(thrownBy(() => readdirSync(d, { recursive: "x", withFileTypes: "y" } as any))).toEqual( + invalidArgType(recursiveMessage("type string ('x')")), + ); + // Valid values still work through the same parser. + expect(readdirSync(d, { recursive: false, withFileTypes: false })).toEqual(["file.txt"]); + }); + + it("readdir (callback) and promises.readdir use the same message", async () => { + using dir = tempDir("fs-readdir-boolean-options-async", { "file.txt": "" }); + const d = String(dir); + expect(await failedCallback(cb => fs.readdir(d, { recursive: "x" } as any, cb))).toEqual( + invalidArgType(recursiveMessage("type string ('x')")), + ); + expect(await failedCallback(cb => fs.readdir(d, { withFileTypes: 1 } as any, cb))).toEqual( + invalidArgType(withFileTypesMessage("type number (1)")), + ); + expect(await rejectedBy(promises.readdir(d, { recursive: "x" } as any))).toEqual( + invalidArgType(recursiveMessage("type string ('x')")), + ); + expect(await rejectedBy(promises.readdir(d, { withFileTypes: null } as any))).toEqual( + invalidArgType(withFileTypesMessage("null")), + ); + }); + }); + + describe("rm options", () => { + const forceMessage = (received: string) => + `The "options.force" property must be of type boolean. Received ${received}`; + const optionsMessage = (received: string) => `The "options" argument must be of type object. Received ${received}`; + + it("rmSync", () => { + using dir = tempDir("fs-rm-boolean-options", { "file.txt": "" }); + const file = join(String(dir), "file.txt"); + expect(thrownBy(() => rmSync(file, { recursive: "x" } as any))).toEqual( + invalidArgType(recursiveMessage("type string ('x')")), + ); + // node spreads the caller's options over its defaults, so an own key holding + // undefined is validated (and rejected) too. + expect(thrownBy(() => rmSync(file, { recursive: undefined }))).toEqual( + invalidArgType(recursiveMessage("undefined")), + ); + expect(thrownBy(() => rmSync(file, { force: "x" } as any))).toEqual( + invalidArgType(forceMessage("type string ('x')")), + ); + expect(thrownBy(() => rmSync(file, { force: null } as any))).toEqual(invalidArgType(forceMessage("null"))); + // node's validateRmOptions checks force before recursive. + expect(thrownBy(() => rmSync(file, { recursive: "x", force: "y" } as any))).toEqual( + invalidArgType(forceMessage("type string ('y')")), + ); + expect(thrownBy(() => rmSync(file, "x" as any))).toEqual(invalidArgType(optionsMessage("type string ('x')"))); + expect(thrownBy(() => rmSync(file, null as any))).toEqual(invalidArgType(optionsMessage("null"))); + // Validation fails before anything is removed. + expect(existsSync(file)).toBe(true); + }); + + it("rm (callback) and promises.rm use the same messages", async () => { + using dir = tempDir("fs-rm-boolean-options-async", { "file.txt": "" }); + const file = join(String(dir), "file.txt"); + expect(await failedCallback(cb => fs.rm(file, { recursive: "x" } as any, cb))).toEqual( + invalidArgType(recursiveMessage("type string ('x')")), + ); + expect(await failedCallback(cb => fs.rm(file, { force: 1 } as any, cb))).toEqual( + invalidArgType(forceMessage("type number (1)")), + ); + expect(await rejectedBy(promises.rm(file, { recursive: "x", force: "y" } as any))).toEqual( + invalidArgType(forceMessage("type string ('y')")), + ); + expect(await rejectedBy(promises.rm(file, { recursive: [] } as any))).toEqual( + invalidArgType(recursiveMessage("an instance of Array")), + ); + expect(await rejectedBy(promises.rm(file, "x" as any))).toEqual( + invalidArgType(optionsMessage("type string ('x')")), + ); + expect(existsSync(file)).toBe(true); + }); + + it("rmdirSync shares the options argument check", () => { + using dir = tempDir("fs-rmdir-options-argument", { "sub": {} }); + const sub = join(String(dir), "sub"); + expect(thrownBy(() => rmdirSync(sub, "x" as any))).toEqual(invalidArgType(optionsMessage("type string ('x')"))); + expect(existsSync(sub)).toBe(true); + }); }); }); diff --git a/test/js/node/test/parallel/test-fs-mkdir.js b/test/js/node/test/parallel/test-fs-mkdir.js index 68c49f659aee..89b8b436d5c9 100644 --- a/test/js/node/test/parallel/test-fs-mkdir.js +++ b/test/js/node/test/parallel/test-fs-mkdir.js @@ -248,8 +248,8 @@ if (common.isMainThread && (common.isLinux || common.isMacOS)) { { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError', - message: typeof Bun === 'undefined' ? 'The "options.recursive" property must be of type boolean.' + - received : 'The "recursive" property must be of type boolean, got ' +typeof recursive, + message: 'The "options.recursive" property must be of type boolean.' + + received } ); assert.throws( @@ -257,8 +257,8 @@ if (common.isMainThread && (common.isLinux || common.isMacOS)) { { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError', - message: typeof Bun === 'undefined' ? 'The "options.recursive" property must be of type boolean.' + - received : 'The "recursive" property must be of type boolean, got ' +typeof recursive, + message: 'The "options.recursive" property must be of type boolean.' + + received } ); });