Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions src/jsc/JSGlobalObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -556,15 +557,17 @@ 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
),
)
.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]>,
Expand All @@ -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
),
Expand Down Expand Up @@ -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
),
Expand Down Expand Up @@ -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
Expand Down
54 changes: 29 additions & 25 deletions src/runtime/node/node_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3220,6 +3220,17 @@
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 <typeof>`
/// wording and node:fs errors have to match node's text.
fn validate_boolean_option(ctx: &JSGlobalObject, value: JSValue, name: &str) -> JsResult<bool> {
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,
}
Expand Down Expand Up @@ -3304,24 +3315,14 @@
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")?;
}

Check warning on line 3325 in src/runtime/node/node_fs.rs

View check run for this annotation

Claude / Claude Code Review

rmdir force/recursive reorder regresses Node compat when both are bad

The `force`-before-`recursive` reorder is correct for `fs.rm` but this parser is shared with `fs.rmdir` (via `RmDir::from_js` → `from_js_impl(ctx, arguments, false)`), and Node's `validateRmdirOptions` does not validate `force` at all — so `fs.rmdirSync(d, { recursive: 'x', force: 'y' })` now names `options.force` where both Node and pre-PR Bun name `options.recursive`. Gating the `force` check on `strict_booleans` (which is exactly what Node's rmdir does) would keep the rm fix without this rmdi
Comment thread
robobun marked this conversation as resolved.
if let Some(delay) = get_option("retryDelay")? {
retry_delay = c_uint::try_from(validators::validate_integer(
ctx,
Expand All @@ -3343,9 +3344,7 @@
.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 {
Expand Down Expand Up @@ -3392,8 +3391,8 @@
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);
Expand Down Expand Up @@ -3485,11 +3484,16 @@
_ => {
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",
)?;
}
}
}
Expand Down
174 changes: 173 additions & 1 deletion test/js/node/fs/fs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.<name>')`,
// 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<unknown>): Promise<Thrown> {
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<Thrown> {
const { promise, resolve, reject } = Promise.withResolvers<Thrown>();
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);
});
});
});

Expand Down
8 changes: 4 additions & 4 deletions test/js/node/test/parallel/test-fs-mkdir.js
Original file line number Diff line number Diff line change
Expand Up @@ -248,17 +248,17 @@ 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(
() => fs.mkdirSync(pathname, common.mustNotMutateObjectDeep({ recursive })),
{
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
}
);
});
Expand Down
Loading