Fixes nested evaluatePure calls and React nested optimized closure side-effect detection (#2166)

Summary:
Release notes: none

Fixes nested `evaluatePure` calls, where before they would collide and overwrite one another. Also ensure React reconciler uses side-effect detection on nested optimized closures. This fixes a React test which should have been failing before, but wasn't.
Closes https://github.com/facebook/prepack/pull/2166

Differential Revision: D8681422

Pulled By: trueadm

fbshipit-source-id: 8941812407d1bda5af0343a09210aeff24a36cff

scripts/__snapshots__/test-react.js.snap

@@ -2482,6 +2482,8 @@ ReactStatistics {
 }
 `;
 
+exports[`Test React with JSX input, JSX output Functional component folding Mutations - not-safe 3 1`] = `"Failed to render React component root \\"App\\" due to side-effects from mutating a property \\"x\\""`;
+
 exports[`Test React with JSX input, JSX output Functional component folding Mutations - safe 1 1`] = `
 ReactStatistics {
   "componentsEvaluated": 1,
@@ -2516,6 +2518,23 @@ ReactStatistics {
 }
 `;
 
+exports[`Test React with JSX input, JSX output Functional component folding Mutations - safe 3 1`] = `
+ReactStatistics {
+  "componentsEvaluated": 1,
+  "evaluatedRootNodes": Array [
+    Object {
+      "children": Array [],
+      "message": "",
+      "name": "App",
+      "status": "ROOT",
+    },
+  ],
+  "inlinedComponents": 0,
+  "optimizedNestedClosures": 0,
+  "optimizedTrees": 1,
+}
+`;
+
 exports[`Test React with JSX input, JSX output Functional component folding Null or undefined props 1`] = `
 ReactStatistics {
   "componentsEvaluated": 1,
@@ -7496,6 +7515,8 @@ ReactStatistics {
 }
 `;
 
+exports[`Test React with JSX input, create-element output Functional component folding Mutations - not-safe 3 1`] = `"Failed to render React component root \\"App\\" due to side-effects from mutating a property \\"x\\""`;
+
 exports[`Test React with JSX input, create-element output Functional component folding Mutations - safe 1 1`] = `
 ReactStatistics {
   "componentsEvaluated": 1,
@@ -7530,6 +7551,23 @@ ReactStatistics {
 }
 `;
 
+exports[`Test React with JSX input, create-element output Functional component folding Mutations - safe 3 1`] = `
+ReactStatistics {
+  "componentsEvaluated": 1,
+  "evaluatedRootNodes": Array [
+    Object {
+      "children": Array [],
+      "message": "",
+      "name": "App",
+      "status": "ROOT",
+    },
+  ],
+  "inlinedComponents": 0,
+  "optimizedNestedClosures": 0,
+  "optimizedTrees": 1,
+}
+`;
+
 exports[`Test React with JSX input, create-element output Functional component folding Null or undefined props 1`] = `
 ReactStatistics {
   "componentsEvaluated": 1,
@@ -12510,6 +12548,8 @@ ReactStatistics {
 }
 `;
 
+exports[`Test React with create-element input, JSX output Functional component folding Mutations - not-safe 3 1`] = `"Failed to render React component root \\"App\\" due to side-effects from mutating a property \\"x\\""`;
+
 exports[`Test React with create-element input, JSX output Functional component folding Mutations - safe 1 1`] = `
 ReactStatistics {
   "componentsEvaluated": 1,
@@ -12544,6 +12584,23 @@ ReactStatistics {
 }
 `;
 
+exports[`Test React with create-element input, JSX output Functional component folding Mutations - safe 3 1`] = `
+ReactStatistics {
+  "componentsEvaluated": 1,
+  "evaluatedRootNodes": Array [
+    Object {
+      "children": Array [],
+      "message": "",
+      "name": "App",
+      "status": "ROOT",
+    },
+  ],
+  "inlinedComponents": 0,
+  "optimizedNestedClosures": 0,
+  "optimizedTrees": 1,
+}
+`;
+
 exports[`Test React with create-element input, JSX output Functional component folding Null or undefined props 1`] = `
 ReactStatistics {
   "componentsEvaluated": 1,
@@ -17539,6 +17596,8 @@ ReactStatistics {
 }
 `;
 
+exports[`Test React with create-element input, create-element output Functional component folding Mutations - not-safe 3 1`] = `"Failed to render React component root \\"App\\" due to side-effects from mutating a property \\"x\\""`;
+
 exports[`Test React with create-element input, create-element output Functional component folding Mutations - safe 1 1`] = `
 ReactStatistics {
   "componentsEvaluated": 1,
@@ -17573,6 +17632,23 @@ ReactStatistics {
 }
 `;
 
+exports[`Test React with create-element input, create-element output Functional component folding Mutations - safe 3 1`] = `
+ReactStatistics {
+  "componentsEvaluated": 1,
+  "evaluatedRootNodes": Array [
+    Object {
+      "children": Array [],
+      "message": "",
+      "name": "App",
+      "status": "ROOT",
+    },
+  ],
+  "inlinedComponents": 0,
+  "optimizedNestedClosures": 0,
+  "optimizedTrees": 1,
+}
+`;
+
 exports[`Test React with create-element input, create-element output Functional component folding Null or undefined props 1`] = `
 ReactStatistics {
   "componentsEvaluated": 1,

scripts/test-react.js

@@ -409,6 +409,12 @@ function runTestSuite(outputJsx, shouldTranspileSource) {
         });
       });
 
+      it("Mutations - not-safe 3", async () => {
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "not-safe3.js");
+        });
+      });
+
       it("Mutations - safe 1", async () => {
         await runTest(directory, "safe.js");
       });
@@ -417,6 +423,10 @@ function runTestSuite(outputJsx, shouldTranspileSource) {
         await runTest(directory, "safe2.js");
       });
 
+      it("Mutations - safe 3", async () => {
+        await runTest(directory, "safe3.js");
+      });
+
       it("Handle mapped arrays", async () => {
         await runTest(directory, "array-map.js");
       });

src/intrinsics/prepack/global.js

@@ -203,7 +203,7 @@ export default function(realm: Realm): void {
         invariant(functionValue instanceof ECMAScriptSourceFunctionValue);
         invariant(typeof functionValue.$Call === "function");
         let functionCall: Function = functionValue.$Call;
-        return realm.evaluatePure(() => functionCall(realm.intrinsics.undefined, []));
+        return realm.evaluatePure(() => functionCall(realm.intrinsics.undefined, []), /*reportSideEffectFunc*/ null);
       }
     ),
     writable: true,

src/react/components.js

@@ -284,24 +284,26 @@ export function evaluateClassConstructor(
   let instanceProperties = new Set();
   let instanceSymbols = new Set();
 
-  realm.evaluatePure(() =>
-    realm.evaluateForEffects(
-      () => {
-        let instance = Construct(realm, constructorFunc, [props, context]);
-        invariant(instance instanceof ObjectValue);
-        for (let [propertyName] of instance.properties) {
-          if (!whitelistedProperties.has(propertyName)) {
-            instanceProperties.add(propertyName);
+  realm.evaluatePure(
+    () =>
+      realm.evaluateForEffects(
+        () => {
+          let instance = Construct(realm, constructorFunc, [props, context]);
+          invariant(instance instanceof ObjectValue);
+          for (let [propertyName] of instance.properties) {
+            if (!whitelistedProperties.has(propertyName)) {
+              instanceProperties.add(propertyName);
+            }
           }
-        }
-        for (let [symbol] of instance.symbols) {
-          instanceSymbols.add(symbol);
-        }
-        return instance;
-      },
-      /*state*/ null,
-      `react component constructor: ${constructorFunc.getName()}`
-    )
+          for (let [symbol] of instance.symbols) {
+            instanceSymbols.add(symbol);
+          }
+          return instance;
+        },
+        /*state*/ null,
+        `react component constructor: ${constructorFunc.getName()}`
+      ),
+    /*reportSideEffectFunc*/ null
   );
 
   return {

src/react/reconcilation.js

@@ -283,10 +283,12 @@ export class Reconciler {
     try {
       this.realm.react.activeReconciler = this;
       let effects = this.realm.wrapInGlobalEnv(() =>
-        this.realm.evaluatePure(() =>
-          evaluateWithNestedParentEffects(this.realm, nestedEffects, () =>
-            this.realm.evaluateForEffects(resolveOptimizedClosure, /*state*/ null, `react nested optimized closure`)
-          )
+        this.realm.evaluatePure(
+          () =>
+            evaluateWithNestedParentEffects(this.realm, nestedEffects, () =>
+              this.realm.evaluateForEffects(resolveOptimizedClosure, /*state*/ null, `react nested optimized closure`)
+            ),
+          this._handleReportedSideEffect
         )
       );
       this._handleNestedOptimizedClosuresFromEffects(effects, evaluatedNode);

src/realm.js

@@ -677,11 +677,9 @@ export class Realm {
   // call.
   evaluatePure<T>(
     f: () => T,
-    reportSideEffectFunc?: (
-      sideEffectType: SideEffectType,
-      binding: void | Binding | PropertyBinding,
-      value: void | Value
-    ) => void
+    reportSideEffectFunc:
+      | null
+      | ((sideEffectType: SideEffectType, binding: void | Binding | PropertyBinding, value: void | Value) => void)
   ) {
     let saved_createdObjectsTrackedForLeaks = this.createdObjectsTrackedForLeaks;
     let saved_reportSideEffectCallback = this.reportSideEffectCallback;
@@ -690,7 +688,15 @@ export class Realm {
     // *other* object is unchanged (pure). These objects are marked
     // as leaked if they're passed to abstract functions.
     this.createdObjectsTrackedForLeaks = new Set();
-    this.reportSideEffectCallback = reportSideEffectFunc;
+    this.reportSideEffectCallback = (...args) => {
+      if (reportSideEffectFunc != null) {
+        reportSideEffectFunc(...args);
+      }
+      // Ensure we call any previously nested side-effect callbacks
+      if (saved_reportSideEffectCallback != null) {
+        saved_reportSideEffectCallback(...args);
+      }
+    };
     try {
       return f();
     } finally {

src/serializer/functions.js

@@ -199,8 +199,9 @@ export class Functions {
       additionalFunctionStack.push(functionValue);
       invariant(functionValue instanceof ECMAScriptSourceFunctionValue);
       let call = this._callOfFunction(functionValue);
-      let effects: Effects = this.realm.evaluatePure(() =>
-        this.realm.evaluateForEffectsInGlobalEnv(call, undefined, "additional function")
+      let effects: Effects = this.realm.evaluatePure(
+        () => this.realm.evaluateForEffectsInGlobalEnv(call, undefined, "additional function"),
+        /*reportSideEffectFunc*/ null
       );
       invariant(effects);
       let additionalFunctionEffects = createAdditionalEffects(

test/react/functional-components/class-root-with-refs.js

@@ -10,15 +10,12 @@ function Child() {
   return <span><SubChild /></span>;
 }
 
-let instance = null;
-
 // we can't use ES2015 classes in Prepack yet (they don't serialize)
 // so we have to use ES5 instead
 var App = (function (superclass) {
   function App () {
     superclass.apply(this, arguments);
     this.divRefWorked = null;
-    instance = this;
   }
 
   if ( superclass ) {
@@ -39,7 +36,7 @@ var App = (function (superclass) {
     renderer.update(<Root />);
     let results = [];
     results.push(['render with class root', renderer.toJSON()]);
-    results.push(['get the ref', instance.divRefWorked]);
+    results.push(['get the ref', renderer.getInstance().divRefWorked]);
     return results;
   };
 

test/react/functional-components/not-safe3.js

@@ -0,0 +1,26 @@
+var React = require('react');
+// the JSX transform converts to React, so we need to add it back in
+this['React'] = React;
+
+
+if (!window.__evaluatePureFunction) {
+  window.__evaluatePureFunction = function(f) {
+    return f();
+  };
+}
+
+class App extends React.Component {
+  constructor() {
+    super();
+    window.__evaluatePureFunction(() => {
+      this.x = 5;
+    });
+  }
+  render() {
+    return <div>{this.x}</div>;
+  }
+}
+
+if (this.__optimizeReactComponentTree) {
+  __optimizeReactComponentTree(App);
+}

test/react/functional-components/safe3.js

@@ -0,0 +1,27 @@
+var React = require('react');
+// the JSX transform converts to React, so we need to add it back in
+this['React'] = React;
+
+
+if (!window.__evaluatePureFunction) {
+  window.__evaluatePureFunction = function(f) {
+    return f();
+  };
+}
+
+class App extends React.Component {
+  constructor() {
+    super();
+    this.x = <div />;
+    this.y = window.__evaluatePureFunction(() => {
+      return this.x;
+    });
+  }
+  render() {
+    return <div>{this.y}</div>;
+  }
+}
+
+if (this.__optimizeReactComponentTree) {
+  __optimizeReactComponentTree(App);
+}

test/react/functional-components/safe4.js

@@ -0,0 +1,36 @@
+if (!window.__evaluatePureFunction) {
+  window.__evaluatePureFunction = function(f) {
+    return f();
+  };
+}
+
+function foo(props) {
+  var abstract = props.props;
+  var outer = {};
+  var res =__evaluatePureFunction(() => {
+    var x = {a: 1};
+    __evaluatePureFunction(() => {
+      var y = {};
+  
+      // we know `outer` won't change so we don't havoc it:
+      abstract(outer);
+  
+      // should this havoc `x` or not?
+      // my intuition is that it *shouldn't* because the inner scope is "pure".
+      // but I think this PR would change that.
+      abstract(x);
+  
+      // this will havoc `y`:
+      abstract(y);
+    });
+    return x;
+  });
+  return res.a;
+}
+function Bar() {
+  foo();
+}
+
+if (this.__optimizeReactComponentTree) {
+  __optimizeReactComponentTree(Bar);
+}