Skip to content

node:assert: add Node's answers for boxed strings to the deep-equality matrix - #47638

Open
robobun wants to merge 3 commits into
mainfrom
robobun/d3fabd31/boxed-string-matrix-rows
Open

node:assert: add Node's answers for boxed strings to the deep-equality matrix#47638
robobun wants to merge 3 commits into
mainfrom
robobun/d3fabd31/boxed-string-matrix-rows

Conversation

@robobun

@robobun robobun commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Behaviour change: none

Problem

  • assert.deepEqual(new String("abc"), new S("abc")) (class S extends String {}) throws ERR_ASSERTION. Node v26.3.0 passes it. The matrix in test/js/node/assert/deep-equal.test.ts has no row for it or for the pairs near it.
  • The comment on the String arm of specialObjectsDequalSlow (src/jsc/bindings/bindings.cpp:2033) says that a String subclass instance is DerivedStringObjectType. Only String.prototype has that type.

Fix

  • Add 44 rows with Node's answers for boxed strings. They cover subclass instances, own properties, Symbol.toStringTag, toString() overrides, String.prototype, and Map and Set members.
  • A row that Bun answers differently has a looseBug or strictBug marker and runs as test.failing. There are 20 loose and 5 strict. No comparison changes.
  • Correct the comment.
  • Verified: deep-equal.test.ts (610 pass) and deep-equals.test.ts (68 pass) on a debug build. All 582 matrix expectations pass under Node v26.3.0.

Background

  • A boxed string is the object that new String("abc") makes. Its JavaScriptCore type is StringObjectType, and a subclass instance has the same type.
  • Legacy assert.deepEqual runs the comparison of Bun.deepEquals(a, b) and expect().toEqual(). Jest's toEqual() rejects the pair above (expect 29.5.0 and 30.5.2), so that comparison keeps its answer. One new test in deep-equals.test.ts pins it for Bun.deepEquals.
  • node:assert, node:util: compare the errors of an Error like Node #35495 gives legacy assert.deepEqual a native entry of its own. The fix for these rows belongs behind that entry. I built it as a separate comparator mode and did not open it. It duplicates that entry. It also needs each Set member and Map key paired once (deepEquals: compare a same-typed pair once, and pair Set and Map entries as node does #45457).
Notes

Repro. Node v26.3.0 prints pass for each line. Bun prints pass, pass, throw.

const assert = require("node:assert");
class S extends String {}
class N extends Number {}
class B extends Boolean {}
const run = fn => { try { fn(); console.log("pass"); } catch { console.log("throw"); } };
run(() => assert.deepEqual(new Number(1), new N(1)));
run(() => assert.deepEqual(new Boolean(true), new B(true)));
run(() => assert.deepEqual(new String("abc"), new S("abc")));

Why toEqual() and Bun.deepEquals keep their answer. expect(new String("abc")).toEqual(new S("abc")) fails in Jest, and the Number and Boolean pairs pass. toEqual() runs iterableEquality first, and that function rejects two iterables whose constructors differ. A boxed string is iterable. A boxed number is not. test/js/bun/test/expect.test.js:1070 has pinned the same answer since #3367.

Where the rows come from. Every expectation is the answer of Node v26.3.0. I ran the test file under Node with a stand-in for bun:test. It runs each matrix test and ignores the test.failing marker. 582 of 582 pass (194 rows, three suites). Main has 150 rows.

The stand-in and how to run it 
// bun-test-shim.mjs
const stack = [];
export const tests = [];
export function describe(name, fn) { stack.push(name); try { fn(); } finally { stack.pop(); } }
const register = kind => (name, fn) => tests.push({ suite: stack.join(" > "), name, fn, kind });
export const test = Object.assign(register("test"), { failing: register("failing"), each: () => () => {} });
function matchers(actual, negate) {
  const check = (ok, message) => { if (ok === negate) throw new Error(message); };
  return {
    toBe: expected => check(Object.is(actual, expected), `expected ${String(actual)} to be ${String(expected)}`),
    toBeNull: () => check(actual === null, `expected ${String(actual)} to be null`),
    get not() { return matchers(actual, !negate); },
  };
}
export const expect = actual => matchers(actual, false);
// run.mjs: node run.mjs test/js/node/assert/deep-equal.test.ts
import { registerHooks } from "node:module";
import { pathToFileURL } from "node:url";
const shim = new URL("./bun-test-shim.mjs", import.meta.url).href;
registerHooks({ resolve: (specifier, context, next) => (specifier === "bun:test" ? { url: shim, shortCircuit: true } : next(specifier, context)) });
await import(pathToFileURL(process.argv[2]).href);
const { tests } = await import(shim);
const matrix = /^(assert\.deepStrictEqual|assert\.deepEqual|util\.isDeepStrictEqual)$/;
let pass = 0;
const failed = [];
for (const t of tests) {
  if (!matrix.test(t.suite) || !/^(accepts|rejects) | deep-strict-equal/.test(t.name)) continue;
  try { t.fn(); pass++; } catch (e) { failed.push(`${t.suite} > ${t.name}: ${e.message}`); }
}
console.log({ pass, failed });

Rows with a marker.

  • looseBug, reports not equal: a boxed string and a subclass instance with the same text (both orders), two subclasses, the pair in an object, in an array, as a Map value, as a Map key and in a Set, two Maps keyed by both kinds with the values swapped, String.prototype against an empty boxed string (both orders) and against an empty subclass instance.
  • looseBug, reports equal: different values of an extra own property, an own undefined property, different out-of-range indexed properties, two instances of one subclass with different text and the same toString() or the same Symbol.toPrimitive, String.prototype against {} (both orders).
  • looseBug and strictBug, throws a TypeError: two null-prototype boxed strings.
  • strictBug: the toString() and Symbol.toPrimitive rows (reports equal) and the two String.prototype rows against an empty boxed string (reports not equal).

Cause of the toString(), Symbol.toPrimitive and null-prototype rows. The String arm reads the text of each operand with toStringInline (src/jsc/bindings/bindings.cpp:2047). That call runs user code, and it throws when the object has no toString. Node reads the internal slot. This PR does not change that read, because the change moves results of assert.deepStrictEqual and util.isDeepStrictEqual.

Rows without a marker that guard a later fix. They agree with Node on main. A fix that only stops the class check breaks them.

  • A subclass instance with an own field, with a field that is not set, with an own undefined property, with a Symbol.toStringTag getter, or with a toString() or Symbol.toPrimitive that returns the text of the other operand.
  • A Set of a boxed string and a subclass instance against a Set of two boxed strings, and a Map keyed by both kinds against a copy.
  • An extra own property of 1 against '1', an own symbol property, and an extra own property that holds the pair. Node accepts these three. Main accepts them only because the loose comparison does not read the own properties of a boxed string.

The fix that is not in this PR. I built a node-only comparator mode. In it, Node's loose rule compares two boxed strings: the Object.prototype.toString tag, the text in the internal slot, and the own enumerable string keys. I compared 2,500 ordered pairs of 50 boxed-string-like values. assert.deepEqual and notDeepEqual agreed with Node on 1,993 pairs before and on 2,484 pairs after. The 12 other entry points kept all 30,000 results. It is not opened for these reasons:

Related. #35495 (native entry for legacy deepEqual), #45457 (pairing of Set and Map entries), #32872 (own properties of a boxed string in Bun.deepEquals), #35391 (Node's loose relation, draft), #23877 (== in legacy deepEqual).

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/bun-object/deep-equals.test.ts

@robobun
node:assert: add Node's answers for boxed strings to the deep-equalit…
34159af 
…y matrix

The matrix gets 43 rows for boxed strings: String subclass instances, own
properties, Symbol.toStringTag, toString() overrides, String.prototype, and
the same values in a Map or a Set. A row that Bun answers differently from
Node v26.3.0 has a looseBug or strictBug marker and runs as test.failing.

One test pins that Bun.deepEquals rejects a boxed string against a String
subclass instance with the same text, as Jest's toEqual() does.

The comment on the String arm of specialObjectsDequalSlow said that a
String subclass instance is DerivedStringObjectType. Only String.prototype
has that type.
@github-actions github-actions Bot added the claude label Nov 1, 2026
@robobun

robobun commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 9:27 AM PT - Nov 1st, 2026

❌ @robobun, your commit d26d1a6 has 40 failures in Build #136463 (All Failures):

🧪   To try this PR locally:

bunx bun-pr 47638

That installs a local version of the PR into your bun-47638 executable, so you can run:

bun-47638 --bun

@robobun

robobun commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Status

  • Reproduced on Node v26.3.0 and on a release build of main (3f3a9ff). With class S extends String {}, assert.deepEqual(new String("abc"), new S("abc")) passes in Node and throws ERR_ASSERTION in Bun.
  • The new rows pass on main as they are: each row that Bun answers differently runs as test.failing. Under Node v26.3.0 all 582 matrix expectations pass.
  • CI: the red tests of build 136463 are 30 files, all of them tests of ahead-of-time compilation (test/aot/ and test/bundler/compile-aot-size.test.ts). 28 of them are red on main too (build 136408). The other two are test/aot/rerun-each-runs-a-test-file-again-over-what-it-imports.test.ts and test/aot/engine/native-5.test.ts. This PR does not touch any of them. The two test files that this PR changes are not among the failures. They pass on a debug build with ASAN and on a release build.
  • PR: node:assert: add Node's answers for boxed strings to the deep-equality matrix #47638

@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between aa8dcff and d26d1a6.

Files selected for processing: 1
  • src/jsc/bindings/bindings.cpp

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

The change updates a comment about boxed-string types and adds deep-equality tests for boxed strings and String subclasses. The tests cover own properties, nested values, Maps, Sets, and comparisons in both operand orders.

Changes

Boxed string equality

Layer / File(s) Summary
Boxed-string comparison behavior
src/jsc/bindings/bindings.cpp, test/js/node/assert/deep-equal.test.ts, test/js/bun/bun-object/deep-equals.test.ts		 The comment identifies String.prototype as having DerivedStringObjectType. Tests cover boxed-string properties and strict and loose comparisons with String subclasses across direct and nested values, Maps, and Sets. The Bun-specific test expects a boxed string and a subclass instance with matching text to compare as unequal in both directions.

Suggested reviewers: jarred-sumner

Merge Risk: Minimal · up to d26d1

No actionable merge-blocking risk is established. The change documents existing boxed-string differences rather than changing comparison behavior; merge after normal CI validation.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title names the main change: adding Node's expected results for boxed strings to the deep-equality test matrix in node:assert. The changeset adds 44 boxed-string rows to deep-equal.test.ts, adds a…
Description check Passed The description follows the template with both required sections, 'What does this PR do?' and 'How did you verify your code works?'. The first section is covered by the Problem, Fix, and Background pa…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: adding Node's expected results for boxed strings to the deep-equality test matrix in node:assert. The changeset adds 44 boxed-string rows to deep-equal.test.ts, adds a deepEquals test, and corrects a comment in bindings.cpp. The title does not mention the comment fix, which is a minor secondary change.

Full details: Description check

Explanation

The description follows the template with both required sections, 'What does this PR do?' and 'How did you verify your code works?'. The first section is covered by the Problem, Fix, and Background parts. The second section lists the verification steps: deep-equal.test.ts (610 pass), deep-equals.test.ts (68 pass), and 582 matrix expectations checked under Node v26.3.0. It also states that the bun-object test did not run locally and defers to CI. The description is long and includes extra notes, but it covers the required content.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

claude[bot]
claude Bot reviewed Nov 1, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/jsc/bindings/bindings.cpp — Users of assert.deepStrictEqual and util.isDeepStrictEqual get wrong answers or a thrown TypeError for boxed strings whose toString is overridden or whose prototype is null. At src/jsc/bindings/bindings.cpp:2049-2051 the String arm reads each operand with toStringInline, which runs user toString/Symbol.toPrimitive and throws on a null-prototype wrapper, instead of reading the [[StringData]] slot like Node's StringPrototypeValueOf. Fix: in the node modes (checkPrototypes or isStrict) read the wrapper's internal value via jsCast<StringObject*>(c)->internalValue() for both operands, keeping toStringInline only for the Jest-compatible Bun.deepEquals path; un-fail the three rows this PR marks. [also at: test/js/node/assert/deep-equal.test.ts:487 - Callers of util.isDeepStrictEqual get a thrown TypeError, not a boolean, for two null-prototype boxed strings, and this PR pins that as an accepted test.failing row instead of a defect.]

    Why this was flagged

    Trigger: assert.deepStrictEqual(new StringWithConstantToString("xyz"), new StringWithConstantToString("xyw")) or util.isDeepStrictEqual(Object.setPrototypeOf(new String("a"), null), Object.setPrototypeOf(new String("a"), null)). Both reach specialObjectsDequalSlow's StringObjectType arm at src/jsc/bindings/bindings.cpp:2031. Line 2049 and 2051 call toStringInline on each wrapper: for the first pair the overridden toString returns "abc" twice so line 2054 reports equal and deepStrictEqual passes where Node throws ERR_ASSERTION; for the null-prototype pair OrdinaryToPrimitive finds no toString/valueOf and throws TypeError, so util.isDeepStrictEqual throws instead of returning true. The PR records both as strictBug test.failing rows at test/js/node/assert/deep-equal.test.ts:452 and :486 but leaves the defect. Node reads the internal slot (isEqualBoxedPrimitive -> StringPrototypeValueOf), never user code. Remedy: read StringObject::internalValue() in the checkPrototypes/isStrict modes.

    Verification: Pre-existing; acknowledged in diff: the new matrix rows pin these outcomes as test.failing in test/js/node/assert/deep-equal.test.ts. At src/jsc/bindings/bindings.cpp:2049-2052 the StringObjectType arm calls toStringInline on the wrapper objects, which looks up user toString/valueOf and throws a TypeError when neither is callable. The base commit has the identical calls, so the base fails by the same route.

Comment thread test/js/node/assert/deep-equal.test.ts Resolved
Comment thread test/js/node/assert/deep-equal.test.ts Resolved
@robobun
test: add the same-class Symbol.toPrimitive row, move the Bun.deepEqu…
aa8dcff 
…als test

The matrix gets the sibling of the toString() row: two instances of one
String subclass with different text and the same Symbol.toPrimitive.

The test that Bun.deepEquals rejects a boxed string against a String
subclass instance moves to test/js/bun/bun-object/deep-equals.test.ts,
beside the other tests of Bun.deepEquals.
github-actions[bot]
github-actions Bot reviewed Nov 1, 2026
View reviewed changes
Comment thread src/jsc/bindings/bindings.cpp Resolved
@robobun
deepEquals: keep the String arm comment to one line
d26d1a6 
The comment now states only the fact that the code cannot show: String.prototype
is the one object with DerivedStringObjectType.
@robobun

robobun commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

On the finding about toStringInline in the String arm (src/jsc/bindings/bindings.cpp:2047):

The finding is correct, and main has the same behaviour. Node reads the internal slot of a boxed string. Bun reads the text through toString() or Symbol.toPrimitive. User code can then change the answer, and a null-prototype boxed string throws a TypeError.

This PR does not change that read. It declares no behaviour change, and a read of the slot moves results of assert.deepStrictEqual and util.isDeepStrictEqual. The rows with a strictBug marker record the difference as a defect, not as accepted behaviour. Each one runs as test.failing, so the change that reads the slot must remove the markers in the same PR. aa8dcff adds the Symbol.toPrimitive row, so both hooks have a row.

claude[bot]
claude Bot reviewed Nov 1, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — both points from the earlier pass are addressed (the same-class Symbol.toPrimitive row is in the matrix, and the Bun.deepEquals pin now lives in test/js/bun/bun-object/deep-equals.test.ts), and the remaining change is a comment correction plus test rows with no comparator change.

What was reviewed:

  • The rewritten comment at src/jsc/bindings/bindings.cpp:2033: consistent with how the DerivedStringObjectType fallthrough and the calculatedClassName check below it behave, and with the existing skipPrototype class S extends String test passing via the class-name skip rather than the type-tag branch.
  • Spot-checked the new strict/loose expectations (own-key counts for fields and undefined props, Symbol.toStringTag tags, boxed-value comparison for toString overrides, consumed Set/Map member pairing) against Node's innerDeepEqual/keyCheck algorithm; every looseBug/strictBug marker matches a path the C++ takes today.
  • The two String.prototype strict rows that were questioned are test.failing on Bun either way, so they cannot turn CI green or red incorrectly on this PR.
Extended reasoning...

The diff touches three files: a one-line comment rewrite in the StringObjectType arm of specialObjectsDequalSlow in src/jsc/bindings/bindings.cpp (logic unchanged), one new Bun.deepEquals test, and 43 new table-driven rows in the node:assert deep-equality matrix where divergent rows run as test.failing. It touches no security-sensitive surface. Both inline comments from the prior run were addressed by the follow-up commits, no CODEOWNERS entry covers the changed paths, and the bug hunt ran dry with no findings. The approval rests on there being no behaviour change: the only risk is a mis-recorded Node expectation, and the ones that were questioned are already marked as Bun-failing so they cannot affect CI outcome.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments
@claude claude[bot] claude[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

claude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@robobun