facebook / hermes

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

Short-hand methods don't have prototype / construct #1371

Closed mhofman closed 1 month ago

mhofman commented 1 month ago

Bug Description

Short hand method in object literals (e.g. { foo () {} } are not constructor functions and as such should not have a prototype property nor have a [[Construct]] behavior.

A similar issue was fixed last year for arrow functions in https://github.com/facebook/hermes/commit/c42491de94aff479e5e83c073eff96a6261da080

Hermes git revision (if applicable): c2f7dcd5e4fa88b6bea12e2b705d53a7ffd54ecf React Native version: N/A OS: Linux Platform (most likely one of arm64-v8a, armeabi-v7a, x86, x86_64): x86_64

Steps To Reproduce

const hasConstruct = target => {
  const proxy = new Proxy(target, {
    construct() {
      return {};
    },
  });
  try {
    new proxy();
    return true;
  } catch (e) {
    return false;
  }
};
for (const [name, fn] of [
  ['function', function () {}],
  ['short-hand method', { foo() {} }.foo],
  ['arrow', () => {}],
]) {
  print(name, 'hasConstruct', hasConstruct(fn));
  print(
    name,
    '.prototype',
    JSON.stringify(Object.getOwnPropertyDescriptor(fn, 'prototype')),
  );
}
  1. eshost -s test-has-construct.js

Expected Behavior

#### JavaScriptCore, Moddable XS, SpiderMonkey, V8
function hasConstruct true
function .prototype {"value":{},"writable":true,"enumerable":false,"configurable":false}
short-hand method hasConstruct false
short-hand method .prototype undefined
arrow hasConstruct false
arrow .prototype undefined

Actual Behavior

#### Hermes
function hasConstruct true
function .prototype {"value":{},"writable":true,"enumerable":false,"configurable":false}
short-hand method hasConstruct true
short-hand method .prototype {"value":{},"writable":true,"enumerable":false,"configurable":false}
arrow hasConstruct false
arrow .prototype undefined
tmikov commented 1 month ago

Thanks for reporting this!

tmikov commented 1 month ago

This was already fixed in Static Hermes in https://github.com/facebook/hermes/commit/00f18c89c720e1c34592bb85a1a8d311e6e99599.

leotm commented 1 month ago

thanks ^ i found and checked out https://github.com/facebook/hermes/tree/static_h which includes https://github.com/facebook/hermes/commit/00f18c89c720e1c34592bb85a1a8d311e6e99599 from last week

then built and ran Hermes, noting

Planned

  • Block scoped variables (let and const), with support for the temporal dead zone

so converted @mhofman's example to vars and can confirm matches expected behaviour

i tried shermes, kept the folder named build (expected), but no luck getting output even with -ljsi -L ../jsi flags, if you happen to know why

leotm commented 1 month ago

can https://github.com/facebook/hermes/commit/00f18c89c720e1c34592bb85a1a8d311e6e99599 be safely cherry-picked into an older fork of Hermes running RN 71+ built from source? e.g. https://github.com/MetaMask/hermes

if not, do we need to upgrade to e.g. RN 72/73/74rc first, then build from source and cherry-pick it?

or is it more likely e.g. RN 75 to ship with a future bundled Static Hermes tag?

(our goal being to run hardened JS on a version of Hermes compliant/standard enough to the JS spec, with mods to SES if really needed to support all engines)

tmikov commented 1 month ago

i tried shermes, kept the folder named build (expected), but no luck getting output

@leotm the output is an executable file called a.out. You can run it. Or you can directly execute the JS with the -exec option.

Please also note that the documentation in the static_h branch isn't up to date (yet), it is just a copy of the main branch.

tmikov commented 1 month ago

can https://github.com/facebook/hermes/commit/00f18c89c720e1c34592bb85a1a8d311e6e99599 be safely cherry-picked into an older fork of Hermes running RN 71+ built from source? e.g. https://github.com/MetaMask/hermes if not, do we need to upgrade to e.g. RN 72/73/74rc first, then build from source and cherry-pick it? or is it more likely e.g. RN 75 to ship with a future bundled Static Hermes tag?

@leotm I doubt that it can be cherry picked as is. Hermes has been effectively frozen for an year (except for critical bug fixes and open source PRs). Meanwhile we have been actively working on the static_h branch and have diverged a lot. Static H will be much more spec compliant (among other things) and will be a backwards compatible drop-in replacement for Hermes.

We are planning to ship it as a replacement for Hermes as soon as possible (small number of months). Even if it isn't included in RN 0.75, it should be possible to replace Hermes manually. Details of the release are still being worked out.

leotm commented 1 month ago

noted ^ thanks this helps with our planning, last final thought (i'll mark Off Topic and raise if better as separate issue)

testing Hermes outside of RN with eshost recommends esvu/jsvu which grabs old Hermes v0.12.0 if successful for the right arch

and obvs hermes-engine-cli-v0.12.0.tgz (afaik ye olde playground version) is old too

is there anything i can do to help to get us an updated hermes-engine-cli release?

and/or output a bit more detailed ./hermes --version like

Hermes JavaScript compiler and Virtual Machine.
  Hermes release version: 0.12.0
  HBC bytecode version: 90
  Commit: <hash>

to determine which commit we're running

tmikov commented 1 month ago

That is a very good question. That version hasn't been updated since we started to bundle Hermes with RN releases years ago. Let me discuss this with the team and I will post back here.