From a1ab58d1c3bd5ac48805283c863745407ccc0904 Mon Sep 17 00:00:00 2001 From: TheHypnoo Date: Mon, 31 Aug 2026 00:40:19 +0200 Subject: [PATCH 1/2] fix(assert): stop regexp matcher fallthrough --- crates/perry-runtime/src/object/assert.rs | 48 +++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/crates/perry-runtime/src/object/assert.rs b/crates/perry-runtime/src/object/assert.rs index 43a1b7a3df..eccd1df092 100644 --- a/crates/perry-runtime/src/object/assert.rs +++ b/crates/perry-runtime/src/object/assert.rs @@ -267,6 +267,11 @@ fn expected_error_matches(thrown: f64, expected: f64) -> bool { if !is_null_or_undefined(message) && regex_test_value(expected, message).unwrap_or(false) { return true; } + // A RegExp is a complete matcher category. Falling through would + // incorrectly pass the RegExp object to `instanceof`, which throws a + // TypeError instead of letting assert.throws/assert.rejects report an + // AssertionError for a non-matching pattern. + return false; } // A plain object validator (e.g. `{ code: "ERR_X" }`) is a property-bag // matcher, never a constructor — its own enumerable keys must each equal @@ -1304,3 +1309,46 @@ pub extern "C" fn js_assert_if_error(value: f64) -> f64 { } throw_assertion(if_error_message(value), value, null_f64(), "ifError", false) } + +#[cfg(test)] +mod tests { + use super::*; + + extern "C" fn throw_nope(_closure: *const crate::closure::ClosureHeader) -> f64 { + let message = crate::string::js_string_from_bytes(b"nope".as_ptr(), 4); + let error = crate::error::js_error_new_with_message(message); + crate::exception::js_throw(crate::value::js_nanbox_pointer(error as i64)) + } + + #[test] + fn throws_non_matching_regexp_reports_assertion_error() { + let block = crate::closure::js_closure_alloc(throw_nope as *const u8, 0); + crate::closure::js_register_closure_arity(throw_nope as *const u8, 0); + let pattern = crate::string::js_string_from_bytes(b"will-not-match".as_ptr(), 14); + let flags = crate::string::js_string_from_bytes(b"".as_ptr(), 0); + let regexp = crate::regex::js_regexp_new(pattern, flags); + + let trap = crate::exception::js_try_push(); + let jumped = unsafe { crate::ffi::setjmp::setjmp(trap as *mut c_int) }; + if jumped == 0 { + js_assert_throws( + crate::value::js_nanbox_pointer(block as i64), + crate::value::js_nanbox_pointer(regexp as i64), + undefined_f64(), + ); + panic!("a non-matching RegExp must throw"); + } + + crate::exception::js_try_end(); + let error = crate::exception::js_get_exception(); + crate::exception::js_clear_exception(); + assert_eq!( + value_to_string(read_property(error, "name")), + "AssertionError" + ); + assert_eq!( + value_to_string(read_property(error, "code")), + "ERR_ASSERTION" + ); + } +} From 10977c0ab179e3299ee1a6d3511bc04b7dc0d613 Mon Sep 17 00:00:00 2001 From: TheHypnoo Date: Mon, 31 Aug 2026 13:28:09 +0200 Subject: [PATCH 2/2] fix(assert): test a RegExp matcher against String(thrown), and make it terminal The previous commit on this branch added `return false` after the RegExp block. That stopped the fallthrough into `instanceof`, but kept the line that caused it: a retry of the pattern against `thrown.message`. Node tests a RegExp matcher against `String(thrown)` and nothing else, and treats RegExp as a terminal matcher category. The whole block collapses to one statement, which fixes both halves at the single point `throws`, `doesNotThrow`, `rejects`, and `doesNotReject` all route through: - An anchored pattern matching the bare message but not the stringified error was wrongly accepted. `assert.throws(() => { throw new Error("nope") }, /^nope/)` passed, where Node reports an AssertionError because `String(err)` is "Error: nope". Same for a thrown non-error carrying a `message` property, whose `String()` is "[object Object]". - A pattern matching neither reached `js_instanceof_dynamic(thrown, regexp)` and surfaced as a TypeError instead of an AssertionError, so throws/rejects reported the wrong error class and doesNotThrow/doesNotReject failed to rethrow the original error. A RegExp value on a validator key (`{ message: /bad/ }`) is a different matcher and is still tested against that property in `object_matcher_matches`. The `thrown.message` retry dates to the original implementation in #1924 and was never a workaround for anything else. The pre-existing `assert/errors/strict-throws-validation.ts` could not catch either bug: `/will-not-match/` matches neither input, so it cannot tell them apart, and it prints only `name` and `code`. The new fixture covers both inputs across all four entry points and matches Node 26.5.1 byte for byte. The unit tests replace the setjmp-based one, which the previous fix also passed. Fixes #9227 Refs #9202 Claude-Session: https://claude.ai/code/session_01QFubuLjMeJBhrHSfCuoPMp --- .../9228-assert-regexp-matcher-input.md | 32 ++++++ crates/perry-runtime/src/object/assert.rs | 104 ++++++++++-------- .../errors/throws-regexp-matcher-input.ts | 57 ++++++++++ 3 files changed, 148 insertions(+), 45 deletions(-) create mode 100644 changelog.d/9228-assert-regexp-matcher-input.md create mode 100644 test-parity/node-suite/assert/errors/throws-regexp-matcher-input.ts diff --git a/changelog.d/9228-assert-regexp-matcher-input.md b/changelog.d/9228-assert-regexp-matcher-input.md new file mode 100644 index 0000000000..849fb2b2c6 --- /dev/null +++ b/changelog.d/9228-assert-regexp-matcher-input.md @@ -0,0 +1,32 @@ +`assert.throws(fn, /re/)` and friends now test the RegExp matcher the way Node +does: against `String(thrown)` only, with a RegExp treated as a terminal +matcher category. + +`expected_error_matches` used to give a non-matching pattern a second chance +against `thrown.message`, then fall through to the `instanceof` / +constructor-name / validation-function checks when that also failed. Two bugs +came out of one block: + +- A pattern that matched the bare message but not the stringified error was + wrongly accepted. `assert.throws(() => { throw new Error("nope") }, /^nope/)` + passed, where Node reports an `AssertionError` because `String(err)` is + `"Error: nope"`. Same for a thrown non-error carrying a `message` property. +- A pattern that matched nothing reached `js_instanceof_dynamic(thrown, regexp)` + and surfaced as a `TypeError` instead of an `AssertionError`, so + `assert.throws`/`rejects` reported the wrong error class and + `doesNotThrow`/`doesNotReject` failed to rethrow the original error. + +A RegExp *value on a validator key* (`{ message: /bad/ }`) is a different +matcher and still tests against that property. + +The pre-existing `assert/errors/strict-throws-validation.ts` could not catch +either bug: its pattern matched both the message and the stringified error, and +it only printed `name`/`code`. `assert/errors/throws-regexp-matcher-input.ts` +covers the two inputs separately across `throws`, `doesNotThrow`, `rejects`, +and `doesNotReject`. + +Known remaining gap: the generated `AssertionError` message is Perry's generic +"The thrown error did not match the expected matcher" rather than Node's +"The input did not match the regular expression /x/. Input:\n\n'Error: nope'\n". +That generic fallback is shared by every matcher category, so it is left for a +separate change. diff --git a/crates/perry-runtime/src/object/assert.rs b/crates/perry-runtime/src/object/assert.rs index eccd1df092..f7dc77a7bf 100644 --- a/crates/perry-runtime/src/object/assert.rs +++ b/crates/perry-runtime/src/object/assert.rs @@ -259,19 +259,18 @@ fn expected_error_matches(thrown: f64, expected: f64) -> bool { if is_null_or_undefined(expected) { return true; } + // A RegExp matcher is a complete, terminal matcher category: Node tests it + // against `String(thrown)` and nothing else, and a non-match is an + // AssertionError — never a fallthrough to the instanceof / + // constructor-name / validation-function checks below (`instanceof` + // against a RegExp throws a TypeError). Retrying the pattern against + // `thrown.message` was a second, non-Node chance that wrongly accepted + // anchored patterns: `/^nope/` matched `new Error("nope")` even though + // `String(err)` is `"Error: nope"`. A RegExp *value on a validator key* + // (`{ message: /bad/ }`) is a different thing and is still tested against + // that property in `object_matcher_matches`. if let Some(matches_thrown) = regex_test_value(expected, thrown) { - if matches_thrown { - return true; - } - let message = read_property(thrown, "message"); - if !is_null_or_undefined(message) && regex_test_value(expected, message).unwrap_or(false) { - return true; - } - // A RegExp is a complete matcher category. Falling through would - // incorrectly pass the RegExp object to `instanceof`, which throws a - // TypeError instead of letting assert.throws/assert.rejects report an - // AssertionError for a non-matching pattern. - return false; + return matches_thrown; } // A plain object validator (e.g. `{ code: "ERR_X" }`) is a property-bag // matcher, never a constructor — its own enumerable keys must each equal @@ -1310,45 +1309,60 @@ pub extern "C" fn js_assert_if_error(value: f64) -> f64 { throw_assertion(if_error_message(value), value, null_f64(), "ifError", false) } -#[cfg(test)] -mod tests { +#[cfg(all(test, feature = "regex-engine"))] +mod regexp_matcher_tests { use super::*; - extern "C" fn throw_nope(_closure: *const crate::closure::ClosureHeader) -> f64 { - let message = crate::string::js_string_from_bytes(b"nope".as_ptr(), 4); - let error = crate::error::js_error_new_with_message(message); - crate::exception::js_throw(crate::value::js_nanbox_pointer(error as i64)) + fn jsstr(bytes: &[u8]) -> *mut crate::StringHeader { + crate::string::js_string_from_bytes(bytes.as_ptr(), bytes.len() as u32) } + fn error_value(message: &[u8]) -> f64 { + let err = crate::error::js_error_new_with_message(jsstr(message)); + crate::value::js_nanbox_pointer(err as i64) + } + + fn regexp_value(pattern: &[u8]) -> f64 { + let re = crate::regex::js_regexp_new(jsstr(pattern), jsstr(b"")); + crate::value::js_nanbox_pointer(re as i64) + } + + /// Node tests a RegExp matcher against `String(thrown)` only. An anchored + /// pattern that matches the bare message must NOT match, because + /// `String(new Error("nope"))` is `"Error: nope"`. #[test] - fn throws_non_matching_regexp_reports_assertion_error() { - let block = crate::closure::js_closure_alloc(throw_nope as *const u8, 0); - crate::closure::js_register_closure_arity(throw_nope as *const u8, 0); - let pattern = crate::string::js_string_from_bytes(b"will-not-match".as_ptr(), 14); - let flags = crate::string::js_string_from_bytes(b"".as_ptr(), 0); - let regexp = crate::regex::js_regexp_new(pattern, flags); - - let trap = crate::exception::js_try_push(); - let jumped = unsafe { crate::ffi::setjmp::setjmp(trap as *mut c_int) }; - if jumped == 0 { - js_assert_throws( - crate::value::js_nanbox_pointer(block as i64), - crate::value::js_nanbox_pointer(regexp as i64), - undefined_f64(), - ); - panic!("a non-matching RegExp must throw"); - } + fn regexp_matcher_tests_the_stringified_error_not_the_message() { + let thrown = error_value(b"nope"); + assert!(!expected_error_matches(thrown, regexp_value(b"^nope"))); + assert!(expected_error_matches( + thrown, + regexp_value(b"^Error: nope$") + )); + } - crate::exception::js_try_end(); - let error = crate::exception::js_get_exception(); - crate::exception::js_clear_exception(); - assert_eq!( - value_to_string(read_property(error, "name")), - "AssertionError" - ); - assert_eq!( - value_to_string(read_property(error, "code")), - "ERR_ASSERTION" + /// A RegExp is a terminal matcher category: a non-match returns `false` so + /// the caller reports an `AssertionError`, instead of falling through to + /// `js_instanceof_dynamic(thrown, regexp)`, which throws a `TypeError`. + #[test] + fn non_matching_regexp_is_terminal() { + assert!(!expected_error_matches( + error_value(b"nope"), + regexp_value(b"will-not-match") + )); + } + + /// A RegExp *value on a validator key* is a different matcher and is still + /// tested against that property, not against the stringified error. + #[test] + fn validator_key_regexp_still_tests_the_property() { + let thrown = error_value(b"bad value"); + let validator = crate::object::js_object_alloc(0, 1); + crate::object::js_object_set_field_by_name( + validator, + jsstr(b"message"), + regexp_value(b"^bad"), ); + let validator = crate::value::js_nanbox_pointer(validator as i64); + assert!(expected_error_matches(thrown, validator)); } } diff --git a/test-parity/node-suite/assert/errors/throws-regexp-matcher-input.ts b/test-parity/node-suite/assert/errors/throws-regexp-matcher-input.ts new file mode 100644 index 0000000000..2d6d0885cc --- /dev/null +++ b/test-parity/node-suite/assert/errors/throws-regexp-matcher-input.ts @@ -0,0 +1,57 @@ +// A RegExp matcher is tested against `String(thrown)` and nothing else, and it +// is a terminal matcher category: a non-match is an AssertionError, never a +// fallthrough to the instanceof / constructor / validation-function checks. +// The pre-existing `strict-throws-validation.ts` only exercised a pattern that +// matches the message *and* the stringified error, so it could not tell the +// two inputs apart. +import assert from "node:assert"; + +function show(label: string, fn: () => void): void { + try { + fn(); + console.log(label + ": pass"); + } catch (err: any) { + console.log(label + ":", err?.name, err?.code ?? err?.operator ?? "no-code"); + } +} + +// `String(new Error("nope"))` is "Error: nope", so an anchored pattern that +// only matches the bare message must NOT match. +show("anchored message-only pattern", () => + assert.throws(() => { throw new Error("nope"); }, /^nope/)); + +// The same pattern anchored against the stringified error does match. +show("anchored stringified pattern", () => + assert.throws(() => { throw new Error("nope"); }, /^Error: nope$/)); + +// A plain object is stringified to "[object Object]"; its `message` property +// is not a second chance for the pattern. +show("plain object with message prop", () => + assert.throws(() => { throw { message: "nope" }; }, /^nope/)); + +// A thrown primitive stringifies to itself. +show("thrown string", () => + assert.throws(() => { throw "nope"; }, /^nope$/)); + +// doesNotThrow with a non-matching RegExp rethrows the original error rather +// than reporting an AssertionError (and must not raise a TypeError from a +// fallthrough into `instanceof`). +show("doesNotThrow non-matching rethrows", () => + assert.doesNotThrow(() => { throw new Error("nope"); }, /will-not-match/)); + +show("doesNotThrow matching reports", () => + assert.doesNotThrow(() => { throw new Error("nope"); }, /nope/)); + +// A RegExp value on a validator *key* is a different matcher: it is tested +// against that property, so an anchored message pattern does match there. +show("validator key regexp", () => + assert.throws(() => { throw new TypeError("bad value"); }, { message: /^bad/ })); + +// Async paths route through the same matcher. +await assert.rejects(async () => { throw new Error("nope"); }, /^nope/).then( + () => console.log("rejects anchored: pass"), + (err: any) => console.log("rejects anchored:", err?.name, err?.code ?? err?.operator)); + +await assert.doesNotReject(async () => { throw new Error("nope"); }, /will-not-match/).then( + () => console.log("doesNotReject non-matching: pass"), + (err: any) => console.log("doesNotReject non-matching:", err?.name, err?.code ?? "no-code"));