From 582a529926ead92d888b7d99711ada796c1f8eff Mon Sep 17 00:00:00 2001
From: Xuan Huang <jsx@fb.com>
Date: Wed, 14 Apr 2021 16:23:25 -0700
Subject: [PATCH] Drop parameters on NativeFunction toString.

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 https://github.com/facebook/hermes/issues/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: https://github.com/tc39/ecma262/issues/2381.

Reviewed By: avp

Differential Revision: D27698165

fbshipit-source-id: bb39438bd21e34fb0c4d7c5df5d97db939648295
---
 lib/VM/JSLib/Function.cpp           | 56 +++++++++++++++--------------
 test/hermes/function-constructor.js | 14 ++++++--
 2 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/lib/VM/JSLib/Function.cpp b/lib/VM/JSLib/Function.cpp
index 3d2bf9bdb4f..0672bd3ef95 100644
--- a/lib/VM/JSLib/Function.cpp
+++ b/lib/VM/JSLib/Function.cpp
@@ -143,7 +143,7 @@ functionPrototypeToString(void *, Runtime *runtime, NativeArgs args) {
   // Extract the name.
   auto propRes = JSObject::getNamed_RJS(
       func, runtime, Predefined::getSymbolID(Predefined::name));
-  if (propRes == ExecutionStatus::EXCEPTION) {
+  if (LLVM_UNLIKELY(propRes == ExecutionStatus::EXCEPTION)) {
     return ExecutionStatus::EXCEPTION;
   }
 
@@ -151,39 +151,43 @@ functionPrototypeToString(void *, Runtime *runtime, NativeArgs args) {
   if (!(*propRes)->isUndefined()) {
     auto strRes =
         toString_RJS(runtime, runtime->makeHandle(std::move(*propRes)));
-    if (strRes == ExecutionStatus::EXCEPTION) {
+    if (LLVM_UNLIKELY(strRes == ExecutionStatus::EXCEPTION)) {
       return ExecutionStatus::EXCEPTION;
     }
     strRes->get()->appendUTF16String(strBuf);
   }
 
-  // Append the named parameters.
-  strBuf.append('(');
-
-  // Extract ".length".
-  auto lengthProp = Callable::extractOwnLengthProperty_RJS(func, runtime);
-  if (lengthProp == ExecutionStatus::EXCEPTION)
-    return ExecutionStatus::EXCEPTION;
+  // Formal parameters and the rest of the body.
+  if (vmisa<NativeFunction>(*func)) {
+    // Use [native code] here because we want to work with tools like Babel
+    // which detect the string "[native code]" and use it to alter behavior
+    // during the class transform.
+    // Also print without synthesized formal parameters to avoid breaking
+    // heuristics that detect the string "() { [native code] }".
+    // \see https://github.com/facebook/hermes/issues/471
+    strBuf.append("() { [native code] }");
+  } else {
+    // Append the synthesized formal parameters.
+    strBuf.append('(');
 
-  // The value of the property is not guaranteed to be meaningful, so clamp it
-  // to [0..65535] for sanity.
-  uint32_t paramCount = (uint32_t)std::min(65535.0, std::max(0.0, *lengthProp));
+    // Extract ".length".
+    auto lengthProp = Callable::extractOwnLengthProperty_RJS(func, runtime);
+    if (lengthProp == ExecutionStatus::EXCEPTION)
+      return ExecutionStatus::EXCEPTION;
 
-  for (uint32_t i = 0; i < paramCount; ++i) {
-    if (i != 0)
-      strBuf.append(", ");
-    char buf[16];
-    ::snprintf(buf, sizeof(buf), "a%u", i);
-    strBuf.append(buf);
-  }
+    // The value of the property is not guaranteed to be meaningful, so clamp it
+    // to [0..65535] for sanity.
+    uint32_t paramCount =
+        (uint32_t)std::min(65535.0, std::max(0.0, *lengthProp));
+
+    for (uint32_t i = 0; i < paramCount; ++i) {
+      if (i != 0)
+        strBuf.append(", ");
+      char buf[16];
+      ::snprintf(buf, sizeof(buf), "a%u", i);
+      strBuf.append(buf);
+    }
 
-  // The rest of the body.
-  if (vmisa<NativeFunction>(*func)) {
-    // Use [native code] here because we want to work with tools like
-    // Babel which detect the string [native code] and use it to alter
-    // behavior during the class transform.
-    strBuf.append(") { [native code] }");
-  } else {
     // Avoid using the [native code] string to prevent extra wrapping overhead
     // in, e.g., Babel's class extension mechanism.
     strBuf.append(") { [bytecode] }");
diff --git a/test/hermes/function-constructor.js b/test/hermes/function-constructor.js
index 219d26bc293..f5a5c7f5ef8 100644
--- a/test/hermes/function-constructor.js
+++ b/test/hermes/function-constructor.js
@@ -55,8 +55,12 @@ print(function(a,b,c){});
 // CHECK-NEXT: function (a0, a1, a2) { [bytecode] }
 print(function(a,b,c,d,e,f,g,h,i,j,k){});
 // CHECK-NEXT: function (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10) { [bytecode] }
-print(Map.toString());
-// CHECK-NEXT: function Map() { [native code] }
+
+// Reconfigured .length
+function foo(x) {}
+Object.defineProperty(foo, "length", { value: 5 })
+print(foo);
+// CHECK-NEXT: function foo(a0, a1, a2, a3, a4) { [bytecode] }
 
 // Non-string .length
 var foo = function badlength(a, b, c){};
@@ -64,6 +68,12 @@ Object.defineProperty(foo, "length", {value:"aa"});
 print(foo);
 // CHECK-NEXT: function badlength() { [bytecode] }
 
+// NativeFunctions are printed as 0-arity.
+print(Map);
+// CHECK-NEXT: function Map() { [native code] }
+print(Math.pow);
+// CHECK-NEXT: function pow() { [native code] }
+
 print('call');
 // CHECK-LABEL: call
 var f = function(a, b) {