Summary:
Release notes: the side-effect detection system within pure scopes has been improved
This PR revamps the entire `evaluatePure` system. The old system had so many small issues with it was the cause of many edge-cases that were hard to patch up. Specifically, the PR makes the following changes:
- `realm.evaluatePure` -> `realm.evaluateWithPureScope` and now can only be used a single instance. Nesting further `realm.evaluateWithPureScope` calls will trigger an invariant and is strictly forbidden. Furthermore, it now only takes a single argument – the function you're wrapped around.
- All places that used pure scope wrappers have been updated to conditionally enable it depending if the `realm.isInPureScope()` returns `true` or `false`.
- `realm.evaluateFunctionForPureEffects` has been added, this works like `evaluateForEffects` except it requires a root function and a callback for side-effects. The callback for side-effects works like the old callback that was `evaluatePure`.
- `realm.evaluateFunctionForPureEffectsInGlobalEnv` has been added a convenience wrapper around `realm.evaluateFunctionForPureEffects`.
- When we leak bindings, we no longer set their value to `undefined` or `realm.intrinsics.undefined`. We now set the value to a special "leaked abstract", specifically – `realm.intrinsics.__leakedValue` – like `topValue` and `bottomValue`.
Unsurprisingly, this now fixes a host of bugs that existed before. Including fixes for https://github.com/facebook/prepack/issues/2598, https://github.com/facebook/prepack/issues/2579, https://github.com/facebook/prepack/issues/2446, https://github.com/facebook/prepack/issues/2599 and probably many other issues too.
The logic for detection of side-effects works very differently from before but after speaking to sebmarkbage last week, he pointed me in this direction to track side-effects rather than how we did it before. We now find side-effects from a given set of effects, rather than in an ad-hoc manor as mutations occur on objects/bindings. This PR requires https://github.com/facebook/prepack/pull/2596 as a dependency as it re-uses the logic.
Closes https://github.com/facebook/prepack/pull/2587. Fixes https://github.com/facebook/prepack/issues/2598. Fixes https://github.com/facebook/prepack/issues/2579. Fixes https://github.com/facebook/prepack/issues/2446. Fixes https://github.com/facebook/prepack/issues/2599.
Pull Request resolved: https://github.com/facebook/prepack/pull/2600
Differential Revision: D10368159
Pulled By: trueadm
fbshipit-source-id: ded248f5cfd8648913cae9b9c697d723a82c59ab
Summary:
Release notes: none
This fixes a React reconciliation bug where lazy ReactElement temporals were not correctly being put into the correct branches. Previously, `wrapReactElementInBranchOrReturnValue` was only called on the "result" of an `applyBranchedLogicValue` where it should have been applied after each ReactElement creation given that `applyBranchedLogicValue` is recursive.
Pull Request resolved: https://github.com/facebook/prepack/pull/2597
Differential Revision: D10298262
Pulled By: trueadm
fbshipit-source-id: 4868ab7f2514734913a626e4e7e7e3259e4199b5
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: https://github.com/facebook/prepack/pull/2571
Differential Revision: D10082133
Pulled By: trueadm
fbshipit-source-id: d7de1834e10a5c4b3f35a90b9676ec72c6e797e2
Summary:
Release notes: none
All these changes are only internal changes related to React.
This PR adds the `pe-functional-components` benchmark as tests to the React reconciler. The test was taken from: https://github.com/facebook/react/tree/master/scripts/bench/benchmarks/pe-functional-components.
In order to make the server side renderer test pass, a few TODOs had to be filled in (logic was missing) and the JSON logic has to be updated to account for empty strings in children that the compiler merges.
Pull Request resolved: https://github.com/facebook/prepack/pull/2560
Differential Revision: D10008375
Pulled By: trueadm
fbshipit-source-id: 3b39a3e6387e23e17532a2343bd84ebebb7ee9cd
Summary:
Release note: none
Closes#2435Closes#1829
Join.composeWithEffects composes a forked completion with subsequent effects. When two or more forks could end normally, this could result in shallow copies of the subsequent effects. These were then joined together and applied, so it was mostly OK. The generator of the subsequent effects, however, ended up being joined with itself and thus transformed the generator tree to a DAG, which is not desirable for the serializer.
The new approach is to extract a join condition from the forked completion and using it to join the subsequent effects with a newly constructed empty effects. The condition ensures that the subsequent effects are applied only in situations where the forked completion is not abrupt.
Extracting this condition makes for complicated abstract expressions and this uncovered some existing bugs and limitations that are also addressed in this pull request. As a side effect, path conditions are now longer and the time to compile unrolled loops with conditional abrupt completions inside their bodies has gone up so much that the unroll limit had to be lowered.
Please note that the expected output React tests has changed because of re-ordering. I'm none too sure that this re-ordering is necessarily benign, so please review carefully.
Pull Request resolved: https://github.com/facebook/prepack/pull/2523
Differential Revision: D9623729
Pulled By: hermanventer
fbshipit-source-id: 737096bba54a7a2ad300dc29882ea1b7829ac745
Summary:
Release notes: none
Currently, when don't really fully deal with abstract conditions fully throughout the Prepack codebase and we usually simply leak/emit a temporal to get around them. So this PR aims at tackling many of the code-sites where they are prevalent by using the internal React bundle to find most of them and address them – with the goal of improving evaluation in cases where we run into conditionals.
With this PR, we now try and resolve different parts of the condition, including conditionals such as `||` and `&&`. This allows us to evaluate potentially far more than before when it comes to product code where we inhabit many deep conditionals. These changes also impact `Object.assign` as it triggered an invariant with `getSnapshot` as it never expected a conditional there. This should also fix https://github.com/facebook/prepack/issues/2323.
I've added some tests to show this but here is an example of the output before and after:
```js
// Input
function fn(x, b) {
var a = x ? b : { a: 2 };
return a.a;
}
// Before
var _2 = function (x, b) {
var _3 = {};
_3.a = 2;
var _$0 = (x ? b : _3).a;
return _$0;
};
// After
var _2 = function (x, b) {
if (x) {
var _$0 = b.a;
}
return x ? _$0 : 2;
};
```
For conditional `Object.assign`, the output looks like this (note: we need materializing rather than leaking to better improve the output):
```js
// Input
function fn(x) {
var a = x ? { a: 1 } : { a: 2 };
return Object.assign({}, a, { b: 1 });
}
// Before
var _2 = function (x) {
var _$0 = {
a: 1
};
var _$1 = {
a: 2
};
({}).a = 1;
({}).a = 2;
var _$2 = {
b: 1
};
var _9 = {};
var _$3 = _$9(_9, x ? _$0 : _$1, _$2);
return _9;
};
// After
var _2 = function (x) {
return {
a: x ? 1 : 2,
b: 1
};
};
```
I also took the time to apply the same small changes to the existing code in `CallExpression` so the logic there could also handle `&&` and `||` cases too. Including is a React test that shows that we can now inline a component that we previously weren't able to do.
Pull Request resolved: https://github.com/facebook/prepack/pull/2503
Differential Revision: D9616500
Pulled By: trueadm
fbshipit-source-id: 3888a62da330c64b0395723f8764c3590adc8491
Summary:
Release notes: none
The `React.Children.map` mock was not fully finished and had side-effects. This fixes that and adds a test to show it properly working.
Pull Request resolved: https://github.com/facebook/prepack/pull/2519
Differential Revision: D9614166
Pulled By: trueadm
fbshipit-source-id: b646d13cc46f3747b07a111dc5bc9de295e25212
Summary:
Starts adding basic support for throws in React. Concretely there are three things this PR does outside of adding tests:
1. Allowing throw side-effects.
2. Removing an invalid invariant. `createdObjects` changes after calling `realm.captureEffects()` and this is expected. Later code which joins/incorporates effects will merge in the captured `createdObjects`.
3. Don’t catch `AbruptCompletion`s and handle them as errors. Instead let them propagate up to the nearest `realm.evaluateForEffects()`. (Or similar function.)
I have not run this against the internal web bundle yet. Against the internal React Native bundle we get pretty far without removing throws with these changes.
Pull Request resolved: https://github.com/facebook/prepack/pull/2502
Reviewed By: trueadm
Differential Revision: D9566580
Pulled By: calebmer
fbshipit-source-id: 3716a6afd5fc3ae824182ee50e38e51d72126dc2
Summary:
Release notes: none
Fixes mixing functionality from React.cloneElement, where the original ReactElement props where missing from being copied to the new props.
Pull Request resolved: https://github.com/facebook/prepack/pull/2505
Differential Revision: D9565338
Pulled By: trueadm
fbshipit-source-id: f04c15b3a640f87de6de8326511e8e4bdfa328a7
Summary:
Release note: Speed up simplifier by using an implication cache per path branch
The realm's path conditions is now a class instances and an explicit tree, along with caches for expressions that have already been checked for true/false using Path.implies on the current set of path conditions.
The AbstractValueImplicationCounter is still there as flag, to be renamed later. It is no longer used as a global k-limit, but enables k-limits on the cost of constructing a new set of path conditions. When React is the target, path conditions are not re-specialized. This leads to a very nice performance win for the large React internal test.
A number of tweaks to the simplifier were needed to get tests to pass. Some of these were borrowed from PR #2460.
Pull Request resolved: https://github.com/facebook/prepack/pull/2494
Reviewed By: trueadm
Differential Revision: D9554820
Pulled By: hermanventer
fbshipit-source-id: 5fdc550499975fe11c0c954b9502cd4eeab2bafe
Summary:
Release notes: none
This unblocks an issue found in https://github.com/facebook/prepack/pull/2494.
After looking into https://github.com/facebook/prepack/pull/2494, which showed that without that PR, some conditions with `&&` weren't being passed correctly to the React reconciler to be properly inlined. With this PR applied however, this issue no longer occurs but we do run into another problem.
The React reconciler will attempt to evaluate, resolve and inline all paths. Typically, if a "value" that a React component returned wasn't that of a valid type (string, ReactElement, null and some other objects) we would throw an `ExpectedBailOut`. This turned out to break logic in common use-case though: `&&` conditions.
Take the below example though:
```js
function Child() {
return <div>This should be inlined</div>;
}
function App(props) {
var a = props.x ? null : function() {};
return a && <Child />;
}
```
In this case with, the React reconciler will see `function () {}` as one of the paths of the `&&` condition and try and inline all paths and hit the fact you can't return `function () {}` and throw the `ExpectedBailOut`.
The realism is that the React reconciler in Prepack is being too smart here. It doesn't need to be concerned with the fact that the valid might not be valid. If the result is not valid, then the user will hit a runtime issue with React with their original input too. Ultimately, all we need to do is return the valid.
There are three tests attached to this PR. Only `simple-24` is a direct test for the changes in this PR, `simple-22` and `simple-23` are regression tests for when https://github.com/facebook/prepack/pull/2494 is applied.
Pull Request resolved: https://github.com/facebook/prepack/pull/2498
Differential Revision: D9539852
Pulled By: trueadm
fbshipit-source-id: c5bba2b9315d7f2af5fdeb612f56e739a7aa2c23
Summary:
`typeof React.forwardRef(...)` is `object` but Prepack was returning `function`. This is particularly important since React Native’s `Animated.createAnimatedComponent` crashes with an invariant if `typeof` is `function`. We can’t compile our React Native internal bundle without this fix.
22cf5dc566/Libraries/Animated/src/createAnimatedComponent.js (L20-L25)f9358c51c8/packages/react/src/forwardRef.js (L12-L43)
Small Prepack core change in `src/evaluators/UnaryExpression.js`.
Pull Request resolved: https://github.com/facebook/prepack/pull/2425
Reviewed By: trueadm
Differential Revision: D9447593
Pulled By: calebmer
fbshipit-source-id: de1e9101f368ea52c8c17c31d04a2e4e0a38dd6b
Summary:
Release notes: adds a `arrayNestedOptimizedFunctionsEnabled` flag to enable nested optimized functions derived from Array.prototype methods (like `map`) and Array.from
This PR puts the existing (unstable) work for Array prototype methods behind a flag. The flag is enabled by default in React and serializer tests.
Pull Request resolved: https://github.com/facebook/prepack/pull/2404
Differential Revision: D9272747
Pulled By: trueadm
fbshipit-source-id: d7e53656a12cd6cff680a9ef0e2580a93d56e34e
Summary:
Release notes: none
Fixes#2393. I created the test as a React test because I couldn't see any way of making a serializer test fail if there was a warning without making many other tests fail or without adding another `// some config` to the test (which I believe we said we would not add anymore as there's already too many). React tests will always fail on side-effect warnings, so this takes better advantage of this.
Pull Request resolved: https://github.com/facebook/prepack/pull/2394
Differential Revision: D9249714
Pulled By: trueadm
fbshipit-source-id: 0e11d13e575eca3b6bd75e437eb083b9294a3aee
Summary:
Release notes: none
Fixes a `defaultProps` bug and also improves bloat slightly by ensuring we state that the default props helper can be omitted if the value it mutates is never used. Furthermore, added `isPure` to the binary expression temporal, so if the value created from it never gets used, then we don't emit the action.
Pull Request resolved: https://github.com/facebook/prepack/pull/2391
Differential Revision: D9179550
Pulled By: trueadm
fbshipit-source-id: b5671a725e347be3876a04b927eef99977757bd8
Summary:
Release notes: none
Follow up to https://github.com/facebook/prepack/pull/2346. This PR cleans up all the React specific nested optimized function logic, removing most of the old legacy code in favor of the simpler/cleaner approach.
This also introduces a `bubbleSideEffectReports` parameter to `evaluatePure` for specific case where we don't want side-effects to be reported to parent `evaluatePure` calls; necessary for the React reconciler to not throw a `UnsupportedSideEffect` which will cause the reconciliation process to hard-fail (instead nested optimize functions bail-out to be inline abstract function calls like before, and we do not try and optimize the nested function).
Lastly, we now throw an `SideEffect` object when evaluating a nested optimized function and detecting a side-effect. There's no point in continuing evaluation of a given side-effectful function, so this speeds things up a bit by terminating execution.
Pull Request resolved: https://github.com/facebook/prepack/pull/2348
Differential Revision: D9138290
Pulled By: trueadm
fbshipit-source-id: 6d468ea0430be0c0190ff9817b2872df368c325d
Summary:
Fixes#2322. The React Compiler was discarding keys for components that were inlined.
Consider:
```jsx
const React = require("react");
__evaluatePureFunction(() => {
function Lambda(props) {
return [<div key="0" />, <Omega key="1" />, <Omega key="2" />];
}
function Omega(props) {
return <div />;
}
__optimizeReactComponentTree(Lambda);
module.exports = Lambda;
});
```
```jsx
(function() {
var _2 = function(props, context) {
_4 === void 0 && $f_0();
return [_4, _7, _7]; // <------------------------- `_7` has no `key`!
};
var $f_0 = function() {
_4 = <div key="0" />;
_7 = <div />;
};
var _4;
var _7;
module.exports = _2;
})();
```
---
Cases where this can be an issue:
- First render mode and we need to hydrate instances based on keys (will this be a thing?)
- Compiled component is re-rendered and a list has a different order. List item components are expensive.
- We inline components with state.
Of course, it’s possible this may be a non-issue.
My solution for this was to wrap the inlined React Element in a `<React.Fragment>` with a key. So my input above becomes:
```jsx
[
<div key="0" />,
<React.Fragment key="1"><div /></React.Fragment>,
<React.Fragment key="2"><div /></React.Fragment>,
]
```
Cloning inlined elements and adding keys is a fragile operation since the element may already have a key or the element may be some other React node like a portal. I opted for wrapping in `<React.Fragment>`. Note that this also means we can hoist `<div />` in the example above.
Thoughts on adding an optimization which clones the element and adds `key={x}` in cases where this is possible? `<React.Fragment>` seems correct in all cases, but cloning with `key={x}` when possible seems like it might be faster.
Also happy to hear other suggestions for maintaining the key on inlined elements.
This has no impact on our internal bundle.
Pull Request resolved: https://github.com/facebook/prepack/pull/2324
Differential Revision: D9034942
Pulled By: calebmer
fbshipit-source-id: 2cdeec611a2c1f67059fa093684b5b51e0bbe809
Summary:
After adding classes to my fuzzer (which does not use first render mode) I found #2316. This PR fixes#2316 and a related issue I found while working on that. (Each fix is in a separate commit.) The second issue I found is scarier since the compiler passes but we get invalid output.
In the following example I observed an invariant violation:
```js
const React = require("react");
__evaluatePureFunction(() => {
function Tau(props) {
return React.createElement(
"div",
null,
React.createElement(Epsilon, {
a: props.z,
}),
React.createElement(Zeta, {
p: props.h,
})
);
}
class Epsilon extends React.Component {
constructor(props) {
super(props);
this.state = {};
}
render() {
return React.createElement(Zeta, { p: this.props.a });
}
}
function Zeta(props) {
return props.p ? null : React.createElement("foobar", null);
}
__optimizeReactComponentTree(Tau);
module.exports = Tau;
});
```
```
=== serialized but not visited values
=== visited but not serialized values
undefined, hash: 792057514635681
referenced by 1 scopes
=>_resolveAbstractConditionalValue alternate(#75)=>ReactAdditionalFunctionEffects(#80)
Invariant Violation: serialized 26 of 27
This is likely a bug in Prepack, not your code. Feel free to open an issue on GitHub.
at invariant (/Users/calebmer/prepack/src/invariant.js:18:15)
at ResidualHeapSerializer.serialize (/Users/calebmer/prepack/src/serializer/ResidualHeapSerializer.js:2465:17)
at statistics.referenceCounts.measure (/Users/calebmer/prepack/src/serializer/serializer.js:259:15)
at PerformanceTracker.measure (/Users/calebmer/prepack/src/statistics.js💯14)
at ast (/Users/calebmer/prepack/src/serializer/serializer.js:238:38)
at statistics.total.measure (/Users/calebmer/prepack/src/serializer/serializer.js:169:17)
at PerformanceTracker.measure (/Users/calebmer/prepack/src/statistics.js💯14)
at Serializer.init (/Users/calebmer/prepack/src/serializer/serializer.js:136:35)
at prepackSources (/Users/calebmer/prepack/src/prepack-standalone.js:66:33)
at compileSource (/Users/calebmer/prepack/scripts/debug-fb-www.js:92:18)
```
Somehow we were visiting `undefined`, but clearly we weren’t serializing it given the source code. Here’s what was happening:
- We’d visit an additional function calling `withCleanEquivalenceSet`.
- The additional function would enqueue an action which visited `<foobar>`.
- Later, we’d execute the `<foobar>` visiting action outside our `withCleanEquivalenceSet` with our default equivalence set.
- The same thing happens with our new root from our `Epsilon` class.
- Except now some effects have been applied that set the `type` for our `<foobar>` React element in our React equivalence set to `undefined`. Since when we created `<foobar>` we modified its `type` property. (Recorded in `modifiedProperties`.)
- Now our `<Zeta>` in `<Tau>` and our `<Zeta>` in `<Epsilon>` share the _exact same_ `<foobar>` thanks to our equivalence set.
- But `<foobar>` has a `type` of `undefined` thanks to the effects we applied. We should be creating a new `<foobar>` since we are in a new optimized function.
- ***Boom!*** We visit `undefined`, but don’t serialize it since the same effects aren’t applied when we serialize.
This test case caught an important flaw in our visiting logic, but it only manifested as an invariant under these very specific conditions. Which is a little scary. In a large example, like our internal bundle, we would of course serialize some `undefined` value but we would have still visited `undefined` instead of the proper type, _and_ we may consider two elements to be equivalent when we shouldn’t since their components may render independently. This issue (I presume) can also affect we bail out on since they create new trees inside the root tree.
While debugging and fixing this issue, I found another with incorrect/suboptimal output that passes Prepack and passes eslint. Given the following input:
```js
require("react");
__evaluatePureFunction(() => {
const React = require("react");
function Tau(props) {
return React.createElement(
"a",
null,
React.createElement("b", null),
React.createElement(Epsilon, null),
React.createElement("c", null)
);
}
class Epsilon extends React.Component {
constructor(props) {
super(props);
this.state = {};
}
render() {
return React.createElement("d", null);
}
}
__optimizeReactComponentTree(Tau);
module.exports = { Tau, Epsilon };
});
```
We get this output:
```js
(function () {
var _$0 = require("react").Component;
var _3 = function (props, context) {
_6 === void 0 && $f_0();
_9 === void 0 && $f_1();
var _8 = <_B />;
var _4 = <a>{_6}{_8}{_9}</a>;
return _4;
};
var _B = class extends _$0 {
constructor(props) {
super(props);
this.state = {};
}
render() {
return _E; // <--------------------------- Incorrect. `_B` may be rendered outside of `_3`.
}
};
var $f_0 = function () {
_6 = <b />;
_E = <d />;
};
var _6;
var _E;
var $f_1 = function () {
_9 = <c />;
};
var _9;
var _0 = {
Tau: _3,
Epsilon: _B
};
module.exports = _0;
})();
```
This happened because the React serializer’s `_lazilyHoistedNodes` logic was implemented in a way that doesn’t play well with the interleaving almost random order of the serializer invocation of React lazily hoisted nodes logic.
Pull Request resolved: https://github.com/facebook/prepack/pull/2318
Differential Revision: D8992253
Pulled By: calebmer
fbshipit-source-id: 4a75e5768ffb7887c3a8afa2a0f3f59e7eac266d
Summary:
Release notes: upgrades Prepack to use Babel 7.0.0-beta.53
This is a big PR that updates all of Prepack to Babel 7. Babylon is now `babel/parser` and pretty much all of the the previous Babel packages are now located in scoped packages. I had to make a bunch of changes around Jest/Flow/Webpack to get this all working. The build times of building Prepack itself seem considerably faster (easily twice as fast locally). I followed most of the Babel 6 -> 7 upgrade guide from the Babel site in terms of changing nodes and type definitions to match the new ones.
Pull Request resolved: https://github.com/facebook/prepack/pull/2256
Differential Revision: D8850583
Pulled By: trueadm
fbshipit-source-id: 2d2aaec25c6a1ccd1ec0c08c5e7e2a71f78ac2d8
Summary:
I generated the following program (now added to test cases) and while I would expect its evaluation in Prepack to terminate reasonably quickly, I did not see the program terminate after five minutes of execution:
https://gist.github.com/calebmer/2bf1d84a5b7849fa732bce69811df10b
After this change that same program’s Prepack evaluation terminates in about two seconds. This change also saves about 2.8s on the evaluation of our internal bundle which is about 3% of the total time.
What was happening? Why did my program fail to terminate execution in five minutes? Why was the internal bundle relatively ok compared to my extreme case?
In my program, I would [expect the component `Mu`](https://gist.github.com/calebmer/2bf1d84a5b7849fa732bce69811df10b#file-program-js-L289-L291) to be inlined about 60 times. Which means there should only be about 60 calls to `ReactElementSet#add` for each of `Mu`’s `div`s. However, after some basic instrumentation I observed way over ten thousand visits to these React elements.
This pair of method calls happens a couple times in `ReactEquivalenceSet` and friends.
5f7256f17e/src/react/ReactEquivalenceSet.js (L233-L234)
Both `getEquivalentPropertyValue`…
5f7256f17e/src/react/ReactEquivalenceSet.js (L255)
…and `getValue`…
5f7256f17e/src/react/ReactEquivalenceSet.js (L104)
…end up calling `ReactElementSet#add` with the same React element. Then `ReactElementSet#add` ends up recursing into children. So the root element is added 2 times, so the root’s children are added 4 times each, so each child of those elements are added 8 times each, and so on. So if a leaf element is `n` deep in the tree it will be added `2^n` times. The most nested `Mu` child was 9 elements deep. `Mu` appeared 60 times at many levels of nesting.
I’m not entirely sure why my generated case was so much worse then our internal bundle. My two best guesses are: lots of repeated components or deeper nesting?
My fix was to move the `getValue` call into `getEquivalentPropertyValue`. This way if `getEquivalentPropertyValue` already finds an equivalent value we can short-circuit and not run `getValue` which will perform the same operation.
Pull Request resolved: https://github.com/facebook/prepack/pull/2243
Reviewed By: trueadm
Differential Revision: D8811463
Pulled By: calebmer
fbshipit-source-id: 4f30a75e96c6592456f5b21ee9c2ee3e40b56d81
Summary:
This will fail CI if we forgot to run `yarn prettier` before committing.
We do the same in React repo. It prevents committing stale files that later cause unexpected changes.
Pull Request resolved: https://github.com/facebook/prepack/pull/2212
Differential Revision: D8784406
Pulled By: gaearon
fbshipit-source-id: ca948b8e088be8886c8ba865f280ba8d72750f69
Summary:
> I’m learning the Prepack/React Fusion codebase, so incorrect assumptions and limited a understanding of systems on my part are both very likely.
While I was experimenting with the React Compiler codebase I found the following invariant violation with the test case after that when using `create-element` as the `reactOutput`.
```
Invariant Violation: value must have been visited
```
```js
const React = require("react");
__evaluatePureFunction(() => {
function Foo(props) {
return <React.Fragment />;
}
__optimizeReactComponentTree(Foo);
module.exports = Foo;
});
```
[[Prepack REPL]](https://prepack.io/repl.html#MYewdgzgLgBASgUwIbFgXhgJwQRwK4CW2AFAETYpSkCUA3AFD0D6TCAbkgDZ5JQIAKebADE8YVAXDFi1GGgB8MAN70YMAGZiJ4GMJAhiAB0whDEWSrVrsUIWBgAeRJQB0wzEgDmAWwRhYAPTyDGoAvoxqLKZQBN4EAF4IzqgAwiDehuB+UAAq2AjEeiB0ETDeIAAmeJwILggAHpmYUBByuvoMoSVAA)
After digging in more, I discovered that this was the same issue which required some clowniness in the React Compiler tests to workaround.
e3f4262c9d/test/react/setupReactTests.js (L116-L119)
The problem with my example is that the React library was not being visited when a `React.Fragment` appeared even though the serializer needed the library to be visited to output code.
This diff makes sure we visit the React library when we see a `React.Fragment` and makes sure we don’t over visit the React library in `jsx` mode.
I also refactored this area of the code and added comments. Let me know if my refactors or comments don’t make sense.
Pull Request resolved: https://github.com/facebook/prepack/pull/2223
Reviewed By: trueadm
Differential Revision: D8774047
Pulled By: calebmer
fbshipit-source-id: 775a626b8a6bd33a5366e6dae6727350835b060e
Summary:
Release notes: none
In an attempt to pull out some of the work from https://github.com/facebook/prepack/pull/2148 to make things easier to consume and review. This PR is one part, where we check the temporal alias values (if an object value has it) and see if we can dedupe the trees. Example below:
Closes https://github.com/facebook/prepack/pull/2193
Differential Revision: D8732542
Pulled By: trueadm
fbshipit-source-id: 352d0048acfef69b5a257c0fe280435fa7baaa7f
Summary:
Release notes: adds React Native mocks to Prepack
This adds React Native mocks to Prepack and a few basic tests to demonstrate inlining of `View` and `Text`.
Closes https://github.com/facebook/prepack/pull/2096
Differential Revision: D8723932
Pulled By: NTillmann
fbshipit-source-id: 38bd265cd8935ebdf30266ec337378b4ea5b09d6
Summary:
Release notes: none
When the `objectWithoutProperties` mock was originally created, my knowledge of Prepack's internals wasn't as good as it was now. Now that I understand how AbstractObjectValues work, we can safely add the known values in `objectWithoutProperties` to the abstract backing object. The backing object was missing these values before and was an empty empty that was partial. This should give more data and value on our internal bundle, where before the values would be lost unnecessarily.
Closes https://github.com/facebook/prepack/pull/2194
Differential Revision: D8716289
Pulled By: trueadm
fbshipit-source-id: 451065473ea09943831f75c0bc15490e73c8d947
Summary:
Currently we have a single giant file with all tests, and a giant snapshot. This is both slow, and hard to work with and iterate on.
In this PR I will refactor our test setup.
- [x] Split it up into multiple files (gets the test running from 45s to 27s)
- [x] Run Prettier on test files
- [x] Split tests further for better performance
- [x] Make it possible to run one test file
- [x] Fix the issue with double test re-runs in watch mode on changes in the test file
- [x] Refactor error handling
- [x] Run Prettier on fixtures
- [x] Add a fast mode with `yarn test-react-fast <Filename>`
- [x] Fix double reruns on failure
Potential followups:
- [x] Figure out why test interruption broke (need https://github.com/facebook/jest/issues/6599 and https://github.com/facebook/jest/issues/6598 fixed)
- [x] Revisit weird things like `this['React']` assignment with a funny comment in every test
Closes https://github.com/facebook/prepack/pull/2187
Differential Revision: D8713639
Pulled By: gaearon
fbshipit-source-id: 5edbfa4e61610ecafff17c0e5e7f84d44cd51168
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
Summary:
Release notes: none
There was a bug, where React abstract object values with the same intrinsic name were incorrectly being de-duped and re-used. This is a normal expected Prepack optimization, but it actually breaks React components that expect to be able to create many values with the same intrinsic name. We now mark such objects and ensure they do not correctly return `true` from `equals()`. Fixes https://github.com/facebook/prepack/issues/2089
Closes https://github.com/facebook/prepack/pull/2126
Differential Revision: D8448171
Pulled By: trueadm
fbshipit-source-id: d21fddd70576e6e55c41070ac43ce2f58a059e7a
Summary:
Release notes: none
This PR moves the logic of sanitizing ReactElements for `firstRenderOnly` mode to the serialization phase instead of during reconciliation. The plan is to create a follow up PR that introduces a new phase after the visitor phase, that skips values that were detected as dead code once the visitor stage finishes, but that's not in scope for this PR.
Closes https://github.com/facebook/prepack/pull/2123
Differential Revision: D8435440
Pulled By: trueadm
fbshipit-source-id: f442525e663a7bc0f9608b96eb7d7a2389a707b7
Summary:
Release notes: none
This is a long standing fix, where ReactElements should be created lazily but are currently not done so. To do this, we now use the `temporalAlias` code paths on lazy branched ReactElements to ensure they are emitted as temporal entries when we "branch" (i.e. conditional) and have many possible routes where ReactElements may or may not be created.
Fixes https://github.com/facebook/prepack/issues/2113. Depends on https://github.com/facebook/prepack/pull/2112
Closes https://github.com/facebook/prepack/pull/2107
Differential Revision: D8383327
Pulled By: trueadm
fbshipit-source-id: bbcff45ddd07406b18bcddce2de0279fb52da1a1
Summary:
Release notes: improves ReactElement creation by re-using ReactElement nodes where possible
We already have a pretty good ReactElement equivalence system to re-use ReactElements. We weren't using it to its full power though. When we have nested ReactElements, we don't use the equivalence system at all, meaning we duplicate lots of ReactElements when we can re-use.
This is a nice performance optimization. I've added a test that should prevent regressions here too.
Closes https://github.com/facebook/prepack/pull/2114
Differential Revision: D8381595
Pulled By: trueadm
fbshipit-source-id: befabd04326a162f8ab6a00e089cdada052cf6c8
Summary:
Release notes: none
Whilst working on adding React Native mocks, I ran into cases where context should be inlining but it wasn't. It now is inlining far better, with more test coverage. Furthermore, we now have a new config flag to pass to `__optimizeReactComponentTree`, in the case of `isRoot` to state that the tree is a root component tree (rather than a a branch of another tree).
Closes https://github.com/facebook/prepack/pull/2098
Differential Revision: D8348381
Pulled By: trueadm
fbshipit-source-id: 5e01bd77437e8bc3d1f22ff47d668897152203a0
Summary:
The code didn't work without it sometimes because conceptually all code outside of `getTrials` is an isolated "bundle". We can only access its internals through the `Root` reference. This fixes the tests to follow this constraint and removes the hack.
Closes https://github.com/facebook/prepack/pull/2102
Differential Revision: D8333634
Pulled By: gaearon
fbshipit-source-id: 1fb24751490042beae55a133f64351a9be9f903c
Summary:
Release notes: none
This fixes an issue where `createDefaultPropsHelper` gets created in a branch of effects, and is cached on the `realm`. Instead `createDefaultPropsHelper` should get created during the init phase, so the function is accessible throughout the entire lifecycle of the realm.
Closes https://github.com/facebook/prepack/pull/2087
Differential Revision: D8290010
Pulled By: trueadm
fbshipit-source-id: 324035cdd3c8a951c44849e1ccbfca9798a551b0
Summary:
Release notes: fixes a range of spread bugs with ReactElements
This is a very important PR for React reconciliation, it fixes many undiscovered bugs and adds a huge amount of test coverage that was previously missing.
Whilst testing quite complex cases of JSX spreads in combination with defaultProps on our internal bundle I noticed that there were some bugs appearing, but because the branches where these bugs were appearing were not used on firstRender, it meant we got away with it on our internal tests.
We now use snapshotting and properly evaluateForEffects when recovering from `Object.assign` with ReactElement creation of config/props. We also properly use the `temporalAlias` to ensure we reference the correct object.
Closes https://github.com/facebook/prepack/pull/2070
Differential Revision: D8243793
Pulled By: trueadm
fbshipit-source-id: e8c37aa6750c0a6d41f12249d8872004da3ab3a6
Summary:
Release notes: none
When in strict mode, emitting property assignments to a frozen object (ReactElement in this case) causes a runtime error. We shouldn't be emitting ReactElement properties when we havoc the object. Unfortunately, we can't do this to all final objects, because our internal snapshot test fails when doing this (because the final object has properties that are conditional and thus delayed, so we need all its bindings to be "flushed" at the point of havocing). As a more thorough future fix, we should probably add a separate codepath for serializing objects that are final (we delay the entire object being emitted until all its properties are ready, like the ReactElement serializer does).
Interestingly, the output contains an unused variable that should be removed NTillmann?
```js
var _0 = function (props, context) {
"use strict";
var _J = props;
var _$0 = _J.b;
var _1 = _$0 !== null;
if (_1) {
var _$1 = _J.b;
var _$2 = _$1.x;
var _5 = _$2 !== null;
if (_5) {
var _$3 = _J.b;
var _$4 = _$3.x;
}
}
var _$5 = _J.c;
var _$6 = _J.someAbstractFunction;
var _A = _1 ? _5 ? _$4 : null : null;
var _8 = { // <-- why has this not been removed? can we not mark the entry as "pure"
a: 1,
b: _A,
c: _$5
};
_8.a = 1;
_8.b = _A;
_8.c = _$5;
var _F = <div a={1} b={_A} c={_$5} />;
var _$7 = props.someAbstractFunction(_F);
return _F;
};
```
Closes https://github.com/facebook/prepack/pull/2065
Differential Revision: D8221498
Pulled By: trueadm
fbshipit-source-id: e508cba2e930bec9c913efae3c653637671098c6
Summary:
Release notes: none
This PR is a general spring cleaning of all the React logic. Nothing is fixed, but the logic for many of the aspects of the React reconciliation have been simplified and tidied up.
- I've removed all cases of mutating ReactElements, we now create new ReactElements instead of mutating – because, well, ReactElements are immutable (as are props).
- Removed ReactSerializerState and moved the logic that previous existed into `realm.react`.
- Simplified the branching logic of the reconciler so we no longer need to pass around state and instead use the existing conditional logic.
Manually tested on all our internal bundles are we have no failures.
Closes https://github.com/facebook/prepack/pull/2039
Reviewed By: gaearon
Differential Revision: D8205619
Pulled By: trueadm
fbshipit-source-id: a84c363038086b490d761dbe18711d617058f05c
Summary:
Release note: none
This PR remove `firstRenderOnly` from a bunch of tests that shouldn't have had it and adds a new test for a big with `props` being incorrectly marked for `refuseSerialization` – `props` can be serialialized without the ReactElement, and the test shows how this might happen.
Closes https://github.com/facebook/prepack/pull/2057
Differential Revision: D8203483
Pulled By: gaearon
fbshipit-source-id: fbfb9cc3bbdadaba5884f87319cb5b54e4a0657a
Summary:
Release notes: none
After discussions last night with sebmarkbage, I noticed that we apply defaultProps in the wrong place when the config is partial object or an abstract object. This now uses a helper function, which is injected once for the entire bundle, that handles `defaultProps` cases as a temporal function entry. Added a regression test too.
Closes https://github.com/facebook/prepack/pull/2036
Differential Revision: D8175799
Pulled By: trueadm
fbshipit-source-id: b6bb4f30ee07f0bd9ac07fc91018c26936696f4b
Summary:
Release notes: none
Now that we have snapshotting from Object.assign and ToObject for abstracts, we can now upgrade the ReactElement creation logic, simplifying a bunch of routes which actually resulted in a bunch of components inlining in tests where they didn't before. Furthermore, it allows us to tighten up the Flow typings in the React reconciler and tidy up some of the checks for `key`s or `ref`s logic, which was a bit hard to read before and didn't take into account objects with multiple values in their values domain. This also fixes a bug with abstract conditional component types not getting the deafultProps values correctly.
Closes https://github.com/facebook/prepack/pull/1996
Differential Revision: D8172083
Pulled By: trueadm
fbshipit-source-id: 0a8a1e0631883c92e914aba1308a109b3afda137
Summary:
Release notes: opt out of optimizing a React component in a tree with `__doNotRender`
This PR makes it possible to opt out of a single component in a React component tree by detecting a `__doNotRender` flag. For example:
```js
function MyComponent(props) {
...
}
MyComponent.__doNotRender = true;
class MyComponent extends React.Component {
render() {
...
}
}
MyComponent.__doNotRender = true;
```
Closes https://github.com/facebook/prepack/pull/2004
Differential Revision: D8123009
Pulled By: trueadm
fbshipit-source-id: d5fc54aea2ea1253e260e41c1fb7d4f2d4717ffd
Summary:
Release notes: none
This fixes an issue that we were experiencing, that turned out to be the correct fix after looking at https://github.com/facebook/prepack/pull/1997. We shouldn't mutate arrays in the reconciliation process, instead we should create new ones when we have new values. Also, we should ensure that we don't mutate ReactElements during reconciliation too, instead we should just create new ones. Mutating existing objects and values breaks down completely when we enter nested effects that need to re-evaluate the same original values, so this fixes a bunch of internal issues we were seeing on our internal FB bundle.
Closes https://github.com/facebook/prepack/pull/1999
Differential Revision: D8105499
Pulled By: trueadm
fbshipit-source-id: 4e6ceb8b02c886c42156a8903c347765f101c840
Summary:
Release notes: adds experimental `react-dom/server` ahead-of-time rendering to string
This PR was part of a small 1-2 day hackathton to see the applicability of creating a server-side renderer based almost entirely to be a 1:1 of `ReactDOMServer` from `react-dom/server` package. This is by no means a full, complete server renderer but is the foundations for us to do further work on this path in the future. Currently, it consists of a single Jest test, to ensure the output of the Hacker News benchmark matches that of the current ReactDOMServer `renderToString` output.
The performance results look very promising (there are some `console.time` lines you can comment out in the test to see the performance for yourself). This implantation essentially compiles away `react` and `react-dom/server` and instead injects runtime helper functions that do things that cannot be statically determined ahead of time.
Lots of features are missing behind `invariant` TODOs. This was done intentionally.
Closes https://github.com/facebook/prepack/pull/1940
Differential Revision: D8075964
Pulled By: trueadm
fbshipit-source-id: 33b3c7ba26b41871ccd15ad8bde4ad257009fed6
