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

[Normative] NativeFunction toString is too permissive vs. the reality #2381

Open
Huxpro opened this issue Apr 10, 2021 · 3 comments
Open

[Normative] NativeFunction toString is too permissive vs. the reality #2381

Huxpro opened this issue Apr 10, 2021 · 3 comments

Comments

@Huxpro
Copy link
Member

Huxpro commented Apr 10, 2021

Description:

Starting from https://tc39.es/Function-prototype-toString-revision/, Function.prototype.toString is specified to return an implementation-dependent String source code representation which must have the syntax of a NativeFunction for built-in Function object, et al.

The "syntax of a NativeFunction", is defined as:

NativeFunction:
  functionPropertyName[~Yield, ~Await]opt(FormalParameters[~Yield, ~Await]){[nativecode]}

In which the nonterminal FormalParameters seems to be not optional? Intuitively, the FormalParameters of a built-in Function object is defined as the title of the correspondent clauses, e.g. 21.3.2.26 Math.pow ( base, exponent ).

However, the reality is that no engine prints formal parameters exactly as the title:

  • Most engines print no parameters at all.
  • Hermes prints synthesized formal parameters from the function arity.

eshost Output:

λ eshost -s -e "Math.pow.toString()"
#### Chakra, V8
function pow() { [native code] }

#### Hermes
function pow(a0, a1) { [native code] }

#### JavaScriptCore, SpiderMonkey
function pow() {
    [native code]
}

Description (continued):

You may wonder "what's the big deal?". Well, turns out there are popular libraries, namely Lodash, that detects NativeFunction by pattern matching the toString result assuming there are no parameters:

  /** Used to detect if a method is native. */
  var reIsNative = RegExp('^' +
    funcToString.call(hasOwnProperty).replace(reRegExpChar, '\\$&')
    .replace(/hasOwnProperty|(function).*?(?=\\\()| for .+?(?=\\\])/g, '$1.*?') + '$'
  );

Thus, Lodash mistakenly thought Hermes doesn't support native Map and resulted in slow path and gigantic performance drop (25-45x) of its implementation of cloneDeep (and presumably others) : facebook/hermes#471 (comment)

@bathos
Copy link
Contributor

bathos commented Apr 10, 2021

nonterminal FormalParameters seems to be not optional

The first alternative is [empty]. It seems like all three examples there conform to the grammar. The bug is pretty squarely in lodash, but they use heuristics for a lot of their tests and may just consider that an acceptable trade. They’ll also get false positives for bound functions and wasm exports in this example.

@Huxpro
Copy link
Member Author

Huxpro commented Apr 10, 2021

The first alternative is [empty].

Fair enough. I can see how they are all valid productions grammar-wise.

That being said, if it's been a commonly used heuristics in the wild JS community. Shall we enforce the spec to use [empty] for Function.prototype.toString instead of the vague description of "must have the syntax of NativeFunction"?

Maybe "must have the syntax of NativeFunction with the FormalParameter nonterminal being [empty]" or something like that?

I'm also very curious why did all every other engines choose to pick the [empty] alternative?

@Huxpro Huxpro changed the title FormalParameters in NativeFunction syntax is not respected FormalParameters in NativeFunction syntax is too permissive comparing to the reality Apr 11, 2021
@Huxpro Huxpro changed the title FormalParameters in NativeFunction syntax is too permissive comparing to the reality [Normative] NativeFunction toString is too permissive vs. the reality Apr 11, 2021
@bathos
Copy link
Contributor

bathos commented Apr 12, 2021

If I understand right, the NativeFunction stuff aimed to standardize an existing facet of web reality. I don’t know what additional context might have informed the decision to include FormalParameters, but I agree that it seems kind of pointless (again, on the surface, not sure of the history here) if all engines either are not including them or would be willing to stop including them.

facebook-github-bot pushed a commit to facebook/hermes that referenced this issue Apr 14, 2021
Summary:
Hermes, historistically, synthsize formal parameters from the
function arity, while all other engines print NativeFunction as
0-arity. This gave the JS community the assumption that this
behavior is standardized and come out heuristics that detect
NativeFunction by matching on `() { [native code] }` and resulted
in Hermes fail the check and trigger librares' slow path.

This diff drop the synthsized parameters for Hermes toString
implementation on NativeFunction to avoid breaking those heuristcs.

This fix #471.

This change alone improved `test-lodash` benchmark on Hermes
from `787` to `43` which is in the same ballpark with others on my local:
`v8 --jitless` (30), `jsc --useJIT=false` (44), and `qjs` (60).

Also see normative dicussion to ECMA-262 on enforcing the spec
closer to the reality: tc39/ecma262#2381.

Reviewed By: avp

Differential Revision: D27698165

fbshipit-source-id: bb39438bd21e34fb0c4d7c5df5d97db939648295
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants