This PR makes all packages share the same typescript version and updates us to
latest versions of typescript, ts-node, typescript-eslint/eslint-plugin and
typescript-eslint/parser.
I also noticed that the tsconfig we were extending (node18-strictest) was
deprecated, so I switched us over to one that's more up to date.
Also had to make a couple of small changes to the playground so that continues
to build correctly.
Previously, we would drop directives inside a component or hook but this is
problematic with reanimated which uses `'worklet'` to mark components from
compilation.
This PR adds a directive to HIRFunction and ReactiveFunction and codegens the
directive add the end. No processing is done on the directives themselves.
Babel seems to store the directives on a BlockStatement, rather than on the
Function but I've stored it on the Function types because we only support
compiling functions and the spec defines directives as occuring in the initial
statement list of a function: > A Directive Prologue is the longest sequence of
ExpressionStatements > occurring as the initial StatementListItems or
ModuleItems of a > FunctionBody, a ScriptBody, or a ModuleBody and where each >
ExpressionStatement in the sequence consists entirely of a > StringLiteral token
followed by a semicolon.
This PR was the result of a long chain of ~yak-shaving~ debugging kicked off as
a result of fixing up invariants. Where this started was that i noticed some
cases of loops where the first instance we saw of a reactive scope was after its
starting instruction. Eg instruction N would have an operand with scope
Start:End, where Start was _before_ N. One of the cases involved a phi with a
backedge. Then i noticed that we assign scopes differently for phis with and
without backedges:
```
[1] let x0 = init;
[2] if (x0 < limit) {
[3] x1 += increment;
}
x2 = phi(x0, x1);
[4] x2;
```
The phi isn't mutated _or_ reassigned after its creation, so we don't assign a
mutable range to the phi or any of its operands. We also don't create a scope
for `x`.
But change the `if` to a `while` and now the phi moves - now there's a backedge:
```
[1] let x0 = init;
[2] while (x0 < limit) {
x2 = phi(x0, x2); // now this is "mutated" later!!!
[3] x2 += increment;
}
[4] x2;
```
What was happening here is that x2 has a mutable range which is "after" the phi
instruction, so it would appear that the phi was actually being mutated later.
Ie, this was treated equivalently to the original "if" version, but with a
mutation:
```
let x = [];
if (cond) {
x = {};
}
mutate(x); // later mutation of the phi
```
But these latter two cases are different! We only need to (should) create a
mutable range for a phi _if its value is actually mutated_. If it's just being
reassigned, well then it shouldn't matter if there are back edges or not.
So this PR implements that intuition: only create a mutable range for a phi if
it is actually _mutated_ later, ie don't assign a mutable range if it is only
_reassigned_ later. Concretely in InferMutableRanges:
* InferMutableLifetimes no longer has to initialize a range for phis during the
first pass (inferMutableRangesForStores=false). We wait to see if the phi is
mutated during the main fixpoint iteration of InferMutableRanges
* The main fixpoint iteration in InferMutableRanges already aliases phi operands
if the phi is later mutated, which will extend the end of the mutable range of
all the operands accordingly.
* Finally, InferMutableLifetimes's second run
(inferMutableRangesForStores=true), we ensure that any phis mutated later have a
valid mutable range, specifically setting the `start` of the range since the
fixpoint only updates the `end` value.
A dependency D from either an instruction or scope is poisoned if there may be a
(non-linear) jump instruction between it and the start of its immediate parent
scope. Poisoned dependencies are added as conditional dependencies to their
parent scope.
(done: reduce false positives in scopes that begin after return/throw) (done:
fix bugs in recording and joining exhaustive conditional deps) (done: flesh out
commit message, clean up PR, add more fixtures)
--- \## Bug details:
Take a simple example: ```js target: { instrA; if (...) { instrB;
break target; } else { instrC; } instrD; // ... } instrE; // ... ```
This diagram shows how we represent this program in the reactive IR. - Blocks
are represented as a list of nodes. - Green nodes show instructions and value
blocks (simplified as a single instruction). - Pink nodes show terminals, which
transfer control to a subtree of nodes. <img width="450" alt="image"
src="https://github.com/facebook/react-forget/assets/34200447/930789f2-39cd-4ea8-b12a-530042807b46">
Prior to this PR, PropagateReactiveScopeDeps was incorrect because it assumed
that a block's instructions are evaluated unconditionally (which is how HIR
basic blocks work). E.g. if a reactive scope enclosed `block 1`, we assume that
`instrA` and `instrD` both will evaluate unconditionally.
This failed to account for `jump` instructions like break, continue, return, and
throw. This may result in invalid hoisting of PropertyLoads (i.e. Forget output
may throw when source does not throw). Note that other terminals (e.g. if and
loops) are not affected as they are self contained subtrees that evaluate
sequentially.
With the changes in this PR, we mark `block 1` as poisoned upon encountering the
`break` instruction. While `block 1` is active and poisoned, it will determine
how visited dependencies are added.
Here, added solid lines show unconditional dependencies, dashed lines show
conditionally accessed dependencies: - dependencies from `instrB, instrC` are
conditional because they are within conditional subtrees - dependencies from
`instrD` are conditional because it is within a poisoned block within its parent
scope.
<img width="450" alt="image"
src="https://github.com/facebook/react-forget/assets/34200447/81980f68-7e65-4bd7-ba94-3f0c26550e5c">
--- Recapping an offline discussion with @josephsavona: this pass would really
benefit from operating on HIR. The minimal work needed for this pass to run on
HIR is to rewrite and reorder `AlignReactiveScopesToBlockScopes` to operate on
HIR.
The following diagram shows what HIR blocks look like for the same code.
Evaluating hoistable PropertyLoad dependencies for a scope enclosing
`instr{A-D}` is much simpler: just evaluate whether the PropertyLoad evaluates
for every path between `bb0` and `bb4`. <img width="250" alt="image"
src="https://github.com/facebook/react-forget/assets/34200447/44b38939-defb-4b29-878d-4445ec6ccc06">
---
conditionals
This change is needed for #2752. To minimize renaming `error.fixture` ->
`fixture` files, I'm reordering this PR to earlier in the stack.
Prior to the fix in #2752, we only expected unconditional accesses within
`depsInCurrentConditional`, which records instructions directly within a
conditional block (not including nested conditional blocks).
RFC: we can either retain break/continue target ids (instead of pruning them in
`buildReactiveFunction`) or re-implement the same logic in `propagateScopeDeps`
(ignore implicit breaks; match unlabeled break / continues to their closest loop
/ while parent terminal).
If we go ahead with this approach, I'll clean up this PR (add relevant types and
comments)
The reanimated babel plugin specifically looks for args to their hooks that are
callbacks, then it workletizes the body of that callback so it can run on the
main thread.
But, forget extracts that callback into a temporary variable and then replaces
the previously inlined callback as an identifier, so that breaks reanimated's
babel plugin. so what happens is some of the previously workletized functions no
longer do after forget runs, which throws a runtime error about a non-worklet
function running on the main thread.
Reanimated expects this: ``` const animatedGProps = useAnimatedProp(function ()
{ ... }) ```
But forget does this: ``` const t0 =function () { ... } const animatedGProps =
useAnimatedProp(t0) ```
With the type definitions, Forget no longer assumes the args to reanimated APIs
escape so Forget does not memoize and they stay as is.
Our current validation fails to detect some invalid cases of mutable ranges —
namely, ranges that are fully or partially uninitialized, with start or
start+end still set to zero.
This PR fixes these cases, starting by validating that the ranges for _all_
reactive scopes are valid: start >= 1, and end <= (last instr id + 1). This
exposed the invalid cases, which are also fixed here:
* During AnalyzeFunctions, we need to reset identifier ranges and scopes when
exiting an inner function. This has to happen *after* the effects have been
translated to the function deps/context operands, in order for
InferRefenceEffects to continue working on the outer function. Previously I did
this at the start of InferReactiveScopeVariables, but that's insufficient bc the
incorrect ranges could also influence InferMutableRanges. AnalyzeFunctions is
the point at which we compute the ranges for identifiers in the inner function,
so it's the most ideal place to clean those up so they don't influence the outer
function.
* In InferMutableLifetimes, we need to ensure that context variable identifiers
end up with a mutable range starting where they are declared, and ending with
their last assignment. We now track declarations and extend their mutable range
to account for each reassignment.
Fixes the repro added in 947832009997bf9149e88e583c46cc39f6a6136c - previously
when computing mutable ranges of phis, we didn't check that all operands had
been visited. This meant that a backedge could allow a phi's mutable range to
start at 0. Then in PropagateScopeDeps, we might see reject dependencies of a
scope since they appeared to start after a scope — only because the scope's
start was incorrectly too early.
The fix here is to initially set phi.id mutable ranges based on only on operands
that are already visited. Then, during/after the fixpoint iteration of
InferMutableRanges, we start account for all operands since we know they've been
visited at least once and have a real range.
Updates InferReactiveScopeVariables to first prune scopes attached during
AnalyzeFunctions. This ensures that after this pass the only scopes that exist
on identifiers in the outer program are those that the pass explicitly inferred,
and not accidentally leftover.
We share identifiers between outer functions and inner function expressions,
which means that scopes inferred during AnalyzeFunctions can be retained on
Identifiers in the outer function. If InferReactiveScopeVariables doesn't happen
to visit an identifier we currently retain that scope information and use it
when constructing scopes, even if there doesn't technically need to be a scope
for that value.
Fixed in the next PR.
What happens here is that the phi node for `i` has its mutable range set to
start at 0, because it has a back edge and we haven't initialized the mutable
ranges of all its operands yet when we iterate the operands and set range.start
= min(start of operand starts).
Then the corresponding scope has its range set to start at 0 too. When
PropagateScopeDeps runs it sees that `b` is from instruction 1, which is after
the start of the scope (0), so it thinks `b` isn't a valid dependency.
The fix is in InferMutableRanges, where we need to make sure that phis ignore
their operand's ranges until those ranges are initialized.
Correct eslint-plugin-react-compiler dependencies
- The eslint plugin doesn't actually depend on the babel plugin as it compiles
in the dependencies. - `zod` and `zod-validation-error` were missing, but
required to run the plugin. - Update to the `hermes-parser` dependency just to
keep it updated.
Our logic to detect hoisting relies on Babel's `isReferencedIdentifier()` to
determine whether a reference to an identifier is a reference or a declaration.
The idea is that we want to find references to variables that may be hoistable,
before the declaration — the definition of hoisting. But due to the bug in
isReferencedIdentifier, we skipped over reassignments of hoisted variables. The
hack here checks if an identifier is a direct child of an AssignmentExpression,
ensuring we visit reassignments.
Adds examples for closures that reassign hoisted let bindings. We currently
compile these as context variables without hoisting, so we hit an invariant in
InferReferenceEffects when the variable is referenced before being declared.
`let` bindings of context variables are lowered to a DeclareContext +
StoreContext, which breaks codegen for `for` loops which expect that all
statements of the init block will lower to variable declarations. The two
instructions produce a variable declaration and a reassignment.
If we have a switch with only a default case, then that code will be executed
unconditionally. PropagateScopeDeps can take advantage of this to record
dependencies in these cases as unconditional, which avoids the issue seen in the
previous PR.
Joe Savona
Sathya Gunasekaran
Lauren Tan
Mofei Zhang
Jan Kassens