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.
---
Thanks to @josephsavona for finding this bug. This is another example of why we
really want hir-everywhere.
Forget output currently nullthrows because we believe `obj.a` is run
unconditionally in source (missing the break/returns out of this scope)
I haven't debugged to understand exactly why this pattern fails, but there are a
few instances of this internally. It's especially weird because
```javascript
// @enableAssumeHooksFollowRulesOfReact
@enableTransitivelyFreezeFunctionExpressions
function Component(props) {
const [_state, setState] = useState();
const a = () => {
return b();
};
const b = () => {
return (
<>
<div onClick={() => onClick(true)} />
<div onClick={() => onClick(false)} /> // <---- only repros if there's a second
call!
</>
);
};
const onClick = (value) => {
setState(value);
};
return <div>{a()}</div>;
}
```
Here, if `b()` only had one nested function expression that called `onClick` it
would work. Also, if we disable `@enableTransitivelyFreezeFunctionExpressions`
then it works.
But the combination of multiple calls plus that mode causes "context variables
are always mutable". I'm guessing we're freezing `onClick` twice and the second
time reports an error since it calls `setState`.
We don't have a `DestructureContext` equivalent of `StoreContext`, so variables
that are declared via destructuring and later reassigned trigger the invariant
that all mentions of a variable must be consistently local or context. The next
PR adds a todo for this case.
We inadvertently think the type annotation on the function expression param is
an identifier and create a LoadLocal for it, which fails. This happens to trip
up on the InferReferenceEffects initialization check, which we had assumed would
only fire for invalid hoisting cases (hence the specific error message).
When PruneMaybeThrows removes maybe-throw terminals, it's possible that the
block in question reassigned a value s.t. it appears as a later phi operand.
That phi has to be rewritten to reflect the updated predecessor block.
Here we track these rewrites (transitively) and rewrite phi operands
accordingly.
Mofei Zhang
Lauren Tan
Sathya Gunasekaran
Joe Savona
Jan Kassens