facebook / hermes

A JavaScript engine optimized for running React Native.
https://hermesengine.dev/
MIT License
9.86k stars 632 forks source link

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

Open ChALkeR opened 2 months ago

ChALkeR commented 2 months ago

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:

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

See ecosystem fallout in https://github.com/feross/buffer/issues/329

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 commented 2 months ago

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 commented 2 months ago

@tmikov this is not about Symbol.species

image

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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

ljharb commented 2 months ago

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

ChALkeR commented 2 months ago

Hm: https://github.com/tc39/proposal-rm-builtin-subclassing?tab=readme-ov-file#prototype-methods-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 commented 2 months ago

I agree that Type 2 is unlikely to be removable.