Make BoundFunction non-lazy

Summary:
We currently create `BoundFunction` as lazy if the target is lazy, and
populate its properties from the target when they are accessed. However,
this doesn't account for the possibility that the target is modified
after the `BoundFunction` is created. If that happens, looking up the
property on the `BoundFunction` will retrieve the new value instead of
the value that was present when the function was bound.

To address this, make `BoundFunction` always non-lazy.

This should have very little effect in practice, since looking up
`target.bind` will itself make `target` non-lazy, so most
`BoundFunction`s should already be non-lazy.

Reviewed By: avp

Differential Revision: D68638177

fbshipit-source-id: 50726e819544efe2590f0ff7c79df9484b77c0b8

lib/VM/Callable.cpp

@@ -102,17 +102,6 @@ void Callable::defineLazyProperties(Handle<Callable> fn, Runtime &runtime) {
     assert(
         cr != ExecutionStatus::EXCEPTION && "failed to define length and name");
     (void)cr;
-  } else if (vmisa<BoundFunction>(fn.get())) {
-    Handle<BoundFunction> boundfn = Handle<BoundFunction>::vmcast(fn);
-    Handle<Callable> target = runtime.makeHandle(boundfn->getTarget(runtime));
-    unsigned int argsWithThis = boundfn->getArgCountWithThis(runtime);
-
-    auto res = BoundFunction::initializeLengthAndName_RJS(
-        boundfn, runtime, target, argsWithThis == 0 ? 0 : argsWithThis - 1);
-    assert(
-        res != ExecutionStatus::EXCEPTION &&
-        "failed to define length and name of bound function");
-    (void)res;
   } else {
     // no other kind of function can be lazy currently
     assert(false && "invalid lazy function");
@@ -536,16 +525,9 @@ CallResult<HermesValue> BoundFunction::create(
       arrHandle);
   auto selfHandle = JSObjectInit::initToHandle(runtime, cell);
 
-  if (target->isLazy()) {
-    // If the target is lazy we can make the bound function lazy.
-    // If the target is NOT lazy, it might have getter/setters on length that
-    // throws and we also need to throw.
-    selfHandle->flags_.lazyObject = 1;
-  } else {
-    if (initializeLengthAndName_RJS(selfHandle, runtime, target, argCount) ==
-        ExecutionStatus::EXCEPTION) {
-      return ExecutionStatus::EXCEPTION;
-    }
+  if (initializeLengthAndName_RJS(selfHandle, runtime, target, argCount) ==
+      ExecutionStatus::EXCEPTION) {
+    return ExecutionStatus::EXCEPTION;
   }
   return selfHandle.getHermesValue();
 }

test/hermes/regress-lazy-bound-function.js

@@ -0,0 +1,49 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes %s | %FileCheck --match-full-lines %s
+
+// We previously implemented BoundFunction as a lazy object if the target was
+// lazy. This resulted in incorrect behaviour if the target was updated after
+// the BoundFunction was created.
+
+function a(){}
+// Looking up `a.bind` would make `a` non-lazy, so this should always work.
+var boundA = a.bind();
+Object.defineProperty(a, "length", {value: 5});
+print(boundA.length);
+// CHECK: 0
+
+function b(arg1, arg2){}
+// `b` remains lazy here because we didn't look up anything on it.
+// Previously, this would have created a lazy `boundB`.
+var boundB = Function.prototype.bind.call(b, undefined, 1);
+// Modify the length property, making `b` non-lazy.
+Object.defineProperty(b, "length", {value: 5});
+// Check that `boundB` preserves the original length minus the bound arg.
+print(boundB.length);
+// CHECK-NEXT: 1
+
+function c(arg1, arg2, arg3){}
+// Modify the length property, making it a getter.
+Object.defineProperty(c, "length", {get: function(){print("c.length called"); return 3;}});
+// Check that binding the function calls the getter.
+var boundC = c.bind(undefined, 1);
+// CHECK-NEXT: c.length called
+
+// Check that the new length is the returned length minus the bound arg.
+print(boundC.length);
+// CHECK-NEXT: 2
+
+function d(arg1, arg2, arg3){}
+// As with `b` above, `d` remains lazy here.
+var boundD = Function.prototype.bind.call(d, undefined, 1);
+// Modify the length property, making it a throwing getter.
+Object.defineProperty(d, "length", {get: function(){throw new Error("d.length called"); }});
+// Check that the getter is not called.
+print(boundD.length);
+// CHECK-NEXT: 2