Adds support for abstract length arrays in React reconcilation and se…

…rialization (#2571)

Summary:
Release notes: none

This PR fixes issues with the React hoisting and equivalence system mechanics and serialization where previous, there was no support for abstract length arrays. This includes an optimization for when the React serializer outputs ReactElement children that are conditionals with one side being an empty value (in this case, we can use an empty string instead).

Tests attached that focus on the areas covered in this PR.
Pull Request resolved: #2571

Differential Revision: D10082133

Pulled By: trueadm

fbshipit-source-id: d7de1834e10a5c4b3f35a90b9676ec72c6e797e2

src/react/ReactEquivalenceSet.js

@@ -216,21 +216,27 @@ export class ReactEquivalenceSet {
     return ((map.get(result): any): ReactSetNode);
   }
 
-  // for arrays: [0] -> [1] -> [2]... as nodes
+  // for arrays: [length] -> ([length] is numeric) -> [0] -> [1] -> [2]... as nodes
   _getArrayValue(array: ArrayValue, visitedValues: Set<Value>): ArrayValue {
     if (visitedValues.has(array)) return array;
     if (array.intrinsicName) return array;
     visitedValues.add(array);
-    let lengthValue = getProperty(this.realm, array, "length");
-    invariant(lengthValue instanceof NumberValue);
-    let length = lengthValue.value;
     let currentMap = this.arrayRoot;
-    let result;
+    currentMap = this.getKey("length", currentMap, visitedValues);
+    let result = this.getEquivalentPropertyValue(array, "length", currentMap, visitedValues);
+    currentMap = result.map;
 
-    for (let i = 0; i < length; i++) {
-      currentMap = this.getKey(i, currentMap, visitedValues);
-      result = this.getEquivalentPropertyValue(array, "" + i, currentMap, visitedValues);
-      currentMap = result.map;
+    let lengthValue = getProperty(this.realm, array, "length");
+    // If we have a numeric lenth that is not abstract, then also check all the array elements
+    if (lengthValue instanceof NumberValue) {
+      invariant(lengthValue instanceof NumberValue);
+      let length = lengthValue.value;
+
+      for (let i = 0; i < length; i++) {
+        currentMap = this.getKey(i, currentMap, visitedValues);
+        result = this.getEquivalentPropertyValue(array, "" + i, currentMap, visitedValues);
+        currentMap = result.map;
+      }
     }
     if (result === undefined) {
       if (this.realm.react.emptyArray !== undefined) {
@@ -241,6 +247,7 @@ export class ReactEquivalenceSet {
     if (result.value === null) {
       result.value = array;
     }
+    invariant(result.value instanceof ArrayValue);
     return result.value;
   }
 

src/react/elements.js

@@ -28,6 +28,7 @@ import {
   createInternalReactElement,
   flagPropsWithNoPartialKeyOrRef,
   flattenChildren,
+  getMaxLength,
   hardModifyReactObjectPropertyBinding,
   getProperty,
   hasNoPartialKeyOrRef,
@@ -418,7 +419,7 @@ export function wrapReactElementWithKeyedFragment(realm: Realm, keyValue: Value,
   Create.CreateDataPropertyOrThrow(realm, fragmentConfigValue, "key", keyValue);
   let fragmentChildrenValue = Create.ArrayCreate(realm, 1);
   Create.CreateDataPropertyOrThrow(realm, fragmentChildrenValue, "0", reactElement);
-  fragmentChildrenValue = flattenChildren(realm, fragmentChildrenValue);
+  fragmentChildrenValue = flattenChildren(realm, fragmentChildrenValue, true);
   return createReactElement(realm, reactFragment, fragmentConfigValue, fragmentChildrenValue);
 }
 
@@ -449,6 +450,13 @@ export function traverseReactElement(
     traversalVisitor.visitRef(refValue);
   }
 
+  const loopArrayElements = (childrenValue: ArrayValue, length: number): void => {
+    for (let i = 0; i < length; i++) {
+      let child = getProperty(realm, childrenValue, "" + i);
+      traversalVisitor.visitChildNode(child);
+    }
+  };
+
   const handleChildren = () => {
     // handle children
     invariant(propsValue instanceof ObjectValue);
@@ -457,13 +465,12 @@ export function traverseReactElement(
       if (childrenValue !== realm.intrinsics.undefined && childrenValue !== realm.intrinsics.null) {
         if (childrenValue instanceof ArrayValue && !childrenValue.intrinsicName) {
           let childrenLength = getProperty(realm, childrenValue, "length");
-          let childrenLengthValue = 0;
           if (childrenLength instanceof NumberValue) {
-            childrenLengthValue = childrenLength.value;
-            for (let i = 0; i < childrenLengthValue; i++) {
-              let child = getProperty(realm, childrenValue, "" + i);
-              traversalVisitor.visitChildNode(child);
-            }
+            loopArrayElements(childrenValue, childrenLength.value);
+          } else if (childrenLength instanceof AbstractValue && childrenLength.kind === "conditional") {
+            loopArrayElements(childrenValue, getMaxLength(childrenLength, 0));
+          } else {
+            invariant(false, "TODO: support other types of array length value");
           }
         } else {
           traversalVisitor.visitChildNode(childrenValue);

src/react/hoisting.js

@@ -61,13 +61,17 @@ function canHoistArray(
 ): boolean {
   if (array.intrinsicName) return false;
   let lengthValue = Get(realm, array, "length");
-  invariant(lengthValue instanceof NumberValue);
-  let length = lengthValue.value;
-  for (let i = 0; i < length; i++) {
-    let element = Get(realm, array, "" + i);
+  if (!canHoistValue(realm, lengthValue, residualHeapVisitor, visitedValues)) {
+    return false;
+  }
+  if (lengthValue instanceof NumberValue) {
+    let length = lengthValue.value;
+    for (let i = 0; i < length; i++) {
+      let element = Get(realm, array, "" + i);
 
-    if (!canHoistValue(realm, element, residualHeapVisitor, visitedValues)) {
-      return false;
+      if (!canHoistValue(realm, element, residualHeapVisitor, visitedValues)) {
+        return false;
+      }
     }
   }
   return true;

src/react/reconcilation.js

@@ -1033,7 +1033,7 @@ export class Reconciler {
         );
         // we can optimize further and flatten arrays on non-composite components
         if (resolvedChildren instanceof ArrayValue && !resolvedChildren.intrinsicName) {
-          resolvedChildren = flattenChildren(this.realm, resolvedChildren);
+          resolvedChildren = flattenChildren(this.realm, resolvedChildren, true);
         }
         if (resolvedChildren !== childrenValue) {
           let newProps = cloneProps(this.realm, propsValue, resolvedChildren);

src/react/utils.js

@@ -21,6 +21,7 @@ import {
   BoundFunctionValue,
   ECMAScriptFunctionValue,
   ECMAScriptSourceFunctionValue,
+  EmptyValue,
   FunctionValue,
   NumberValue,
   ObjectValue,
@@ -239,17 +240,31 @@ export function forEachArrayValue(
   mapFunc: (element: Value, index: number) => void
 ): void {
   let lengthValue = Get(realm, array, "length");
-  invariant(lengthValue instanceof NumberValue, "TODO: support non-numeric length on forEachArrayValue");
-  let length = lengthValue.value;
+  let isConditionalLength = lengthValue instanceof AbstractValue && lengthValue.kind === "conditional";
+  let length;
+  if (isConditionalLength) {
+    length = getMaxLength(lengthValue, 0);
+  } else {
+    invariant(lengthValue instanceof NumberValue, "TODO: support other types of array length value");
+    length = lengthValue.value;
+  }
   for (let i = 0; i < length; i++) {
     let elementProperty = array.properties.get("" + i);
     let elementPropertyDescriptor = elementProperty && elementProperty.descriptor;
     if (elementPropertyDescriptor) {
       invariant(elementPropertyDescriptor instanceof PropertyDescriptor);
       let elementValue = elementPropertyDescriptor.value;
-      if (elementValue instanceof Value) {
-        mapFunc(elementValue, i);
+      // If we are in an array with conditional length, the element might be a conditional join
+      // of the same type as the length of the array
+      if (isConditionalLength && elementValue instanceof AbstractValue && elementValue.kind === "conditional") {
+        invariant(lengthValue instanceof AbstractValue);
+        let lengthCondition = lengthValue.args[0];
+        let elementCondition = elementValue.args[0];
+        // If they are the same condition
+        invariant(lengthCondition.equals(elementCondition), "TODO: support cases where the condition is not the same");
       }
+      invariant(elementValue instanceof Value);
+      mapFunc(elementValue, i);
     }
   }
 }
@@ -259,28 +274,66 @@ export function mapArrayValue(
   array: ArrayValue,
   mapFunc: (element: Value, descriptor: Descriptor) => Value
 ): ArrayValue {
-  let lengthValue = Get(realm, array, "length");
-  invariant(lengthValue instanceof NumberValue, "TODO: support non-numeric length on mapArrayValue");
-  let length = lengthValue.value;
-  let newArray = Create.ArrayCreate(realm, length);
   let returnTheNewArray = false;
-
-  for (let i = 0; i < length; i++) {
-    let elementProperty = array.properties.get("" + i);
-    let elementPropertyDescriptor = elementProperty && elementProperty.descriptor;
-    if (elementPropertyDescriptor) {
-      invariant(elementPropertyDescriptor instanceof PropertyDescriptor);
-      let elementValue = elementPropertyDescriptor.value;
-      if (elementValue instanceof Value) {
-        let newElement = mapFunc(elementValue, elementPropertyDescriptor);
-        if (newElement !== elementValue) {
-          returnTheNewArray = true;
+  let newArray;
+
+  const mapArray = (lengthValue: NumberValue): void => {
+    let length = lengthValue.value;
+
+    for (let i = 0; i < length; i++) {
+      let elementProperty = array.properties.get("" + i);
+      let elementPropertyDescriptor = elementProperty && elementProperty.descriptor;
+      if (elementPropertyDescriptor) {
+        invariant(elementPropertyDescriptor instanceof PropertyDescriptor);
+        let elementValue = elementPropertyDescriptor.value;
+        if (elementValue instanceof Value) {
+          let newElement = mapFunc(elementValue, elementPropertyDescriptor);
+          if (newElement !== elementValue) {
+            returnTheNewArray = true;
+          }
+          Create.CreateDataPropertyOrThrow(realm, newArray, "" + i, newElement);
+          continue;
         }
-        Create.CreateDataPropertyOrThrow(realm, newArray, "" + i, newElement);
-        continue;
       }
+      Create.CreateDataPropertyOrThrow(realm, newArray, "" + i, realm.intrinsics.undefined);
     }
-    Create.CreateDataPropertyOrThrow(realm, newArray, "" + i, realm.intrinsics.undefined);
+  };
+
+  let lengthValue = Get(realm, array, "length");
+  if (lengthValue instanceof AbstractValue && lengthValue.kind === "conditional") {
+    returnTheNewArray = true;
+    let [condValue, consequentVal, alternateVal] = lengthValue.args;
+    newArray = Create.ArrayCreate(realm, 0);
+    realm.evaluateWithAbstractConditional(
+      condValue,
+      () => {
+        return realm.evaluateForEffects(
+          () => {
+            invariant(consequentVal instanceof NumberValue);
+            mapArray(consequentVal);
+            return realm.intrinsics.undefined;
+          },
+          null,
+          "mapArrayValue consequent"
+        );
+      },
+      () => {
+        return realm.evaluateForEffects(
+          () => {
+            invariant(alternateVal instanceof NumberValue);
+            mapArray(alternateVal);
+            return realm.intrinsics.undefined;
+          },
+          null,
+          "mapArrayValue alternate"
+        );
+      }
+    );
+  } else if (lengthValue instanceof NumberValue) {
+    newArray = Create.ArrayCreate(realm, lengthValue.value);
+    mapArray(lengthValue);
+  } else {
+    invariant(false, "TODO: support other types of array length value");
   }
   return returnTheNewArray ? newArray : array;
 }
@@ -602,21 +655,68 @@ export function hasNoPartialKeyOrRef(realm: Realm, props: ObjectValue | Abstract
   return false;
 }
 
-function recursivelyFlattenArray(realm: Realm, array, targetArray): void {
-  forEachArrayValue(realm, array, item => {
-    if (item instanceof ArrayValue && !item.intrinsicName) {
-      recursivelyFlattenArray(realm, item, targetArray);
+export function getMaxLength(value: Value, maxLength: number): number {
+  if (value instanceof NumberValue) {
+    if (value.value > maxLength) {
+      return value.value;
+    } else {
+      return maxLength;
+    }
+  } else if (value instanceof AbstractValue && value.kind === "conditional") {
+    let [, consequentVal, alternateVal] = value.args;
+    let consequentMaxVal = getMaxLength(consequentVal, maxLength);
+    let alternateMaxVal = getMaxLength(alternateVal, maxLength);
+    if (consequentMaxVal > maxLength && consequentMaxVal >= alternateMaxVal) {
+      return consequentMaxVal;
+    } else if (alternateMaxVal > maxLength && alternateMaxVal >= consequentMaxVal) {
+      return alternateMaxVal;
+    }
+    return maxLength;
+  }
+  invariant(false, "TODO: support other types of array length value");
+}
+
+function recursivelyFlattenArray(realm: Realm, array, targetArray: ArrayValue, noHoles: boolean): void {
+  forEachArrayValue(realm, array, _item => {
+    let element = _item;
+    if (element instanceof ArrayValue && !element.intrinsicName) {
+      recursivelyFlattenArray(realm, element, targetArray, noHoles);
     } else {
       let lengthValue = Get(realm, targetArray, "length");
       invariant(lengthValue instanceof NumberValue);
-      Properties.Set(realm, targetArray, "" + lengthValue.value, item, true);
+      if (noHoles && element instanceof EmptyValue) {
+        // We skip holely elements
+        return;
+      } else if (noHoles && element instanceof AbstractValue && element.kind === "conditional") {
+        let [condValue, consequentVal, alternateVal] = element.args;
+        invariant(condValue instanceof AbstractValue);
+        let consquentIsHolely = consequentVal instanceof EmptyValue;
+        let alternateIsHolely = alternateVal instanceof EmptyValue;
+
+        if (consquentIsHolely && alternateIsHolely) {
+          // We skip holely elements
+          return;
+        }
+        if (consquentIsHolely) {
+          element = AbstractValue.createFromLogicalOp(
+            realm,
+            "&&",
+            AbstractValue.createFromUnaryOp(realm, "!", condValue),
+            alternateVal
+          );
+        }
+        if (alternateIsHolely) {
+          element = AbstractValue.createFromLogicalOp(realm, "&&", condValue, consequentVal);
+        }
+      }
+      Properties.Set(realm, targetArray, "" + lengthValue.value, element, true);
     }
   });
 }
 
-export function flattenChildren(realm: Realm, array: ArrayValue): ArrayValue {
+export function flattenChildren(realm: Realm, array: ArrayValue, noHoles: boolean): ArrayValue {
   let flattenedChildren = Create.ArrayCreate(realm, 0);
-  recursivelyFlattenArray(realm, array, flattenedChildren);
+  recursivelyFlattenArray(realm, array, flattenedChildren, noHoles);
   flattenedChildren.makeFinal();
   return flattenedChildren;
 }

test/react/FunctionalComponents-test.js

@@ -132,6 +132,22 @@ it("Simple 25", () => {
   runTest(__dirname + "/FunctionalComponents/simple-25.js");
 });
 
+it("Simple 26", () => {
+  runTest(__dirname + "/FunctionalComponents/simple-26.js");
+});
+
+it("Simple 27", () => {
+  runTest(__dirname + "/FunctionalComponents/simple-27.js");
+});
+
+it("Simple 28", () => {
+  runTest(__dirname + "/FunctionalComponents/simple-28.js");
+});
+
+it("Simple 29", () => {
+  runTest(__dirname + "/FunctionalComponents/simple-29.js");
+});
+
 it("Bound type", () => {
   runTest(__dirname + "/FunctionalComponents/bound-type.js");
 });

test/react/FunctionalComponents/simple-26.js

@@ -0,0 +1,32 @@
+var React = require("react");
+
+function App(props) {
+  var arr = [1, 2, 3];
+
+  if (props.cond) {
+    arr.push(4);
+  } else {
+    arr.pop();
+  }
+  return (
+    <div>
+      <span x={arr} />
+      <span x={arr} />
+    </div>
+  );
+}
+
+App.getTrials = function(renderer, Root) {
+  let results = [];
+  renderer.update(<Root cond={true} />);
+  results.push(["abstract array length on prop (cond: true)", renderer.toJSON()]);
+  renderer.update(<Root cond={false} />);
+  results.push(["abstract array length on prop (cond: false)", renderer.toJSON()]);
+  return results;
+};
+
+if (this.__optimizeReactComponentTree) {
+  __optimizeReactComponentTree(App);
+}
+
+module.exports = App;