Repository navigation
deepStrictEqual fails for Sets that contain both object references and equivalent object values #53423
Description
Activity
I have verified that the issue does not occur with Node 18 and yet it occurs systematically with Node 20 and up.
it seems like it has somthing to do with x being an array saved in constant. example:
const x = ["x"]; const a = new Set([x, ["y"]]); const b = new Set([x, ["y"]]); assert.deepStrictEqual(a, b); // AssertionErrorbut:
const a = new Set([["x"], ["y"]]); const b = new Set([["x"], ["y"]]); assert.deepStrictEqual(a, b); // nullcan i work on this?
@DanielBelz1997 Yup, your analysis is correct. Take a look at the
setEquivfunction here.Starting on line 447, a new "SafeSet" is created that contains all the values in
athat are not inb(as determined byb.has(), whch checks by reference). When you create a referenceconst x = ['x'], it doesn’t get put into that SafeSet since it’s in both sets. Whereas a newly constructed object would be put in since it can’t be in both sets.The bug lies on line 468, where we loop over the values in
b. Since the referencexis inbbut not in the SafeSet, the call on line 471,setHasEqualElement(‹SafeSet›, ‹x›, ...), fails. In your second example you have no references, so all the values inaget put into that SafeSet when it’s created. Then when we loop overb, the callsetHasEqualElement(...)passes for all those values.Reacted by belzDaniel@chharvey thank you for the direction! I will get on that as soon as possible.
This is a great catch! The bug seems to only occur for sets where both contain a reference identical object and another non-reference identical object.
To fix it, it should be enough to add a check here to verify the reference identical one does not exist in the other set
node/lib/internal/util/comparisons.js
Line 505 in 1839eb2
if (!a.has(val) && !setHasEqualElement(set, val, strict, memo))
An alternative would be to undo some of the set changes from #46593. It was not an issue before due to always adding all objects to the intermediate set.
I would just run the benchmark on both options and see what has less impact.
- linked a pull request that will close this issueassert,util: correct comparison when both contain same reference #53431
on Jun 13, 2024 - added a commit that references this issue
on Jun 17, 2024 - added a commit that references this issue
on Jun 20, 2024 - added a commit that references this issue
on Jun 20, 2024 - added a commit that references this issue
on Jul 24, 2024
Version
v20.0.0
Platform
Darwin MacBookPro2019.local 22.6.0 Darwin Kernel Version 22.6.0: Wed Oct 4 21:25:26 PDT 2023; root:xnu-8796.141.3.701.17~4/RELEASE_X86_64 x86_64
Subsystem
No response
What steps will reproduce the bug?
Run the following JavaScript in Node v20:
An AssertionError is reported, which is unexpected.
How often does it reproduce? Is there a required condition?
It is reproducible 100% of the time. It is not reproducible in v18.20.2, but reproducible starting in v20. No required condition.
What is the expected behavior? Why is that the expected behavior?
The expected behavior is that the assertion should pass. Both sets contain a reference to
x(reference-equal) and also a new object['y'](deep-equal), so the sets should be deeply equal.What do you see instead?
An assertion error is reported:
Additional information
The bug occurs only when comparing
Setobjects, with a mix of elements where some elements are objects compared by reference (===) and other elements are objects compared by deep equality.I believe it was introduced by #46593. Looking at the changed code, a new "SafeSet" is created only containing the values (of type
'object') inathat are not inb. So this SafeSet only contains['y']. But then on line 468 of comparisons.js,for (const val of b), every value ofbis checked, including the referencex, which causes the failure because the SafeSet does not contain it.