buffer: add from(ArrayBufferView) support

[LiviaMedeiros]

Fixes: #43862

CITGM with Buffer.from throwing SyntaxError when isArrayBufferView(value) && !(value instanceof Uint8Array): https://ci.nodejs.org/job/citgm-smoker/2970/

[aduh95]

Can you add tests with TypedArrays that have a byteOffset that is not 0?

[tniessen]

The current behavior appears to be consistent with new Uint8Array(typedArray) and Uint8Array.from(typedArray) (except for 64-bit integers, but that should not be difficult to fix).

> Buffer.from(Uint16Array.from([1, 0xff, 0xffff]))
<Buffer 01 ff ff>
> Uint8Array.from(Uint16Array.from([1, 0xff, 0xffff]))
Uint8Array(3) [ 1, 255, 255 ]

The behavior proposed here appears to go directly against the design of TypedArray.from() and TypedArray constructors.

> Buffer.from(Uint16Array.from([1, 0xff, 0xffff]))
<Buffer 01 00 ff 00 ff ff>
> Uint8Array.from(Uint16Array.from([1, 0xff, 0xffff]))
Uint8Array(3) [ 1, 255, 255 ]

I don't think we should make breaking changes that further increase the gap between Buffer and Uint8Array.

[LiviaMedeiros]

The current behavior appears to be consistent with new Uint8Array(typedArray) and Uint8Array.from(typedArray)

That's a good point.

I think, compatibility between Buffer and Uint8Array isn't that important here.

TypedArray.from(arrayLike[, mapFn[, thisArg]]) is basically a version of Array.from(), and it's not compatible with Buffer.from() anyway. We can't do Buffer.from({ length: 10 }, (_, i) => i ** 2), we can't do Uint8Array.from(new ArrayBuffer(10)), etc.

The fact that any TypedArray's constructor and .from can be used with incompatible TypedArrays is almost always nothing but footgun; it can be easily and explicitly done in userland (e.g. [...abv]), and CITGM doesn't seem to catch a single usage for that.

If we do care about this compatibility, the documentation requires adjustments because special casing for Buffer and Uint8Array doesn't make any sense. We should either get rid of it and keep only Buffer.from(array) signature which covers TypedArrays; or replace it with Buffer.from(typedArray) entry to explain what this method does in this case. WDYT?

Independently from that, we can add a separate method like Buffer.fromArrayBufferView() that will make a byte-per-byte copy regardless of type.

[nodejs-github-bot]

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

[aduh95]

@tniessen did #43863 (comment) convinced you to change your mind or are you still blocking it? To be clear, I haven't looked into it myself (I'm simply going over the open semver-major PRs that contain breaking changes and should be released in the next major version. PRs in preparation for v19.0.0), I'm just asking.

[tniessen]

Thanks @aduh95, I should have replied here earlier.

are you still blocking it?

Yes. This is a breaking change to a stable API that, as far as I am aware, no users have asked for, and which goes directly against the design of the base class Uint8Array, which is a standardized API.

The fact that any TypedArray's constructor and .from can be used with incompatible TypedArrays is almost always nothing but footgun; it can be easily and explicitly done in userland (e.g. [...abv]), and CITGM doesn't seem to catch a single usage for that.

It can certainly be a footgun. But the same thing can be said about platform-dependent byte representations of multi-byte data types, and the behavior proposed here can also easily and explicitly be done in userland.

[LiviaMedeiros]

Superseded by better alternative w/o breaking changes: #46500.

Thanks for the reviews!

[jasnell]

I had completely missed this! My apologies for duplicating the work.