-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Comments
The first alternative is |
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 Maybe "must have the syntax of NativeFunction with the FormalParameter nonterminal being I'm also very curious why did all every other engines choose to pick the |
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. |
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
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:
In which the nonterminal
FormalParameters
seems to be not optional? Intuitively, theFormalParameters
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:
eshost Output:
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:Thus, Lodash mistakenly thought Hermes doesn't support native
Map
and resulted in slow path and gigantic performance drop (25-45x) of its implementation ofcloneDeep
(and presumably others) : facebook/hermes#471 (comment)The text was updated successfully, but these errors were encountered: