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

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

Closed
2 tasks
mhofman opened this issue Apr 11, 2024 · 13 comments
Closed
2 tasks

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

mhofman opened this issue Apr 11, 2024 · 13 comments
Labels
bug Something isn't working

Comments

@mhofman
Copy link

mhofman commented Apr 11, 2024

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 c42491d

  • 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 git revision (if applicable): c2f7dcd
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
Copy link
Contributor

tmikov commented Apr 11, 2024

Thanks for reporting this!

@tmikov
Copy link
Contributor

tmikov commented Apr 12, 2024

This was already fixed in Static Hermes in 00f18c8.

@tmikov tmikov closed this as completed Apr 12, 2024
@leotm
Copy link

leotm commented Apr 12, 2024

thanks ^ i found and checked out https://github.com/facebook/hermes/tree/static_h
which includes 00f18c8 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
Copy link

leotm commented Apr 12, 2024

can 00f18c8 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
Copy link
Contributor

tmikov commented Apr 12, 2024

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
Copy link
Contributor

tmikov commented Apr 12, 2024

can 00f18c8 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
Copy link

leotm commented Apr 12, 2024

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
Copy link
Contributor

tmikov commented Apr 12, 2024

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.

@leotm
Copy link

leotm commented Sep 9, 2024

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.

thank you for v0.13.0!
hopefully we're fetching latest versions now via eshost and jsvu
if not, i can submit PRs to both repos

@leotm
Copy link

leotm commented Sep 9, 2024

any chance publishing it? on/to https://www.npmjs.com/package/hermes-engine-cli (still v0.12.0) cc @fbmal7

@leotm
Copy link

leotm commented Sep 9, 2024

any chance publishing it? on/to https://www.npmjs.com/package/hermes-engine-cli (still v0.12.0) cc @fbmal7

i've currently integrated hermes-engine-cli (plus .bin symlinks) relying on npmjs in

if it won't be updated/published there, then will need to revert to fetching the .tar.gz then unpacking it as before

@tmikov
Copy link
Contributor

tmikov commented Sep 10, 2024

@leotm we are currently trying to formulate a strategy for publishing new versions of Hermes/Static Hermes, so any ideas/asks/suggestions are very welcome. Can you please describe the needs of your use case?

At the moment, our main distribution vehicle is React Native itself - when React Native makes a release, they build and package Hermes and include it in their distribution. This avoids any possible confusion about which Hermes version goes with which RN release.

The previous main distribution mechanism - hermes-engine.npm and hermes-engine-cli.npm - has been deprecated. At least for now, we don't have plans to restart it, since it appears that all use cases are served by RN. It also appears that - shockingly (at least to me) - NPM is really really bad at efficiently distributing binaries for different platforms, which is why RN chose a different mechanism for distributing our binaries - Maven.

We also plan to continue publishing .tgz distributions, although it is not clear who is using them. I guess they exist for the possibility that we have users outside of React Native.

@leotm
Copy link

leotm commented Sep 11, 2024

ideally a hermes-cli release alongside each RN/Hermes version, unless per major version is more feasible? like current/latest v0.13.0 for RN0.75.x,

the goal being to keep RN apps secure once there, so running Hardened JS (proposed to ECMA TC39) on Hermes, then running LavaMoat to sandbox deps with Compartments

hermes-engine-cli.npm would be ideal for dependabot/renovate to PR updates tracked in package.json, but .tgz distros work ^ no comment RE npm the distros, i feel the users consuming Hermes outside of RN are eshost (esvu/jsvu) and anyone else wanting to ensure compat with Hermes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants