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.

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

Incorrect result for ArrayBuffer values #94

Closed
famzah opened this issue Sep 18, 2021 · 2 comments
Closed

Incorrect result for ArrayBuffer values #94

famzah opened this issue Sep 18, 2021 · 2 comments

Comments

@famzah
Copy link

famzah commented Sep 18, 2021

Hello,

If I compare two ArrayBuffer variables, the result is always true. Here is a PoC:

import { default as assert } from "assert";
import deepEqual from "deep-equal";

const buffer1 = new ArrayBuffer(8); // initial value of 0's
const buffer2 = new ArrayBuffer(8); // initial value of 0's

const view1 = new Int8Array(buffer1);
const view2 = new Int8Array(buffer2);

view1.fill(9); // change all values to 9's
//view2.fill(9);

//console.dir(buffer1);
//console.dir(buffer2);

console.log(
  deepEqual(buffer1, buffer2)
);

assert.deepEqual(buffer1, buffer2);
//assert.deepStrictEqual(buffer1, buffer2);

deepEqual() returns true while Node.js built-in assert.deepEqual() fails correctly with the following:

AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:

ArrayBuffer {
  [Uint8Contents]: <09 09 09 09 09 09 09 09>,
  byteLength: 8
}

should loosely deep-equal

ArrayBuffer {
  [Uint8Contents]: <00 00 00 00 00 00 00 00>,
  byteLength: 8
}
    at // ...cut... //
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: ArrayBuffer {
    [Uint8Contents]: <09 09 09 09 09 09 09 09>,
    byteLength: 8
  },
  expected: ArrayBuffer {
    [Uint8Contents]: <00 00 00 00 00 00 00 00>,
    byteLength: 8
  },
  operator: 'deepEqual'
}

One options to fix this would be to test if the value is of type ArrayBuffer, then create a temporary view of it and compare this view. This works correctly with deepEqual(). However, there may be a more generic fix which I'm unaware of.

@ljharb
Copy link
Member

ljharb commented Sep 18, 2021

We indeed don't have any special support for ArrayBuffers, so it treats them as regular objects.

You should already be able to compare view1 and view2, at least :-)

As for creating a temporary typed array around the buffers, would that work no matter the length of the buffers? or would i need to always make a Uint8Array to ensure that it did work?

@famzah
Copy link
Author

famzah commented Sep 22, 2021

I'm not an expert in JavaScript but my here is my research.

Docs say that ArrayBuffer is an array of bytes, often referred to in other languages as a "byte array". This means that its internal representation is of integer elements between 0 and 255 which is also confirmed if we dump a new ArrayBuffer(8):

ArrayBuffer {
  [Uint8Contents]: <00 00 00 00 00 00 00 00>,
  byteLength: 8
}

👍 Therefore, representing an ArrayBuffer to a TypedArray of type Uint8Array will always give accurate results for comparison.

Here is some code to prove it: https://gist.github.com/famzah/ab6e9202fea64898ab36eb2daf1074cb

@ljharb ljharb closed this as completed in b47d59e Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants