---
Toggle default to true, since this should be a no-op refactor.
Tests:
- test fixtures
- ran on Store + - and saw no difference in compiled output
- [diff](P744101621) with
`enableTreatHooksAsFunctions=false`
- [diff](P744105565) with
`enableTreatHooksAsFunctions=true`
Adds a new feature flag which tells the compiler to assume that hooks follow the
Rules of React. Specifically, the idea that since any hook could be wrapped in a
giant `useMemo()` call, all arguments to hooks have to be treated as if they're
owned by React — and therefore become immutable — and that the return value of
the hook is immutable.
Our default is to assume that hooks break the rules, but in practice nearly
every component follows them.
Updates the InferReferenceEffects logic for CallExpression to work similarly to
MethodCall, where we take into account the function signature (if present) when
inferring the effects and return kind.
Defines the `Boolean`, `String`, and `Number` global functions. This will be
useful for allowing developers to wrap statements that produce a primitive in a
way that Forget knows about in order to optimize better.
Handles some edge-cases where we previously flattened away some of the structure
of a labeled block, instead ensuring that we retain the original shape. See the
output.
## Test Plan
Tested on the internal app we're focused on (w useMemo inlining enabled), it
works fine.
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.
Joe Savona
Mofei Zhang
Sathya Gunasekaran
mofeiZ