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.
Found when running the compiler on a large swath of internal code.
PruneMaybeThrows rewrites terminals, but the logic to update subsequent phis was
incorrectly dropping phis rather than rewriting them. Fixed in the next PR.
---
Reusing optionalMemberExpression nodes recently led to a bug when compiling
Forget playground.
```js
// the two a?.b's here should be different nodes!
if (a?.b !== $[0]) {
// ...
$[0] = a?.b;
}
```
Forget playground uses `babel-plugin-react-forget` and `next/babel`. Reusing the
same node in two positions in the AST lead to invalid mutations:
- the first `a?.b` is visited and transpiled to `a === void 0 ? ...`, which (1)
inserts nodes between the original node and its parent and (2) mutates `a?.b` in
place to a non-optional call
- the second `a?.b` in source gets updated to `a.b` and does not get visited
again
```js
// Source in `EditorImpl.tsx`
compilerOutput.kind === "err" ? compilerOutput.error.details : []
// Forget transformed:
if ($[2] !== compilerOutput.kind || $[3] !== compilerOutput.error?.details) {
t4 = compilerOutput.kind === "err" ? compilerOutput.error.details : [];
$[2] = compilerOutput.kind;
// this is good!
$[3] = compilerOutput.error?.details;
$[4] = t4;
} else {
t4 = $[4];
}
// After next/babel
if ($[2] !== compilerOutput.kind || $[3] !== ((_compilerOutput$error =
compilerOutput.error) === null || _compilerOutput$error === void 0 ? void 0 :
_compilerOutput$error.details)) {
t4 = compilerOutput.kind === "err" ? compilerOutput.error.details : [];
$[2] = compilerOutput.kind;
// Oh no!!
$[3] = _compilerOutput$error.details;
$[4] = t4;
} else {
t4 = $[4];
}
```
---
This should make it easier to grep through error diagnostics to understand state
of the codebase:
- no matching dependences -> likely that source is ignoring eslint failures
- differences in ref.current access -> non-backwards compatible refs
- subpath -> should be fixable in the compiler (unless source is ignore eslint
failures)
---
Previously (in #2663), we check that inferred dependencies exactly match source
dependencies. As @validatePreserveExistingMemoizationGuarantees checks that
Forget output does not invalidate *any more* than source, we now allow more
specific inferred dependencies when neither the inferred dep or matching source
dependency reads into a ref (based on `.current` access).
See added comments for more details
I need to do more debugging to figure out exactly why the example earlier fails
— but whatever it is, it's clearly a matter of the fbt plugin relying on some
specifics of source locations.
Here we just detect multiple instances of `<fbt:enum>` within a given `<fbt>`
tag and throw a todo.
<img width="553" alt="Screenshot 2024-03-19 at 4 41 15 PM"
src="https://github.com/facebook/react-forget/assets/6425824/e87ee704-6c67-4e10-824b-71e97e7e19f5">
Slightly improves source locations for JSX elements so that the opening and
closing tag have distinct locations that match up with source. The identifier
itself within the closing tag still has the wrong location, but at least this is
an improvement.
Doesn't fix the fbt thing but it was worth a try.
Fbt enums appear to rely on source locations and something that we're doing
(maybe destructuring?) isn't preserving locations such that the fbt plugin
breaks.
Fbt violates the JSX spec by using a lowercase function as a tagname, even
though lowercase names are reserved for builtins. Here we detect cases where
there is an `<fbt>` tag where `fbt` is a local identifier and throw a todo.
The example earlier in the stack had unreachable code in the output because
there was an unnecessary memoization block around an assignment. This was a
holdover from before we moved the logic to expand mutable ranges for phis from
LeaveSSA to InferMutableRanges. We were conservatively assigning a mutable range
to all variables with a phi, even those that didn't strictly need one.
Removing the range extension logic in LeaveSSA fixed the issue, but uncovered
the fact that AlignReactiveScopesToBlockScopes was missing a case to handle
optionals.
## Test Plan
Synced internally and ran a snapshot/comparison of compilation before/after
(P1197734337 for those curious). The majority of components get fewer memo slots
thanks to not needing to memoize non-allocating value block expressions like
ternaries/optionals. In a few cases, the fact that we're no longer assigning a
mutable range for value blocks (unless there is actually a mutation!) means we
get more fine-grained memoization and increase the number of memoization blocks.
So overall this appears to be correct, improve memoization, and reduce code
size.
Extracts a helper from the repro earlier in the stack into a helper in
shared-runtime. This makes it easy to verify that memoization is actually
working.
This case is specific to early return inside an inlined IIFE (which can often
occur as a result of dropping manual memoization). When we inline IIFEs, as a
reminder we wrap the body in a labeled block and convert returns to assignment
of a temporary + break out of the label.
Those reassignments themselves are getting a reactive scope assigned since the
reassigned value has a mutable range. They don't really need a mutable range or
scope, though. And then the presence of the `break` statements means that we can
sometimes exit out of the scope before reaching the end - leading to unreachable
code.
This can only occur though where _all the values are already memoized_. So the
code works just fine and even memoizes just fine - it's just that we have some
extraneous scopes and there is technically unreachable code. I'll fix in a
follow-up, adding a repro here.
of dependencies from source
---
`validatePreserveExistingMemoizationGuarantees` previously checked
- manual memoization dependencies and declarations (the returned value) do not
"lose" memoization due to inferred mutations
```
function useFoo() {
const y = {};
// bail out because we infer that y cannot be a dependency of x as its
mutableRange
// extends beyond
const x = useMemo(() => maybeMutate(y), [y]);
// similarly, bail out if we find that x or y are mutated here
return x;
}
```
- manual memoization deps and decls do not get deopted due to hook calls
```
function useBar() {
const x = getArray();
useHook();
mutate(x);
return useCallback(() => [x], [x]);
}
```
This PR updates `validatePreserveExistingMemoizationGuarantees` with the
following correctness conditions:
*major change* All inferred dependencies of reactive scopes between
`StartMemoize` and `StopMemoize` instructions (e.g. scopes containing manual
memoization code) must either:
1. be produced from earlier within the same manual memoization block
2. exactly match an element of depslist from source
This assumes that the source codebase mostly follows the `exhaustive-deps` lint
rule, which ensures that deps lists are (1) simple expressions composing of
reads from named identifiers + property loads and (2) exactly match deps usages
in the useMemo/useCallback itself.
---
Validated that this does not change source by running internally on ~50k files
(no validation on `main`, no validation on this PR, and validation on this PR).
---
Previously, we always emitted `Memoize dep` instructions after the function
expression literal and depslist instructions
```js
// source
useManualMemo(() => {...}, [arg])
// lowered
$0 = FunctionExpression(...)
$1 = LoadLocal (arg)
$2 = ArrayExpression [$1]
$3 = Memoize (arg)
$4 = Call / LoadLocal
$5 = Memoize $4
```
Now, we insert `Memoize dep` before the corresponding function expression
literal:
```js
// lowered
$0 = StartMemoize (arg) <---- this moved up!
$1 = FunctionExpression(...)
$2 = LoadLocal (arg)
$3 = ArrayExpression [$2]
$4 = Call / LoadLocal
$5 = FinishMemoize $4
```
Design considerations:
- #2663 needs to understand which lowered instructions belong to a manual
memoization block, so we need to emit `StartMemoize` instructions before the
`useMemo/useCallback` function argument, which contains relevant memoized
instructions
- we choose to insert StartMemoize instructions to (1) avoid unsafe instruction
reordering of source and (2) to ensure that Forget output does not change when
enabling validation
This PR only renames `Memoize` -> `Start/FinishMemoize` and hoists
`StartMemoize` as described. The latter may help with stricter validation for
`useCallback`s, although testing is left to the next PR.
#2663 contains all validation changes
Joe Savona
Lauren Tan
Sathya Gunasekaran
Jan Kassens
Mofei Zhang