Summary:
Release notes: none
This fixes a bug with nested optimized functions referencing nested residual functions and makes our internal bundle compile.
The issue was that we weren't checking if the function we get from `tryGetOptimizedFunctionRoot` was defined inside another optimized function, like we do already 5-6 lines up in the other if statement. This is an important thing, as the next line will result in `undefined` being returned. When `undefined` is returned, we use the `MainGenerator` rather than the `OptimizedFunction` generator, which means all declared values look at the wrong body.
Pull Request resolved: https://github.com/facebook/prepack/pull/2397
Differential Revision: D9223735
Pulled By: trueadm
fbshipit-source-id: 8e1cb1cc1b201b1ae1a804bc61a4bdc8790a3eea
Summary:
Release notes: Progress in support for nested optimized functions
This fixes#2392.
Control flow of nested optimized functions is independent of
their parents. This change reflects that.
Added regression tests.
Pull Request resolved: https://github.com/facebook/prepack/pull/2395
Differential Revision: D9204607
Pulled By: NTillmann
fbshipit-source-id: 1c75ff7493871a4abc3c36dc63f00663c5f6e31b
Summary:
Release Notes: None
When optimizing a function fails, we can usually continue to emit a correct program while ignoring that call to `__optimize`, thus most of these errors should be `Warning`s and not `FatalError`s.
A few times (like when we have multiple sets of `Effects` for one optimized function), we can sensibly continue Prepacking code, but our output may be incorrect. In these cases, we can issue a `RecoverableError` instead of a `FatalError`
Pull Request resolved: https://github.com/facebook/prepack/pull/2371
Differential Revision: D9186240
Pulled By: cblappert
fbshipit-source-id: c30fc63fb713a3bf504cc1ec4f02410a8566cc42
Summary:
When we create a joined descriptor in an abstract object we need to preserve any attributes of that property that were already there. To do that, we just need to read the first descriptor of the elements since all elements must have the same attributes (or we fatal).
Pull Request resolved: https://github.com/facebook/prepack/pull/2390
Differential Revision: D9193129
Pulled By: sebmarkbage
fbshipit-source-id: 32db3e88ae65de9a09ff6de72ed5b91ec02cf090
Summary:
Release notes: Fixing source map support.
I kept getting seemingly garbage source locations in error messages, and looked into that.
I found various issues:
- We in-place update positions, but they are actually shared between locations, and thus we may revisit positions. When we do, we map them again, and so on...
- Locations are also shared between nodes, so we kept revisiting and rewriting yet again.
- The actual mapping doesn't pay attention to the filename, so we apply the wrong mapping altogether, especially in the presence of multiple input files. I am not fixing this, but added a TODO, and opened #2353.
- Another remaining but not blocking issue is that something goes wrong with end-positions: They are sometimes mapped to some seemingly random position in the right line. If anyone knows anything about this, please let me know...
Pull Request resolved: https://github.com/facebook/prepack/pull/2356
Differential Revision: D9141809
Pulled By: NTillmann
fbshipit-source-id: d765e99706d69e3c792fba4c553d4110963067eb
Summary:
Attempting to fix#1547.
**Summary**:
I started work with supporting the `return` keyword in switch statements. I came to the method of "pushing" the return up through the `switch` statement by throwing the `ReturnCompletion`.
Next, I worked on the code snippet provided in #1547, which required supporting `ForkedAbruptCompletion`. I again came to the conclusion of just throwing the `Completion`, but I am not positive this is the correct way. Any guidance here would be appreciated.
Then it was adding support for `ThrowCompletion`, which ended up being the same method.
Since `continue` can only be used within a loop, I made a small test using static variables all known at optimization time, and prepack correctly evaluated the function with no additional changes necessary. However, using a variable not known at optimization time yielded an error with tag PP0037.
*Note*: I left the `if (r instanceof PossiblyNormalCompletion) {...}` throw untouched in-case I am missing what some of the possible completion types are that might trigger this (other than the ones I have touched).
**Tests**:
1. Simple Return
```javascript
function f(x, c) {
switch (x) {
default: return 42;
}
}
```
```javascript
var _0 = function (x, c) {
return 42;
};
```
2. Cases Return
```javascript
function f(x, c) {
switch (x) {
case 0: return 12;
case 1: return 24;
default: return 42;
}
}
```
```javascript
var _0 = function (x, c) {
var _9 = 0 === x;
var _6 = 1 === x;
var _3 = _6 ? 24 : 42;
var _1 = _9 ? 12 : _3;
return _1;
};
```
3. Task Snippet
```javascript
(function () {
function f(x, c) {
switch (x) {
case 0: if (c) return 42; else return 99;
case 1: return 23;
}
}
__optimize(f);
global.inspect = function() { return f(0, 1); }
})();
```
```javascript
(function () {
var _$0 = this;
var _1 = function (x, c) {
var _D = 0 === x;
var _3 = c ? 42 : 99;
var _A = 1 === x;
var _7 = _A ? 23 : void 0;
var _2 = _D ? _3 : _7;
return _2;
};
var _0 = function () {
return _1(0, 1);
};
_$0.inspect = _0;
}).call(this);
```
4. Throw Support
```javascript
function f(x, c) {
switch (x) {
case 0: throw 12;
case 1: throw 24;
default: throw 42;
}
}
```
```javascript
var _0 = function (x, c) {
var _1 = 0 === x;
if (_1) {
throw 12;
} else {
var _5 = 1 === x;
if (_5) {
throw 24;
} else {
throw 42;
}
}
};
```
5. Continue Support (optimization-time known variables)
```javascript
function f() {
let counter = 0;
for (let i = 0; i < 5; i++) {
switch (i) {
case 0: counter++; break;
case 1: counter += 2; break;
case 2: counter += 3; break;
case 3: continue;
default: return counter;
}
}
}
```
```javascript
var _0 = function () {
return 6;
};
```
6. Continue Support (unknown variable):
```javascript
function f(max) {
let counter = 0;
for (let i = 0; i < max; i++) {
switch (i) {
case 0: counter++; break;
case 1: counter += 2; break;
case 2: counter += 3; break;
case 3: continue;
default: return counter;
}
}
}
```
```
In stdin(3:23) FatalError PP0037: failed to recover from a for/while loop bail-out due to unsupported logic in loop body (https://github.com/facebook/prepack/wiki/PP0037)
Prepack failed, reporting 1 fatal error.
```
Pull Request resolved: https://github.com/facebook/prepack/pull/2309
Reviewed By: hermanventer
Differential Revision: D9169333
Pulled By: zjijz
fbshipit-source-id: 3a9b2738679fcd6dac8fff9881b7464c85243723
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:
Resolves#2186. Embeds the value `__empty` in array and object literals for properties that are conditionally set, instead of handling such values via assignments and deletes. Raises an exception if there is a cycle in object or array creation (InstantRender does not support these).
Pull Request resolved: https://github.com/facebook/prepack/pull/2364
Differential Revision: D9169335
Pulled By: sb98052
fbshipit-source-id: f83d85677b30f10f3c548349e93ce792fc6c1ca0
Summary:
Release notes: none
This is a follow up PR that addresses the post-land comments made in the PR #2357 from sebmarkbage.
Pull Request resolved: https://github.com/facebook/prepack/pull/2381
Differential Revision: D9160458
Pulled By: trueadm
fbshipit-source-id: d4b4fd98d88c0d91d4b4a4f14caf5179d68eff91
Summary:
Release Notes: None
We weren't properly adding leaked bindings to the optimized function's FunctionInstance, so state didn't get cleaned up properly between passes of the serializer.
There was also a bug in emitter invariants. Loosening the invariant causes the test cases to pass.
Also the invariant seems to have been incorrect in the first place. I suspect it is just the direct negation of [this line](393624f00f/src/serializer/Emitter.js (L103)).
Added slightly minimized repros from the issue.
I am not quite sure what we are trying to test with the last line of this invariant. I think what we're trying to test with the last line of this invariant is: If we're emitting to an additional function, it must be the case that referencedDeclaredValues has `val`. Here the test also passes when `referencedDeclaredValues.has(val)` is `false` which seems wrong to me.
I'm also dubious that top level optimized functions don't consider the MainGenerator as their parent generator, as this means that any derived ids declared in the MainGenerator shouldn't be considered declared in optimized functions (was able to repro that in conditions2 test after tightening invariants). For the sake of unblocking React Compiler, I'll look into that in a future PR.
Solves #2359.
Pull Request resolved: https://github.com/facebook/prepack/pull/2382
Differential Revision: D9158973
Pulled By: cblappert
fbshipit-source-id: a6915d0f125708739022d8576d5f007107e76b6b
Summary:
Release Notes:
- Added CLI flag to specify path to prepack. This was the solution suggested by D. Aurelio r/e getting path to zipped version of prepack on sc machines.
Pull Request resolved: https://github.com/facebook/prepack/pull/2347
Reviewed By: yinghuitan
Differential Revision: D9133562
Pulled By: caiismyname
fbshipit-source-id: d1338607efd1ed9b7002704de02b38d8b2ad407e
Summary:
Release notes: Reduced memory usage of Prepack
The visitor computes some data structures that live on,
but it also maintains some internal temporary state that
should be released once the visitor is done.
This factors out the resulting data from the stateful visitor.
As a side-effect, we also need to pass less redundant stuff
through various levels of constructors.
Pull Request resolved: https://github.com/facebook/prepack/pull/2379
Differential Revision: D9150907
Pulled By: NTillmann
fbshipit-source-id: 8556a111eae67c7e6032d148e568c12b644b4e69
Summary:
Release notes: Reduce memory usage of running Prepack by 3% in some scenarios
This fixes#2365.
This is realized via a stateful SourceFileCollection.
On a small internal benchmark, this saved around 3% of total node memory usage
(the parsed AST that we keep around uses an order of magnitude more memory
than the original source file and source map,
11MB of source files + source map vs 88MB of parsed AST).
Pull Request resolved: https://github.com/facebook/prepack/pull/2377
Differential Revision: D9150888
Pulled By: NTillmann
fbshipit-source-id: b38a8176c4f1e4633366bd48b7d396aec023e7c3
Summary:
Release notes: Fixing source map support with multiple source files
This fixes issue #2353: We now track multiple --srcmapIn arguments,
and match which one applies by comparing the basenames:
The convention is that sourcemaps share the same basename with an appended .map.
Pull Request resolved: https://github.com/facebook/prepack/pull/2362
Differential Revision: D9138802
Pulled By: NTillmann
fbshipit-source-id: d359ccd372b7d87445ecc1a2bdb509ba158e0200
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:
Release notes: none
This PR removes the `makeNotPartial` method and all its call-sites in favour of explicitly telling execution paths that we want own properties even if partial. This stops the mutation of objects where we set the partial boolean flag, which actually is a very common cause of side-effects in pure functions. Furthermore, the whole `makeNotPartial` before and after was potentially error-prone if the `try/catch` was omitted by mistake and it was generally a code smell.
I also made it test-runner log the failing test name upon error, which helps with debugging.
Pull Request resolved: https://github.com/facebook/prepack/pull/2357
Differential Revision: D9137606
Pulled By: trueadm
fbshipit-source-id: c0c59615f7a7d46836d26a3b2aecfb89274933d8
Summary:
Release notes: None
- Make formatting of error locations more useful.
- Factored out redundant code that initialize ECMAScriptSource functions
(and as part of that possibly a small improvement in locality: record
`uniqueOrderedTag` only the first time a function is encountered,
not effectively the last time)_
- Made PP1003 recoverable (not sure what's fatal about it)
- Deleted dead `functionExpressions` property
Pull Request resolved: https://github.com/facebook/prepack/pull/2352
Differential Revision: D9136199
Pulled By: NTillmann
fbshipit-source-id: 279efcc97310ccdb11907b74f4f166851b816819
Summary:
Release Notes: None
Fixes#2266.
Looking at the `__optimize` code, what we want is for every call to `__optimize` to optimize that function, even in conditonal contexts. In this case, it makes no sense to store the functions to optimize in a special global because we don't care about the applying/reverting of effects. It simplifies the code significantly to be able to store these values in the realm instead.
The only case where this would matter is if effects containing an __optimize call get created then discarded (I am unsure if there are any legitimate cases of this in Prepack at the moment, but I created a CompilerDiagnostic just in case).
Pull Request resolved: https://github.com/facebook/prepack/pull/2303
Differential Revision: D9134968
Pulled By: cblappert
fbshipit-source-id: 040f7ccc24f928e6b8daa068521d3848caffc4d2
Summary:
Release Notes: None
Now bindingAssignments will have their containing optimized function visit them.
Fixes#2335 and test case 3 of #2252
Test output currently fails lint, once #2339 lands and I rebase past it, that'll go away.
Pull Request resolved: https://github.com/facebook/prepack/pull/2344
Reviewed By: trueadm
Differential Revision: D9133987
Pulled By: cblappert
fbshipit-source-id: 243fc56a6ff7552c801762e2668ea5e4607599fd
Summary:
Release notes: None
- Remove some dead code from test runner.
- Simplify test runner by removing option that's only used once for no good reason.
Pull Request resolved: https://github.com/facebook/prepack/pull/2350
Differential Revision: D9125266
Pulled By: NTillmann
fbshipit-source-id: 04eb9c17d8a3e7b2f93c246652691759e8a797c8
Summary:
I extended the `--serializer` command line argument I added in #2290 to now support `--serializer abstract-scalar`. What this mode does is it converts all boolean, string, number, and symbol literals into abstract values. I did not choose to extend this logic to object and array literals just yet since scalars alone showed some interesting results.
What I really want here is a review of the results.
Full suite execution results are real bad. **18%** pass rate. I dug a bit into why.
```
=== RESULTS ===
Passes: 3356 / 17780 (18%)
ES5 passes: 2276 / 12045 (18%)
ES6 passes: 1080 / 5735 (18%)
Skipped: 13375
Timeouts: 28
```
I was mostly interested in the runtime failures we see since that means Prepack is serializing invalid code. However, I found ~14k failures in the Prepack stage (more on this in a bit) and ~3k failures in the runtime stage. This means ~80% of tests _fail to compile_ with this abstract transformation applied.
Why are these tests failing? I took the first 4 items of the stack traces from errors thrown in the Prepack stage, sorted, and ranked them. [Here’s the result.](https://gist.github.com/calebmer/29e27613325fd99fa04be7ab4a9641c0) The top 5 with thousands of hits are:
```
7538 of:
at AbstractValue.throwIfNotConcrete (/Users/calebmer/prepack/src/values/AbstractValue.js:536:11)
at ToImplementation.ToStringPartial (/Users/calebmer/prepack/src/methods/to.js:717:69)
at NativeFunctionValue._index.NativeFunctionValue [as callback] (/Users/calebmer/prepack/src/intrinsics/ecma262/String.js:34:37)
at NativeFunctionValue.callCallback (/Users/calebmer/prepack/src/values/NativeFunctionValue.js:121:12)
4595 of:
at AbstractValue.throwIfNotConcrete (/Users/calebmer/prepack/src/values/AbstractValue.js:536:11)
at NativeFunctionValue.func.defineNativeMethod [as callback] (/Users/calebmer/prepack/src/intrinsics/ecma262/Object.js:328:41)
at NativeFunctionValue.callCallback (/Users/calebmer/prepack/src/values/NativeFunctionValue.js:121:12)
at functionCall (/Users/calebmer/prepack/src/methods/call.js:308:26)
1454 of:
at AbstractValue.throwIfNotConcrete (/Users/calebmer/prepack/src/values/AbstractValue.js:536:11)
at NativeFunctionValue.func.defineNativeMethod [as callback] (/Users/calebmer/prepack/src/intrinsics/ecma262/Object.js:364:41)
at NativeFunctionValue.callCallback (/Users/calebmer/prepack/src/values/NativeFunctionValue.js:121:12)
at functionCall (/Users/calebmer/prepack/src/methods/call.js:308:26)
1351 of:
at invariant (/Users/calebmer/prepack/src/invariant.js:18:15)
at EvalPropertyNamePartial (/Users/calebmer/prepack/src/evaluators/ObjectExpression.js:59:7)
at _default (/Users/calebmer/prepack/src/evaluators/ObjectExpression.js:80:21)
at LexicalEnvironment.evaluateAbstract (/Users/calebmer/prepack/src/environment.js:1368:20)
1053 of:
at AbstractValue.throwIfNotConcrete (/Users/calebmer/prepack/src/values/AbstractValue.js:536:11)
at NativeFunctionValue.obj.defineNativeMethod [as callback] (/Users/calebmer/prepack/src/intrinsics/ecma262/ObjectPrototype.js:35:39)
at NativeFunctionValue.callCallback (/Users/calebmer/prepack/src/values/NativeFunctionValue.js:121:12)
at functionCall (/Users/calebmer/prepack/src/methods/call.js:308:26)
```
This means there may be some low hanging fruit.
Here are my questions for you.
- Did you expect results like this?
- What is our ideal test262 pass rate with this transformation applied?
- What happens to React Compiler or other projects when these errors are thrown? (As I understand it, we bail out and don’t optimize the code, but do optimize the code around it.)
- Do you think my methodology is flawed?
It’s also possible that something in my methodology is wrong, but I didn’t spend much time investigating these failures as I spent investigating the failures I found in #2290.
My goal with this test suite is to build an understanding of what “correctness” for the React Compiler against all JavaScript code looks like. (Not just the few bundles we’ve selected to look at.) I don’t think these results suggest that we only safely compile 18% of the language, but it’s a data point. I’ll be looking into fixing a selection of these issues to better understand their nature or if I need to change methodologies.
Pull Request resolved: https://github.com/facebook/prepack/pull/2297
Differential Revision: D9120572
Pulled By: calebmer
fbshipit-source-id: b394f1e8da034c9985366010e3e63fd55fd94168
Summary:
Release notes: none
This gives a unique kind to the abstract value created by the React elements logic. Otherwise we hit this invariant, https://github.com/facebook/prepack/blob/master/src/utils/havoc.js#L493-L500 with our internal bundle. The abstract value in this case should have always had a kind given it has no args, values or types.
Pull Request resolved: https://github.com/facebook/prepack/pull/2349
Differential Revision: D9117711
Pulled By: trueadm
fbshipit-source-id: 1e41042f4747e5b990474d3356bf9627cde76374
Summary:
- Bugfixes, refactors
- extended use of simplification to prepack more code
- improved shape modeling
- Added ability to materialize objects without havocing
- new `--reproUnconditionally` and `--reproOnFatal` options in Prepack
- creates zip file with all files needed to debug the failure
- creates `repro.sh` script that can open Nuclide debugger, pre-populating the original arguments causing the failure
Reviewed By: NTillmann
Differential Revision: D9101750
fbshipit-source-id: c7a2a7fc1c7814f02937f786df01ec3cb8effe4e
Summary:
This is part of a larger set of features for the object model to fully model the Set/Get phases for partial and abstract objects. I wanted to break it down into smaller steps.
This is just a plain move of GetPartial and SetPartial to get.js/properties.js. All other property modeling for concrete keys are already there. You can tell that a lot of imports and logic becomes duplicated because they do similar things.
An alternative could be to move all the logic into ObjectValue but that's not how the spec is structured.
This also causes trouble for me trying to generalize these operations for abstract object values which may not consult the ObjectValue's virtual dispatch.
Pull Request resolved: https://github.com/facebook/prepack/pull/2345
Differential Revision: D9105679
Pulled By: sebmarkbage
fbshipit-source-id: 9d77d633eb238e4dd46e2ad43485fc885e85152c
Summary:
Release notes: unknown abstract arrays with numeric properties now have their map methods auto optimize as nested optimized functions
This PR introduces automatic collection of nested optimized functions for abstract array methods (for now just `Array.prototype.map` and `Array.from`). It also re-works part of the React nested optimized function logic for unknown arrays so that the same approach is used for both optimized functions and React component trees. If the nested optimized function has side-effects when evaluating it (i.e. it's not a "pure function") then we don't try and treat it as an nested optimized function and fallback to havocing the function as we did before.
In the interests of keeping this PR small: **this PR is one of many** to fully add support, support edge-cases and remove legacy implementations. I plan on following up to this PR with another that gets rid of a lot of duplicate logic in the React reconciler to do with nested optimized functions and unify it to a single place.
Pull Request resolved: https://github.com/facebook/prepack/pull/2346
Differential Revision: D9109608
Pulled By: trueadm
fbshipit-source-id: 77e7fcd5f514caf14607a9a48bb8ff149fe221b2
Summary:
Release notes: none
In some places we have type information but don't fully use the benefits it brings and instead go down a non-optimal path. This PR addresses two fairly simple cases where this occurs:
- when we have a method call on an abstract value that has a primitive type, where the name of the method matches that of one of the methods on the primitive prototype, then use that method.
- when we have a type of "array", use all the logic for unknown abstract arrays rather than generate an abstract value with a type of array (which has no optimized paths, unlike unknown arrays).
- adds temporals for `toString` and other built-ins because of existing logic around joining them and the fact that they can throw if called on `undefined`, it makes them unsafe. Previous we rarely went down these code paths to create template abstracts, because we guarded in pure scope for abstract base values in `CallExpression`.
Pull Request resolved: https://github.com/facebook/prepack/pull/2321
Differential Revision: D9081455
Pulled By: trueadm
fbshipit-source-id: 5be2ec87eb582d50f7b4f72ef08debee8977b0e2
Summary:
Release Notes: None
The original issue here was that `nested` is defined inside of `fn2` which is a non-optimized function called by `fn` (an optimized function). That caused Prepack to not detect that `nested` was nested in `fn2`.
The fix is to use `CreatedObjects` to test for nesting instead of the environment lookup. The environment lookup fails because `nested` is evaluated with `fn2`'s effects applied but _not in `fn2`'s environment_.
This PR also adds a command I use frequently to test a single failing `test-runner` test as well as a way to skip lint because some tests can't pass lint.
Addresses the first test case of #2337.
Pull Request resolved: https://github.com/facebook/prepack/pull/2339
Differential Revision: D9074916
Pulled By: cblappert
fbshipit-source-id: 720003b965d9a9a6842d512ea41cd6402361342e
Summary:
Release notes: none
I plan on re-using the same logic in `_callOfFunction` for nested optimized array method functions + some React stuff, so this PR makes the logic re-usable.
Pull Request resolved: https://github.com/facebook/prepack/pull/2341
Differential Revision: D9061066
Pulled By: trueadm
fbshipit-source-id: cc5c3efd579f4039eb45b14558ca434775a724d4
Summary:
Release notes: none
Changes invariant to accept SymbolValue as well as strings. I found to be hitting this invariant in the internal bundle. I'll put up a regression test next week as this is a tricky one to repro it seems.
Pull Request resolved: https://github.com/facebook/prepack/pull/2333
Differential Revision: D9046722
Pulled By: trueadm
fbshipit-source-id: 1fc43f17c8e4e637aeaa3156319b6ae4995c7dad
Summary:
This PR consists of several changes to fix and improve shape modeling:
- Made all types exported to be use them in type-checking for InstantRender model generation
- Removed type information from link node (fix)
- Changed type name from `ShapeDescriptorOfPrimitive` to `ShapeDescriptorOfScalar` to make it clear
- Refined semantics of universe by allowing unknown (`undefined`) shapes in universe.
Pull Request resolved: https://github.com/facebook/prepack/pull/2334
Differential Revision: D9046663
Pulled By: hotsnr
fbshipit-source-id: fa197994bba3ccf7eb6b5d39b24861831c25089c
Summary:
This is Part 2/4 of #2185, which is being broken out into focused pieces. This piece separates materialization from the leaking and havocing logic, deals with flow issues and avoids increasing the maximum flow cycle size by adding the materialization routine to singletons.js.
But it does not introduce the possibly problematic pairing of ObjectValues with the generators in which they were leaked. Objects are materialized in the current generator, just as before.
trueadm needs this for #2321. I suggest he review it before NTillmann and hermanventer to check that it meets his needs.
Pull Request resolved: https://github.com/facebook/prepack/pull/2325
Differential Revision: D9039487
Pulled By: sb98052
fbshipit-source-id: d3c83aa6d8ef0939e8157ca19bd31265dd05e9c4
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:
In case the havoced object has a setter on it.
This is fixes this particular bug but it's also useful to be able to pass the correct receiver to properties.js rather than unwrapping it like we do now. This will be evident in a follow up PR.
This also lets emitPropertyAssignment deal with abstract values which is a common pattern and will become more common with widened objects.
Pull Request resolved: https://github.com/facebook/prepack/pull/2338
Differential Revision: D9035746
Pulled By: sebmarkbage
fbshipit-source-id: 2abb1a3eb047de1739dec94259a803c4c45e416d
Summary:
Coercing a symbol to a number or primitive implicitly always throws an error. We generated fatal compiler errors. Instead, this should just be treated as a throw.
This is neat because that means other serializers doesn't have to deal with the expressions involving symbols.
Pull Request resolved: https://github.com/facebook/prepack/pull/2300
Differential Revision: D9035734
Pulled By: sebmarkbage
fbshipit-source-id: 60c0d089e63b7b36b6757dab0c9928c19514c5d6
Summary:
Follow up to #2219
This also includes two relevant bonus fixes: 2aa1ad35a4 and e96ac8c0c1
Pull Request resolved: https://github.com/facebook/prepack/pull/2329
Differential Revision: D9033950
Pulled By: sebmarkbage
fbshipit-source-id: b28b608b7449f0bbd7781c7d6aab5717d638d904
Summary:
This solves the remaining issues in #2300.
If a binary expression can't be resolved as pure (such as when applied to values of two different types), we can instead try conditions separately and join the effects.
If the left value is conditional, we try each possible value separately until we hit a non-conditional value. Then we try any conditional right values.
Pull Request resolved: https://github.com/facebook/prepack/pull/2332
Differential Revision: D9032487
Pulled By: sebmarkbage
fbshipit-source-id: 0f89a680ad0b710e90c1dd7a4c127c2247527c99
Summary:
Release Notes:
- Created DebugReproManager to capture all sourcefiles touched by Prepack (to include minimal subset of useful sourcefiles in the debug package)
- `--repro` splits into two: `--reproUnconditionally` which will create a debug package _regardless_ of Prepack's success/failure, and `--reproOnFatal`, which will _only_ create a debug package if Prepack outputs a `FatalError`.
- The debug package now includes all relevant sourcefiles (except for node modules), a copy of the version of Prepack (lib) that was used when the package was created, and a script to `yarn install` the relevant modules for the included version of Prepack, then start the Nuclide Prepack debugger with the proper parameters for files in the debug package (including original prepack arguments). This is in addition to the original input files.
- The impact of having the `DebugReproManager` on in `--reproOnFatal` mode all the time is negligible, as show in the table below. This flag will be always be enabled on Sandcastle builds so that failures are more easily debugged.
- The time difference between no repro flag and `--reproOnFatal` seems like it can be written off as simple variance between runs. The larger increase when actually creating the zip comes from reading and zipping the files, which takes time proportional how many files are touched.
- SourceMapManager was refactored to not use `Invariant` or `SourceFile`s. This is so that `DebugReproManager` import it without increasing the flow cycle, and allows the `DebugReproManager` to be passed from Prepack to the CLI to create the repro package.
- The repro option introduces a potential for a subtle race condition that is addressed as follows:
- The last `if (!success && reproMode === "none") process.exit(1);` must check reproMode because `generateDebugRepro` involves an async process (directory zipping). If there is an ongoing repro and the process exits, the repro may terminate prematurely, causing no repro to be generated. Instead, this only triggers if there is no repro -- if there is, the `generateDebugRepro` function will handle process exiting if it needs to.
Usage:
```node [prepack] [files to prepack] --reproOnFatal /Absolute/path/to/repro/bundle.zip --debugBuckRoot /buck/root```
or
```node [prepack] [files to prepack] --reproUnconditionally /Absolute/path/to/repro/bundle.zip --debugBuckRoot /buck/root```
Demo: https://www.dropbox.com/s/p62ves2p55fyyl7/--repro%20annotated%20demo.mp4?dl=0
Pull Request resolved: https://github.com/facebook/prepack/pull/2289
Differential Revision: D9002841
Pulled By: caiismyname
fbshipit-source-id: 623b362f963095f1cd8163684fd6e76596e7c4fc
Summary:
Previously, I created two props, `accessibilityRole` and `accessibilityStates` for view. These props were intended to be a cross-platform solution to replace `accessibilityComponentType` on Android and `accessibilityTraits` on iOS.
In this stack, I ran a code mod to replace instances of the two old properties used in our codebase with the new ones.
For this diff, I did a search for all the remnant uses of `accessibilityComponentType` that was not caught by my script, and I manually changed them to `accessibilityRole` and `accessibilityStates`. If the same prop also set `accessibilityTraits` I also removed that here because the two new props works on both platforms.
It was difficult to write a script for this, because most of them were contextual changes.
Out of the contextual changes, most of them followed one of these two patterns:
Before:
```
const accessibilityComponentType = 'button';
const accessibilityTraits = ['button'];
if (this.props.checked) {
accessibilityTraits.push('selected');
}
if (this.props.disabled) {
accessibilityTraits.push('disabled');
}
contentView = (
<AdsManagerTouchableHighlight
accessibilityComponentType={accessibilityComponentType}
accessibilityTraits={accessibilityTraits}
```
After:
const accessibilityRole = 'button';
const accessibilityStates = [];
if (this.props.checked) {
accessibilityStates.push('selected');
}
if (this.props.disabled) {
accessibilityStates.push('disabled');
}
contentView = (
<AdsManagerTouchableHighlight
accessibilityRole={accessibilityRole}
accessibilityStates={accessibilityStates}
Before:
```
<PressableBackground
accessible={this.props.accessible}
accessibilityLabel={this.props.accessibilityLabel}
accessibilityTraits={this.props.accessibilityTraits}
```
After:
```
<PressableBackground
accessible={this.props.accessible}
accessibilityLabel={this.props.accessibilityLabel}
accessibilityRole={this.props.accessibilityRole}
accessibilityRole={this.props.accessibilityStates}
```
In addition to changing the props on the components,
Another fix I had to do was to add props accessibilityRole and accessibilityStates to components that don't directly inherit properties from view including text input and touchables.
Reviewed By: PeteTheHeat
Differential Revision: D8943499
fbshipit-source-id: fbb40a5e5f5d630b0fe56a009ff24635d4c8cc93
Summary:
* Enhanced dead code elimination for optimized functions
* Much of the buildNode and inline Babel logic has been moved to a dedicated ResidualOperationSerializer class
* Provide a way to temporarily disable effects tracking
* Simplified forked completion constructors
* React components can have their props modelled via `__optimizeReactComponentTree`
Reviewed By: simonhj
Differential Revision: D9004189
fbshipit-source-id: 80936cbc66ad9dc350bbf0dcf0f1ff228a147f43
Summary:
This pull request breaks out the first part of #2185, which deduplicates object creation when leaking is unconditional to begin with. No changes are made to how leaking works. Only to how leak information is used by the serializer and visitor.
Pull Request resolved: https://github.com/facebook/prepack/pull/2302
Differential Revision: D8997109
Pulled By: sb98052
fbshipit-source-id: e0ea2e7ea574a9f2be71b70be289831c7f797d9d
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:
Previous error message:
`Property access conflicts with write in optimized function (unknown function) at 11:11 to 11:16`
New error message:
`Write to property "func" on <unnamed object> at optimized function fn[20:13 22:22] conflicts with access in function nested[11:11]`
Output generated off "Failure 2" in issue #2252
Pull Request resolved: https://github.com/facebook/prepack/pull/2305
Differential Revision: D8979410
Pulled By: cblappert
fbshipit-source-id: a4f507d8a77a44a0f4ca7619faf3a9fe03dabf45
