Repository navigation
[Fix] compare the values wrapped by two boxed primitives - #111
Conversation
Two boxed primitives of the same kind were equal whatever they wrapped: `deepEqual(Object(1), Object(2))` was true, in loose and strict mode, and so were boxed `true` and `false`, boxed BigInts and boxed Symbols, alone or nested in an array or an object. Only the kind was compared, and `String` objects were told apart just because of their index keys. Compare the wrapped primitives with `Object.is`, read through the intrinsic `valueOf` so that an overridden one is never called. This is what node's `assert.deepEqual` and `assert.deepStrictEqual` do: a boxed `NaN` still equals another boxed `NaN`, and a boxed `0` no longer equals a boxed `-0`. Assisted-By: Claude Sonnet 5.5 <noreply@anthropic.com> Note that all of the transitive dependencies of `unbox-primitive` are already production dependencies. Co-authored-by: nimbly1 <258366730+nimbly1@users.noreply.github.com> Co-authored-by: Jordan Harband <ljharb@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
===========================================
+ Coverage 98.30% 100.00% +1.69%
===========================================
Files 2 2
Lines 236 238 +2
Branches 99 100 +1
===========================================
+ Hits 232 238 +6
+ Misses 4 0 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
A note on the red checks. I think neither is caused by this change.
|
f6cceb4 to
3b11d93
Compare
ljharb
left a comment
There was a problem hiding this comment.
Thanks, this is a great catch - two boxed primitives of the same kind comparing equal regardless of their wrapped value was definitely a bug, and matching node's assert here (including boxed 0 vs -0 in loose mode) is the right call.
I pushed a couple of changes and squashed them into your commit:
- Use
unbox-primitiveinstead of a localunboxhelper. The helper calledcallBoundwith a dynamically built name on every comparison; intrinsics should be cached at module level with explicit string arguments.unbox-primitivealready does exactly that (and only uses the intrinsicvalueOf/toString, so overridden methods are still never called), and all of its transitive dependencies are already production dependencies of this package, so it only adds the one package. This also frees up a few lines undermax-lines. - More tests:
- boxed primitives that share an overridden
valueOf- both as an own property and via a shared[[Prototype]]- to make sure the wrapped value is read through the intrinsic and not the user-provided method - boxed primitives as
Setelements, and asMapkeys and values, since those go through a different code path (setHasEqualElement/mapHasEqualEntry) than arrays and plain objects, and were also returningtruebefore this fix
- boxed primitives that share an overridden
All the new "not equal" assertions fail without the fix, and the ASSERT=1 failures are unchanged from main.
deepEqualtreats two boxed primitives of the same kind as equal, whatever they wrap:This holds for boxed numbers, booleans, BigInts and symbols.
internalDeepEqualcompares only the kind of the box (whichBoxedPrimitive) and never the primitive inside it.Stringobjects were already told apart, but only because of their index keys.The fix compares the wrapped primitives with
Object.is. It reads them through the intrinsicvalueOf, so an overriddenvalueOfis never called (the existing "valueOfis called for boxed primitives" test still passes). Node'sassert.deepEqualandassert.deepStrictEqual(Node 24) compare boxed primitives the same way in both modes:assertObject(1),Object(2)Object(true),Object(false)Object(NaN),Object(NaN)Object(0),Object(-0)One behavior change to check:
Object(0)andObject(-0)are now different in loose mode too, as they are in Node. Plain0and-0are still equal in loose mode. Tell me if you would rather keep boxed0and-0equal in loose mode, and I will limit the check.Checked: 60 new assertions in the "boxed primitives" test in
test/cmp.js. 28 of them fail without the change.npx tape 'test/**/*.js'passes (720 tests), andeslint --ext=js,mjs .has no errors (the 13 warnings were there before).index.jsis 399 lines, so I kept the change small for themax-lineslimit of 400.I also compared 65 pairs of values against Node's
util.isDeepStrictEqual. The differences went from 14 to 9. The remaining 9 are other things, and I did not touch them: properties keyed by symbols,WeakMapandPromiseinstances, typed arrays that holdNaNor-0, invalid dates,Errorcause, andRegExplastIndex.Not run: the
npm auditstep ofnpm test. Thenyccoverage numbers read 0% on my Windows machine, so I am not quoting coverage. WithASSERT=1, 26 tests fail before and after this change (theWeakMapones among them), and the new assertions pass.Generated with Claude Code