Treat CreateThis as leaking the function

Summary:
The resulting value from a construct call (via the `new` operator)
should be considered as leaking the function. This is because the
constructed `this` can access the function via its parent's
`.constructor` property.

This may pessimize inlining, if there was previously only a single
callsite. It also hurts type inference, since the types of the parameters
can no longer be inferred.

Reviewed By: avp

Differential Revision: D67207850

fbshipit-source-id: 35dda62f6b01d090b3fc16a7af66c7809654efe6

include/hermes/Optimizer/Scalar/Utils.h

@@ -36,11 +36,6 @@ Value *isStoreOnceVariable(Variable *V);
 /// whole lifetime of the variable is constant.
 Value *isStoreOnceStackLocation(AllocStackInst *AS);
 
-/// \return True if \p V is an instruction that may be used in a constructor
-/// invocation of the \p closure. In the absence of other instructions that
-/// manipulate the closure, these instructions cannot leak the closure.
-bool isConstructionSetup(Value *V, Value *closure);
-
 /// \return a list of known callsites of \p F based on its users.
 /// It is possible that \p F has additional unknown callsites, call
 /// \c F->allCallsitesKnown() to check that.

lib/Optimizer/Scalar/FunctionAnalysis.cpp

@@ -198,9 +198,13 @@ void analyzeCreateCallable(BaseCreateCallableInst *create) {
         continue;
       }
 
-      // Construction setup instructions can't leak the closure on their own,
-      // but don't contribute to the call graph.
-      if (isConstructionSetup(closureUser, closureInst)) {
+      if (auto *CTI = llvh::dyn_cast<CreateThisInst>(closureUser)) {
+        assert(
+            CTI->getClosure() == closureInst &&
+            "Closure must be closure argument to CreateThisInst");
+        // CreateThis leaks the closure because the created object can still
+        // access the function via its parent's `.constructor` prototype.
+        F->getAttributesRef(M)._allCallsitesKnownInStrictMode = false;
         continue;
       }
 

lib/Optimizer/Scalar/Utils.cpp

@@ -72,27 +72,6 @@ Value *hermes::isStoreOnceStackLocation(AllocStackInst *AS) {
   return res;
 }
 
-bool hermes::isConstructionSetup(Value *V, Value *closure) {
-  // Constructor invocations access the closure, to read the "prototype"
-  // property and to create the "this" argument. However, in the absence of
-  // other accesses (like making "prototype" a setter), these operations
-  // cannot leak the closure.
-  if (auto *LPI = llvh::dyn_cast<LoadPropertyInst>(V))
-    if (LPI->getObject() == closure)
-      if (auto *LS = llvh::dyn_cast<LiteralString>(LPI->getProperty()))
-        if (LS->getValue().str() == "prototype")
-          return true;
-
-  if (auto *CTI = llvh::dyn_cast<CreateThisInst>(V)) {
-    (void)CTI;
-    assert(
-        CTI->getClosure() == closure &&
-        "Closure must be closure argument to CreateThisInst");
-    return true;
-  }
-  return false;
-}
-
 llvh::SmallVector<BaseCallInst *, 2> hermes::getKnownCallsites(Function *F) {
   llvh::SmallVector<BaseCallInst *, 2> result{};
   for (Instruction *user : F->getUsers()) {

test/Optimizer/cache-new-object-analysis.js

@@ -42,7 +42,7 @@ return new simple(1, 2);
 // CHECK-NEXT:       ReturnInst %4: object
 // CHECK-NEXT:function_end
 
-// CHECK:function simple(x: any, y: any): undefined [allCallsitesKnownInStrictMode]
+// CHECK:function simple(x: any, y: any): undefined
 // CHECK-NEXT:%BB0:
 // CHECK-NEXT:  %0 = LoadParamInst (:any) %<this>: any
 // CHECK-NEXT:  %1 = CoerceThisNSInst (:object) %0: any

test/Optimizer/callee.js

@@ -81,7 +81,7 @@ function load_store_multiple_test() {
 // CHECK-NEXT:  %0 = GetParentScopeInst (:environment) %VS0: any, %parentScope: environment
 // CHECK-NEXT:  %1 = CreateFunctionInst (:object) %0: environment, %"foo 1#"(): functionCode
 // CHECK-NEXT:  %2 = CreateThisInst (:undefined|object) %1: object, empty: any
-// CHECK-NEXT:  %3 = CallInst (:number) %1: object, %"foo 1#"(): functionCode, true: boolean, empty: any, undefined: undefined, 0: number, 12: number
+// CHECK-NEXT:  %3 = CallInst (:any) %1: object, %"foo 1#"(): functionCode, true: boolean, empty: any, undefined: undefined, %2: undefined|object, 12: number
 // CHECK-NEXT:  %4 = GetConstructedObjectInst (:object) %2: undefined|object, 12: number
 // CHECK-NEXT:       ReturnInst %4: object
 // CHECK-NEXT:function_end
@@ -117,9 +117,10 @@ function load_store_multiple_test() {
 // CHECK-NEXT:       ReturnInst 12: number
 // CHECK-NEXT:function_end
 
-// CHECK:function "foo 1#"(k: number): number [allCallsitesKnownInStrictMode]
+// CHECK:function "foo 1#"(k: any): any
 // CHECK-NEXT:%BB0:
-// CHECK-NEXT:       ReturnInst 12: number
+// CHECK-NEXT:  %0 = LoadParamInst (:any) %k: any
+// CHECK-NEXT:       ReturnInst %0: any
 // CHECK-NEXT:function_end
 
 // CHECK:function ping(): number [allCallsitesKnownInStrictMode]

test/Optimizer/constructor_callee.js

@@ -51,7 +51,7 @@ function ctor_load_store_test() {
 // CHECK-NEXT:  %0 = GetParentScopeInst (:environment) %VS0: any, %parentScope: environment
 // CHECK-NEXT:  %1 = CreateFunctionInst (:object) %0: environment, %use_this(): functionCode
 // CHECK-NEXT:  %2 = CreateThisInst (:undefined|object) %1: object, empty: any
-// CHECK-NEXT:  %3 = CallInst (:undefined|object) %1: object, %use_this(): functionCode, true: boolean, empty: any, undefined: undefined, %2: undefined|object, 12: number
+// CHECK-NEXT:  %3 = CallInst (:any) %1: object, %use_this(): functionCode, true: boolean, empty: any, undefined: undefined, %2: undefined|object, 12: number
 // CHECK-NEXT:  %4 = GetConstructedObjectInst (:object) %2: undefined|object, %2: undefined|object
 // CHECK-NEXT:       ReturnInst %4: object
 // CHECK-NEXT:function_end
@@ -69,17 +69,19 @@ function ctor_load_store_test() {
 // CHECK-NEXT:       ReturnInst %5: object
 // CHECK-NEXT:function_end
 
-// CHECK:function use_this(k: number): undefined|object [allCallsitesKnownInStrictMode]
+// CHECK:function use_this(k: any): any
 // CHECK-NEXT:%BB0:
-// CHECK-NEXT:  %0 = LoadParamInst (:undefined|object) %<this>: undefined|object
-// CHECK-NEXT:       StorePropertyStrictInst 12: number, %0: undefined|object, "k": string
-// CHECK-NEXT:       ReturnInst %0: undefined|object
+// CHECK-NEXT:  %0 = LoadParamInst (:any) %<this>: any
+// CHECK-NEXT:  %1 = LoadParamInst (:any) %k: any
+// CHECK-NEXT:       StorePropertyStrictInst %1: any, %0: any, "k": string
+// CHECK-NEXT:       ReturnInst %0: any
 // CHECK-NEXT:function_end
 
-// CHECK:function "use_this 1#"(k: number): undefined [allCallsitesKnownInStrictMode]
+// CHECK:function "use_this 1#"(k: any): undefined
 // CHECK-NEXT:%BB0:
-// CHECK-NEXT:  %0 = LoadParamInst (:undefined|object) %<this>: undefined|object
-// CHECK-NEXT:       StorePropertyStrictInst 12: number, %0: undefined|object, "k": string
+// CHECK-NEXT:  %0 = LoadParamInst (:any) %<this>: any
+// CHECK-NEXT:  %1 = LoadParamInst (:any) %k: any
+// CHECK-NEXT:       StorePropertyStrictInst %1: any, %0: any, "k": string
 // CHECK-NEXT:       ReturnInst undefined: undefined
 // CHECK-NEXT:function_end
 

test/Optimizer/constructor_callee_regexp.js

@@ -40,10 +40,11 @@ function ctor_this_test() {
 // CHECK-NEXT:       ReturnInst %3: object
 // CHECK-NEXT:function_end
 
-// CHECK:function use_this(k: number): object [allCallsitesKnownInStrictMode]
+// CHECK:function use_this(k: any): object
 // CHECK-NEXT:%BB0:
-// CHECK-NEXT:  %0 = LoadParamInst (:undefined|object) %<this>: undefined|object
-// CHECK-NEXT:       StorePropertyStrictInst 12: number, %0: undefined|object, "k": string
-// CHECK-NEXT:  %2 = CreateRegExpInst (:object) "regexp": string, "": string
-// CHECK-NEXT:       ReturnInst %2: object
+// CHECK-NEXT:  %0 = LoadParamInst (:any) %<this>: any
+// CHECK-NEXT:  %1 = LoadParamInst (:any) %k: any
+// CHECK-NEXT:       StorePropertyStrictInst %1: any, %0: any, "k": string
+// CHECK-NEXT:  %3 = CreateRegExpInst (:object) "regexp": string, "": string
+// CHECK-NEXT:       ReturnInst %3: object
 // CHECK-NEXT:function_end

test/Optimizer/function-analysis-construction-store.js

@@ -34,12 +34,12 @@ function main() {
 // CHECK-NEXT:  %0 = GetParentScopeInst (:environment) %VS0: any, %parentScope: environment
 // CHECK-NEXT:  %1 = CreateFunctionInst (:object) %0: environment, %f(): functionCode
 // CHECK-NEXT:  %2 = CreateThisInst (:undefined|object) %1: object, empty: any
-// CHECK-NEXT:  %3 = CallInst (:undefined) %1: object, %f(): functionCode, true: boolean, empty: any, undefined: undefined, 0: number
+// CHECK-NEXT:  %3 = CallInst (:undefined) %1: object, %f(): functionCode, true: boolean, empty: any, undefined: undefined, %2: undefined|object
 // CHECK-NEXT:  %4 = GetConstructedObjectInst (:object) %2: undefined|object, undefined: undefined
 // CHECK-NEXT:       ReturnInst %4: object
 // CHECK-NEXT:function_end
 
-// CHECK:function f(): undefined [allCallsitesKnownInStrictMode]
+// CHECK:function f(): undefined
 // CHECK-NEXT:%BB0:
 // CHECK-NEXT:       ReturnInst undefined: undefined
 // CHECK-NEXT:function_end

test/Optimizer/function-analysis-construction.js

@@ -33,12 +33,12 @@ function main() {
 // CHECK-NEXT:  %0 = GetParentScopeInst (:environment) %VS0: any, %parentScope: environment
 // CHECK-NEXT:  %1 = CreateFunctionInst (:object) %0: environment, %f(): functionCode
 // CHECK-NEXT:  %2 = CreateThisInst (:undefined|object) %1: object, empty: any
-// CHECK-NEXT:  %3 = CallInst (:undefined) %1: object, %f(): functionCode, true: boolean, empty: any, undefined: undefined, 0: number
+// CHECK-NEXT:  %3 = CallInst (:undefined) %1: object, %f(): functionCode, true: boolean, empty: any, undefined: undefined, %2: undefined|object
 // CHECK-NEXT:  %4 = GetConstructedObjectInst (:object) %2: undefined|object, undefined: undefined
 // CHECK-NEXT:       ReturnInst %4: object
 // CHECK-NEXT:function_end
 
-// CHECK:function f(): undefined [allCallsitesKnownInStrictMode]
+// CHECK:function f(): undefined
 // CHECK-NEXT:%BB0:
 // CHECK-NEXT:       ReturnInst undefined: undefined
 // CHECK-NEXT:function_end

test/Optimizer/inline-new-target.js

@@ -62,8 +62,8 @@ function outer2(){
 // CHECK-NEXT:       StoreFrameInst %1: environment, %2: object, [%VS1.foo]: object
 // CHECK-NEXT:  %4 = CreateFunctionInst (:object) %1: environment, %bar(): functionCode
 // CHECK-NEXT:  %5 = CreateThisInst (:undefined|object) %4: object, empty: any
-// CHECK-NEXT:  %6 = CreateThisInst (:undefined|object) %2: object, empty: any
-// CHECK-NEXT:       ReturnInst %2: object
+// CHECK-NEXT:  %6 = CallInst (:object) %4: object, %bar(): functionCode, true: boolean, %1: environment, undefined: undefined, %5: undefined|object
+// CHECK-NEXT:       ReturnInst %6: object
 // CHECK-NEXT:function_end
 
 // CHECK:function foo(): undefined|object
@@ -72,7 +72,10 @@ function outer2(){
 // CHECK-NEXT:       ReturnInst %0: undefined|object
 // CHECK-NEXT:function_end
 
-// CHECK:function bar(): object [allCallsitesKnownInStrictMode,unreachable]
+// CHECK:function bar(): object
 // CHECK-NEXT:%BB0:
-// CHECK-NEXT:       UnreachableInst
+// CHECK-NEXT:  %0 = GetParentScopeInst (:environment) %VS1: any, %parentScope: environment
+// CHECK-NEXT:  %1 = LoadFrameInst (:object) %0: environment, [%VS1.foo]: object
+// CHECK-NEXT:  %2 = CreateThisInst (:undefined|object) %1: object, empty: any
+// CHECK-NEXT:       ReturnInst %1: object
 // CHECK-NEXT:function_end

test/Optimizer/inline-sibling-constructors.js

@@ -45,9 +45,18 @@ function outer() {
 // CHECK-NEXT:       ReturnInst %4: object
 // CHECK-NEXT:function_end
 
-// CHECK:function Point(x: any, y: any, z: any): undefined [allCallsitesKnownInStrictMode,unreachable]
+// CHECK:function Point(x: any, y: any, z: any): undefined
 // CHECK-NEXT:%BB0:
-// CHECK-NEXT:       UnreachableInst
+// CHECK-NEXT:  %0 = LoadParamInst (:any) %<this>: any
+// CHECK-NEXT:  %1 = GetNewTargetInst (:undefined|object) %new.target: undefined|object
+// CHECK-NEXT:       CacheNewObjectInst %0: any, %1: undefined|object, "x": string, "y": string, "z": string
+// CHECK-NEXT:  %3 = LoadParamInst (:any) %x: any
+// CHECK-NEXT:  %4 = LoadParamInst (:any) %y: any
+// CHECK-NEXT:  %5 = LoadParamInst (:any) %z: any
+// CHECK-NEXT:       StorePropertyStrictInst %3: any, %0: any, "x": string
+// CHECK-NEXT:       StorePropertyStrictInst %4: any, %0: any, "y": string
+// CHECK-NEXT:       StorePropertyStrictInst %5: any, %0: any, "z": string
+// CHECK-NEXT:       ReturnInst undefined: undefined
 // CHECK-NEXT:function_end
 
 // CHECK:function makePoint(x: any, y: any, z: any): object

test/Optimizer/regress-construct.js

@@ -46,9 +46,9 @@ function main() {
 // CHECK-NEXT:       ReturnInst %5: object
 // CHECK-NEXT:function_end
 
-// CHECK:function y(): undefined [allCallsitesKnownInStrictMode,unreachable]
+// CHECK:function y(): undefined
 // CHECK-NEXT:%BB0:
-// CHECK-NEXT:       UnreachableInst
+// CHECK-NEXT:       ReturnInst undefined: undefined
 // CHECK-NEXT:function_end
 
 // CHECK:function value(): null|object

test/hermes/regress-new.js

@@ -0,0 +1,27 @@
+/**
+ * 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 -O %s | %FileCheck --match-full-lines %s
+// RUN: %hermes -O -emit-binary -out %t.hbc %s && %hermes %t.hbc | %FileCheck --match-full-lines %s
+// RUN: %shermes -exec %s | %FileCheck --match-full-lines %s
+
+print("regress new");
+// CHECK: regress new
+
+// We correctly detect that constructing `this` in a construct call leaks its target closure, since it's accessible via `.constructor`.
+(function () {
+  function target({ a, b }) {
+    return a + b;
+  }
+
+  function parent() {
+    return new target({});
+  }
+
+  print(parent().constructor({ a: 10, b: 20 }));
+// CHECK-NEXT: 30
+})();