assert, util: *DeepEqual() handles ArrayBuffers #22266

calebsander · 2018-08-11T06:49:49Z

Previously, all ArrayBuffers and SharedArrayBuffers were considered equal in assert.deepEqual(), assert.deepStrictEqual(), and util.isDeepStrictEqual(). Now, ArrayBuffers and SharedArrayBuffers must have the same byte lengths and contents to be considered equal. In loose mode, an ArrayBuffer is considered equal to a SharedArrayBuffer if they have the same contents, whereas in strict mode, the buffers must be both ArrayBuffers or both SharedArrayBuffers.

Here are 2 examples, both of which succeeded before but now throw:

assert.deepEqual(new Uint8Array([1, 2, 3]).buffer, new Uint8Array([4, 5, 6]).buffer)
assert.deepStrictEqual(new ArrayBuffer(3), new ArrayBuffer(4))

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

benjamingr · 2018-08-11T08:17:59Z

Hey, thank you so much for contributing to Node! We appreciate it.

Have you seen this PR: https://github.com/nodejs/node/pull/22258/files it reworks deepStrictEqual and does this: https://github.com/nodejs/node/pull/22258/files#diff-f4c4bd6d97fd57913c4fc72e673578d6R148

cc @BridgeAR

calebsander · 2018-08-11T17:41:39Z

No, I actually hadn't seen that PR. Funny coincidence that I made this one the same day. Seems like they shouldn't have any conflicts.

BridgeAR · 2018-08-11T15:53:12Z

lib/internal/util/comparisons.js

@@ -46,6 +46,10 @@ function isObjectOrArrayTag(tag) {
  return tag === '[object Array]' || tag === '[object Object]';
 }

+function isArrayBuffer(tag) {
+  return tag === '[object ArrayBuffer]' || tag === '[object SharedArrayBuffer]';
+}


Please use util.types.isAnyArrayBuffer instead. This value could be manipulated by using the Symbol.toStringTag.

BridgeAR · 2018-08-11T16:02:30Z

lib/internal/util/comparisons.js

@@ -116,6 +120,11 @@ function strictDeepEqual(val1, val2, memos) {
    // if they both only contain numeric keys, we don't need to exam further
    return keyCheck(val1, val2, true, memos, val1.length,
                    val2.length);
+  } else if (isArrayBuffer(val1Tag)) {
+    if (!areSimilarTypedArrays(new Uint8Array(val1),
+                               new Uint8Array(val2), 300)) {


Please change this to if (val1.byteLength !== val2.byteLength || compare(new Uint8Array(val1), new Uint8Array(val2)) !== 0) {. Otherwise the value is converted twice.

lib/internal/util/comparisons.js

@@ -170,6 +179,12 @@ function looseDeepEqual(val1, val2, memos) {
  } else if (isArguments(val1Tag) || isArguments(val2Tag)) {
    return false;
  }
+  if (isArrayBuffer(val1Tag) && isArrayBuffer(val2Tag)) {
+    if (!areSimilarTypedArrays(new Uint8Array(val1),
+                               new Uint8Array(val2), 300)) {


BridgeAR · 2018-08-11T16:29:15Z

test/parallel/test-assert-typedarray-deepequal.js

+  [new ArrayBuffer(2), new SharedArrayBuffer(3)],
+  [
+    new Uint8Array(new ArrayBuffer(3)).fill(1).buffer,
+    new Uint8Array(new SharedArrayBuffer(3)).fill(2).buffer


This should already fail because of the different type in strict mode. I would therefore change the test to fill(1) on both sides. In that case it would probably pass in loose mode.

There is already a test for that under looseEqualArrayPairs. Here I'm testing that buffers of different types with the same length but different contents are neither loosely nor strictly equal.

BridgeAR · 2018-08-11T17:55:41Z

This is great! Thanks a lot for the addition. Just left a few comments.

I am still uncertain how it's possible to have a better error output tough.

calebsander · 2018-08-11T18:12:15Z

Yeah, I was also wondering whether the diff message could be improved. I think it is currently inferred based on the output of util.inspect(), so improving the message would require a better output from util.inspect() on ArrayBuffers.

BridgeAR · 2018-08-11T18:38:57Z

@calebsander yes, it does use util.inspect(). That is also very detailed in inspecting values. However, in this case it fails badly. I wonder if it makes sense to actually show a Uint8Array representation of an ArrayBuffer next to it. That way the difference would be clear but AFAIK the original type can not be reconstructed and that might confuse people.

calebsander · 2018-08-11T20:50:56Z

When passing an ArrayBuffer to util.inspect(), it is just raw binary data, much like a Node Buffer; it seems perfectly reasonable to list its bytes like util.inspect() does for a Buffer. Compare:

util.inspect(Buffer.from([1, 2, 3])) // <Buffer 01 02 03>
util.inspect(new Uint8Array([1, 2, 3]).buffer) // ArrayBuffer { byteLength: 3 }

I think it would make sense for the result to look something like ArrayBuffer { byteLength: 3 } [ 1, 2, 3 ], to parallel the result of util.inspect() on a Uint8Array. Regardless, I would put this in a separate PR.

benjamingr

I like this and the technical changes. Ping @nodejs/tsc since this is a breaking change.

mcollina

LGTM

benjamingr · 2018-08-12T14:59:22Z

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

Edit: Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16415/console

BridgeAR · 2018-08-20T00:21:53Z

@calebsander would you be so kind and rebase? In that case the PR should be ready.

Previously, all ArrayBuffers were considered equal in assert.deepEqual() and assert.deepStrictEqual(). Now, ArrayBuffers and SharedArrayBuffers must have the same byte lengths and contents to be considered equal. In loose mode, an ArrayBuffer is considered equal to a SharedArrayBuffer if they have the same contents, whereas in strict mode, the buffers must be both ArrayBuffers or both SharedArrayBuffers.

calebsander · 2018-08-20T00:59:39Z

Done

BridgeAR · 2018-08-20T01:27:19Z

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

BridgeAR · 2018-08-20T10:02:39Z

Landed in 439b75b

@calebsander thanks a lot for your contribution and congratulations on your first commit to Node.js! 🎉

Previously, all ArrayBuffers were considered equal in assert.deepEqual() and assert.deepStrictEqual(). Now, ArrayBuffers and SharedArrayBuffers must have the same byte lengths and contents to be considered equal. In loose mode, an ArrayBuffer is considered equal to a SharedArrayBuffer if they have the same contents, whereas in strict mode, the buffers must be both ArrayBuffers or both SharedArrayBuffers. PR-URL: nodejs#22266 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Aug 11, 2018

benjamingr added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 11, 2018

BridgeAR reviewed Aug 11, 2018

View reviewed changes

benjamingr approved these changes Aug 11, 2018

View reviewed changes

mcollina approved these changes Aug 11, 2018

View reviewed changes

TimothyGu approved these changes Aug 12, 2018

View reviewed changes

benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 12, 2018

BridgeAR approved these changes Aug 13, 2018

View reviewed changes

jasnell approved these changes Aug 13, 2018

View reviewed changes

joyeecheung mentioned this pull request Aug 15, 2018

CI failures: 20180815 nodejs/reliability#13

Closed

calebsander added 2 commits August 19, 2018 17:39

Address review comments

ce25374

targos closed this Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert, util: *DeepEqual() handles ArrayBuffers #22266

assert, util: *DeepEqual() handles ArrayBuffers #22266

calebsander commented Aug 11, 2018

benjamingr commented Aug 11, 2018

calebsander commented Aug 11, 2018 •

edited

Loading

BridgeAR Aug 11, 2018

BridgeAR Aug 11, 2018

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

BridgeAR Aug 11, 2018

calebsander Aug 11, 2018

BridgeAR commented Aug 11, 2018

calebsander commented Aug 11, 2018

BridgeAR commented Aug 11, 2018 •

edited

Loading

calebsander commented Aug 11, 2018 •

edited

Loading

benjamingr left a comment

mcollina left a comment

benjamingr commented Aug 12, 2018 •

edited

Loading

BridgeAR commented Aug 20, 2018

calebsander commented Aug 20, 2018

BridgeAR commented Aug 20, 2018

BridgeAR commented Aug 20, 2018 •

edited

Loading

assert, util: *DeepEqual() handles ArrayBuffers #22266

assert, util: *DeepEqual() handles ArrayBuffers #22266

Conversation

calebsander commented Aug 11, 2018

Checklist

benjamingr commented Aug 11, 2018

calebsander commented Aug 11, 2018 • edited Loading

BridgeAR Aug 11, 2018

Choose a reason for hiding this comment

BridgeAR Aug 11, 2018

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

BridgeAR Aug 11, 2018

Choose a reason for hiding this comment

calebsander Aug 11, 2018

Choose a reason for hiding this comment

BridgeAR commented Aug 11, 2018

calebsander commented Aug 11, 2018

BridgeAR commented Aug 11, 2018 • edited Loading

calebsander commented Aug 11, 2018 • edited Loading

benjamingr left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

benjamingr commented Aug 12, 2018 • edited Loading

BridgeAR commented Aug 20, 2018

calebsander commented Aug 20, 2018

BridgeAR commented Aug 20, 2018

BridgeAR commented Aug 20, 2018 • edited Loading

calebsander commented Aug 11, 2018 •

edited

Loading

BridgeAR commented Aug 11, 2018 •

edited

Loading

calebsander commented Aug 11, 2018 •

edited

Loading

benjamingr commented Aug 12, 2018 •

edited

Loading

BridgeAR commented Aug 20, 2018 •

edited

Loading