assert: fix TypeError on deepStrictEqual with null Map key or Set member

[semx]

assert.deepStrictEqual() (and util.isDeepStrictEqual()) throw a TypeError instead of comparing when the first Map has a null (or other primitive) key that lines up against object-only keys in the other map:

const assert = require('node:assert');

const a = new Map([[null, 1], [{}, 2]]);
const b = new Map([[{}, 9], [{}, 9]]);

assert.deepStrictEqual(a, b);
// TypeError: Cannot read properties of null (reading 'constructor')
// expected: an AssertionError (the maps are not deeply equal)

The same happens for an undefined key. Set has the identical problem for a null/undefined member (once the set is large enough to skip the small-set fast path):

assert.deepStrictEqual(new Set([null, {}, {}]), new Set([{}, {}, {}]));
// TypeError: Cannot read properties of null (reading 'constructor')

It only triggers in strict mode when the other collection's keys/members are all objects and their count equals the first collection's size.

Cause

In mapObjectEquiv and setObjectEquiv (lib/internal/util/comparisons.js), primitive/null keys and members are resolved directly via b.has() / b.get(), but that handling was gated behind extraChecks (array.length !== a.size). When the counts match, the gate is skipped and the primitive/null key/member falls through to objectComparisonStart, which dereferences .constructor and throws on null/undefined.

Fix

Handle primitive/null keys and members unconditionally — they can only match by identity and can never match through the object comparator — so they are always resolved by direct lookup and never reach objectComparisonStart. The collections above now compare as unequal (throwing an AssertionError, as expected) instead of throwing a TypeError. Object comparison is unchanged.

Added regression cases (null and undefined keys/members, for both Map and Set) to test/parallel/test-assert-deep.js.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.17%. Comparing base (c909c63) to head (5a3b354).
⚠️ Report is 32 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #64449   +/-   ##
=======================================
  Coverage   90.17%   90.17%           
=======================================
  Files         771      771           
  Lines      265451   265453    +2     
  Branches    50459    50465    +6     
=======================================
+ Hits       239361   239365    +4     
+ Misses      17059    17047   -12     
- Partials     9031     9041   +10     

Files with missing lines	Coverage Δ
lib/internal/util/comparisons.js	99.53% <100.00%> (+<0.01%)	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[semx]

This has two approvals (thanks @ljharb, @jasnell) and is still labelled needs-ci. Could a collaborator kick off a CI run?

Happy to rebase first if that helps — the branch is otherwise unchanged since the reviews.

[semx]

Following up: this has two approvals (thanks @ljharb, @jasnell) and is only waiting on CI to run so it can land. Could a collaborator start CI / apply the label so it can move through the commit-queue? Happy to rebase if needed. Thanks!

[ljharb]

First I think it needs a rebase?

[semx]

Rebased onto current main and force-pushed. There was one conflict, in lib/internal/util/comparisons.js: main has since landed an equivalent fix for the Map path (mapObjectEquiv), so I dropped that now-redundant hunk and kept the still-needed Set-path fix (setObjectEquiv) plus all four null/undefined regression tests. The final diff is just the Set fix + tests; the branch is one commit on top of main and both files pass node --check. Should be good for CI now — thanks!

[semx]

Rebased onto current main — conflict-free (the earlier Map-path hunk stays dropped since mapObjectEquiv landed; this is just the Set-path fix plus the null/undefined regression tests). The PR is mergeable and only needs-ci; the fresh workflow runs are sitting in action_required, so they need a collaborator to approve the run for this fork PR. If someone could approve the Actions run and start CI, it should be ready to move through the commit-queue. Thanks again @ljharb @jasnell for the reviews.

[semx]

Rebased onto current main, thanks. Ready for a CI run whenever a collaborator has a minute.

[MikeMcC399]

Rebased onto current main, thanks. Ready for a CI run whenever a collaborator has a minute.

Many thanks for rebasing! That was successful in GitHub Actions CI.

Note that the needs-ci PRs that need a full CI run. label is a little misleading.

The needs-ci label identifies pull requests that require a full Jenkins CI run. It is a classification, not an indication that CI is still pending.

In any case I've now requested a Jenkins CI run, so we'll see what happens!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/77411/

[MikeMcC399]

CI was finally successful! 🎉

[nodejs-github-bot]

Landed in 6b28d88