Skip to content

node:assert, node:util: compare RegExp lastIndex with === in deep equality - #48501

Open
robobun wants to merge 2 commits into
mainfrom
robobun/e096e443/regexp-lastindex-strict-equal
Open

robobun wants to merge 2 commits into
mainfrom
robobun/e096e443/regexp-lastindex-strict-equal

Conversation

@robobun

@robobun robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

Background

  • lastIndex is an own data property of a RegExp. Only user code stores -0 or NaN in it.
  • SameValue and === differ only for those two pairs.
  • Found by a differential run against Node. No issue reports it.
  • Considered one shared helper for both sites. It wraps one engine call and has no third caller.

Downsides

Notes

Repro.

import assert from "node:assert";
import util from "node:util";
const ok = f => { try { f(); return true; } catch { return false; } };
const re = lastIndex => Object.assign(/a/g, { lastIndex });
for (const [name, a, b] of [["-0 vs 0", -0, 0], ["NaN vs NaN", NaN, NaN]])
  console.log(
    name,
    ok(() => assert.deepStrictEqual(re(a), re(b))),
    util.isDeepStrictEqual(re(a), re(b)),
    util.isDeepStrictEqual(re(a), re(b), true),
    ok(() => assert.partialDeepStrictEqual(re(a), re(b))),
  );
Runtime -0 vs 0 NaN vs NaN
Node v26.12.1 and v26.3.0 true true true true false false false false
Bun 1.3.14 true true true true true true true true
Bun 1.4.0, 1.4.2 and main (89452d2) false false false false true true true true
this change true true true true false false false false

Before 1.4.0, assert.deepStrictEqual called Bun.deepEquals, which does not read lastIndex. So 1.3.x accepted every lastIndex pair.

Why ===. Node's documentation says that lastIndex is "always compared" and names no operator. Its general line for strict mode says that primitives use Object.is. The implementation uses === for lastIndex in the loose, strict and partial modes. For node:* modules the implementation is the reference.

Verdict probe. 39 lastIndex pairs (numbers, strings, BigInts, undefined, null, booleans, objects, symbols, and one RegExp object on both sides), 7 positions (top level, array element, object property, Map value, Map key, Set member, Error cause) and 5 API forms (assert.deepStrictEqual, assert.notDeepStrictEqual, util.isDeepStrictEqual with 2 and 3 arguments, assert.partialDeepStrictEqual). Each container holds one RegExp.

  • verdicts: 0 of 1365 cells differ from Node v26.12.1 and v26.3.0 (before: 105). 105 cells changed, all lastIndex -0/0, 0/-0 or NaN/NaN.
  • Bun.deepEquals cells changed: 0 of 546. Loose assert.deepEqual and assert.notDeepEqual cells changed: 0 of 546.

Sets and Maps with several RegExps. Here re(n) is /a/g with lastIndex n. All rows are assert.deepStrictEqual.

Comparison Node v26.12.1 main this change
new Set([re(0), re(-0), re(1)]) against new Set([re(0), re(1), re(1)]) throws throws passes
new Map([[re(0), 1], [re(-0), 2]]) against a copy passes passes throws
For scale: new Map([[re(0), 1], [re(0), 2]]) against a copy passes throws throws
For scale: new Set([{a:0}, {a:0}, {a:1}]) against new Set([{a:0}, {a:1}, {a:1}]) throws passes passes

A grid against Node v26.12.1: collections of /a/g RegExps with lastIndex from 0, -0, 1 and NaN, every pair of collections, assert.deepStrictEqual. util.isDeepStrictEqual with 2 and 3 arguments gives the same counts.

Shape Pairs Wrong on main Wrong with this change Newly wrong
Set of 2 256 31 0 0
Set of 3 4096 387 144 108
Map, 2 RegExp keys, equal values 256 31 0 0
Map, 3 RegExp keys, equal values 4096 387 144 108
Map, 2 RegExp keys, distinct values 256 25 17 2
Map, 3 RegExp keys, distinct values 4096 137 125 6
Array of 3 4096 135 0 0
Map, 3 RegExp values 4096 135 0 0

In total, of 21,248 pairs, 1,268 are wrong on main and 430 are wrong with this change. 224 are newly wrong. Every newly wrong cell holds both a 0 and a -0. assert.partialDeepStrictEqual has 0 wrong cells with this change.

The Set and Map walks take the first deep-equal member and do not pair members one to one. On main, re(0) and re(-0) are different members, so these two inputs do not reach that walk. #47806 and #44636 are open for the pairing. Partial mode gives Node's verdict on all four rows. Two test.failing rows (strictBug) pin the first two verdicts. If the pairing lands first, those two markers come off.

What stays different from Node in the same two arms.

Measurements. Release builds (LTO, linux-x64) of 98cda0d plus this change, with and without the two tokens. Tools: llvm-nm, llvm-objdump, llvm-size, gdb, and a perf_event_open(2) wrapper that counts instructions:u. The container has no perf, valgrind, strace or bloaty.

  • util.isDeepStrictEqual(/a/g, /a/g): 1971.74 -> 2003.74 instructions per call. With skipPrototype: 1609.03 -> 1641.03. assert.partialDeepStrictEqual: 922.52 -> 940.53. Each number is from 2e6 calls with the empty loop subtracted, the median of 5 interleaved runs. The spread is 1.89, 1.90 and 0.02.
  • gdb stepi over one warm call, from host function entry to return: jsFunctionIsDeepStrictEqual 1926 -> 1958, with skipPrototype 1562 -> 1594, jsFunctionPartialDeepStrictEqual 862 -> 880.
  • The strict cost is 2 x 16. Bun__deepEquals asks the RegExp arm once for each operand order. Partial mode asks once.
  • lastIndex compare, equal int32 path (release): strict site 29 instructions / 1 call (baseline 13 / 0), partial site 25 / 1 (baseline 14 / 0). The span is from the first lastIndex load through the branch after the exception check. The call is the out-of-line JSC::JSValue::strictEqual (303 bytes, present in both binaries, 13 instructions for two int32 values).
  • Bun.deepEquals(/a/g, /a/g): 485.07 -> 484.08 instructions per call. With the strict flag: 490.37 -> 489.37. Date pair: 374.06 -> 374.05. util.isDeepStrictEqual({a:1},{a:1}): 1143.14 -> 1143.14. The -1.00 is a code layout change in specialObjectsDequalSlow.
  • release text: specialObjectsDequalSlow 10,097 -> 9,868 B, PartialDeepEqual::compareBranch 21,924 -> 21,577 B, bun text 142,830,298 -> 142,829,786 B (-512 B).
  • mallocs per 20,000 calls (heapStats().mimalloc.malloc_normal_count, BUN_JSC_useJIT=0): strict 40,020 -> 40,020, skipPrototype 40,025 -> 40,025, partial 20,018 -> 20,018.
  • validateExceptionChecks (debug build, both test files and a rope string as lastIndex): 0 unchecked-exception reports.

Tests.

  • 22 tests fail on the unfixed debug build of main 89452d2 (15 in deep-equal.test.ts, 7 in assert.test.cjs): 18 for the fixed verdicts and 4 test.failing checks of the two collection rows. All 519 tests of the two files pass with the fix.
  • Every new expectation also passes as a node:test file under Node v26.12.1 and v26.3.0.
  • The new block in deep-equal.test.ts uses a plain loop, test() and node:assert. node:assert: read properties and pair Set and Map entries as node's objEquiv does in strict deep equality #47806 converts that file to node:test, which has no test.each and no expect. The file merged with node:assert: read properties and pair Set and Map entries as node's objEquiv does in strict deep equality #47806 passes under Node v26.12.1 (1903 tests).
  • Suites run on the debug build (ASAN): all files of test/js/node/assert/, test/js/bun/bun-object/deep-equals.test.ts, deep-equals-temporal.test.ts, deep-match.spec.ts, test/js/bun/test/expect.test.js, and the vendored test-util-isDeepStrictEqual.js, test-assert.js, test-assert-deep-with-error.js, test-assert-typedarray-deepequal.js, test-assert-class.js, test-assert-checktag.js, test-assert-class-destructuring.js, test-runner-assert.js.
  • The change has no platform-specific code. Windows and macOS were not run locally.

Other designs weighed.

…ality

assert.deepStrictEqual, util.isDeepStrictEqual and
assert.partialDeepStrictEqual compared the lastIndex of two RegExps
with SameValue. Node's areSimilarRegExps() uses ===. So a lastIndex of
-0 was unequal to 0, and NaN was equal to NaN.

Both native sites now call JSC::JSValue::strictEqual.
Write the nesting tests in deep-equal.test.ts with a plain loop, test()
and node:assert. Add a "0" against 0 case. Add two rows for Sets and
Maps that hold RegExps whose lastIndex differs only by the sign of zero.
@robobun

robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:22 PM PT - Nov 10th, 2026

✅ @robobun, your commit df4bb80d8f6e65912e418ff04dcb962fbd4fed58 passed in Build #139858! 🎉


🧪   To try this PR locally:

bunx bun-pr 48501

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

bun-48501 --bun

@robobun

robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

How this was reproduced: the script under "Repro" in the PR notes compares two /a/g RegExps with assert.deepStrictEqual, util.isDeepStrictEqual (2 and 3 arguments) and assert.partialDeepStrictEqual.

  • Node v26.12.1 and v26.3.0 print -0 vs 0 true true true true and NaN vs NaN false false false false.
  • Bun 1.4.0, Bun 1.4.2 and a debug build of main (89452d2) print -0 vs 0 false false false false and NaN vs NaN true true true true.
  • With this change, Bun prints the same lines as Node.

On main's src/, bun bd test test/js/node/assert/deep-equal.test.ts test/js/node/assert/assert.test.cjs fails 22 tests. With this change, all 519 tests pass.

CI, build #139858: all 185 finished jobs passed. The two test files of this PR ran in the debian 13 x64-asan lane and passed. Two other tests failed once in the parallel batch and passed when run alone: test/js/bun/gc/wasm-memory-dropped.test.ts (ubuntu 25.04, x64 and aarch64) and test/regression/issue/29283.test.ts (darwin 27 aarch64). This diff does not touch what they test. Four darwin x64 test jobs are still queued.

PR: #48501

@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The RegExp lastIndex comparison in two deep-equality paths now uses JavaScript strict equality instead of SameValue. Tests cover direct and nested comparisons, including signed zero, NaN, differing types, and collection contexts.

Changes

RegExp lastIndex equality

Layer / File(s) Summary
Strict comparison and regression coverage
src/jsc/bindings/bindings.cpp, src/jsc/modules/NodeUtilTypesModule.cpp, test/js/node/assert/assert.test.cjs, test/js/node/assert/deep-equal.test.ts
Both comparison paths now use strict equality for RegExp lastIndex. Tests cover direct and nested values, signed zero, NaN, differing types, and collection cases. The deep-equality matrix also records current Set and Map mismatches.

Possibly related issues

  • oven-sh/bun#34830 — Reports incorrect equality for Maps with distinct RegExp keys. This PR adds RegExp comparison cases in Maps, but does not establish that it fixes the reported behavior.

Possibly related PRs

  • oven-sh/bun#38510: Changes Set and Map entry pairing, which is relevant to the collection cases added here.
  • oven-sh/bun#47806: Changes strict Node deep-equality walks and Set/Map comparison, which intersect with the updated RegExp comparison behavior.

Suggested reviewers: alii, dylan-conway

Merge Risk: Minimal · up to df4bb

The RegExp comparison change has no established merge-blocking issue in the supplied evidence; proceed with normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly summarizes the main change: RegExp lastIndex comparisons use === in deep equality.
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.
Description check Passed The description explains the problem, the comparison change, its scope, and the verification performed. It does not use the template’s exact headings, but it covers both required sections.
  • 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

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 89452d2 and df4bb80.

Files selected for processing: 4

  • src/jsc/bindings/bindings.cpp
  • src/jsc/modules/NodeUtilTypesModule.cpp
  • test/js/node/assert/assert.test.cjs
  • test/js/node/assert/deep-equal.test.ts

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

@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.

Beyond the inline findings, I also checked whether the bindings.cpp change reaches bun:test's expect().toEqual/toStrictEqual or Bun.deepEquals — it does not: the edited arm is gated on mode.checkPrototypes, and the only instantiations with checkPrototypes=true are Bun__deepEqualsNodeStrict/Bun__deepEqualsNodeStrictSkipProto (the node:assert / node:util entry points); bun:test and Bun.deepEquals instantiate with checkPrototypes=false. Both new strictEqual calls keep the existing RETURN_IF_EXCEPTION under the throw scope, which matters since lastIndex can hold any user value. I did not run the test suites.

Extended reasoning...

The diff swaps JSC::sameValue for JSValue::strictEqual at the two RegExp lastIndex comparison sites used by node:assert/node:util deep equality and adds test matrix rows; it touches no security-sensitive surface. The ruled-out note records that bun:test matchers and Bun.deepEquals are unaffected because checkPrototypes is only true on the node entry points, and that exception checks after the new calls remain in place.

Comment thread test/js/node/assert/deep-equal.test.ts Resolved
Comment thread src/jsc/bindings/bindings.cpp Resolved

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant