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:
I want to better understand the bugs we have in abstract evaluation. One of my ideas to do this is to replace all the literals in test262 tests with abstract values and see what our test coverage is. The first step to do this is to serialize the test262 sources after running them in Prepack and checking if their generated JavaScript runs correctly. This PR does that by adding a `--serializer` flag.
With my methodology, all test262 harnesses are serialized every time since they are in the global scope. This unfortunately seems to be unavoidable since doing otherwise would change program semantics and break the tests.
Running `time yarn test-test262` takes about 2 minutes and has a 98% pass rate. Running `time yarn test-test262 --serializer` takes about 5 minutes and has a 95% pass rate. [Here’s a diff of the two results.](https://gist.github.com/calebmer/1c9fe396b63ba055458c599c2be18a58)
[Here is a selection of some of the bugs](https://gist.github.com/calebmer/1ac381096a4aa7be1fc7dc2163276ab4) in the serializer caught by running test262 with the Prepack serializer. I might open issues for these, but they can be a bit pedantic. I might fix some of them depending on how important they are to the React code we want to compile.
Notably we have:
- Lots of invariants being triggered. I particularly saw [a lot of this invariant](7d355ef4c5/src/serializer/ResidualHeapVisitor.js (L531)). (An example of this is [`07.md`](https://gist.github.com/calebmer/1ac381096a4aa7be1fc7dc2163276ab4#file-07-md).)
- Lots of values that are visited, but not serialized. (An example of this is [`01.md`](https://gist.github.com/calebmer/1ac381096a4aa7be1fc7dc2163276ab4#file-01-md).)
- Some incorrect outputs only caught by executing code. Could not be caught by static analysis. Particularly around class serialization.
I’ll build off this PR to get coverage for the abstract evaluator next, but I found these results interesting on their own so decided to do this in two PRs.
Pull Request resolved: https://github.com/facebook/prepack/pull/2290
Differential Revision: D8908479
Pulled By: calebmer
fbshipit-source-id: aa57d47611fbd92af33e4647fed7bf7990fb6de1
Summary:
Release note: fix test262 to fail CircleCI test if not enough tests pass
The check for the number of tests that pass returned 1 to its caller, who just ignored it and then returned 0. Instead of that, now just call process.exit(1) when the check fails.
Also updated the expected number of ES6 tests that pass. It appears that updating Babel had a positive effect on those. Possibly it also causes one more ES5 test to fail when running locally (but not on Circle). That might be because of an ES5 test that now times out and an ES6 test that now does not. I have not investigated this as it seems of little importance right now.
Pull Request resolved: https://github.com/facebook/prepack/pull/2263
Reviewed By: trueadm
Differential Revision: D8859210
Pulled By: hermanventer
fbshipit-source-id: 724dcde05927cc914f6f9517f14dc230b8b0ad2e
Summary:
Release note: Added --expectedCounts parameter to test262-runner so that success can depend on the value of the time-out and the version of the test suite that is used.
Pull Request resolved: https://github.com/facebook/prepack/pull/2228
Differential Revision: D8775828
Pulled By: hermanventer
fbshipit-source-id: 284bdb3526467f634f41a151e3995719af751e49
Summary:
Release note: Updated the test262 submodule to latest version
Also updated test262-runner.js to deal with time out errors showing up as diagnostics.
Filter out new tests that depend on errors being generated during the parsing phase. Since we depend on Babel for parsing, such issues are out of scope for us.
Pull Request resolved: https://github.com/facebook/prepack/pull/2216
Differential Revision: D8755005
Pulled By: hermanventer
fbshipit-source-id: 0c929904984d13efccbd3ad1ca125137ca275ef0
Summary:
Turns out os.cpus() can return [] and undefined.
Additionally, it seems to me like any number below 4 could lead to an empty pool so I tried to address that too.
Reviewed By: NTillmann
Differential Revision: D8332896
fbshipit-source-id: 60e082a4f0d18e72f95b76732a50adeccb2d333d
Summary:
This should resolve#415.
Relevant part of the spec here: https://www.ecma-international.org/ecma-262/6.0/#sec-let-and-const-declarations-runtime-semantics-evaluation
For `// 3. Let env be the running execution context’s LexicalEnvironment.`, I assumed that the `env` variable passed to the function already fulfills and it's not necessary to retrieve it here.
I've omitted the `ReturnIfAbrupt(Value)` steps from the spec, as this is also done for the rest of the implementation in this file.
test262 has one additional test pass as a result of this change, though somewhat surprisingly it's for `for`.
I've verified in the command line REPL that destructuring assignment works as expected in the basic case.
Closes https://github.com/facebook/prepack/pull/1967
Differential Revision: D8027013
Pulled By: NTillmann
fbshipit-source-id: 845527bcca55c845aca993fd2c97349efcbd5f59
Summary:
Release note: Add source annotations to satisfy latest version of Flow
Flow now complains about missing properties in more situations.
Closes https://github.com/facebook/prepack/pull/1612
Differential Revision: D7324360
Pulled By: hermanventer
fbshipit-source-id: 0aa40f2442a13c87258179f2151864cf9c921215
Summary:
Release note: Changed some erroneous console.log statements to console.error
This addresses issue https://github.com/facebook/prepack/issues/1201. Went through all uses of console.log in the codebase and converted them to console.error if that's what they're supposed to be.
Closes https://github.com/facebook/prepack/pull/1253
Reviewed By: hermanventer
Differential Revision: D6550963
Pulled By: jhalley
fbshipit-source-id: aec0f2f9a0e5745f563628b849fde110388b12cd
Summary:
Move the implementations of the to methods behind an interface so that we can reduce the Flow cycle length (as well as avoiding increasing it when a planned future request adds a new import to to.js).
This is a mechanical refactor with no new functionality.
Closes https://github.com/facebook/prepack/pull/1219
Differential Revision: D6494524
Pulled By: hermanventer
fbshipit-source-id: b5dbe45155e0276a9d42f2b26a90bb9b220ac1ea
Summary:
It seems that the numbers were meant for another version of the test262 repository.
The original pull request updated the test262 repository label, but the bot that synchronizes GitHub with our internal repo removed that update. That also causes the new test runner to fail, so disable that until we update the label properly.
Closes https://github.com/facebook/prepack/pull/1215
Differential Revision: D6477069
Pulled By: hermanventer
fbshipit-source-id: ed17b12c1e81d51465f9c4b76adf684961390fb0
Summary:
This method provides an alternative way to run Test262 through a
stream of compiled test contents for each scenario (default, strict
mode), including all the necessary harness contents.
The logic to capture an execution success or failure is similar to
the original one - from scripts/test262-runner.js - and improvements
should be done as follow ups.
The filtering system is now done through a configuration yaml file,
using tests metadata and path locations. This filter is used as an
object that can be extended with further logic, but still offers a
single point to check for filtering.
The report system requires some improvements and these should also
be done as follow-ups. For now they provide a report for each
folder and the total results. Although, the results data contain
enough information to highly expand the report.
Some further improvements are expected and planned, this work
should be at least ready for an initial round for feedback review.
Closes https://github.com/facebook/prepack/pull/1122
Reviewed By: cblappert
Differential Revision: D6456700
Pulled By: hermanventer
fbshipit-source-id: ba56efcaa4eab847594cb7c1fbc908cf1fc66a80
Summary:
Some versions of test262 include comments in yaml blocks that cannot parse correctly.
Make sure that the test262-runner exits with nonzero code when one of the workers throws an exception so that it is friendly to invocation from shell scripts.
Closes https://github.com/facebook/prepack/pull/1167
Differential Revision: D6359879
Pulled By: cblappert
fbshipit-source-id: f507cc1a7e28c0930df1dc4c6e574b4d8ed58b6d
Summary: Adds new `test-coverage-all` command to `package.json` which will run and compose all test coverage information into one html.
Reviewed By: hermanventer
Differential Revision: D6052366
fbshipit-source-id: 208259fa79185267f6c7a51ca8baf08ffb707e97
Summary:
This seems to have been added to the standard since the last time a looked. Re-enable a test that was already looking for it. Mask "caller" for non strict functions, lest more tests fall over.
Closes https://github.com/facebook/prepack/pull/1049
Differential Revision: D5984228
Pulled By: hermanventer
fbshipit-source-id: 430a5c85b211cb0c5cfb8429c2713246892bebb5
Summary:
Updated todo comments with task numbers.
Not included in this list:
1) node related todos: Sebastian should do that
2) serializer related todos: Nikolai should do that
3) partial evaluator todos: I should do that, but it will take some time.
Closes https://github.com/facebook/prepack/pull/1024
Differential Revision: D5950510
Pulled By: hermanventer
fbshipit-source-id: 131601c18115d5bf8c1a4e1f6a87e45619acc0ba
Summary:
The rest runner did double counting that went awry when certain tests were either just ES5 or just ES6. I just go annoyed at the lack of precision while debugging test failures and this has been lying around on my machine for far too long.
Closes https://github.com/facebook/prepack/pull/874
Differential Revision: D5586527
Pulled By: hermanventer
fbshipit-source-id: 561743c545ec8c54b64a0b96d972b3de31807b3e
Summary:
Update Babylon and adjust expected pass count to match what I see on my laptop.
The difference was due to my version of Babylon (which is the latest for some or other reason I don't understand) giving a syntax error that did not occur with the version on the CI machine. Updating the yarn lock file gets the CI machine in sync with my laptop with respect to those tests.
Note that some String.prototype tests only pass with recent versions of Node.
Closes https://github.com/facebook/prepack/pull/831
Differential Revision: D5489742
Pulled By: hermanventer
fbshipit-source-id: 5732e47718c53d9f20de04fd64c1389524f33309
Summary:
Two improvements:
1. The CI has 10340 ES6 tests passing so reflect the latest status in test runner.
2. Always show test failure groups summary even in success situation. This allows us to run the test locally and compare failure groups summary to the baseline summary in CI to find out which specific test group is failing. Saving much time in troubleshooting test failure!
Closes https://github.com/facebook/prepack/pull/820
Differential Revision: D5447438
Pulled By: yinghuitan
fbshipit-source-id: 87fa62693cb6e1365978e5cb635e3732fc2ded09
Summary:
Report errors rather than throw IntrospectionErrors. Added test cases to get 100% coverage of ForOfStatement.js. Fixed a few bug that came to light because of the new tests.
Closes https://github.com/facebook/prepack/pull/803
Differential Revision: D5411294
Pulled By: hermanventer
fbshipit-source-id: 818a5fb4c8112fc33c8531602eee5c0f6ecf25cb
Summary:
Per discussion in #729, this sets up prettier but doesn't actually run it on the codebase.
Closes https://github.com/facebook/prepack/pull/759
Reviewed By: Kishore-B-Rao
Differential Revision: D5343135
Pulled By: NTillmann
fbshipit-source-id: 638740d48b6fa797fa79b8b5fd3c032497cb0132
Summary:
This now works in the repl:
```
$ yarn repl
yarn repl v0.24.6
$ node lib/repl-cli.js
> class Foo { constructor() { this.x = 2 }}
undefined
> let f = new Foo()
undefined
> f
{ x: 2 }
> class Bar extends Foo { constructor() { super() } }
undefined
> let b = new Bar()
undefined
> b
{ x: 2 }
```
Closes https://github.com/facebook/prepack/pull/762
Differential Revision: D5339029
Pulled By: hermanventer
fbshipit-source-id: 6ad6b7f80b95bf5edd5ba2c5971dc8a82912dd14
Summary:
Figured it was a logical next step after the work I did in #669.
Closes https://github.com/facebook/prepack/pull/761
Differential Revision: D5336505
Pulled By: hermanventer
fbshipit-source-id: 968502411daba78825ad21395fb637064823af0e
Summary:
Complain if the operand might be a badly behaved abstract value.
[PP0008 might be a symbol or an object with an unknown valueOf or toString or Symbol.toPrimitive method](https://github.com/facebook/prepack/wiki/PP0008)
Closes https://github.com/facebook/prepack/pull/749
Differential Revision: D5320843
Pulled By: hermanventer
fbshipit-source-id: 37f7fa789430109899c2cfa00ce50f7c7e1cc6ec
Summary:
Addresses #630. I copied in the ECMA spec and then wrote the code to implement it, including any new methods that were necessary.
Closes https://github.com/facebook/prepack/pull/669
Differential Revision: D5294316
Pulled By: hermanventer
fbshipit-source-id: d79a2e6599bcfe09c5962eab385f891de47cccf6
* initial skipping tests
* fixing lint and flow issues
* rebased and added environmental var check
* fix indentation and added test
* change env var, timeouts cause test failure, added test
We used initialize all possible environments in a single intrinsics entry
point. That doesn't quite scale to many and complex environments. So the
goal of this refactor is to invert the initialization of globals.
Now a Realm is created with only the standard ECMA262 intrinsics. This is
also important because we should be able to create new realms from within
user code to model things like iframes, node.js contextify and the new
standard Realm API that is used to create new realms. All of these
basically starts out with a standard realm that can then be expanded.
Then the actual initialization of the global happens outside of the
serializer so that the serializer dependency isn't coupled to any
particular set of environments.
To do this I created a "global.js" file for each environment inside of
intrinsics.
It's interesting to note that "console" and timers isn't part of the
standard realm.
Another interesting one is that "global" isn't actually part of the DOM
environment yet. It might be if this proposal gets through:
https://github.com/tc39/proposal-global
However, since so many tests depend on global, including in test262,
I model it as already part of ECMA262 globals.
I also had move the initialization of the realm out of the serializer API
so that it is possible to initialize the realm with arbitrary intrinsics
before that happens.
However, when I did that I had to move the attaching of of the generator
into the serializer because otherwise mutations to intrinsics during
initialization gets tracked as something to serialize.
This might help avoid the cycle problem that construct_realm was meant to
fix so that we can just go back to using a simple constructor for Realm.
This moves things around in a more common idiomatic folder structure / naming convention. This extracts the scripts that are only used to test the library from the source of the library itself.
/src/scripts -> /scripts
This is no longer part of the build process. Therefore we use babel-node to run the files as needed. The exception is test-runner that is just ran in plain node so that it works with the coverage reports.
The lib folder now includes only and all the library files required to use the package and no longer test runners.
Because of that I moved all the dependencies that were related only to our test scripts and website build to devDependencies. That way we minimize the required dependencies to use Prepack downstream as an npm package.
Then I configured the package.json to only list lib/ and bin/ folders as part of the package. This lets `npm publish` ignore all other files so that people installing the package don't need to include all our testing code.
I added files to bin/ and package.json which is a convention to make this package installable and usable as a command-line tool.
Renamed run_util -> prepack-cli and repl -> repl-cli to separate these from library variants.
The coverage reports now get created in the root instead of lib folder so that they don't get included in the build.
I bumped the version number to one minor above what is already on npm since we can't publish earlier than that.
