Skip to content

Commit

Permalink
assert: fix loose set and map comparison
Browse files Browse the repository at this point in the history 
The fast path did not anticipate different ways to express a loose
equal string value (e.g., 1n == '+0001'). This is now fixed with the
downside that all primitives that could theoretically have equal
entries must go through a full comparison.

Only some strings, symbols, undefined, null and NaN can be detected
in a fast path as those entries have a strictly limited set of
possible equal entries.

PR-URL: #22495
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
  • Loading branch information
@BridgeAR @targos
BridgeAR authored and targos committed Sep 5, 2018
1 parent ff8fadc commit bdd3afb
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 108 deletions.
181 changes: 77 additions & 104 deletions lib/internal/util/comparisons.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,23 +354,52 @@ function setHasEqualElement(set, val1, strict, memo) {
return false;
}

// Note: we currently run this multiple times for each loose key!
// This is done to prevent slowing down the average case.
function setHasLoosePrim(a, b, val) {
const altValues = findLooseMatchingPrimitives(val);
if (altValues === undefined)
return false;
// See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness#Loose_equality_using
// Sadly it is not possible to detect corresponding values properly in case the
// type is a string, number, bigint or boolean. The reason is that those values
// can match lots of different string values (e.g., 1n == '+00001').
function findLooseMatchingPrimitives(prim) {
switch (typeof prim) {
case 'undefined':
return null;
case 'object': // Only pass in null as object!
return undefined;
case 'symbol':
return false;
case 'string':
prim = +prim;
// Loose equal entries exist only if the string is possible to convert to
// a regular number and not NaN.
// Fall through
case 'number':
if (Number.isNaN(prim)) {
return false;
}
}
return true;
}

let matches = 1;
for (var i = 0; i < altValues.length; i++) {
if (b.has(altValues[i])) {
matches--;
}
if (a.has(altValues[i])) {
matches++;
}
function setMightHaveLoosePrim(a, b, prim) {
const altValue = findLooseMatchingPrimitives(prim);
if (altValue != null)
return altValue;

return b.has(altValue) && !a.has(altValue);
}

function mapMightHaveLoosePrim(a, b, prim, item, memo) {
const altValue = findLooseMatchingPrimitives(prim);
if (altValue != null) {
return altValue;
}
const curB = b.get(altValue);
if (curB === undefined && !b.has(altValue) ||
!innerDeepEqual(item, curB, false, memo)) {
return false;
}
return matches === 0;
const curA = a.get(altValue);
return curA === undefined && a.has(altValue) ||
innerDeepEqual(item, curA, false, memo);
}

function setEquiv(a, b, strict, memo) {
Expand All @@ -390,8 +419,19 @@ function setEquiv(a, b, strict, memo) {
// hunting for something thats deep-(strict-)equal to it. To make this
// O(n log n) complexity we have to copy these values in a new set first.
set.add(val);
} else if (!b.has(val) && (strict || !setHasLoosePrim(a, b, val))) {
return false;
} else if (!b.has(val)) {
if (strict)
return false;

// Fast path to detect missing string, symbol, undefined and null values.
if (!setMightHaveLoosePrim(a, b, val)) {
return false;
}

if (set === null) {
set = new Set();
}
set.add(val);
}
}

Expand All @@ -402,96 +442,18 @@ function setEquiv(a, b, strict, memo) {
if (typeof val === 'object' && val !== null) {
if (!setHasEqualElement(set, val, strict, memo))
return false;
} else if (!a.has(val) && (strict || !setHasLoosePrim(b, a, val))) {
} else if (!strict &&
!a.has(val) &&
!setHasEqualElement(set, val, strict, memo)) {
return false;
}
}
return set.size === 0;
}

return true;
}

// See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness#Loose_equality_using
function findLooseMatchingPrimitives(prim) {
switch (typeof prim) {
case 'number':
if (prim === 0) {
return ['', '0', false];
}
if (prim === 1) {
return ['1', true];
}
return ['' + prim];
case 'string':
if (prim === '' || prim === '0') {
return [0, false];
}
if (prim === '1') {
return [1, true];
}
const number = +prim;
if ('' + number === prim) {
return [number];
}
return;
case 'undefined':
return [null];
case 'object': // Only pass in null as object!
return [undefined];
case 'boolean':
if (prim === false) {
return ['', '0', 0];
}
return ['1', 1];
}
}

// This is a ugly but relatively fast way to determine if a loose equal entry
// currently has a correspondent matching entry. Otherwise checking for such
// values would be way more expensive (O(n^2)).
// Note: we currently run this multiple times for each loose key!
// This is done to prevent slowing down the average case.
function mapHasLoosePrim(a, b, key1, memo, item1, item2) {
const altKeys = findLooseMatchingPrimitives(key1);
if (altKeys === undefined)
return false;

const setA = new Set();
const setB = new Set();

let keyCount = 1;

setA.add(item1);
if (b.has(key1)) {
keyCount--;
setB.add(item2);
}

for (var i = 0; i < altKeys.length; i++) {
const key2 = altKeys[i];
if (a.has(key2)) {
keyCount++;
setA.add(a.get(key2));
}
if (b.has(key2)) {
keyCount--;
setB.add(b.get(key2));
}
}
if (keyCount !== 0 || setA.size !== setB.size)
return false;

for (const val of setA) {
if (typeof val === 'object' && val !== null) {
if (!setHasEqualElement(setB, val, false, memo))
return false;
} else if (!setB.has(val) && !setHasLoosePrim(setA, setB, val)) {
return false;
}
}
return true;
}

function mapHasEqualEntry(set, map, key1, item1, strict, memo) {
// To be able to handle cases like:
// Map([[{}, 'a'], [{}, 'b']]) vs Map([[{}, 'b'], [{}, 'a']])
Expand Down Expand Up @@ -521,9 +483,17 @@ function mapEquiv(a, b, strict, memo) {
// almost all possible cases.
const item2 = b.get(key);
if ((item2 === undefined && !b.has(key) ||
!innerDeepEqual(item1, item2, strict, memo)) &&
(strict || !mapHasLoosePrim(a, b, key, memo, item1, item2))) {
return false;
!innerDeepEqual(item1, item2, strict, memo))) {
if (strict)
return false;
// Fast path to detect missing string, symbol, undefined and null
// keys.
if (!mapMightHaveLoosePrim(a, b, key, item1, memo))
return false;
if (set === null) {
set = new Set();
}
set.add(key);
}
}
}
Expand All @@ -533,11 +503,14 @@ function mapEquiv(a, b, strict, memo) {
if (typeof key === 'object' && key !== null) {
if (!mapHasEqualEntry(set, a, key, item, strict, memo))
return false;
} else if (!a.has(key) &&
(strict || !mapHasLoosePrim(b, a, key, memo, item))) {
} else if (!strict &&
(!a.has(key) ||
!innerDeepEqual(a.get(key), item, false, memo)) &&
!mapHasEqualEntry(set, a, key, item, false, memo)) {
return false;
}
}
return set.size === 0;
}

return true;
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-assert-deep.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,13 +364,13 @@ assertDeepAndStrictEqual(
new Map([[null, 3]])
);
assertOnlyDeepEqual(
new Map([[null, undefined]]),
new Map([[undefined, null]])
new Map([[undefined, null], ['+000', 2n]]),
new Map([[null, undefined], [false, '2']]),
);

assertOnlyDeepEqual(
new Set([null, '']),
new Set([undefined, 0])
new Set([null, '', 1n, 5, 2n, false]),
new Set([undefined, 0, 5n, true, '2', '-000'])
);
assertNotDeepOrStrict(
new Set(['']),
Expand Down

0 comments on commit bdd3afb

Please sign in to comment.