I realized a wayyyy simpler approach to inlining a lambda: wrap it in a labeled
block. The transformation is roughly as follows:
```javascript
// Before
const x = useMemo(() => {
if (a) {
return b;
}
return c;
}, [a, b, c]);
return x;
// After
let x;
label: {
if (a) {
x = b;
break label;
}
x = c;
break label;
}
return x;
```
The key to making this work is fixing up some edge cases in labeled blocks,
hence the previous PRs.
This is part of a stack to fix some edge cases in inlining of useMemo closures.
In this first step, I'm disabling `shrink()` in order to retain more information
about the data flow. For example,
```
label: if (cond) {
break label;
}
return foo;
```
Would previously have shrunk away the if body, making the IfTerminal.consequent
point directly to the fallthrough block (w the return). Now we retain a separate
block.
- delete output files when we detect input files are deleted
- enable test fixtures in nested directories
- exit with error code when we detect failures
Note that the test failure on this PR is expected and will be fixed by #1608 (or
happy to abandon that PR and fold the changes)
---
Looks like we delete `.js` files but missed `.expect` files. Jest probably
didn't catch this because the basename of the fixtures had duplicates (in
rule-of-hooks)
---
(`react-forget-runtime` package seems to be synced to -.)
RFC: useRenderCounter hook:
- tracks # renders (increments on render path)
- exposes a global renderCounterRegistry (counting # renders in alive / mounted
components)
Next PR will modify BabelPlugin to add codegen
```js
// Similar to how we're currently importing `isForgetEnabled`
import {isInstrumentForgetEnabled_Secret} from "ReactForgetFeatureFlag";
// ...
function Component_uncompiled(props) {
if (isInstrumentForgetEnabled_Secret) {
useRenderCounter();
}
// ...
}
function Component_forget(props) {
if (isInstrumentForgetEnabled_Secret) {
useRenderCounter();
}
// ...
}
```
I originally created a separate test for the mode with JSX memoization disabled,
but we can merge this into the main compiler-test and enable the feature with a
pragma.
Reverts #1502, but flips test flags (e.g. `inlineUseMemo` by default, unless a
test specifies `@inlineUseMemo false`. I figured this add less thrash for test
fixtures, but happy to just do a clean revert (or remove the pragma altogether
and always pass `inlineUseMemo: true`)
---
In `lower`, we now ensure that all context variables are declared by a
`DeclareContext` instruction. `DeclareContext` always produces a `let`
declaration, and `StoreContext` is always a reassign. There are a few reasons we
need `DeclareContext`:
- DeclareLocal assumes it is storing to a SSA-fied identifier (which always
stores an immutable primitive). This does not fit context variables.
- Without DeclareContext, we need custom logic in some passes to initialize
identifier / context state (e.g. `MutableRange`, ValueKind, etc) for the
`StoreContext` that declares the context.
This PR stack models context variables as concrete identifiers (with references
to context variables modeled by `Place` referencing the context variable
identifier). @josephsavona pointed out that this is abusing the notion of
Identifier/Place, as context variables are essentially interior properties of a
ContextEnvironment. Since we are not modeling `ContextEnvironment` implicitly or
explicitly, all inference for context variables is essentially pointer analysis.
---
This PR adds LoadContext and StoreContext to handle reading and writing to
context variables.
A context variable is any variable that is declared within a Forget-compiled
function and reassigned within a closure. Conceptually, we want to treat these
variables as attributes of a `EnvironmentContext` variable (as most javascript
VMs do).
- context variables currently do not participate in type inference (i.e. we do
not produce type equations for loads from context variables). In the future, we
can try typing this as `Phi(assignment1Type, assignment2Type, ...)`.
- context variables are always treated as `Effect.Mutable`.
- context variables do not participate in SSA, or certain optimizing passes
(e.g. dead code elimination, constant propagation, etc).
There is some still follow ups:
- From my understanding, we should introduce a `DeclareContext` instruction.
- currently, declaring a context variable (without initializing it) is broken.
This is because the declaration lowers to `DeclareLocal`, which assumes it is
storing to a SSA-fied identifier.
```js
let x;
x = 4;
() => { x = {}; };
```
- DeclareContext will also make some initialization logic easier. In this PR, I
added some hack-y code to handle initializing effects / mutable ranges / other
inference state for the first StoreContext.
- Handle or bail on stores to context variables through destructuring assignment
-
~~Next PR:~~
- ~~Change closures to track reassigned identifiers (to extend mutable range of
primitives)~~
Enables hooks validation in playground. Also adds a tab to show the output of
validation (in case it passes) with the inferred post dominator tree. We can use
this to debug the dominator in case of false negatives.
<img width="1724" alt="Screenshot 2023-05-11 at 11 07 08 AM"
src="https://github.com/facebook/react-forget/assets/6425824/8f7ae472-8415-4899-aedf-c8f26094ebfe">
Incorporates the fixtures from eslint-plugin-react-hooks using a script, so that
we can easily update them in the future. For each fixture we run the compiler
with and without hooks validation first so that we know if the fixture is
expected to pass — we have some false positives and false negatives that i can
work through. For example we accidentally think that `userFetch()` is a hook,
oops. Fixtures that should pass but error, or that should error but pass, are
marked as `todo.<name>` or `todo.error.<name>`.
While i was here i added the ability to have fixtures in subdirectories for
grouping purposes.
---
Try to fix bug from #1589:
> If a declaration for an immutable identifier (i.e. one that is not later
re-assigned, since undefined is a primitive) is sandwiched between mutations, we
currently do not record it as an output or hoist it out of the reactive scope.
One simple fix is to add all declared (and later referenced) identifiers as
declarations of a reactive scope. This has some undesired effects (e.g.
additional instructions + memo cache slots), but in practice, this shouldn't be
happening often.
Alternatively, we could 1.) add a pass to hoist declarations, 2.) account for
this in constant propagation, or 3.) add a bailout
---
Record incorrect output.
If a declaration for an immutable identifier (i.e. one that is not later
re-assigned, since `undefined` is a primitive) is sandwiched between mutations,
we currently do not record it as an output or hoist it out of the reactive
scope.
While optimizing per @josephsavona's suggestions in #1592, I noticed that we
were clearing quite a few require cache entries.
As of this PR, `Object.keys(require.cache)` holds
- 1258 entries total
- 67 files compiled from Forget source code (this is what
`ts.createWatchCompilerHost` modifies)
- 1120 babel source files (from node_modules)
When working on watch mode, I'm almost always making changes to Forget source or
test fixture files. It's a bit faster to just clear those entries (assuming that
babel has no global state we need to invalidate).
On my computer, re-running tests in watch mode (triggered by source code
changes) takes:
| | All tests | One test (filter) |
|-- |--------|----------|
| current | 4.7s | 1.8s |
| this PR | 1.8s | 0.1s |
---
Some typed functions need to annotate callees or arguments as `Effect.Store`.
This PR modifies alias analysis (`InferAliasForStores`) to account for this
Snap currently has a bug in which the require cache is not correctly cleared
when running in filter mode (#tests < 2 * #workers).
- We're currently clearing all entries in the require cache of worker threads,
including `jest-worker` and `snap/dist/...` files
- jest-worker seems to `require` these files on every dispatch (i.e.
`worker.compile` seems to call `require(`compiler-worker`).compile`)
I noticed some instances of this error when running forget on an internal
product. I previously fixed the case if a logical/conditional used only for side
effects (not assigned to a variable) but the new cases were assigned to an
unused variable. I double-checked and we’ve actually fixed all the steps after
these invariants so we can just remove them and support these cases.
When we calculate the dependencies of a FunctionExpression we were only adding
new items if the binding identifier had not been seen yet. That is correct for
`capturedIds` since its the set of identifiers, but incorrect for `capturedRefs`
since its an array of all the distinct places. This meant that if a function
expression referenced multiple properties of the same binding, we'd only record
the first one. We now correctly record all of them.
Tidies up the implementation a bit, splitting the single function and class into
distinct computeDominatorTree() and computePostDominatorTree() functions and
helper classes.
React will retry or abort components that throw (depending on a few conditions),
so from React's perspective a `throw` statement is not a normal exit node. Thus
the Rules of Hooks really have a caveat: the set of hooks that are called _in an
execution that returns successfully_ must be consistent. Examples such as the
following are therefore allowed:
```javascript
function Component(props) {
if (props.cond) {
throw new Error(...);
}
useHook();
}
```
By modeling `throw` as an exit node, we rejected cases such as this. This diff
changes to not model throws as exit nodes. #1584 changes this to make it an
option, since some cases will want to consider throw as an exit node.
Incorporates the fixtures from eslint-plugin-react-hooks using a script, so that
we can easily update them in the future. For each fixture we run the compiler
with and without hooks validation first so that we know if the fixture is
expected to pass — we have some false positives and false negatives that i can
work through. For example we accidentally think that `userFetch()` is a hook,
oops. Fixtures that should pass but error, or that should error but pass, are
marked as `todo.<name>` or `todo.error.<name>`.
While i was here i added the ability to have fixtures in subdirectories for
grouping purposes.
See the code comments for more, but the basic idea here is that we use the post
dominator tree to find the set of basic blocks which are guaranteed reachable in
each function. Those are the only blocks where it is safe to call hooks, and we
error for hook calls in any other blocks.
Implements an efficient algorithm for computing the dominator (or post
dominator) tree of a CFG, following
https://www.cs.rice.edu/~keith/Embed/dom.pdf. This is used/tested in the next PR
to validate that hooks are called unconditionally.
note: I clean up the implementation quite a bit late in the stack in #1584
I noticed this while demoing Forget to React Org alum Christoph Nakazawa — in
array.map calls (and other APIs that take a lambda as input) we sometimes end up
memoizing the lambda. It's technically correct since the function _could_ return
the lambda, and then we'd need it to be memoized. It's tricky because array.map
is often called on nested objects, where even if we had type inference on the
outer value we wouldn't know for sure that the inner property is an Array and
not some other data type with a custom .map. For example in
`data.feedback.comments.edges.map(edge => ...)`, even if we knew that `data` was
an Object, we wouldn't know that data.feedback.comments.edges is an Array
without cross-file type knowledge.
But it's definitely wasteful to memoize these lambdas, so we should brainstorm
options. One option that stands out right away: if the lambda has zero
dependencies, then we could lift it out to module scope and refer to it by name.
Adds a validation pass to check that the only thing you can do with hooks is
call them. A follow-up PR (still early WIP) will check the other aspect of the
rules of hooks, that they are not called conditionally. That's a more involved
algorithm.
We previously disallowed OptionalMemberExpression inside a normal
MemberExpression, eg `(a?.b).c`. The new representation handles this case
correctly so we can remove the restriction.
Our previous lowering for OptionalMemberExpression reordered the evaluation of
properties, such that we had to restrict the allowed properties to those that
were safe for reordering. With the new representation we preserve order of
evaluation, so we can relax the restriction. This unblocks a few cases in an
internal product.
Now that _all_ optional expression types use the new representation, the
optionality of all PropertyLoad and ComputedLoad is modeled via control flow (in
HIR) and the structure of OptionalExpression (in ReactiveFunction). Thus we no
longer need the `optional` properties on these load instructions — they're
optional if they're part of an OptionalExpression.
Earlier PRs in the stack change the way we lower OptionalMemberExpression, but
only when they ultimately appear inside some OptionalCallExpression. This PR
ensures that _all_ OptionalMemberExpressions get the new lowering. Note that one
test case has what is arguably a regression, but the new behavior is also
reasonable: if we see both `a.b?.c` and `a.b.c.` as dependencies of a scope, we
previously inferred `a.b.c` as the dependency, but we now infer `a.b` as the
dependency. This isn't as optimal as what we had before, but it also seems good
enough for now. Also note that some cases are improved: `foo(a.b?.c)` would
previously have taken `a.b` as a dependency, we now take the full value of
`a.b?.c` as a dependency - more precise.
So overall i'm inclined to land and follow-up on the one regression, since the
overall model is more cohesive.
call
When we traverse an OptionalExpression in PropagateScopeDependencies, we
previously considered the entire value to be optional. With the changes in this
stack to more accurately model OptionalMemberExpression, the `object` portion of
an OptionalMemberExpression is now evaluated within the OptionalExpression. This
PR refines the handling of OptionalExpression accordingly, so that we only treat
the optional portion as conditional.
The previous OptionalCall terminal and reactive value kinds are now used not
just for optional calls, but for optional member expressions that appear within
an optional call. This PR renames those data types to OptionalTerminal and
OptionalExpression for clarity.
Extends the new modeling of the previous diff to OptionalMemberExpression. In an
example such as `a?.b?.c`, we now model that not only is the `.c` conditional,
but our control flow graph accurately reflects the fact that the `.c` is only
evaluated if `a.b` exists. Previously we knew it was conditional but the CFG
allowed a path from a being null through to evaluation of `.c`.
Joe Savona
Mofei Zhang
Sathya Gunasekaran
mofeiZ