Summary:
Release notes: none
Fixes https://github.com/facebook/prepack/issues/2574. This PR adds abstract value support to `Object.getOwnPropertySymbols` like we have done other internal methods (like `Object.keys` and `Array.from`) where we know the internal method creates an array with unknown numeric properties.
Pull Request resolved: https://github.com/facebook/prepack/pull/2575
Differential Revision: D10114237
Pulled By: trueadm
fbshipit-source-id: 07301147e2dff1ab370243a8dc9648745bbbbb96
Summary:
…pecify modules
Release Notes: None
Sometimes it'll be useful to allow the user to specify which specific modules you want to speculatively execute. This allows that by turning `--initializeMoreModules` into `--modulesToInitialize <ALL | comma separated list of modules>`
Updated tests as well.
Pull Request resolved: https://github.com/facebook/prepack/pull/2576
Differential Revision: D10092554
Pulled By: cblappert
fbshipit-source-id: bf601e14c2be59c865ae9513c914f39325521945
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 notes: none
This fixes#2555.
We were waiting on the wrong thing in the serializer.
Adding regression tests.
Pull Request resolved: https://github.com/facebook/prepack/pull/2556
Differential Revision: D9949808
Pulled By: NTillmann
fbshipit-source-id: a4ef5ece8c5dab6fa579a20dbb35ce7bf794bfc0
Summary:
Release Notes: None
Optimized functions will now be evaluated with the path condition at the time of function closure creation.
Resolves issue #2422
Pull Request resolved: https://github.com/facebook/prepack/pull/2537
Differential Revision: D9886282
Pulled By: cblappert
fbshipit-source-id: cae0282903d639ff0d94d0f4091c6f8ef9ef9a98
Summary:
Release Notes: None
Before, during referentialization, we would always default to the global scope if the value was accessed in more than one optimized function scope. Now we look for the outermost optimized function.
This logic already existed in ResidualHeapSerializer, so I refactored it out into `serializer/utils.js`.
Fixes#2428
Pull Request resolved: https://github.com/facebook/prepack/pull/2544
Differential Revision: D9926077
Pulled By: cblappert
fbshipit-source-id: c4ee6c07c7409534e9be14df25b582078a7ec77c
Summary:
This is a helper for inspecting values at a particular node in the AST. E.g.
```js
let n = global.__abstract ? __abstract("number", "10") : 10;
let x = {foo:1};
let y = {foo:2};
let c = __abstract("boolean", "c");
let i = 0;
let obj = {};
do {
i++;
obj.j = i;
obj.foo = c ? x : y;
} while (i < n);
__debugValue(obj); // Breaks with obj in context
inspect = function() {
return i + " " + obj.j;
};
```
Pull Request resolved: https://github.com/facebook/prepack/pull/2554
Differential Revision: D9922908
Pulled By: sb98052
fbshipit-source-id: dab9cae64a461283d8dec7f0bb7f8fae87a01c78
Summary:
The following test case currently fails:
```js
function F() {
this.a = 1;
this.b = 2;
if (global.__abstract) return global.__abstract(undefined, "undefined");
}
const result = new F();
global.inspect = () => JSON.stringify(result);
```
We hit this bug in our internal React Native bundle. I only added support for `base` construction kinds since the template for `derived` construction kinds would get more complicated.
Pull Request resolved: https://github.com/facebook/prepack/pull/2535
Differential Revision: D9906309
Pulled By: trueadm
fbshipit-source-id: 49a71ceaf30a851075879295e63e98ce7e1bbe2d
Summary:
Release notes: User-level stack overflows no longer crash Prepack.
This fixes#2552, and actually a bit more:
- We used to piggy-back on `pushContext` to count stack frames. However, there is no one-to-one correspondance of calls and context, so that wasn't a very good proxy. (In fact, there seem to be calls that don't push a context.) So we now count calls and constructs explicitly.
- Instead of just throwing a `FatalError` when the stack space is exceeded, which in turn causes an invariant violation since there must not be a `FatalError` thrown without a compiler diagnostics having beein issue. Now there is a regular error code PP0045.
Added error handler regression test.
Pull Request resolved: https://github.com/facebook/prepack/pull/2553
Differential Revision: D9889090
Pulled By: NTillmann
fbshipit-source-id: f6f863ee9ef73f258692f215ef75b63b737f5394
Summary:
This change ensures that the code generated via optimized array loop operators is correct in the InstantRender setting. It enables the optimization of such functions when the `--instantRender` option is set. Materialization is prevented by ensuring that post-optimization, objects that are reachable from the function are not mutated. Recoverable errors are issued. We also degrade all InstantRender bailouts to recoverable errors, to facilitate debugging. We add a constraint that optimized functions may not be reused.
Resolves#2451#2448
Pull Request resolved: https://github.com/facebook/prepack/pull/2547
Differential Revision: D9816268
Pulled By: sb98052
fbshipit-source-id: 2112b199de50b80a7a9852a794c082be3bf122e9
Summary:
Release Notes: None
This PR makes it so we no longer report conflicts for child optimized functions reading from values that their parents have written.
Couple of things for discussion:
- This creates somewhat confusing behavior because the child functions will take the concrete value after the parent functions' effects have been applied:
```javascript
(function () {
let obj = {p: 42};
function f() {
obj.p += 3;
function g() { return obj.p; } // Will optimize to `return 47;`
obj.p += 1;
__optimize(g);
obj.p += 1;
return g;
}
__optimize(f);
global.f = f;
})();
```
- This PR creates the Parent/Child relationship based off `AdditionalFunctionEffects.parentAdditionalFunction` which goes off of syntactic nesting of functions instead of nesting of `__optimize` calls as in the issue. I believe basing the nesting off of `__optimize` calls could lead to somewhat unintuitive results (especially considering we store `parentAdditionalFunction` in `AdditionalFunctionEffects` to be the syntactic parent).
I am not sure if it is needed by Instant Render NTillmann? If it is, we may want to consider changing `AdditionalFunctionEffects.parentAdditionalFunction` to be based off of `__optimize` call nesting as well for consistency.
As an example a slight modification on the example above:
```javascript
(function () {
let obj = {p: 42};
function g() { return obj.p; }
function f() {
obj.p += 3;
__optimize(g);
obj.p += 1;
return [g, obj];
}
__optimize(f);
global.f = f;
})();
```
With this PR, we report an error. Based off of `__optimize` nesting, we would optimize `g` to `return 46;`. I would expect to see `return 42;` `return 45;` or `return obj.p;` in this case.
- To resolve the above issues, in the future, we could make any values modified by parent optimized functions into abstract values during evaluation for child optimized functions to force their accesses to be recorded in generators .
Resolves#2351
Pull Request resolved: https://github.com/facebook/prepack/pull/2542
Differential Revision: D9803741
Pulled By: cblappert
fbshipit-source-id: baca233c8de81633332b25f0776ed1a9d6c95a60
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: providing __replaceFunctionImplementation_unsafe built-in
This new built-in allows replacing the method implementation (capture environment, body, ...) of source functions.
As a result, all method calls executed by the Prepack interpreter will be redirected,
and the replacement will carry over to the prepacked code.
Pull Request resolved: https://github.com/facebook/prepack/pull/2533
Reviewed By: hermanventer
Differential Revision: D9693654
Pulled By: NTillmann
fbshipit-source-id: bd28997965e641f58f89f7119fa477c535c0e539
Summary:
Release note: none
This attempts to reduce exponential blowup in the simplifier by doing more caching while computing implications and by imposing a depth limit on the number of simplify and implies calls.
It also tries to make implies and impliesNot more symmetrical, so that things are less ad-hoc.
Pull Request resolved: https://github.com/facebook/prepack/pull/2530
Differential Revision: D9664026
Pulled By: hermanventer
fbshipit-source-id: f7a9135b06298a2b77ad05bf377982a9b37e4ad1
Summary:
Release notes: Fixed bugs that could cause generated code to throw
A refactor of my previous attempt to address #2327, which I had to abandon because it was incompatible with the current implementation of certain modeling primitives. Like the previous version, this PR is an intermediate fix that will be refined in a follow up PR. It consists of three changes:
1) It adds a new helper that discharges values from a union after deriving the abstract value in it if needed.
2) It makes conditionally temporal values safe using a helper called `convertToTemporalIfArgsAreTemporal`.
3) It makes the the abstract and concrete members explicit in the interface to Abstract Concrete Unions. Presently, two code sites make the assumption that the abstract member is the first element of `args`, while all others traverse the list to locate it.
Follow ups to come:
1) Update `convertToTemporalIfArgsAreTemporal` to produce a conditional value left to be simplified by the serializer.
2) Enforce the protocol *temporal args should imply temporal results* more generally (#2489).
Fixes#2327 and #2406
Pull Request resolved: https://github.com/facebook/prepack/pull/2513
Differential Revision: D9636173
Pulled By: sb98052
fbshipit-source-id: 22d63dfb9d0da4b1f6eba4e2f6f88760f5eb03ca
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:
This PR implements a step of the way to getting leaked value analysis working for optimized Array operators. It is desirable to leak as little as possible, so that the operators can be specialized to take into account values in the environment in which they run. In the beginning, we are focusing on the narrow range of scenarios in which this is possible. We will start by enforcing the assumptions that we rely on, and make sure that the code that we generate is correct. Once we have correct code, we will start progressively relaxing the assumptions to increase coverage. The overall plan can be found here: #2452.
More specifically, this PR transitively materializes objects reachable via reads to bindings in the optimized function. This is necessary to snapshot the contents of those objects at specialization time.
Fixes#2405.
Pull Request resolved: https://github.com/facebook/prepack/pull/2456
Differential Revision: D9498939
Pulled By: sb98052
fbshipit-source-id: 16853f97dc781505dba29dce7f28996a0a4e7749
Summary:
Release notes: landing two previously reverted PRs
I found the issue.
Flow doesn't support optional fields in classes. This effectively becomes enforced with Babel 7 since it treats the type annotations as field initializers. Which means that these fields always gets created.
We happened to only use this newer plugin for class fields internally which broke our builds only there.
2b4546d Use `undefined` instead of missing to represent absent field in descriptors. Fixed all the callsites that checks for `hasOwnProperty` or `in` that I could find.
922d40c Fixes a Flow issue in the text printer.
I filed a [follow up issue for ObjectValue](https://github.com/facebook/prepack/issues/2510) since it has the same problem.
Pull Request resolved: https://github.com/facebook/prepack/pull/2511
Reviewed By: trueadm
Differential Revision: D9569949
Pulled By: sebmarkbage
fbshipit-source-id: f8bf84c4385de4f0ff6bcd45badacd3b8c88c533
Summary:
The following example fails with an invariant:
```js
try {
try {
const b = __abstract("boolean", "false");
if (b) throw new Error("throw");
} catch (error) {}
} catch (error) {
console.log(error.message);
}
```
```
Invariant Violation: assuming that false equals true is asking for trouble
debug-fb-www.js:208
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 PathImplementation.withCondition (/Users/calebmer/prepack/src/utils/paths.js:132:17)
at joinTryBlockWithHandlers (/Users/calebmer/prepack/src/evaluators/TryStatement.js:81:35)
at _default (/Users/calebmer/prepack/src/evaluators/TryStatement.js:30:47)
at LexicalEnvironment.evaluateAbstract (/Users/calebmer/prepack/src/environment.js:1379:20)
at LexicalEnvironment.evaluate (/Users/calebmer/prepack/src/environment.js:1367:20)
at LexicalEnvironment.evaluateCompletion (/Users/calebmer/prepack/src/environment.js:1102:19)
at LexicalEnvironment.evaluateCompletionDeref (/Users/calebmer/prepack/src/environment.js:1095:23)
at _default (/Users/calebmer/prepack/src/evaluators/Program.js:235:17)
at LexicalEnvironment.evaluateAbstract (/Users/calebmer/prepack/src/environment.js:1379:20)
```
What happens is we end up with a completion structure as follows when going into the outer `catch`:
```
- JoinedNormalAndAbrubtCompletions (joinCondition = x)
- SimpleNormalCompletion
- JoinedNormalAndAbrubtCompletions (joinCondition = x)
- ThrowCompletion
- SimpleNormalCompletion
```
The inner `JoinedNormalAndAbrubtCompletions` is the inner `try` block completion. The outer `JoinedNormalAndAbrubtCompletions` is the join of the inner `try`/`catch` blocks. Notably the `ThrowCompletion` is completely unreachable. However it is still there.
Ideally we would refine the completions. I tried this, but realized the `composedWith`, `pathConditionsAtCreation`, and `savedEffects` on `JoinedNormalAndAbrubtCompletions` were more to handle then I originally bargained for to fix my original test case. Instead I picked a simple fix for this specific case of `try`/`catch`. When we `AbstractValue.createJoinConditionForSelectedCompletions` we get a concrete `false` value. So I check if it is false and don’t execute the catch block if it is. The condition can never be concretely true. Otherwise we’d unconditionally catch an error in the block above.
Happy to take suggestions for a more general fix.
Pull Request resolved: https://github.com/facebook/prepack/pull/2507
Differential Revision: D9583244
Pulled By: calebmer
fbshipit-source-id: 7693efef5e967c90d5a4c54f10ef2c137f264ef8
Summary:
Release notes: none
Looking through our internal bundles and there are frequent cases where `instanceof` is used where the left-hand side is a primitive and the right-hand side is an abstract value. In the case where the left-hand side is a primitive and the right-hand side is a simple object, the instanceof binary expression should always return `false`.
Pull Request resolved: https://github.com/facebook/prepack/pull/2506
Differential Revision: D9567344
Pulled By: trueadm
fbshipit-source-id: 7333f3b81627657c184c77d4cfffd4511bee0cbf
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: Refactor of joined descriptors. Expect a slight performance regression. Will fatal in more cases that we would previously silently possibly generate the wrong code.
Currently we have three types of property descriptors. 1) Descriptors used by internal slots. 2) Normal concrete descriptors. 3) Abstract joined descriptors.
These are all typed as arbitrary an inexact object where all the fields are optional. That means that essentially any object with any typo can be assigned to a descriptor.
We are also not forced to deal with the joined descriptor case. Almost everywhere we assume that joined descriptors are really just a generic descriptor object which doesn't have any attributes on it.
There are so many small bugs related to this so I figured it's time to start dealing with it.
This PR turns this inexact object into nominally typed PropertyDescriptor, AbstractJoinedDescriptor and InternalSlotDescriptor. Essentially the same model as values.
Thanks to this Flow forces me to ensure that I've covered all of these cases. I've dealt with it in the cases I figured I could figure it out and where it was necessary such as the serializer/join/widen. In all other cases where we don't know, I ensure that we throw a fatal error instead of assuming that a joined descriptor is an empty descriptor.
Pull Request resolved: https://github.com/facebook/prepack/pull/2473
Differential Revision: D9526419
Pulled By: sebmarkbage
fbshipit-source-id: 1d3556b2d4608c02ba2570651f4e6a765fa0eea6
Summary:
gaearon found a regression to the cases I fixed in #2255. The following code:
```js
function fn1(arg) {
if (arg.foo()) {
if (arg.bar()) {
return 42;
}
}
}
function fn2(arg) {
if (arg.foo()) {
if (arg.bar()) {
return 1;
}
} else {
return 2;
}
}
if (global.__optimize) {
__optimize(fn1);
__optimize(fn2);
}
```
Now compiles to:
```js
var fn1, fn2;
(function() {
var _$4 = this;
var _0 = function(arg) {
var _$0 = arg.foo();
if (_$0) {
var _$1 = arg.bar();
}
if (_$0) {
var _$1 = arg.bar(); // <----------------------------- `arg.bar()` is redeclared.
}
return _$0 ? (_$1 ? 42 : void 0) : void 0;
};
var _8 = function(arg) {
var _$2 = arg.foo();
var _$3 = arg.bar(); // <------------------------------- `arg.bar()` should be inside `if (_$2)`
return _$2 ? (_$3 ? 1 : void 0) : 2;
};
_$4.fn1 = _0;
_$4.fn2 = _8;
}.call(this));
```
After looking through the commits, I found that #2274 regressed this case. After reverting that PR I found that these cases were fixed again, but interestingly the test added in that PR also passed after I reverted the change.
I’m opening this PR with the failing tests so hermanventer can take a look. I haven’t looked too deeply into this case, so I’m probably missing something important. Maybe some race condition between the landing of #2255 and #2274?
Pull Request resolved: https://github.com/facebook/prepack/pull/2288
Differential Revision: D9540424
Pulled By: calebmer
fbshipit-source-id: 3ef03068cb575617d64d73fdc1f9431188effdda
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:
Release notes: this might increase code size slighty, if the simplifier isn't fully working, in the rare case of conditional objects. I haven't observed any cases of that happening.
_Reviewing the individual commits might be helpful._
The main motivation of these changes is to prepare to avoid leaking the inner object when AbstractObjectValue is wrapping a template which prevents us from modeling templates without true identity.
To do that we need to stop leaking it to the Receiver since the Receiver can be passed to user space. Currently, we unwrap abstract objects [we really need to stop that since it leaks the receiver](cf8184904e).
It used to be that we special cased simple cases in AbstractObjectValue but since OrdinarySet operations are pretty complex, sometimes it's a simple assignment and sometimes the receiver leaks to a more complex operation.
Additionally, we assume that all these operations in AbstractObjectValue can be expressed as pure operations because they're simple objects. However, that's not the case because in properties.js/get.js we emit generator entries for intrinsic objects, havoced objects, widened properties, etc. These are at the very least unnecessary operations to perform when the AbstractObjectValue is actually representing one of the other branches. However, in the case of havoced objects, we don't really know if it is a simple object so it can actually be observable side-effects.
The simplest way of dealing with all these issues is to simply deal with side-effectful operations. So I do that by evaluating each path for effects and joining the effects using the evaluateWithAbstractConditional helper. This also lets us deal with non-simple abstract objects as a bonus.
This doesn't fully get us all the way to no longer leaking receivers. In a follow up I plan to deal with defIneOwnProperty, getOwnProperty, and a few other operations that also leak the inner object template.
Pull Request resolved: https://github.com/facebook/prepack/pull/2461
Differential Revision: D9510853
Pulled By: sebmarkbage
fbshipit-source-id: 4e03b1fb5b168a68b70120630341056bbf5f0e57
Summary:
Fixes#2458.
This is a new version of my change in #2459. I closed that PR since hermanventer had doubts which I confirmed after discovering my “fix” crashed my React Native internal bundle in a different way demonstrating its incorrectness.
After thinking about how to workaround this issue (since I was blocked on this) and fixing related issue #2466 I think I have a proper fix and I’m ready to defend it.
Note that this PR is based on top of #2466. The changes in [this commit](4896087869) are the ones to look at.
The failing test case is the following. Annotated with some comments to help demonstrate the issue:
```js
if (!global.__evaluatePureFunction) global.__evaluatePureFunction = f => f();
const result = global.__evaluatePureFunction(() => {
let x, y, z;
function f() {
const getNumber = global.__abstract ? global.__abstract("function", "(() => 1)") : () => 1;
const b1 = global.__abstract ? global.__abstract("boolean", "true") : true;
const b2 = global.__abstract ? global.__abstract("boolean", "!false") : true;
const b3 = global.__abstract ? global.__abstract("boolean", "!!true") : true;
x = getNumber(); // Modify binding
if (!b1) throw new Error("abrupt"); // Compose JoinedNormalAndAbruptCompletions
y = getNumber(); // Modify binding
if (!b2) throw new Error("abrupt"); // Compose JoinedNormalAndAbruptCompletions
z = getNumber(); // Modify binding
if (!b3) throw new Error("abrupt"); // Compose JoinedNormalAndAbruptCompletions
if (global.__fatal) global.__fatal();
return x + y + z;
}
f();
return x + y + z;
});
global.inspect = () => result;
```
What is happening is that we modify three bindings in `f` before we throw a `FatalError`. `x`, `y`, and `z`. We also have three `JoinedNormalAndAbruptCompletions` since we’re using abstract booleans and throwing if the boolean is false. Our realm when we throw the `FatalError` ends up looking like:
```
Realm {
savedCompletion: JoinedNormalAndAbruptCompletions {
savedEffects: { modifiedBindings: { z } },
composedWith: JoinedNormalAndAbruptCompletions {
savedEffects: { modifiedBindings: { y } },
composedWith: JoinedNormalAndAbruptCompletions {
savedEffects: { modifiedBindings: { x } },
}
}
}
}
```
When we throw the `FatalError` we want to restore all our bindings to a state before we started the `evaluateForEffects()` call so that we can leave a residual call in pure scope. However, the way the code is currently written we _don’t look into `savedCompletion.composedWith`_ even though there may be more bindings there. Instead we only look at `savedCompletion` so we only restore the `z` binding.
My previous fix modified `stopEffectCaptureAndUndoEffects()` to chase `composedWith`, but that broke the contract of `stopEffectCaptureAndUndoEffects()` as hermanventer mentioned. So instead I updated the `finally` block in `evaluateForEffects()` which cleans up modified bindings of a `savedCompletion` (in case we throw in the middle of evaluation) to more explicitly clean up the bindings of the saved completion and all composed completions. This preserves and fixes our effect cleanup behavior without changing the meaning of `stopEffectCaptureAndUndoEffects()`.
hermanventer I know you mentioned you wanted to look at this issue, sorry if I’m stepping on your toes. Since I was blocked I thought a bit more about this issue and figured I was directionally correct, but the specific fix in #2459 was wrong. Also sorry for the PR churn. Thought I wasn’t going to come back to this issue.
Pull Request resolved: https://github.com/facebook/prepack/pull/2467
Differential Revision: D9498191
Pulled By: calebmer
fbshipit-source-id: 6b95df969b2230302d94342a392e9d8cb81329db
Summary:
Fixes#2432. I found this while trying to compile our internal React Native bundle.
Prepack was incorrectly cleaning up lexical scopes in the case of a stack overflow.
This bug is a bit difficult to reproduce. Follow these steps to reproduce:
1. Produce a stack overflow in code Prepack compiles. With a recursive loop for instance.
2. Wrap in `__evaluatePureFunction`. This should catch the stack overflow and emit a residual function call.
3. Add empty functions for `global.require` and `global.__d`. You need to do this so that we don’t crash [in tracer code trying to resolve these variables](c18bd7ee97/src/methods/function.js (L82-L90)) but we instead crash [in `PrepareForOrdinaryCall`](c18bd7ee97/src/methods/function.js (L100)).
I used a `try`/`catch` to fix this which is the obvious solution, but perhaps not the most efficient. Happy to write a slightly uglier solution for performance if we feel like it is necessary.
Pull Request resolved: https://github.com/facebook/prepack/pull/2433
Differential Revision: D9469599
Pulled By: calebmer
fbshipit-source-id: bd120c2e4bd450da3364c722550977d6566ed89a
Summary:
Release notes: None
Remove dead _callRequireAndAccelerate function
Remove dead isModuleInitialized function
Remove dead --accelerateUnsupportedRequires option
Deleting dead code around checking uniqueness of previousDependencies
Add global.__eagerlyRequireModuleDependencies helper to effectively disable lazy module initialization in a code block (needed for InstantRender, to avoid having to deal with conditionally defined modules when prepacking nested callbacks)
Pull Request resolved: https://github.com/facebook/prepack/pull/2482
Differential Revision: D9492955
Pulled By: NTillmann
fbshipit-source-id: 91fe30168ffa848502982e042772462530b982a5
Summary:
Release note: Simplify equality expressions where types are known
Fixes: #2317
If the types or the arguments are known we can simplify equality expressions involving different typed arguments to false in some cases.
Pull Request resolved: https://github.com/facebook/prepack/pull/2481
Differential Revision: D9491269
Pulled By: hermanventer
fbshipit-source-id: 4e5657e23d13fafa7d48adcf01a7626fa63a2328
Summary:
Fixes#2464.
When we bail out of a function call because of a `FatalError` we did not leak the function we called. This means modified bindings were not appropriately leaked. See issue #2464 where the result is `undefined` instead of a reference to the leaked value.
This PR also adds a utility `__fatal` function for throwing a fatal error. Allowing us to more clearly express issues involving `FatalError`s. Used in the test.
Pull Request resolved: https://github.com/facebook/prepack/pull/2466
Differential Revision: D9482612
Pulled By: calebmer
fbshipit-source-id: 792d29818245ad9f631227d923efb56228df21e0
Summary:
Fixes#2468.
We weren’t using the correct `initializerAstNodeName` in two places. Also some free small efficiency wins.
Pull Request resolved: https://github.com/facebook/prepack/pull/2469
Differential Revision: D9481902
Pulled By: calebmer
fbshipit-source-id: 07dd44ae0141a74c50296c61828706da11cd26d2
Summary:
Fixes#2442 by not marking immutable bindings as modified. Its very possible that my fix is incorrect. There’s a bunch of related logic for `binding.hasLeaked`/`binding.mutable`/`modifiedBindings` and I certainly didn’t find them all to get a holistic view of what’s happening. From the code I did look at, this fix seems to be safe since there’s no way for a havoced function to change an immutable binding.
I ran into this issue while trying to compile our internal React Native bundle with the React Compiler.
Let me walk through this repro to the invariant.
```js
const havoc = __abstract("function", "(() => {})");
global.result = __evaluatePureFunction(() => {
const b = __abstract("boolean", "true");
if (b) {
var g = function f(i) {
return i === 0 ? 0 : f(i - 1) + 1;
};
havoc(g);
} else {
return;
}
return g(5);
});
```
1. Create a function expression that refers to itself inside a construct that wraps the evaluation in `evaluateForEffects()`. (I used an if-statement.)
2. Havoc the function expression. This will add the function’s reference to itself to `modifiedBindings`. However, the original [function expression skipped (3rd arg)](3eb1e8e8ea/src/evaluators/FunctionExpression.js (L108)) adding its binding to `modifiedBindings`.
3. Outside of `evaluateForEffects()` call the function.
4. Observe the invariant firing:
3eb1e8e8ea/src/environment.js (L339)
According to `modifiedBindings`, we only initialize the function expression _after_ its definition. So if the function expression wants to recursively refer to itself, it can’t since the effects we’ve recorded/applied puts us in a state where the binding is not available.
Pull Request resolved: https://github.com/facebook/prepack/pull/2443
Reviewed By: trueadm
Differential Revision: D9447861
Pulled By: calebmer
fbshipit-source-id: de4460bd184012416e80eb65f86dae7ade9dc34e
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:
Fixes#2437.
The code to handle for-statement bailouts was failing on nested for-in/for-out statements since while they had a variable declaration node in their AST the variable declaration behaves very differently. Notably there must be exactly one declaration, the declaration may not have an initializer, and the declaration may only be replaced by a `LeftHandSideExpression` not a expression or statement.
This PR does two things:
1. Replace the variable declarations in for-in/for-of statements with a `LeftHandSideExpression` which will then perform `DestructuringAssignment` with loop values in the evaluator.
2. Recognize that a `LeftHandSideExpression` in a for-in/for-of expression modifies bindings in the serializer residual function visitor.
Pull Request resolved: https://github.com/facebook/prepack/pull/2438
Reviewed By: trueadm
Differential Revision: D9446786
Pulled By: calebmer
fbshipit-source-id: e7f13a3c722602270a87cf8d933de1283b8121d1
Summary:
Fixes#2432.
`evaluateWithoutEffects` sets the realm generator to `undefined`. However, I found a case in #2432 where a generator is required in `evaluateWithoutEffects`.
What happens is that we build an error stack in `evaluateWithoutEffects`.
3eb1e8e8ea/src/realm.js (L1607-L1612)
As a part of building that error stack we try to get `EventEmitter.name`. However, `EventEmitter` is havoced! So we want to create an abstract temporal value `EventEmitter.name`. But creating a temporal value requires a generator since we need to add a generator entry.
3eb1e8e8ea/src/methods/properties.js (L1361-L1367)
This PR solves the issue by adding a throw-away generator to `evaluateWithoutEffects`. Other strategies might be a bit complicated since `evaluateWithoutEffects` is a very uncommon case. Removing `evaluateWithoutEffects` is also not a great option since in this case we would add a temporal `EventEmitter.name` when we never actually use it.
Pull Request resolved: https://github.com/facebook/prepack/pull/2436
Reviewed By: trueadm
Differential Revision: D9446792
Pulled By: calebmer
fbshipit-source-id: 14b36da1fb39dc9ab2a166ecb02d0f062b2bdffe
Summary:
This test bedeviled me for quite some time because I thought it was testing for aliasing effects - it isn't. This is my fault, since I posted this in my repro. Aliasing effects are still to be dealt with in nested optimized functions. Anyhow, simplifying to prevent somebody else from having this confusion.
Pull Request resolved: https://github.com/facebook/prepack/pull/2453
Reviewed By: trueadm
Differential Revision: D9420733
Pulled By: sb98052
fbshipit-source-id: 755ccd90b1e64afdfe6ec627a3306e8baf5952ea
Summary:
Fixes#2447 by adding a separate codepath for when we encounter arrays with widened numeric properties, otherwise we hit an invariant when trying to deal the `length` property (which always remains abstract for arrays with widened numeric properties).
Pull Request resolved: https://github.com/facebook/prepack/pull/2450
Differential Revision: D9402265
Pulled By: trueadm
fbshipit-source-id: 772634bd0a69af23410e65c63b89cbfcc89d8b5b
Summary:
Fixes#2419 and #2386. I ran into this issue while testing an internal React Native bundle. This invariant assumes that all property bindings emitted in a pure scope are also in an optimized function. This is not true for `__evaluatePureFunction` which the React Compiler wraps around initialization code so that globals outside of the closure are not havoced.
Pull Request resolved: https://github.com/facebook/prepack/pull/2420
Differential Revision: D9363247
Pulled By: calebmer
fbshipit-source-id: 4f1634165b6fe15f95b5b7332432fccacc596821
Summary:
Release note: Rewrote the joining logic to always do a full join at every join point
Closes#2151#2222#2279
I've spent a lot of time in the last few months trying to sort out problems that arise from effects being applied too many or too few times. Fixing these feel a bit like playing wack a mole and in the end no fix goes unpunished.
Stepping back a bit from the fray, it seems to me that the root cause of all this pain is the fact that joins of different kinds of completions get delayed.
Before we had path conditions and the simplifier this seemed like a rather good thing since exceptional paths did not contribute values to the normal paths and we thus had fewer abstract values to deal with and fewer places where Prepack would grind to a halt.
In the current state of things, however, it seems perfectly possible to join in all branches at every join point. I've had to decrease some limits, in particular the number of times we go around a loop with conditional exits. I've also had to make the test runner impose a limit on how many times the simplifier can invoke Path.implies.
Nevertheless, the tests seem to pass and hopefully this will also fix quite a lot of bugs that have been unresolved for many months already.
Pull Request resolved: https://github.com/facebook/prepack/pull/2402
Differential Revision: D9236263
Pulled By: hermanventer
fbshipit-source-id: 92a25b591591297afeba536429226c5a0291f451
