Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Adds support for abstract length arrays in React reconcilation and serialization #2571

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/invariant.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { InvariantError } from "./errors.js";

export default function invariant(condition: boolean, format: string = ""): void {
if (condition) return;
debugger;
const message = `${format}
This is likely a bug in Prepack, not your code. Feel free to open an issue on GitHub.`;
let error = new InvariantError(message);
Expand Down
19 changes: 13 additions & 6 deletions src/react/ReactEquivalenceSet.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,15 +222,21 @@ export class ReactEquivalenceSet {
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;

for (let i = 0; i < length; i++) {
currentMap = this.getKey(i, currentMap, visitedValues);
result = this.getEquivalentPropertyValue(array, "" + i, currentMap, visitedValues);
currentMap = result.map;
if (lengthValue instanceof AbstractValue) {
currentMap = this.getKey("length", currentMap, visitedValues);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing to what happens below for the concrete case, I don't understand how just using "length" once as the key here is supposed to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should also add the length for the concrete case too.

result = this.getEquivalentPropertyValue(array, "length", currentMap, visitedValues);
} else {
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) {
Expand All @@ -241,6 +247,7 @@ export class ReactEquivalenceSet {
if (result.value === null) {
result.value = array;
}
invariant(result.value instanceof ArrayValue);
return result.value;
}

Expand Down
19 changes: 13 additions & 6 deletions src/react/elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
createInternalReactElement,
flagPropsWithNoPartialKeyOrRef,
flattenChildren,
getMaxLength,
hardModifyReactObjectPropertyBinding,
getProperty,
hasNoPartialKeyOrRef,
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
16 changes: 10 additions & 6 deletions src/react/hoisting.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
154 changes: 118 additions & 36 deletions src/react/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -238,19 +238,33 @@ export function forEachArrayValue(
array: ArrayValue,
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;
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);
const forEachArray = (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) {
mapFunc(elementValue, i);
}
}
}
};

let lengthValue = Get(realm, array, "length");

if (lengthValue instanceof AbstractValue && lengthValue.kind === "conditional") {
let [condValue, consequentVal, alternateVal] = lengthValue.args;
invariant(consequentVal instanceof NumberValue, "TODO: support other types of array length value");
forEachArray(consequentVal);
invariant(alternateVal instanceof NumberValue, "TODO: support other types of array length value");
forEachArray(alternateVal);
} else if (lengthValue instanceof NumberValue) {
forEachArray(lengthValue);
} else {
invariant(false, "TODO: support other types of array length value");
}
}

Expand All @@ -259,28 +273,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;
}
Expand Down Expand Up @@ -602,21 +654,51 @@ export function hasNoPartialKeyOrRef(realm: Realm, props: ObjectValue | Abstract
return false;
}

function recursivelyFlattenArray(realm: Realm, array, targetArray): void {
forEachArrayValue(realm, array, item => {
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 [condValue, 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, startingIndex: number): number {
// forEachArrayValue can also deal with abstract lengths, which means that it doesn't iterate through
// arrays in a sequential order, the order might change. However, the callback tells us the current index
// that of the element in the array, so we can use that to know where to place elements in our flattened array.
// We do this now with math arithmetic and pass around integer from recursive recursivelyFlattenArray calls
// so that we can move around the "baseIndex" accordingly.
let length = getProperty(realm, array, "length");
let baseIndex = startingIndex;

forEachArrayValue(realm, array, (item: Value, i: number) => {
let index = baseIndex + i;
if (item instanceof ArrayValue && !item.intrinsicName) {
recursivelyFlattenArray(realm, item, targetArray);
baseIndex = recursivelyFlattenArray(realm, item, targetArray, index) - i;
} else {
let lengthValue = Get(realm, targetArray, "length");
invariant(lengthValue instanceof NumberValue);
Properties.Set(realm, targetArray, "" + lengthValue.value, item, true);
Properties.Set(realm, targetArray, "" + index, item, true);
}
});
let newBaseIndexToReturn = baseIndex + getMaxLength(length, 0);
return newBaseIndexToReturn;
}

export function flattenChildren(realm: Realm, array: ArrayValue): ArrayValue {
let flattenedChildren = Create.ArrayCreate(realm, 0);
recursivelyFlattenArray(realm, array, flattenedChildren);
recursivelyFlattenArray(realm, array, flattenedChildren, 0);
flattenedChildren.makeFinal();
return flattenedChildren;
}
Expand Down
6 changes: 6 additions & 0 deletions src/serializer/ResidualHeapSerializer.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ export class ResidualHeapSerializer {
let environment = realm.$GlobalEnv.environmentRecord;
invariant(environment instanceof GlobalEnvironmentRecord);
this.globalEnvironmentRecord = environment;
this.emptySerializesToEmptyString = false;
}

emitter: Emitter;
Expand Down Expand Up @@ -269,6 +270,7 @@ export class ResidualHeapSerializer {
referentializer: Referentializer;
additionalFunctionGenerators: Map<FunctionValue, Generator>;
_residualOptimizedFunctions: ResidualOptimizedFunctions;
emptySerializesToEmptyString: boolean;

// function values nested in additional functions can't delay initializations
// TODO: revisit this and fix additional functions to be capable of delaying initializations
Expand Down Expand Up @@ -2047,6 +2049,10 @@ export class ResidualHeapSerializer {
}

_serializeEmptyValue(): BabelNodeExpression {
if (this.emptySerializesToEmptyString) {
invariant(this.realm.react.enabled, "emptySerializesToEmptyString is a React optimization");
return t.stringLiteral("");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than output the empty variable, instead we generate an empty string as the React serialization expects this for cases where the value is a string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The special "empty value" is used as a marker value as part of comparisons. It should never leak through as the actual value of anything that's exposed. For that reason, I don't understand how this can fix anything?

Also, the identity if that "empty value" matters. Replacing it in some contexts with something else (that isn't even unique), seems highly dubious to me. Why is this correct?

Copy link
Contributor Author

@trueadm trueadm Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove the string literal optimization, you will see __empty leaked out to the output. I was under the assumption that this could happen? If not, I can easily make a repro as a separate issue and remove the above optimization.

#2573

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed this form the PR, but now the tests fail as __empty is no longer an empty string but an empty object which gets serialized out and is invalid as a return value for a React render.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a different approach that removes holes from the arrays in the reconciler so we shouldn't hit the same issue but the still overall problem with __empty exists, see the issue above.

}
this.needsEmptyVar = !this.realm.instantRender.enabled;
return emptyExpression;
}
Expand Down
2 changes: 2 additions & 0 deletions src/serializer/ResidualReactElementSerializer.js
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,9 @@ export class ResidualReactElementSerializer {
_serializeReactElementChild(child: Value, reactElement: ReactElement): ReactElementChild {
let reactElementChild = this._createReactElementChild();
this._serializeNowOrAfterWaitingForDependencies(child, reactElement, () => {
this.residualHeapSerializer.emptySerializesToEmptyString = true;
let expr = this.residualHeapSerializer.serializeValue(child);
this.residualHeapSerializer.emptySerializesToEmptyString = false;

reactElementChild.expr = expr;
reactElementChild.type = "NORMAL";
Expand Down
8 changes: 8 additions & 0 deletions test/react/FunctionalComponents-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,14 @@ 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("Bound type", () => {
runTest(__dirname + "/FunctionalComponents/bound-type.js");
});
Expand Down
32 changes: 32 additions & 0 deletions test/react/FunctionalComponents/simple-26.js
Original file line number Diff line number Diff line change
@@ -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;
Loading