Summary:
Fixes#2151#2222#2279#2393#2399#2404#2411#2414#2415
Added a fuzz testing tool
Added test cases
Turn crash in JSON.stringify into a diagnostic
Adds a `arrayNestedOptimizedFunctionsEnabled` flag to enable nested optimized functions derived from Array.prototype methods (like `map`) and Array.from
Refactor assignment on partial or possibly deleted property
Rewrote the joining logic to always do a full join at every join point
Removed last remnants of delayUnsupportedRequires
Reviewed By: cblappert
Differential Revision: D9349841
fbshipit-source-id: 74a16dbb015777d59d23fdfde77abbe2489c292a
Summary:
Release notes: None
Did a little bit of prettying up the code in _getTarget,
renaming additional to optimized, and added a TODO #2426
annotation where issue #2426 happens in the code.
Pull Request resolved: https://github.com/facebook/prepack/pull/2427
Differential Revision: D9350285
Pulled By: NTillmann
fbshipit-source-id: 56f1c7ab1a62ca39091d06c91c0700d0cad447ad
Summary: The require method is now __r.
Reviewed By: NTillmann
Differential Revision: D9329676
fbshipit-source-id: 0c3385d593abcf93c5bb3116273b1e128b9e2950
Summary:
Release notes: None
Every native function value needs an intrinsic name,
otherwise the serializer will fail with an invariant violation.
This gives an intrinsic "name" to `ThrowTypeError`
that meets the requirements of the ECMAScript spec.
Pull Request resolved: https://github.com/facebook/prepack/pull/2424
Differential Revision: D9344248
Pulled By: NTillmann
fbshipit-source-id: c0e00652c50a555e7c43009d1394c969fbfff9b1
Summary:
The previous changes on "Do full joins" didn't remove all
uses of the delayUnsupportedRequires feature. This does it.
In particular, this fixes the internal prepack-test.
Reviewed By: hermanventer
Differential Revision: D9287509
fbshipit-source-id: f810f5e08a10cbd14272d776799a2a8f1442d1c8
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
Summary:
The goal of this refactoring is to move more of the special case logic around properties as deeply as possible so that when we expand the object model, we don't have to add special cases through all the indirections. This will be clear in a follow up where I try to model the multiple identities.
This reverts the fix in #1183. That fix is no longer needed for its original purpose because those getters are now modeled as pure and the unnecessary accesses gets eliminated by the serializer.
I also remove the `Receiver.$Delete(P)` hack in `OrdinarySet` when a property might have been deleted. I'll need that because this hack doesn't work on objects that only allow weak deletions (such as object set templates). Instead, I model possibly deleted properties in ValidateAndApplyPropertyDescriptor.
It turns out that much of it doesn't matter if it has been deleted or not since you can't delete a non-configurable property.
The React environment calls Set/DefineOwnProperty on partials during initialization. These must read to get the current descriptor. However, a temporal read of the actual value can't be emitted at this point. So I emit a placeholder value that shouldn't be serialized. This is something the environment initialization must already consider. We can't have it read partial values before there is an environment to consider.
Pull Request resolved: https://github.com/facebook/prepack/pull/2389
Differential Revision: D9274895
Pulled By: sebmarkbage
fbshipit-source-id: b8e8aee697121552243f7a67b7c19c62768f4729
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
During React reconciliation we wrongly reset `pathConditions`. This was done originally because of a pushing false invariant, that I incorrectly thought was a bug because of React specific code.
However, today, I re-worked the original test code to remove all React logic and I found that the invariant occurs even without React specific logic. This strongly suggests that this is still an existing bug in Prepack and adding `pathConditions` logic, which gets around it, was the wrong strategy and should be reverted.
Pull Request resolved: https://github.com/facebook/prepack/pull/2415
Differential Revision: D9264730
Pulled By: trueadm
fbshipit-source-id: 2d3703e638c894eb0929b9a30a7d74ccbd998ea3
Summary:
Release notes: none
This adds a check in the React utils component function caller to ensure that the `createObjects` are correct at evaluation time.
Pull Request resolved: https://github.com/facebook/prepack/pull/2414
Differential Revision: D9264727
Pulled By: trueadm
fbshipit-source-id: 6435bf1a4ccfaf98278377ad48147a557c1608d4
Summary:
Release Notes: None
Previous PRs fixed these issues, but didn't necessarily add the test case from the issue. This PR adds the exact test cases from the issues.
Closes#2358Closes#2398
Pull Request resolved: https://github.com/facebook/prepack/pull/2410
Differential Revision: D9259118
Pulled By: cblappert
fbshipit-source-id: 3eb347044865a11e2ce5262b2338b7b6e1958c60
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:
This PR adds the fuzzer I’ve been working on to the Prepack codebase so other people can contribute, provide feedback, and run it against their changes. The new commands added are:
- `yarn fuzz`: Starts generating tests in the foreground and logs progress. If it finds an error it will try and shrink it before returning the shrunken program to you with the invalid results.
- `yarn fuzz-sample`: See a selection of the programs generated by the fuzzer.
- `yarn fuzz-overnight`: Spin up a worker for each CPU and try to find failing test cases. Your computer will be basically unusable while you run this, so leave it running overnight. Failed test cases will be saved in `fuzzer/overnight.sqlite` so in the morning you can use `sqlite3` to inspect the errors the fuzzer found.
The fuzzer generates programs with an optimized function and executes them twice:
1. In Node.js
2. In Node.js after running Prepack
Then compares the results. If the results are different then the fuzzer will attempt to shrink the program to something easier to debug and return this to you. See [this gist for a sample of generated programs](https://gist.github.com/calebmer/6a727c1f4aa8c08d51940e60d29d336a). Here’s an example of a function you might see:
```js
function f3(a1, a2, a3, a4, a5, a6) {
2;
var x2;
if (0) {
return a1 ? false : a2;
} else {
var x1 = a3;
x2 = x1;
}
var x6;
if (x2) {
var x3;
if (x2) {
x3 = x2;
} else {
x3 = a4;
}
var x4 = x3;
x6 = x4;
} else {
var x5;
if (a5) {
x5 = x2;
} else {
x5 = a6;
}
x6 = f2(x5);
}
return x6;
}
```
So far I’ve reported four bugs reduced from test cases found by this version of the fuzzer. I’ve reported a couple more from old fuzzers I used, but these four from the current version. The shrinking process is not that good and it takes a while as the generated program can get large, so you’ll usually have to do some manual shrinking to get good bug reports. I only ran `yarn fuzz-overnight` for about an hour. It found 28 failures and I reduced those down to these 4.
- #2354
- #2355
- #2361
- #2363
I expect I’ll find more bugs as these get fixed and I add more JavaScript features to the fuzzer. The features I currently have are:
- Scalar primitives (number, string, boolean, null/undefined)
- Functions
- Conditional expressions
- If statements
- Variable declarations
Not too many features, but enough to catch a handful of bugs.
> **Note:** If this PR is too much to review, I’ve created [`calebmer/prepack-fuzzer`](https://github.com/calebmer/prepack-fuzzer) in which my work is broken up into commits as I made them. I then copied these files over to the Prepack repo.
The fuzzer in this PR is a rewrite from the [fuzzer I started with](https://gist.github.com/calebmer/75dd75ebe556681d3a628e75eaffc403). The main lessons I learned from that one are that I should start with a general JS fuzzer instead of a React fuzzer (since adding JS features after the fact to fuzzer designed for React is difficult) and that all nested structures need to be generated with one recursive generator from the generator library I’m using.
To generate programs I use [`leebyron/testcheck`](https://github.com/leebyron/testcheck-js) which is actually a JS compiled version of the official Clojure library [`clojure/test.check`](https://github.com/clojure/test.check). `testcheck` is designed for generative property testing at a much smaller scale then program fuzzing. So I am abusing the library a bit to use it as a fuzzer. My reasoning is that I wanted to write the fuzzer in JS (to have access to the Babel AST) and I didn’t want to write my own case generating framework. If we outgrow `testcheck` then we can keep the DSL, but rewrite the generation/shrinking logic. Although its been working fine for me so far. (Yet I am using a forked version which simply uses some unpublished features in the `testcheck` repo.)
The generator code in `fuzzer/src/gen.js` may look odd to you. It uses immutable.js and a **state monad** implemented with a JS generator so the code looks imperative. I need state since generating various program components depends on things like “what variables are declared at a given point in time,” but because I’m limited to only using a single recursive generator (based on performance lessons I learned from my first fuzzer) I can’t pass around state at the generator level and must instead maintain state at the result level. At first I tried hacking together some imperative state, but when shrinking programs `testcheck` replays some generators to get new programs. So what do you do when you need a stateful process that needs to be replayed? You use a monad.
I could try to fix the bugs I found, but I’d like to find more bugs. I need to add back support for React components and I need to add more language features.
_Which JS language features are the most interesting to fuzz?_
I ranked all the kinds of AST nodes in our internal React bundle [and got this](https://gist.github.com/calebmer/be5e2bad4b12af683522096544fc9568). I’ll be starting with that, but the Prepack team has a better intuition around what’s good for fuzzing. I know there’s been a discussion around temporals recently that I haven’t really been following. What would be good ways to trigger this behavior?
Pull Request resolved: https://github.com/facebook/prepack/pull/2374
Differential Revision: D9180836
Pulled By: calebmer
fbshipit-source-id: 59d3fb59ecc1026a865672e2833f7482ed72139a
Summary:
Release Notes: None
Previously, `tryGetOptimizedFunctionRoot` would only get the root successfully if the root was one of the optimized functions or the parent of one of the residual functions. Now we look at the optimized functions as well as any of their parents. Added 2 test cases.
Fixes#2399
Pull Request resolved: https://github.com/facebook/prepack/pull/2401
Differential Revision: D9242153
Pulled By: cblappert
fbshipit-source-id: 9bdffce23d5dd19d9ce5a6b09e1ee9647ca6e5cb
Summary:
Weekly release v0.2.47:
- Many bug fixes
- Progress in support for nested optimized functions
- Fixing source map support
- Improved support for abstract behavior in switch statements
- Reduced memory usage of Prepack
Reviewed By: cblappert
Differential Revision: D9230689
fbshipit-source-id: 2094183fe4183089dfa584b865c9997147475e45
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