Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

assert,util: always verify both sides #30764

Closed
wants to merge 3 commits into from
Closed

assert,util: always verify both sides #30764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3802007
assert,util: stricter type comparison using deep equal comparisons
BridgeAR Dec 2, 2019
17aad5d
test: run more assert tests
BridgeAR Dec 2, 2019
668ab35
fixup: fix isRegExp and add more tests
BridgeAR Dec 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 67 additions & 22 deletions lib/internal/util/comparisons.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const {
} = primordials;

const { compare } = internalBinding('buffer');
const types = require('internal/util/types');
const {
isAnyArrayBuffer,
isArrayBufferView,
Expand All @@ -34,8 +35,17 @@ const {
isBigIntObject,
isSymbolObject,
isFloat32Array,
isFloat64Array
} = require('internal/util/types');
isFloat64Array,
isUint8Array,
isUint8ClampedArray,
isUint16Array,
isUint32Array,
isInt8Array,
isInt16Array,
isInt32Array,
isBigInt64Array,
isBigUint64Array
} = types;
const {
getOwnNonIndexProperties,
propertyFilter: {
Expand Down Expand Up @@ -107,6 +117,33 @@ function isEqualBoxedPrimitive(val1, val2) {
return false;
}

function isIdenticalTypedArrayType(a, b) {
// Fast path to reduce type checks in the common case.
const check = types[`is${a[Symbol.toStringTag]}`];
if (check !== undefined && check(a)) {
return check(b);
}
// Manipulated Symbol.toStringTag.
for (const check of [
isUint16Array,
isUint32Array,
isInt8Array,
isInt16Array,
isInt32Array,
isFloat32Array,
isFloat64Array,
isBigInt64Array,
isBigUint64Array,
isUint8ClampedArray,
isUint8Array
]) {
if (check(a)) {
return check(b);
}
}
return false;
}

// Notes: Type tags are historical [[Class]] properties that can be set by
// FunctionTemplate::SetClassName() in C++ or Symbol.toStringTag in JS
// and retrieved using Object.prototype.toString.call(obj) in JS
Expand All @@ -115,15 +152,8 @@ function isEqualBoxedPrimitive(val1, val2) {
// There are some unspecified tags in the wild too (e.g. typed array tags).
// Since tags can be altered, they only serve fast failures
//
// Typed arrays and buffers are checked by comparing the content in their
// underlying ArrayBuffer. This optimization requires that it's
// reasonable to interpret their underlying memory in the same way,
// which is checked by comparing their type tags.
// (e.g. a Uint8Array and a Uint16Array with the same memory content
// could still be different because they will be interpreted differently).
//
// For strict comparison, objects should have
// a) The same built-in type tags
// a) The same built-in type tag.
// b) The same prototypes.

function innerDeepEqual(val1, val2, strict, memos) {
Expand Down Expand Up @@ -164,9 +194,10 @@ function innerDeepEqual(val1, val2, strict, memos) {
if (val1Tag !== val2Tag) {
return false;
}

if (ArrayIsArray(val1)) {
// Check for sparse arrays and general fast path
if (val1.length !== val2.length) {
if (!ArrayIsArray(val2) || val1.length !== val2.length) {
return false;
}
const filter = strict ? ONLY_ENUMERABLE : ONLY_ENUMERABLE | SKIP_SYMBOLS;
Expand All @@ -176,25 +207,28 @@ function innerDeepEqual(val1, val2, strict, memos) {
return false;
}
return keyCheck(val1, val2, strict, memos, kIsArray, keys1);
}
if (val1Tag === '[object Object]') {
} else if (val1Tag === '[object Object]') {
return keyCheck(val1, val2, strict, memos, kNoIterator);
}
if (isDate(val1)) {
if (DatePrototypeGetTime(val1) !== DatePrototypeGetTime(val2)) {
} else if (isDate(val1)) {
if (!isDate(val2) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no failing tests that necessitate any change to the Date or RegExp or Error or boxed primitive logic - can you add those tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boxed primitives already have such test case.

I could add more tests for the other types but I actually like things like these to keep them around for coverage tasks at Code and Learn events. I am also fine to add the tests though, if you think that should be done in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's that much of a shortage of tasks for those events, then so be it, but it seems unwise to make untested changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added multiple tests and I indeed forgot to add one check 😆
There are a couple more cases that are not yet completely covered: Set, Map, ArrayBuffer and non-native errors.

DatePrototypeGetTime(val1) !== DatePrototypeGetTime(val2)) {
return false;
}
} else if (isRegExp(val1)) {
if (!areSimilarRegExps(val1, val2)) {
if (!isRegExp(val2) || !areSimilarRegExps(val1, val2)) {
return false;
}
} else if (isNativeError(val1) || val1 instanceof Error) {
// Do not compare the stack as it might differ even though the error itself
// is otherwise identical.
if (val1.message !== val2.message || val1.name !== val2.name) {
if ((!isNativeError(val2) && !(val2 instanceof Error)) ||
val1.message !== val2.message ||
val1.name !== val2.name) {
return false;
}
} else if (isArrayBufferView(val1)) {
if (!isIdenticalTypedArrayType(val1, val2))
return false;
if (!strict && (isFloat32Array(val1) || isFloat64Array(val1))) {
if (!areSimilarFloatArrays(val1, val2)) {
return false;
Expand Down Expand Up @@ -223,12 +257,23 @@ function innerDeepEqual(val1, val2, strict, memos) {
}
return keyCheck(val1, val2, strict, memos, kIsMap);
} else if (isAnyArrayBuffer(val1)) {
if (!areEqualArrayBuffers(val1, val2)) {
if (!isAnyArrayBuffer(val2) || !areEqualArrayBuffers(val1, val2)) {
return false;
}
}
if ((isBoxedPrimitive(val1) || isBoxedPrimitive(val2)) &&
!isEqualBoxedPrimitive(val1, val2)) {
} else if (isBoxedPrimitive(val1)) {
if (!isEqualBoxedPrimitive(val1, val2)) {
return false;
}
} else if (ArrayIsArray(val2) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why check isArray of val2 way down here, instead of only up by the place where val1 is checked (as in #30743)?

Copy link
Member Author

@BridgeAR BridgeAR Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's less CPU work.

In #30743 all entries have to check both sides of both types. But if the arguments are e.g., sets, we'd only have to know that one side is not a set and then continue until we match sets. There we verify the other side.
If it's neither of the explicitly listed entries, we'll verify that the right side is none of the already tested types.

isArrayBufferView(val2) ||
isSet(val2) ||
isMap(val2) ||
isDate(val2) ||
isRegExp(val2) ||
isAnyArrayBuffer(val2) ||
isBoxedPrimitive(val2) ||
isNativeError(val2) ||
val2 instanceof Error) {
return false;
}
return keyCheck(val1, val2, strict, memos, kNoIterator);
Expand Down
91 changes: 75 additions & 16 deletions test/parallel/test-assert-deep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,7 @@ class MyDate extends Date {

const date2 = new MyDate('2016');

// deepEqual returns true as long as the time are the same,
// but deepStrictEqual checks own properties
assert.notDeepEqual(date, date2);
assert.notDeepEqual(date2, date);
assertNotDeepOrStrict(date, date2);
assert.throws(
() => assert.deepStrictEqual(date, date2),
{
Expand Down Expand Up @@ -142,9 +139,7 @@ class MyRegExp extends RegExp {
const re1 = new RegExp('test');
const re2 = new MyRegExp('test');

// deepEqual returns true as long as the regexp-specific properties
// are the same, but deepStrictEqual checks all properties
assert.notDeepEqual(re1, re2);
assertNotDeepOrStrict(re1, re2);
assert.throws(
() => assert.deepStrictEqual(re1, re2),
{
Expand Down Expand Up @@ -684,7 +679,7 @@ assert.throws(
}
);

assert.deepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14));
assertDeepAndStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14));

assert.throws(() => { assert.deepEqual(new Date(), new Date(2000, 3, 14)); },
AssertionError,
Expand All @@ -702,7 +697,7 @@ assert.throws(
'notDeepEqual("a".repeat(1024), "a".repeat(1024))'
);

assert.notDeepEqual(new Date(), new Date(2000, 3, 14));
assertNotDeepOrStrict(new Date(), new Date(2000, 3, 14));

assertDeepAndStrictEqual(/a/, /a/);
assertDeepAndStrictEqual(/a/g, /a/g);
Expand Down Expand Up @@ -743,7 +738,7 @@ a2.b = true;
a2.a = 'test';
assert.throws(() => assert.deepEqual(Object.keys(a1), Object.keys(a2)),
AssertionError);
assert.deepEqual(a1, a2);
assertDeepAndStrictEqual(a1, a2);

// Having an identical prototype property.
const nbRoot = {
Expand Down Expand Up @@ -899,14 +894,12 @@ assert.throws(

/* eslint-enable */

assert.deepStrictEqual({ a: 4, b: '1' }, { b: '1', a: 4 });
assertDeepAndStrictEqual({ a: 4, b: '1' }, { b: '1', a: 4 });

assert.throws(
() => assert.deepStrictEqual([0, 1, 2, 'a', 'b'], [0, 1, 2, 'b', 'a']),
AssertionError);

assert.deepStrictEqual(a1, a2);

// Prototype check.
function Constructor1(first, last) {
this.first = first;
Expand All @@ -926,7 +919,7 @@ assert.throws(() => assert.deepStrictEqual(obj1, obj2), AssertionError);
Constructor2.prototype = Constructor1.prototype;
obj2 = new Constructor2('Ryan', 'Dahl');

assert.deepStrictEqual(obj1, obj2);
assertDeepAndStrictEqual(obj1, obj2);

// Check extra properties on errors.
{
Expand Down Expand Up @@ -1047,7 +1040,7 @@ assert.throws(
Object.defineProperty(a, 'getTime', {
value: () => 5
});
assert.deepStrictEqual(a, b);
assertDeepAndStrictEqual(a, b);
}

// Verify that extra keys will be tested for when using fake arrays.
Expand All @@ -1064,7 +1057,7 @@ assert.throws(
Object.defineProperty(a, 'length', {
value: 2
});
assert.notDeepStrictEqual(a, [1, 1]);
assertNotDeepOrStrict(a, [1, 1]);
}

// Verify that changed tags will still check for the error message.
Expand Down Expand Up @@ -1123,3 +1116,69 @@ assert.throws(
// The descriptor is not compared.
assertDeepAndStrictEqual(a, { a: 5 });
}

// Verify object types being identical on both sides.
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are the only tests that fail without your change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct. The second commit is just there to prevent any further issues like these. It's somewhat independent but it fit together. Would you rather separate the two commits into two PRs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah this is fine to me, was highlighting here in reference to #30764 (comment)

let a = Buffer.from('test');
let b = Object.create(
Object.getPrototypeOf(a),
Object.getOwnPropertyDescriptors(a)
);
Object.defineProperty(b, Symbol.toStringTag, {
value: 'Uint8Array'
});
assertNotDeepOrStrict(a, b);

a = new Uint8Array(10);
b = new Int8Array(10);
Object.defineProperty(b, Symbol.toStringTag, {
value: 'Uint8Array'
});
Object.setPrototypeOf(b, Uint8Array.prototype);
assertNotDeepOrStrict(a, b);

a = [1, 2, 3];
b = { 0: 1, 1: 2, 2: 3 };
Object.setPrototypeOf(b, Array.prototype);
Object.defineProperty(b, 'length', { value: 3, enumerable: false });
Object.defineProperty(b, Symbol.toStringTag, {
value: 'Array'
});
assertNotDeepOrStrict(a, b);

a = new Date(2000);
b = Object.create(
Object.getPrototypeOf(a),
Object.getOwnPropertyDescriptors(a)
);
Object.defineProperty(b, Symbol.toStringTag, {
value: 'Date'
});
assertNotDeepOrStrict(a, b);

a = /abc/g;
b = Object.create(
Object.getPrototypeOf(a),
Object.getOwnPropertyDescriptors(a)
);
Object.defineProperty(b, Symbol.toStringTag, {
value: 'RegExp'
});
assertNotDeepOrStrict(a, b);

a = [];
b = /abc/;
Object.setPrototypeOf(b, Array.prototype);
Object.defineProperty(b, Symbol.toStringTag, {
value: 'Array'
});
assertNotDeepOrStrict(a, b);

a = Object.create(null);
b = new RangeError('abc');
Object.defineProperty(a, Symbol.toStringTag, {
value: 'Error'
});
Object.setPrototypeOf(b, null);
assertNotDeepOrStrict(a, b, assert.AssertionError);
}