TypedArrays are not spec-compliant and break Buffer with many ecosystem packages

[ChALkeR]

Bug Description

See explainer, test and a monkey-patch fix at https://github.com/ExodusMovement/patch-broken-hermes-typed-arrays

> Buffer.alloc(10).subarray(0).toString('hex')
'0,0,0,0,0,0,0,0,0,0'
// What?

Sections of specification broken in Hermes:

• ECMAScript® Language Specification, %TypedArray%.prototype.subarray
• ECMAScript® Language Specification, %TypedArray%.prototype.map
• ECMAScript® Language Specification, %TypedArray%.prototype.filter
• ECMAScript® Language Specification, %TypedArray%.prototype.slice

See https://github.com/ExodusMovement/patch-broken-hermes-typed-arrays/blob/main/index.js for links to specific spec steps

See ecosystem fallout in feross/buffer#329

• I have run gradle clean and confirmed this bug does not occur with JSC
• The issue is reproducible with the latest version of React Native. Hermes

Hermes git revision (if applicable): 0410fb4 (latest main)
React Native version: 🤷🏻
OS:
Platform (most likely one of arm64-v8a, armeabi-v7a, x86, x86_64):

Steps To Reproduce

See full explainer, test and a monkey-patch fix at https://github.com/ExodusMovement/patch-broken-hermes-typed-arrays

chalker@Nikitas-Air hermes_workingdir % cat bad-inheritance.js 
var TestArray = function (...args) {
  print('called TestArray constructor')
  var buf = new Uint8Array(...args)
  Object.setPrototypeOf(buf, TestArray.prototype)
  return buf
}

Object.setPrototypeOf(TestArray.prototype, Uint8Array.prototype)
Object.setPrototypeOf(TestArray, Uint8Array)

var arr = new TestArray(10)
var mapped = arr.map((_, i) => i * 10)
print(mapped.constructor.name)
chalker@Nikitas-Air hermes_workingdir % jsc bad-inheritance.js                       
called TestArray constructor
called TestArray constructor
TestArray
chalker@Nikitas-Air hermes_workingdir % ./build_release/bin/hermes bad-inheritance.js
called TestArray constructor
Uint8Array

The Expected Behavior

Implementation of TypedArray per spec, perhaps without Symbol.species but with working inheritance

See spec refs above

[tmikov]

Thank you for the detailed and well-researched bug report. We appreciate the effort you put into diagnosing the issue and linking to the relevant sections of the ECMAScript specification.

To provide some context, the behavior you're observing is actually by design in Hermes. As documented in our Excluded from Support section, we deliberately do not support Symbol.species or the use of .constructor property when creating new arrays in Array.prototype methods (the same reasoning applies to TypedArray.

Supporting this would require introducing a slow path in every case where a new object is returned that doesn't match the expected type. That would add a ton of complexity for a use case that didn't seem very important.

Frankly, we didn't realize the importance of this for implementing something Buffer on top of TypedArray. Additionally, it looks like in this case the returned object is of the same type.

I wonder whether it would be sufficient, as an intermediate step, to only support the case where .constructor is required to return an instance of the type in question.

[ChALkeR]

@tmikov this is not about Symbol.species

TypedArraySpeciesCreate falls back to defaultConstructor on step 1 -- that is what this report is about
Missing Symbol.Species is not the issue here

Unsupported Symbol.Species should have been replaced with default constructor of the object, not with nothing

The testcase doesn't use Symbol.species

[ChALkeR]

use of constructor property when creating new Arrays in Array.prototype methods

Ah, haven't noticed that! This breaks a lot of things though in a subtle unexpected way

E.g.: https://github.com/bitcoinjs/bitcoinjs-lib/blob/8d9775c20da03ab40ccbb1971251e71d117bcd8b/ts_src/psbt.ts#L1727-L1734

I wonder whether it would be sufficient, as an intermediate step, to only support the case where .constructor is required to return an instance of the type in question.

Unsure
Subtle spec differences may be breaking

A previous Buffer version relied on Symbol.species even

[tmikov]

@ChALkeR so, it appears the intermediate step we proposed is the actual spec-compliant behavior of TypedArray. That is fortunate. We will implement at least the partial support through constructor and will consider adding incremental Symbol.species support.

[ChALkeR]

Symbol.species is... at danger i think?
cc @ljharb perhaps?

[ljharb]

Symbol.species usage is being evaluated, and it's unclear which parts of it, if any, would be removed.

[ChALkeR]

Hm: https://github.com/tc39/proposal-rm-builtin-subclassing?tab=readme-ov-file#prototype-methods-3

In that taxonomy:

• That stage-1 proposal is about removing Type 2-4 and keeping only Type 1
• This issue is about Type 2 not working correctly (which Buffer and ecosystem relies upon)
• Symbol.species is Type 3 in that taxonomy

I'm not sure if it's worth to add Symbol.species but I don't think that Type 2 can realistically be removed

[ljharb]

I agree that Type 2 is unlikely to be removable.