Throw an error when side-effectful logic happens in a React component tree

Summary:
Release notes: the React reconciler now throw a FatalError upon encountering side-effects in a render

This PR revamps the current React system's restrictions for what you can and can't do during the React reconcilation phase. This is a pretty large update but provides much better boundaries between what is "safe" and not "safe", thus reducing the constraints.

1. A new error `ReconcilerRenderBailOut` is thrown if something occurs in the React reconciliation that causes the render to fail and it's a hard fail – no recovering and continuing.
2. If you mutate a binding/object outside the render phase, given the React component render phase is meant to be "pure", a `ReconcilerRenderBailOut` will be thrown.
3. If you `throw` during the React reconciliation phase, again a `ReconcilerRenderBailOut` will be thrown.

In the future, we should maybe loosen the constraints around all this and maybe allow `throw`, but right now it's causing too much friction. We should attempt to make React components render phase as pure as possible – as it results in much better optimizations by a compiler because we can assert far more without things tripping us up.

Another point, regarding (1), is that we should ideally be able to recover from the error thrown in Prepack. The reason we can't and is something that might be a very deep issue in Prepack, is that effects don't properly restore when we have nested PossiblyNormalCompletions at work. Bindings get mutated on records from changes made within `evaluateForEffects` but never get reset when in nested PossiblyNormalCompletion. If I remove all the things that can cause `PossiblyNormalCompletion`s then everything works fine and bindings do get restored. We can remove the constraint on (1) once we've found and fixed that issue.
Closes https://github.com/facebook/prepack/pull/1860

Differential Revision: D7950562

Pulled By: trueadm

fbshipit-source-id: 4657e68b084c7069622e88c9655823b5f1f9386f

scripts/debug-fb-www.js

@@ -192,7 +192,7 @@ function printReactEvaluationGraph(evaluatedRootNode, depth) {
     let line;
     if (status === "inlined") {
       line = `${chalk.gray(`-`)} ${chalk.green(name)} ${chalk.gray(`(${status + message})`)}`;
-    } else if (status === "unsupported_completion" || status === "unknown_type" || status === "bail-out") {
+    } else if (status === "unknown_type" || status === "bail-out" || status === "fatal") {
       line = `${chalk.gray(`-`)} ${chalk.red(name)} ${chalk.gray(`(${status + message})`)}`;
     } else {
       line = `${chalk.gray(`-`)} ${chalk.yellow(name)} ${chalk.gray(`(${status + message})`)}`;
@@ -247,6 +247,11 @@ readComponentsList()
   })
   .catch(e => {
     console.log(`\n${chalk.inverse(`=== Compilation Failed ===`)}\n`);
-    console.error(e.nativeStack || e.stack);
+    if (e.__isReconcilerFatalError) {
+      console.error(e.message + "\n");
+      printReactEvaluationGraph(e.evaluatedNode, 0);
+    } else {
+      console.error(e.nativeStack || e.stack);
+    }
     process.exit(1);
   });

scripts/test-react.js

@@ -85,6 +85,20 @@ function runTestSuite(outputJsx, shouldTranspileSource) {
     stripFlow: true,
   };
 
+  let checkForReconcilerFatalError = false;
+
+  async function expectReconcilerFatalError(func) {
+    checkForReconcilerFatalError = true;
+    try {
+      await func();
+    } catch (e) {
+      expect(e.__isReconcilerFatalError).toBe(true);
+      expect(e.message).toMatchSnapshot();
+    } finally {
+      checkForReconcilerFatalError = false;
+    }
+  }
+
   function compileSourceWithPrepack(source) {
     let code = `(function(){${source}})()`;
     let serialized;
@@ -92,6 +106,9 @@ function runTestSuite(outputJsx, shouldTranspileSource) {
     try {
       serialized = prepackSources([{ filePath: "", fileContents: code, sourceMapContents: "" }], prepackOptions);
     } catch (e) {
+      if (e.__isReconcilerFatalError && checkForReconcilerFatalError) {
+        throw e;
+      }
       errorsCaptured.forEach(error => {
         console.error(error);
       });
@@ -274,25 +291,59 @@ function runTestSuite(outputJsx, shouldTranspileSource) {
       });
 
       it("Simple 8", async () => {
-        await runTest(directory, "simple-8.js");
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "simple-8.js");
+        });
       });
 
       it("Simple 9", async () => {
-        await runTest(directory, "simple-9.js");
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "simple-9.js");
+        });
       });
 
       it("Simple 10", async () => {
-        await runTest(directory, "simple-10.js");
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "simple-10.js");
+        });
       });
 
       it("Simple 11", async () => {
-        await runTest(directory, "simple-11.js");
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "simple-11.js");
+        });
       });
 
       it("Simple 12", async () => {
         await runTest(directory, "simple-12.js");
       });
 
+      it("Simple 13", async () => {
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "simple-13.js");
+        });
+      });
+
+      it("Mutations - not-safe 1", async () => {
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "not-safe.js");
+        });
+      });
+
+      it("Mutations - not-safe 2", async () => {
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "not-safe2.js");
+        });
+      });
+
+      it("Mutations - safe 1", async () => {
+        await runTest(directory, "safe.js");
+      });
+
+      it("Mutations - safe 2", async () => {
+        await runTest(directory, "safe2.js");
+      });
+
       it("Handle mapped arrays", async () => {
         await runTest(directory, "array-map.js");
       });
@@ -626,10 +677,6 @@ function runTestSuite(outputJsx, shouldTranspileSource) {
         await runTest(directory, "simple-2.js", true);
       });
 
-      it("Class component as root with refs", async () => {
-        await runTest(directory, "class-root-with-refs.js", true);
-      });
-
       it("componentWillMount", async () => {
         await runTest(directory, "will-mount.js", true);
       });
@@ -741,7 +788,9 @@ function runTestSuite(outputJsx, shouldTranspileSource) {
       });
 
       it("fb-www 12", async () => {
-        await runTest(directory, "fb12.js");
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "fb12.js");
+        });
       });
 
       it("fb-www 13", async () => {
@@ -753,11 +802,15 @@ function runTestSuite(outputJsx, shouldTranspileSource) {
       });
 
       it("fb-www 15", async () => {
-        await runTest(directory, "fb15.js");
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "fb15.js");
+        });
       });
 
       it("fb-www 16", async () => {
-        await runTest(directory, "fb16.js");
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "fb16.js");
+        });
       });
 
       it("fb-www 17", async () => {
@@ -769,7 +822,9 @@ function runTestSuite(outputJsx, shouldTranspileSource) {
       });
 
       it("fb-www 19", async () => {
-        await runTest(directory, "fb19.js");
+        await expectReconcilerFatalError(async () => {
+          await runTest(directory, "fb19.js");
+        });
       });
 
       it("fb-www 20", async () => {

src/environment.js

@@ -68,7 +68,7 @@ function deriveGetBinding(realm: Realm, binding: Binding) {
 export function havocBinding(binding: Binding) {
   let realm = binding.environment.realm;
   let value = binding.value;
-  if (!binding.hasLeaked && (value instanceof ObjectValue && !value.isFinalObject())) {
+  if (!binding.hasLeaked && !(value instanceof ObjectValue && value.isFinalObject)) {
     realm.recordModifiedBinding(binding).hasLeaked = true;
   }
 }
@@ -244,7 +244,7 @@ export class DeclarativeEnvironmentRecord extends EnvironmentRecord {
     invariant(binding && !binding.initialized, `shouldn't have the binding ${N}`);
 
     // 3. Set the bound value for N in envRec to V.
-    if (!skipRecord) this.realm.recordModifiedBinding(binding).value = V;
+    if (!skipRecord) this.realm.recordModifiedBinding(binding, V).value = V;
     else binding.value = V;
 
     // 4. Record that the binding for N in envRec has been initialized.
@@ -294,7 +294,7 @@ export class DeclarativeEnvironmentRecord extends EnvironmentRecord {
         invariant(realm.generator);
         realm.generator.emitBindingAssignment(binding, V);
       } else {
-        realm.recordModifiedBinding(binding).value = V;
+        realm.recordModifiedBinding(binding, V).value = V;
       }
     } else {
       // 6. Else,

src/evaluators/ThrowStatement.js

@@ -24,5 +24,8 @@ export default function(
 ): Value {
   let exprRef = env.evaluate(ast.argument, strictCode);
   let exprValue = Environment.GetValue(realm, exprRef);
+  if (realm.isInPureScope() && realm.reportSideEffectCallback !== undefined) {
+    realm.reportSideEffectCallback("EXCEPTION_THROWN", undefined, ast.loc);
+  }
   throw new ThrowCompletion(exprValue, ast.loc);
 }

src/react/errors.js

@@ -10,6 +10,7 @@
 /* @flow */
 
 import { type ReactEvaluatedNode } from "../serializer/types.js";
+import { FatalError } from "../errors.js";
 
 // ExpectedBailOut is like an error, that gets thrown during the reconcilation phase
 // allowing the reconcilation to continue on other branches of the tree, the message
@@ -23,6 +24,12 @@ export class ExpectedBailOut extends Error {}
 // and an alternative complex class component route being used
 export class SimpleClassBailOut extends Error {}
 
+// When the reconciler detectes a side-effect in pure evaluation, it throws one
+// of these errors. This will fall straight through the the wrapping React
+// component render try/catch, which will then throw an appropiate
+// ReconcilerFatalError along with information on the React component stack
+export class UnsupportedSideEffect extends Error {}
+
 // NewComponentTreeBranch only occur when a complex class is found in a
 // component tree and the reconciler can no longer fold the component of that branch
 export class NewComponentTreeBranch extends Error {
@@ -32,3 +39,19 @@ export class NewComponentTreeBranch extends Error {
   }
   evaluatedNode: ReactEvaluatedNode;
 }
+
+// Used when an entire React component tree has failed to optimize
+// this means there is a programming bug in the application that is
+// being Prepacked
+export class ReconcilerFatalError extends FatalError {
+  constructor(message: string, evaluatedNode: ReactEvaluatedNode) {
+    super(message);
+    evaluatedNode.status = "FATAL";
+    evaluatedNode.message = message;
+    this.evaluatedNode = evaluatedNode;
+    // used for assertions in tests
+    this.__isReconcilerFatalError = true;
+  }
+  evaluatedNode: ReactEvaluatedNode;
+  __isReconcilerFatalError: boolean;
+}

src/react/reconcilation.js

@@ -9,7 +9,7 @@
 
 /* @flow */
 
-import { Realm, type Effects } from "../realm.js";
+import { Realm, type Effects, type SideEffectType } from "../realm.js";
 import {
   AbstractObjectValue,
   AbstractValue,
@@ -28,28 +28,30 @@ import {
 } from "../values/index.js";
 import { ReactStatistics, type ReactSerializerState, type ReactEvaluatedNode } from "../serializer/types.js";
 import {
-  isReactElement,
-  valueIsClassComponent,
+  createReactEvaluatedNode,
+  evaluateWithNestedParentEffects,
+  flattenChildren,
   forEachArrayValue,
-  valueIsLegacyCreateClassComponent,
+  getComponentName,
+  getComponentTypeFromRootValue,
+  getLocationFromValue,
+  getProperty,
+  getReactSymbol,
+  getValueFromFunctionCall,
+  isReactElement,
+  sanitizeReactElementForFirstRenderOnly,
+  setProperty,
+  valueIsClassComponent,
   valueIsFactoryClassComponent,
   valueIsKnownReactAbstraction,
-  getReactSymbol,
-  flattenChildren,
-  getProperty,
-  setProperty,
-  createReactEvaluatedNode,
-  getComponentName,
-  sanitizeReactElementForFirstRenderOnly,
-  getValueFromFunctionCall,
-  evaluateWithNestedParentEffects,
-  getComponentTypeFromRootValue,
+  valueIsLegacyCreateClassComponent,
 } from "./utils";
 import { Get } from "../methods/index.js";
 import invariant from "../invariant.js";
 import { FatalError, CompilerDiagnostic } from "../errors.js";
 import { BranchState, type BranchStatusEnum } from "./branching.js";
 import * as t from "babel-types";
+import { Completion } from "../completions.js";
 import {
   getInitialProps,
   getInitialContext,
@@ -59,11 +61,17 @@ import {
   createClassInstanceForFirstRenderOnly,
   applyGetDerivedStateFromProps,
 } from "./components.js";
-import { ExpectedBailOut, SimpleClassBailOut, NewComponentTreeBranch } from "./errors.js";
-import { AbruptCompletion, Completion } from "../completions.js";
+import {
+  ExpectedBailOut,
+  NewComponentTreeBranch,
+  ReconcilerFatalError,
+  SimpleClassBailOut,
+  UnsupportedSideEffect,
+} from "./errors.js";
 import { Logger } from "../utils/logger.js";
-import type { ClassComponentMetadata, ReactComponentTreeConfig, ReactHint } from "../types.js";
+import type { ClassComponentMetadata, PropertyBinding, ReactComponentTreeConfig, ReactHint } from "../types.js";
 import { createInternalReactElement } from "./elements.js";
+import type { Binding } from "../environment.js";
 
 type ComponentResolutionStrategy =
   | "NORMAL"
@@ -134,8 +142,7 @@ export class Reconciler {
     props: ObjectValue | AbstractObjectValue | null,
     context: ObjectValue | AbstractObjectValue | null,
     evaluatedRootNode: ReactEvaluatedNode
-  ): Effects | null {
-    let failed = false;
+  ): Effects {
     const renderComponentTree = () => {
       // initialProps and initialContext are created from Flow types from:
       // - if a functional component, the 1st and 2nd paramater of function
@@ -158,51 +165,24 @@ export class Reconciler {
         this.statistics.optimizedTrees++;
         return result;
       } catch (error) {
-        if (error.name === "Invariant Violation") {
-          throw error;
-        }
-        if (error instanceof ExpectedBailOut) {
-          this.logger.logWarning(
-            componentType,
-            `__optimizeReactComponentTree() React component tree failed due expected bail-out - ${error.message}`
-          );
-          evaluatedRootNode.message = error.message;
-          evaluatedRootNode.status = "BAIL-OUT";
-        } else if (error instanceof AbruptCompletion) {
-          this.logger.logWarning(
-            componentType,
-            `__optimizeReactComponentTree() React component tree failed due runtime runtime exception thrown`
-          );
-          evaluatedRootNode.status = "ABRUPT_COMPLETION";
-        } else {
-          this.logger.logWarning(
-            componentType,
-            `__optimizeReactComponentTree() React component tree failed due to - ${error.message}`
-          );
-          evaluatedRootNode.message = "evaluation failed on new component tree branch";
-          evaluatedRootNode.status = "BAIL-OUT";
-        }
-        failed = true;
-        return this.realm.intrinsics.undefined;
+        this._handleComponentTreeRootFailure(error, evaluatedRootNode);
+        // flow belives we can get here, when it should never be possible
+        invariant(false, "renderReactComponentTree error not handled correctly");
       }
     };
 
     let effects = this.realm.wrapInGlobalEnv(() =>
-      this.realm.evaluatePure(() =>
-        // TODO: (sebmarkbage): You could use the return value of this to detect if there are any mutations on objects other
-        // than newly created ones. Then log those to the error logger. That'll help us track violations in
-        // components. :)
-        this.realm.evaluateForEffects(
-          renderComponentTree,
-          /*state*/ null,
-          `react component: ${getComponentName(this.realm, componentType)}`
-        )
+      this.realm.evaluatePure(
+        () =>
+          this.realm.evaluateForEffects(
+            renderComponentTree,
+            /*state*/ null,
+            `react component: ${getComponentName(this.realm, componentType)}`
+          ),
+        this._handleReportedSideEffect
       )
     );
     this._handleNestedOptimizedClosuresFromEffects(effects, evaluatedRootNode);
-    if (failed) {
-      return null;
-    }
     return effects;
   }
 
@@ -221,8 +201,7 @@ export class Reconciler {
     context: ObjectValue | AbstractObjectValue | null,
     branchState: BranchState | null,
     evaluatedNode: ReactEvaluatedNode
-  ): Effects | null {
-    let failed = false;
+  ): Effects {
     const renderOptimizedClosure = () => {
       let baseObject = this.realm.$GlobalEnv.environmentRecord.WithBaseObject();
       // we want to optimize the function that is bound
@@ -279,11 +258,9 @@ export class Reconciler {
         this.statistics.optimizedNestedClosures++;
         return result;
       } catch (error) {
-        if (error.name === "Invariant Violation") {
-          throw error;
-        }
-        failed = true;
-        return this.realm.intrinsics.undefined;
+        this._handleComponentTreeRootFailure(error, evaluatedNode);
+        // flow belives we can get here, when it should never be possible
+        invariant(false, "renderNestedOptimizedClosure error not handled correctly");
       }
     };
 
@@ -295,9 +272,6 @@ export class Reconciler {
       )
     );
     this._handleNestedOptimizedClosuresFromEffects(effects, evaluatedNode);
-    if (failed) {
-      return null;
-    }
     return effects;
   }
 
@@ -1280,6 +1254,46 @@ export class Reconciler {
     }
   }
 
+  _handleComponentTreeRootFailure(error: Error | Completion, evaluatedRootNode: ReactEvaluatedNode): void {
+    if (error.name === "Invariant Violation") {
+      throw error;
+    } else if (error instanceof ReconcilerFatalError) {
+      throw new ReconcilerFatalError(error.message, evaluatedRootNode);
+    } else if (error instanceof UnsupportedSideEffect) {
+      throw new ReconcilerFatalError(
+        `Failed to render React component root "${evaluatedRootNode.name}" due to ${error.message}`,
+        evaluatedRootNode
+      );
+    } else if (error instanceof Completion) {
+      let value = error.value;
+      invariant(value instanceof ObjectValue);
+      let message = getProperty(this.realm, value, "message");
+      let stack = getProperty(this.realm, value, "stack");
+      invariant(message instanceof StringValue);
+      invariant(stack instanceof StringValue);
+      throw new ReconcilerFatalError(
+        `Failed to render React component "${evaluatedRootNode.name}" due to a JS error: ${message.value}\n${
+          stack.value
+        }`,
+        evaluatedRootNode
+      );
+    }
+    let message;
+    if (error instanceof ExpectedBailOut) {
+      message = `Failed to optimize React component tree for "${evaluatedRootNode.name}" due to an expected bail-out: ${
+        error.message
+      }`;
+    } else if (error instanceof FatalError) {
+      message = `Failed to optimize React component tree for "${
+        evaluatedRootNode.name
+      }" due to a fatal error during evaluation: ${error.message}`;
+    } else {
+      // if we don't know what the error is, then best to rethrow
+      throw error;
+    }
+    throw new ReconcilerFatalError(message, evaluatedRootNode);
+  }
+
   _resolveComponentResolutionFailure(
     error: Error | Completion,
     reactElement: ObjectValue,
@@ -1289,6 +1303,24 @@ export class Reconciler {
   ): Value {
     if (error.name === "Invariant Violation") {
       throw error;
+    } else if (error instanceof ReconcilerFatalError) {
+      throw error;
+    } else if (error instanceof UnsupportedSideEffect) {
+      throw new ReconcilerFatalError(
+        `Failed to render React component "${evaluatedNode.name}" due to ${error.message}`,
+        evaluatedNode
+      );
+    } else if (error instanceof Completion) {
+      let value = error.value;
+      invariant(value instanceof ObjectValue);
+      let message = getProperty(this.realm, value, "message");
+      let stack = getProperty(this.realm, value, "stack");
+      invariant(message instanceof StringValue);
+      invariant(stack instanceof StringValue);
+      throw new ReconcilerFatalError(
+        `Failed to render React component "${evaluatedNode.name}" due to a JS error: ${message.value}\n${stack.value}`,
+        evaluatedNode
+      );
     }
     let typeValue = getProperty(this.realm, reactElement, "type");
     let propsValue = getProperty(this.realm, reactElement, "props");
@@ -1299,25 +1331,20 @@ export class Reconciler {
       // NO-OP (we don't queue a newComponentTree as this was already done)
     } else {
       // handle abrupt completions
-      if (error instanceof AbruptCompletion) {
-        let evaluatedChildNode = createReactEvaluatedNode("ABRUPT_COMPLETION", getComponentName(this.realm, typeValue));
-        evaluatedNode.children.push(evaluatedChildNode);
+      let evaluatedChildNode = createReactEvaluatedNode("BAIL-OUT", getComponentName(this.realm, typeValue));
+      evaluatedNode.children.push(evaluatedChildNode);
+      this._queueNewComponentTree(typeValue, evaluatedChildNode);
+      this._findReactComponentTrees(propsValue, evaluatedNode, "NORMAL_FUNCTIONS");
+      if (error instanceof ExpectedBailOut) {
+        evaluatedChildNode.message = error.message;
+        this._assignBailOutMessage(reactElement, error.message);
+      } else if (error instanceof FatalError) {
+        let message = "evaluation failed";
+        evaluatedChildNode.message = message;
+        this._assignBailOutMessage(reactElement, message);
       } else {
-        let evaluatedChildNode = createReactEvaluatedNode("BAIL-OUT", getComponentName(this.realm, typeValue));
-        evaluatedNode.children.push(evaluatedChildNode);
-        this._queueNewComponentTree(typeValue, evaluatedChildNode);
-        this._findReactComponentTrees(propsValue, evaluatedNode, "NORMAL_FUNCTIONS");
-        if (error instanceof ExpectedBailOut) {
-          evaluatedChildNode.message = error.message;
-          this._assignBailOutMessage(reactElement, error.message);
-        } else if (error instanceof FatalError) {
-          let message = "evaluation failed";
-          evaluatedChildNode.message = message;
-          this._assignBailOutMessage(reactElement, message);
-        } else {
-          evaluatedChildNode.message = `unknown error`;
-          throw error;
-        }
+        evaluatedChildNode.message = `unknown error`;
+        throw error;
       }
     }
     // a child component bailed out during component folding, so return the function value and continue
@@ -1355,7 +1382,8 @@ export class Reconciler {
     if (value instanceof ObjectValue && isReactElement(value)) {
       return this._resolveReactElement(componentType, value, context, branchStatus, branchState, evaluatedNode);
     } else {
-      throw new ExpectedBailOut("unsupported value type during reconcilation");
+      let location = getLocationFromValue(value.expressionLocation);
+      throw new ExpectedBailOut(`invalid return value from render${location}`);
     }
   }
 
@@ -1480,4 +1508,30 @@ export class Reconciler {
       }
     }
   }
+
+  _handleReportedSideEffect(
+    sideEffectType: SideEffectType,
+    binding: void | Binding | PropertyBinding,
+    expressionLocation: any
+  ): void {
+    let location = getLocationFromValue(expressionLocation);
+
+    if (sideEffectType === "MODIFIED_BINDING") {
+      let name = binding ? `"${((binding: any): Binding).name}"` : "unknown";
+      throw new UnsupportedSideEffect(`side-effects from mutating the binding ${name}${location}`);
+    } else if (sideEffectType === "MODIFIED_PROPERTY" || sideEffectType === "MODIFIED_GLOBAL") {
+      let name = "";
+      let key = ((binding: any): PropertyBinding).key;
+      if (typeof key === "string") {
+        name = `"${key}"`;
+      }
+      if (sideEffectType === "MODIFIED_PROPERTY") {
+        throw new UnsupportedSideEffect(`side-effects from mutating a property ${name}${location}`);
+      } else {
+        throw new UnsupportedSideEffect(`side-effects from mutating the global object property ${name}${location}`);
+      }
+    } else if (sideEffectType === "EXCEPTION_THROWN") {
+      throw new UnsupportedSideEffect(`side-effects from throwing exception${location}`);
+    }
+  }
 }

src/react/utils.js

@@ -10,10 +10,11 @@
 /* @flow */
 
 import { Realm, Effects } from "../realm.js";
-import { PossiblyNormalCompletion, AbruptCompletion } from "../completions.js";
+import { AbruptCompletion, PossiblyNormalCompletion } from "../completions.js";
 import type { BabelNode, BabelNodeJSXIdentifier } from "babel-types";
 import {
   AbstractObjectValue,
+  AbstractValue,
   Value,
   NumberValue,
   ObjectValue,
@@ -27,18 +28,16 @@ import {
   BooleanValue,
 } from "../values/index.js";
 import { Generator } from "../utils/generator.js";
-import type { Descriptor, ReactHint, PropertyBinding, ReactComponentTreeConfig } from "../types";
+import type { Descriptor, ReactHint, PropertyBinding, ReactComponentTreeConfig } from "../types.js";
 import { Get, cloneDescriptor } from "../methods/index.js";
 import { computeBinary } from "../evaluators/BinaryExpression.js";
 import type { ReactSerializerState, AdditionalFunctionEffects, ReactEvaluatedNode } from "../serializer/types.js";
 import invariant from "../invariant.js";
-import { Create, Properties } from "../singletons.js";
+import { Create, Properties, To } from "../singletons.js";
 import traverse from "babel-traverse";
 import * as t from "babel-types";
 import type { BabelNodeStatement } from "babel-types";
 import { CompilerDiagnostic, FatalError } from "../errors.js";
-import { To } from "../singletons.js";
-import AbstractValue from "../values/AbstractValue";
 
 export type ReactSymbolTypes =
   | "react.element"
@@ -700,12 +699,10 @@ export function createReactEvaluatedNode(
     | "NEW_TREE"
     | "INLINED"
     | "BAIL-OUT"
-    | "WRITE-CONFLICTS"
+    | "FATAL"
     | "UNKNOWN_TYPE"
     | "RENDER_PROPS"
     | "FORWARD_REF"
-    | "UNSUPPORTED_COMPLETION"
-    | "ABRUPT_COMPLETION"
     | "NORMAL",
   name: string
 ): ReactEvaluatedNode {
@@ -853,3 +850,12 @@ export function sanitizeReactElementForFirstRenderOnly(realm: Realm, reactElemen
   }
   return reactElement;
 }
+
+export function getLocationFromValue(expressionLocation: any) {
+  // if we can't get a value, then it's likely that the source file was not given
+  // (this happens in React tests) so instead don't print any location
+  return expressionLocation
+    ? ` at location: ${expressionLocation.start.line}:${expressionLocation.start.column} ` +
+        `- ${expressionLocation.end.line}:${expressionLocation.end.line}`
+    : "";
+}

src/realm.js

@@ -68,6 +68,8 @@ export type PropertyBindings = Map<PropertyBinding, void | Descriptor>;
 
 export type CreatedObjects = Set<ObjectValue>;
 
+export type SideEffectType = "MODIFIED_BINDING" | "MODIFIED_PROPERTY" | "EXCEPTION_THROWN" | "MODIFIED_GLOBAL";
+
 let effects_uid = 0;
 
 export class Effects {
@@ -316,6 +318,9 @@ export class Realm {
   createdObjects: void | CreatedObjects;
   createdObjectsTrackedForLeaks: void | CreatedObjects;
   reportObjectGetOwnProperties: void | (ObjectValue => void);
+  reportSideEffectCallback:
+    | void
+    | ((sideEffectType: SideEffectType, binding: void | Binding | PropertyBinding, expressionLocation: any) => void);
   reportPropertyAccess: void | (PropertyBinding => void);
   savedCompletion: void | PossiblyNormalCompletion;
 
@@ -625,17 +630,27 @@ export class Realm {
   // that it created itself. This promises that any abstract functions inside of it
   // also won't have effects on any objects or bindings that weren't created in this
   // call.
-  evaluatePure<T>(f: () => T) {
+  evaluatePure<T>(
+    f: () => T,
+    reportSideEffectFunc?: (
+      sideEffectType: SideEffectType,
+      binding: void | Binding | PropertyBinding,
+      value: void | Value
+    ) => void
+  ) {
     let saved_createdObjectsTrackedForLeaks = this.createdObjectsTrackedForLeaks;
+    let saved_reportSideEffectCallback = this.reportSideEffectCallback;
     // Track all objects (including function closures) created during
     // this call. This will be used to make the assumption that every
     // *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;
     try {
       return f();
     } finally {
       this.createdObjectsTrackedForLeaks = saved_createdObjectsTrackedForLeaks;
+      this.reportSideEffectCallback = saved_reportSideEffectCallback;
     }
   }
 
@@ -1295,16 +1310,56 @@ export class Realm {
 
   // Record the current value of binding in this.modifiedBindings unless
   // there is already an entry for binding.
-  recordModifiedBinding(binding: Binding): Binding {
+  recordModifiedBinding(binding: Binding, value?: Value): Binding {
+    const isDefinedInsidePureFn = root => {
+      let context = this.getRunningContext();
+      let { lexicalEnvironment: env, function: func } = context;
+
+      invariant(func instanceof FunctionValue);
+      if (root instanceof FunctionEnvironmentRecord && func === root.$FunctionObject) {
+        return true;
+      }
+      if (this.createdObjectsTrackedForLeaks !== undefined && !this.createdObjectsTrackedForLeaks.has(func)) {
+        return false;
+      }
+      env = env.parent;
+      while (env) {
+        if (env.environmentRecord === root) {
+          return true;
+        }
+        env = env.parent;
+      }
+      return false;
+    };
+
+    if (
+      this.modifiedBindings !== undefined &&
+      !this.modifiedBindings.has(binding) &&
+      value !== undefined &&
+      this.isInPureScope() &&
+      this.reportSideEffectCallback !== undefined
+    ) {
+      let env = binding.environment;
+
+      if (
+        !(env instanceof DeclarativeEnvironmentRecord) ||
+        (env instanceof DeclarativeEnvironmentRecord && !isDefinedInsidePureFn(env))
+      ) {
+        this.reportSideEffectCallback("MODIFIED_BINDING", binding, value.expressionLocation);
+      }
+    }
+
     if (binding.environment.isReadOnly) {
       // This only happens during speculative execution and is reported elsewhere
       throw new FatalError("Trying to modify a binding in read-only realm");
     }
-    if (this.modifiedBindings !== undefined && !this.modifiedBindings.has(binding))
+
+    if (this.modifiedBindings !== undefined && !this.modifiedBindings.has(binding)) {
       this.modifiedBindings.set(binding, {
         hasLeaked: binding.hasLeaked,
         value: binding.value,
       });
+    }
     return binding;
   }
 
@@ -1324,6 +1379,21 @@ export class Realm {
   // there is already an entry for binding.
   recordModifiedProperty(binding: void | PropertyBinding): void {
     if (binding === undefined) return;
+    if (this.isInPureScope()) {
+      let object = binding.object;
+      invariant(object instanceof ObjectValue);
+      const createdObjectsTrackedForLeaks = this.createdObjectsTrackedForLeaks;
+
+      if (createdObjectsTrackedForLeaks !== undefined && !createdObjectsTrackedForLeaks.has(object)) {
+        if (binding.object === this.$GlobalObject) {
+          this.reportSideEffectCallback &&
+            this.reportSideEffectCallback("MODIFIED_GLOBAL", binding, object.expressionLocation);
+        } else {
+          this.reportSideEffectCallback &&
+            this.reportSideEffectCallback("MODIFIED_PROPERTY", binding, object.expressionLocation);
+        }
+      }
+    }
     if (this.isReadOnly && (this.getRunningContext().isReadOnly || !this.isNewObject(binding.object))) {
       // This only happens during speculative execution and is reported elsewhere
       throw new FatalError("Trying to modify a property in read-only realm");

src/serializer/functions.js

@@ -45,6 +45,7 @@ import {
   getComponentName,
   convertConfigObjectToReactComponentTreeConfig,
 } from "../react/utils.js";
+import { ReconcilerFatalError } from "../react/errors.js";
 import * as t from "babel-types";
 
 type AdditionalFunctionEntry = {
@@ -165,10 +166,10 @@ export class Functions {
       environmentRecordIdAfterGlobalCode
     );
     if (additionalFunctionEffects === null) {
-      // TODO we don't support this yet, but will do very soon
-      // to unblock work, we'll just return at this point right now
-      evaluatedNode.status = "UNSUPPORTED_COMPLETION";
-      return;
+      throw new ReconcilerFatalError(
+        `Failed to optimize React component tree for "${evaluatedNode.name}" due to an unsupported completion`,
+        evaluatedNode
+      );
     }
     effects = additionalFunctionEffects.effects;
     let value = effects.result;
@@ -234,47 +235,6 @@ export class Functions {
     return noOpFunc;
   }
 
-  _forEachBindingOfEffects(effects: Effects, func: (binding: Binding) => void): void {
-    let [result, , nestedBindingsToIgnore] = effects.data;
-    for (let [binding] of nestedBindingsToIgnore) {
-      func(binding);
-    }
-    if (result instanceof PossiblyNormalCompletion || result instanceof JoinedAbruptCompletions) {
-      this._forEachBindingOfEffects(result.alternateEffects, func);
-      this._forEachBindingOfEffects(result.consequentEffects, func);
-    }
-  }
-
-  _hasWriteConflictsFromReactRenders(
-    bindings: Set<Binding>,
-    effects: Effects,
-    nestedEffectsList: Array<Effects>,
-    evaluatedNode: ReactEvaluatedNode
-  ): boolean {
-    let ignoreBindings = new Set();
-    let failed = false;
-    // TODO: should we also check realm.savedEffects?
-    // ref: https://github.com/facebook/prepack/pull/1742
-
-    for (let nestedEffects of nestedEffectsList) {
-      this._forEachBindingOfEffects(nestedEffects, binding => {
-        ignoreBindings.add(binding);
-      });
-    }
-
-    this._forEachBindingOfEffects(effects, binding => {
-      if (bindings.has(binding) && !ignoreBindings.has(binding)) {
-        failed = true;
-      }
-      bindings.add(binding);
-    });
-
-    if (failed) {
-      evaluatedNode.status = "WRITE-CONFLICTS";
-    }
-    return failed;
-  }
-
   optimizeReactComponentTreeRoots(
     statistics: ReactStatistics,
     react: ReactSerializerState,
@@ -303,18 +263,6 @@ export class Functions {
         logger.logInformation(`  Evaluating ${evaluatedRootNode.name} (root)`);
       }
       let componentTreeEffects = reconciler.renderReactComponentTree(componentType, null, null, evaluatedRootNode);
-      if (componentTreeEffects === null) {
-        if (this.realm.react.verbose) {
-          logger.logInformation(`  ✖ ${evaluatedRootNode.name} (root)`);
-        }
-        continue;
-      }
-      if (this._hasWriteConflictsFromReactRenders(bindings, componentTreeEffects, [], evaluatedRootNode)) {
-        if (this.realm.react.verbose) {
-          logger.logInformation(`  ✖ ${evaluatedRootNode.name} (root - write conflicts)`);
-        }
-        continue;
-      }
       if (this.realm.react.verbose) {
         logger.logInformation(`  ✔ ${evaluatedRootNode.name} (root)`);
       }
@@ -371,18 +319,6 @@ export class Functions {
         branchState,
         evaluatedNode
       );
-      if (closureEffects === null) {
-        if (this.realm.react.verbose) {
-          logger.logInformation(`    ✖ function "${getComponentName(this.realm, func)}"`);
-        }
-        continue;
-      }
-      if (this._hasWriteConflictsFromReactRenders(bindings, closureEffects, nestedEffects, evaluatedNode)) {
-        if (this.realm.react.verbose) {
-          logger.logInformation(`    ✖ function "${getComponentName(this.realm, func)}" (write conflicts)`);
-        }
-        continue;
-      }
       if (this.realm.react.verbose) {
         logger.logInformation(`    ✔ function "${getComponentName(this.realm, func)}"`);
       }
@@ -429,18 +365,6 @@ export class Functions {
         logger.logInformation(`    Evaluating ${evaluatedNode.name} (branch)`);
       }
       let branchEffects = reconciler.renderReactComponentTree(branchComponentType, null, null, evaluatedNode);
-      if (branchEffects === null) {
-        if (this.realm.react.verbose) {
-          logger.logInformation(`    ✖ ${evaluatedNode.name} (branch)`);
-        }
-        continue;
-      }
-      if (this._hasWriteConflictsFromReactRenders(bindings, branchEffects, [], evaluatedNode)) {
-        if (this.realm.react.verbose) {
-          logger.logInformation(`    ✖ ${evaluatedNode.name} (branch - write conflicts)`);
-        }
-        continue;
-      }
       if (this.realm.react.verbose) {
         logger.logInformation(`    ✔ ${evaluatedNode.name} (branch)`);
       }

src/serializer/types.js

@@ -161,12 +161,10 @@ export type ReactEvaluatedNode = {
     | "NEW_TREE"
     | "INLINED"
     | "BAIL-OUT"
-    | "WRITE-CONFLICTS"
+    | "FATAL"
     | "UNKNOWN_TYPE"
     | "RENDER_PROPS"
     | "FORWARD_REF"
-    | "UNSUPPORTED_COMPLETION"
-    | "ABRUPT_COMPLETION"
     | "NORMAL",
 };
 

test/react/first-render-only/class-root-with-refs.js

@@ -1,55 +0,0 @@
-var React = require('react');
-// the JSX transform converts to React, so we need to add it back in
-this['React'] = React;
-
-function SubChild() {
-  return <span>Hello world</span>;
-}
-
-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 ) {
-    App.__proto__ = superclass;
-  }
-  App.prototype = Object.create( superclass && superclass.prototype );
-  App.prototype.constructor = App;
-  App.prototype._renderChild = function () {
-    return <Child />;
-  };
-  App.prototype.divRefFunc = function divRefFunc (ref) {
-    this.divRefWorked = true;
-  };
-  App.prototype.render = function render () {
-    return <div ref={this.divRefFunc}>{this._renderChild()}</div>;
-  };
-  App.getTrials = function(renderer, Root) {
-    renderer.update(<Root />);
-    let results = [];
-    results.push(['render with class root', renderer.toJSON()]);
-    results.push(['get the ref', instance.divRefWorked]);
-    return results;
-  };
-
-  return App;
-}(React.Component));
-
-if (this.__optimizeReactComponentTree) {
-  __optimizeReactComponentTree(App, {
-    firstRenderOnly: true,
-  });
-}
-
-module.exports = App;

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

@@ -0,0 +1,13 @@
+var x = 0;
+
+function foo() {
+  x++; // not safe
+}
+function Bar() {
+  foo();
+  return null;
+}
+
+if (this.__optimizeReactComponentTree) {
+  __optimizeReactComponentTree(Bar);
+}

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

@@ -0,0 +1,16 @@
+var x = 0;
+
+function foo() {
+  function foo2() {
+    x++; // not safe
+  }
+  foo2();
+}
+function Bar() {
+  foo();
+  return null;
+}
+
+if (this.__optimizeReactComponentTree) {
+  __optimizeReactComponentTree(Bar);
+}

test/react/functional-components/safe.js

@@ -0,0 +1,15 @@
+function foo() {
+  var x = 0;
+  function foo2() {
+    x++; // safe
+  }
+  foo2();
+}
+function Bar() {
+  foo();
+  return null;
+}
+
+if (this.__optimizeReactComponentTree) {
+  __optimizeReactComponentTree(Bar);
+}

test/react/functional-components/safe2.js

@@ -0,0 +1,17 @@
+function foo() {
+  var x = 0;
+  function foo2() {
+    function foo3() {
+      x++; // safe
+    }
+    foo3();
+  }
+  foo2();
+}
+function Bar() {
+  foo();
+}
+
+if (this.__optimizeReactComponentTree) {
+  __optimizeReactComponentTree(Bar);
+}

test/react/functional-components/simple-13.js

@@ -0,0 +1,41 @@
+var React = require("react");
+this["React"] = React;
+
+
+function URI(other) {
+  if (!other) {
+    return;
+  }
+  if (other.foo) {
+    if (other.baz) {
+      throw new Error();
+    }
+    this.bar = other;
+    throw new Error();
+  }
+  throw new Error();
+}
+
+function App(props) {
+  var first = new URI(props.initial);
+  return React.createElement(Child, null, function() {
+    new URI(first.bar);
+    return null;
+  });
+}
+
+function Child(props) {
+  var children = props.children;
+  return children();
+}
+
+__optimizeReactComponentTree(App, {
+  firstRenderOnly: true
+});
+
+App.getTrials = function(renderer, Root) {
+  // Just compile, don't run
+  return [];
+};
+
+module.exports = App;