Updates two points in the compiler that were easy to miss when adding new
terminals:
* HIRBuilder's `removeUnreachableFallthroughs()` nulls out unreachable
fallthroughs, but this had a non-exhaustive `if` statement. It now uses a helper
function which internally has an exhaustive switch.
* LeaveSSA needs to schedule block fallthroughs, but had a non-exhaustive `if`
statement. It also uses a helper function which internally has an exhaustive
switch.
cc @poteto since you ran into this (ie the compiler not alerting you to update
these places) w your diffs.
Discovered this in a recent attempt at syncing Forget to Meta, it seems
that calling path.stop() is unsafe as it appears to have strange
behavior in plugins that come after. This resulted in `import type
{...}` not being compiled away in the post-babel output which isn't
valid JS syntax. Removing the `stop()` calls fixes it
Test plan: made these changes locally, synced my local changes to Meta and reran
- in simulator and observe that it now runs and doesn't throw a syntax error
This is a more general version of the change from #1521. That PR ensured that
LoadLocal temporaries accessed outside the instruction's scope are correctly
promoted. However, we have a similar pattern with PropertyLoad.
This PR adds a general mechanism for handling these type of indirections: any
LoadLocal/PropertyLoad temporary accessed when it's defining scope is not active
will be promoted to a declaration of the defining scope. Notably, we do this in
a way that ensures that the dependencies are preserved, ie that we correctly
view the operand of LoadLocal/PropertyLoad as a dependency of the current scope.
Supports EmptyStatement nodes by ignoring them. Note no tests because
format-on-save clears away any empty statements and there is, rather
frustratingly, no way to tell VSCode not to format on save for a specific file
via the file contents itself or project configuration (at least, not that i can
find).
Uses the new ExpressionStatement instruction to ensure that logical and
conditional expressions are never pruned. This addresses an issue where we were
unable to construct a ReactiveFunction for unused logical/conditional bc there
wasn't a single Identifier assigned in both branches. The ExpressionStatement
ensures that the result is used, that we don't prune the phi, and that both
branches have a single assignment target.
In theory we could be more sophisticated with DCE and still prune these
instructions if their operands are also safe to prune, but in practice you're
only like to have a logical/conditional as an expression statement (in the
source) if it's for side effects.
Adds an `ExpressionStatement` instruction variant to model values that are
otherwise "unused" but which we don't want to remove. The next diff changes
BuildHIR to use this where appropriate.
We previously represented JsxExpressions using builtin tags - `<div>`, `<b>` etc
- by lowering the tag name to a Primitive with the string name of the tag.
However, by lowering into an independent value, it was possible that the lowered
tag name could be grouped into a different memo slot, such that we ended up with
output like:
```javascript
let t0;
if (c_1) {
...
t0 = "div"
...
} else { ... }
return <t0>{children}</t0>
```
This is obviously wrong. It's also wrong to rename `t0` -> `T0`, because React
treats that as a custom component, not a builtin. The right thing is to
explicitly model builtin components, which this PR does by making
`JsxExpression.tag` be a union of Place | BuiltinTag.
Ensures that temporaries used in JsxExpression tags are named with a capital
letter so that they are treated as custom components rather than builtins.
---
Changes:
- Added `testfilter.txt`
```
// @only
call
capture-param-mutate
jsx-spread
```
or
```
// @skip
call
error.todo-kitchensink
```
- grouped all commands under `--mode`
```js
// runs all tests
yarn snap
// runs all tests and updates fixtures
yarn snap --mode update
// runs only tests that pass `testfilter.txt`
yarn snap --mode filter
// run in watch mode
yarn snap --mode watch
```
- in watch mode, toggle between running all tests or filtered tests
```
386 Tests, 386 Passed, 0 Failed
Completed in 4994 ms
Current mode = NORMAL, run all test fixtures.
Waiting for input or file changes...
u - update all fixtures
f - toggle (turn on) filter mode
q - quit
[any] - rerun tests
> f
PASS call
PASS capture_mutate-across-fns
PASS timers
3 Tests, 3 Passed, 0 Failed
Completed in 39 ms
Current mode = FILTER, filter test fixtures by "testfilter.txt"
Waiting for input or file changes...
u - update all fixtures
f - toggle (turn off) filter mode
q - quit
[any] - rerun tests
```
---
- `runner.ts` is pretty large now, happy to split it up into multiple files
- I'd also like to refactor `watch` to make its shared state and control flow
explicit
A bit of a hack -
We currently trigger test runs when we detect changes in the test fixtures
directory. This trigger is also hit when we run `snap` in update mode, since
updating performs file writes.
This PR will ignore subscription changes (callbacks) that trigger within 5
seconds of the last update.
It seems difficult to be more granular with a timestamp, since `@parcel/watcher`
doesn't give us the file change timestamp and (from my understanding), other
promises and tasks can be queued to run between the update and callback.
Run snap tester on a forked node process so runs can be interrupted. (Currently,
`Ctrl+C` is not handled until after all test fixtures finish compiling). This
*feels* a bit heavy-handed, but main.ts is pretty small and doesn't do much
other than listen for input / signals. Would love feedback here since I haven't
really worked with nodejs / JS cli tools before
- main.ts
new file that spawns forked runner
- pipe stdin/out/err to and from the child runner process, details described in
comments.
- listens for exit event of child
- runner.ts
- added logic to listen for interrupts and clean up (not really familiar with
how file and tsc watchers are implemented, so we try to call 'close' on them
just in case they need to release locks / do other cleanup)
---
`yarn snap --sync` currently fails on `error.file-has-non-critical-errors`. This
is because we're relying on a globally overwritten `console.error` function to
report non-fatal errors. However, executing `Promise.all(...)` on a single
nodejs thread will interleave calls to `run` (which is an async function).
---
Next PRs: skip / only tests, pretty diffing
This PR:
1. Add help messages:
```
$ node packages/snap/dist/runner.js --help
Options:
--version Show version number [boolean]
--sync Run compiler in main thread (instead of using worker threads
or subprocesses). Defaults to false.
[boolean] [default: true]
--worker-threads Run compiler in worker threads (instead of subprocesses).
Defaults to true. [boolean] [default: true]
--watch Run in watch mode. Defaults to false (single run).
[boolean] [default: false]
--update Run in update mode. Update mode only affects the first run,
subsequent runs (in watch mode) require typing `u` to
update. Defaults to false. [boolean] [default: false]
--help Show help [boolean]
✨ Done in 0.62s.
```
```
...
386 Tests, 386 Passed, 0 Failed
Completed in 4434 ms
Waiting for input or file changes...
u - update fixtures
q - quit
[any] - rerun tests
```
2. Surface typescript diagnostics; skip test fixtures if source code has errors
```
$ node packages/snap/dist/runner.js
src/Optimization/ConstantPropagation.ts:87:3 - error TS1434: Unexpected keyword
or identifier.
src/Optimization/ConstantPropagation.ts:87:3 - error TS2304: Cannot find name
'lt'.
src/Optimization/ConstantPropagation.ts:87:6 - error TS2552: Cannot find name
'hasChanges'. Did you mean 'onhashchange'?
src/Optimization/ConstantPropagation.ts:135:11 - error TS2552: Cannot find name
'hasChanges'. Did you mean 'onhashchange'?
src/Optimization/ConstantPropagation.ts:155:10 - error TS2552: Cannot find name
'hasChanges'. Did you mean 'onhashchange'?
Compilation failed (5 errors).
Found errors in Forget source code, skipping test fixtures.
✨ Done in 10.73s.
```
```
Compiling...
src/Optimization/ConstantPropagation.ts:86:39 - error TS2552: Cannot find name
'HIRFunctin'. Did you mean 'HIRFunction'?
Compilation failed (1 error).
Test: Found errors in Forget source code, skipping test fixtures.
Waiting for input or file changes...
u - update fixtures
q - quit
[any] - rerun tests
```
DevTools relies on built-in hook names at their call site to be unprefixed in
order to correctly track them. This PR updates our Babel plugin to:
- Check if there are any existing import declarations of `import { /* ... /* }
from 'react';`
- If true, we add the specifier `unstable_useMemoCache as useMemoCache`
- Otherwise, we synthesize a new import declaration
- In Codegen we now emit `useMemoCache(n)` rather than
`React.unstable_useMemoCache(n)`
I forgot to remove this when we removed the diagram output. This brings test
time from 6s -> 5s on my machine, still much slower than the new test runner in
#1486.
#1507 Ensured that declarations of reactive scopes were propagated to parent
reactive scopes as necessary to ensure that those declarations would be
available at the appropriate block scope. This meant that some scopes that were
previously pruned would no longer be pruned. Specifically, an outer scope wo any
declarations, but which contained a nested scope _with_ a propagated
declaration, would now end up with non-empty declarations and not be pruned.
This PR changes to track the declaring scope of each declaration, so we still
prune scopes that don't have any of their own declarations.
Originally I defined `Stack` as an interface to ensure both Node and Empty
variants would have an identical API. But exporting an interface allows a
developer to define other implementations, when we really want to ensure that a
Stack is precisely a Node or Empty instance. This PR changes to exporting a
union of `Stack = Node | Empty`, and makes the interface private to the module.
Fixed a bug identified in repro cases earlier in the stack. The case is where
some later value is composed of several values, say A and B, where A is an
identifier that is reassigned within B. Also, the mutable range of B surrounds
the evaluation of A. In this case, the reference to A gets lowered to a
temporary (say a t0 = LoadLocal A), and that temporary is created within the
reactive scope for B.
PropagateScopeDependencies bypasses LoadLocal indirections, and considers the
reference to the temporary (t0) as if it was a reference to the identifier (A).
That breaks the whole reason we lower Identifiers to temporaries - to preserve
evaluation order.
This PR fixes the bug by promoting temporaries to names values if they are
referenced outside their defining scope. So, the reference to t0 stays a
reference to t0, which correctly preserves the value of A at the right point in
time.
This is all much easier to see in the new test case.
When lowering a JSX element we were correctly lowering to a temporary in all but
one case: the common case of an identifier. That is fine in practice but breaks
in the presence of the tag identifier being reassigned in the props/children.
This PR fixes to always lower the tag to a temporary.
This PR updates ConstantPropagation to support propagating global references:
```javascript
// Before
const x = Math;
foo(x);
// After
foo(Math);
```
This is a generally useful optimization but also helps with a subset of cases
around JSX element tags, which are frequently globals.
During codegen, when we now cache and restore the temporary values map as we
enter and exit the scope. This ensures that any temporaries within the reactive
scope are only visible within that scope, and not to subsequent code. In a
subsequent PR this surfaces a bug with temporaries not correctly exported from a
reactive scope.
In codegen when we lower an operand we check to see if we have an already
lowered value for it (stored in `cx.temp`). Currently we silently handle missing
values by emitting a raw identifier, but this is really an error. This PR adds
validation, which uncovered a few places where we legitimately won't have a
value - things like function parameters that got swapped for temporaries bc of
destructuring. We populate those as `null` values now, and fail if a temporary
had a truly missing value.
Adds an option to always throw errors regardless of severity (default, ie the
status quo), or when the flag is disabled, only critical errors will be thrown.
Any error that isn't considered a critical error (see
`CompilerError.isCritical()`) since it might indicate that the compiler is
buggy, while non-critical errors will result in that file being skipped for
compilation, but otherwise continue compiling other files
* JSX tag value temporaries getting promoted due to being sandwiched inside the
mutation of some other item
* Incorrect order-of-evaluation for jsx tag relative to props/children
snapshotting
As demo'd on our sync. This is meant as a replacement for `yarn test` just for
fixtures. On my machine, a test run from a steady state of Jest watch mode takes
6 seconds. With this script, it takes ~800ms.
Workflow: make edits in `packages/snap/`, then `yarn build` in that directory to
build the test runner.
To run tests, `node packages/snap/dist/runner.js` from the main forget/
directory to run tests. Pass `--watch` for watch mode, `--update` to update
snapshots. Note that this _only_ updates .expect.md files, it does not update
Jest's `__snapshots__` directory (this is intentional, our use of Jest snapshots
is a hack that this script is meant to replace).
When running in watch mode, ctrl-c or q will quit, 'u' will update snapshots,
and any other key will re-run tests. Tests will re-run on changes to the source
code (after an incremental TS rebuild) and on changes to the fixtures.
Main things that are missing:
* Don't run tests if TS compilation had errors (the errors should be logged to
console already, but we need to wire that up so that we abort tests if there are
errors)
* Actually delete stale files in update mode (there is some commented-out code
to double-check and then uncomment)
* Improve diff view: just print the diffed segments, not the whole file diff.
See the API at https://github.com/facebook/jest/tree/main/packages/jest-diff
(right now we're using `diff()` but should probably use one of the lower-level
functions)
* Properly parse/validate args with `yargs`, add a help option, etc
* In watch mode, when tests finish print a list of commands so users know what
they can do (like Jest does)
* Support skipping tests and selecting a single test to focus. Suggestions:
* Name files with `skip.` to skip
* Allow passing a test name to the script to run only tests matching that
pattern, eg `node runner.js -p <pattern>`
* In watch mode, support typing 'p' to prompt the user for a pattern. After
that, support typing 'a' to clear the pattern and run all tests.
Fix for bug demonstrated in #1506. When we add variables as output of their
defining scope, we need to propagate this information upwards to all parent
scopes which are not current active.
Minimal(ish) repro of a bug we saw internally, where an output of a nested
reactive scope is defined at the wrong block scope, and so later references to
that value are invalid.
The simplified structure is:
```
scope0 inputs=[] outputs=[] {
scope1 inputs=[] outputs=[t0] {
t0 = ...
}
}
t0
```
Note that `t0` correctly appears as an output of the inner scope1, but not as an
output of the outer scope0. We need to propagate outputs upward as necessary to
ensure they are available at the right block scope: in this case, that would add
`t0` as an output of scope0.
An earlier version of PropagateScopeDependencies did this but it looks like it
got lost along the way (not a big deal)
This aligns the playground configuration with our internal compiler
configuration to make it easier to repro compilation issues on playground. There
is a bug that doesn't repro right now and i suspect it's because of different
hooks being configured.
Test plan:
Before: playground output has no obvious bugs, but is different than internal
compilation output where the bug occurs
After: playground output matches internal compilation output w the bug
Making ReturnTerminal.value non-nullable broke our optimization to elide final
value-less return statements. We now check if a return value is explicitly
`undefined` and elide the value in this case, which then also propagates to
allow removing the final `return` statement of a function if the value is
missing.
Updates our Babel plugin to add an import to React if we succesfully compiled a
function and cached one or more values. For now this logic is entirely in the
Babel plugin itself, but long term we should move this into codegen and teach
Forget to start the pipeline by lowering the whole Program node to an HIR (which
would also allow us to understand imports and other module scope values being
used). Right now it's not possible to add in codegen (without some ugly code) as
a file containing multiple components would result in duplicate imports being
generated
Previously useMemo inlining created a new StoreLocal assignment (not
reassignment!) instruction for every return value. This breaks when the return
is inside a block (like an if-block) as the scope is tied to the block.
For example: ``` let x = useMemo(() => { if (...) { return { ... }; } })
``` would become: ``` if (...) { const temp = { ... }; } const x = temp; ```
This PR instead changes the inlining to declare a temporary in the function
prologue and then reassign values to it when replacing return statements.
``` let x = useMemo(() => { if (...) { return { ... }; } }) ```
becomes
``` let temp; if (...) { temp = { ... }; } const x = temp; ```
Joe Savona
Sathya Gunasekaran
Lauren Tan
Mofei Zhang