---
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
Remove private header from playground
Before we miss removing this from the public release, I think we can remove this
header now already. We're still behind a secret URL + password.
Fixes T180504437. We expected `<fbt:param>` to always have no surrounding
whitespace or have both leading and trailing whitespace, it can have one but not
the other, though such cases are rare in practice.
Repro from T180504728 which reproduced internally and on playground, neither of
which have #2687 yet. That PR (earlier in this stack) already fixes the issue,
so i'm just adding the repro to help prevent regressions.
While i'm here, we know that there are a variety of cases that are not supported
yet around combining value blocks with other syntax constructs. Since we're
aware of these cases and detect them, we can make this a todo instead of an
invariant.
We need to revisit the conversion from value blocks into ReactiveFunction. Or
just revisit ReactiveFunction altogether (see my post about what this would look
like). For now, makes this case a todo.
"Support" in the sense of dropping these on the floor and compiling, rather than
bailing out with a todo.
We already don't make any guarantees about which type annotations we'll preserve
through to the output, so it seems fine for now to just drop type aliases.
I addressed some of the cases that lead to this invariant but there were still
more. In this case, we have scopes like this:
```
scope @1 declarations=[t$0] {
let t$0 = ArrayExpression []
if (...) {
return null;
}
}
scope @2 deps=[t$0] declarations=[t$1] {
let t$1 = Jsx children=[t$0] ...
}
```
Because scope 1 has an early return, PropagateEarlyReturns wraps its contents in
a label and converts the returns to breaks:
```
scope @1 declarations=[t$0] earlyReturn={t$2} {
let t$2
bb0: {
let t$0 = ArrayExpression []
if (...) {
t$2 = null;
break bb0;
}
}
}
scope @2 deps=[t$0] declarations=[t$1] {
let t$1 = Jsx children=[t$0] ...
}
```
But then MergeReactiveScopesThatInvalidateTogether smushes them together:
```
scope @1 declarations=[t$1] earlyReturn={t$2} {
let t$2
bb0: {
let t$0 = ArrayExpression [] // <--- Oops! We're inside a block now
if (...) {
t$2 = null;
break bb0;
}
}
let t$1 = Jsx children=[t$0] ...
}
```
Note that the `t$0` binding is now created inside the labeled block, so it's no
longer accessible to the Jsx instruction which follows the labeled block. This
isn't an issue with promoting temporaries or propagating outputs, but a simple
issue of the labeled block (used for early return) introducing a new block
scope. The solution here is to simply reorder the passes so that we transform
for early returns after other optimizations. This means the jsx element will
basically move inside the labeled block, solving the scoping issue:
```
scope @1 declarations=[t$1] earlyReturn={t$2} {
let t$2
bb0: {
let t$0 = ArrayExpression [] // ok, same block scope as its use
if (...) {
t$2 = null;
break bb0;
}
let t$1 = Jsx children=[t$0] // note this moved inside the labeled block
}
}
```
I addressed some of the cases that lead to this invariant but there were still
more. In this case, we have scopes like this:
```
scope @1 declarations=[t$0] {
let t$0 = ArrayExpression []
if (...) {
return null;
}
}
scope @2 deps=[t$0] declarations=[t$1] {
let t$1 = Jsx children=[t$0] ...
}
```
Because scope 1 has an early return, PropagateEarlyReturns wraps its contents in
a label and converts the returns to breaks:
```
scope @1 declarations=[t$0] earlyReturn={t$2} {
let t$2
bb0: {
let t$0 = ArrayExpression []
if (...) {
t$2 = null;
break bb0;
}
}
}
scope @2 deps=[t$0] declarations=[t$1] {
let t$1 = Jsx children=[t$0] ...
}
```
But then MergeReactiveScopesThatInvalidateTogether smushes them together:
```
scope @1 declarations=[t$1] earlyReturn={t$2} {
let t$2
bb0: {
let t$0 = ArrayExpression [] // <--- Oops! We're inside a block now
if (...) {
t$2 = null;
break bb0;
}
}
let t$1 = Jsx children=[t$0] ...
}
```
Note that the `t$0` binding is now created inside the labeled block, so it's no
longer accessible to the Jsx instruction which follows the labeled block. This
isn't an issue with promoting temporaries or propagating outputs, but a simple
issue of the labeled block (used for early return) introducing a new block
scope. The solution (in the next PR) is to simply reorder the passes so that we
transform for early returns after other optimizations. This means the jsx
element will basically move inside the labeled block, solving the scoping issue:
```
scope @1 declarations=[t$1] earlyReturn={t$2} {
let t$2
bb0: {
let t$0 = ArrayExpression [] // ok, same block scope as its use
if (...) {
t$2 = null;
break bb0;
}
let t$1 = Jsx children=[t$0] // note this moved inside the labeled block
}
}
```
This was an oversight in codegen. The entire pipeline supports multiple values
in a for initializer, but codegen was dropping all but the first initializer.
Fixes T180504437. In MergeOverlappingReactiveScopes we track the active scopes
and mark them as "ended" when reaching the first instruction after their mutable
range. However, in cases of interleaving that will be merged, we could
previously mark a scope as complete when it's original range was completed, even
though the range would get extended post-merge. The fix here detects
interleaving earlier, and eagerly updates the mutable ranges of the merged
scopes to ensure that neither is "ended" earlier than it should.
The repro here fails without this change.
Fixes T175282980. InferReactiveScopeVariables had logic to force assigning a
scope to MethodCall property lookups with the idea of forcing the method call
lookup to be in the same scope as the method call itself. But this doesn't work
if we never assign a scope to the method call! That can happen if we're able to
infer that the method call produces a primitive and doesn't need memoization.
This PR changes things so that:
* InferReactiveScopeVariables no longer assumes that MethodCall property values
need a scope
* We run a separate pass that ensures that _if_ a MethodCall has a scope, that
it's property is in the scope, and that otherwise its property doesn't get a
scope. This is similar to the existing passes that force a single scope for
related instructions like ObjectMethod+ObjectExpression and fbt operands/calls.
Fixes T180509722. What happened is that the logic in LeaveSSA to find
declarations within for initializers wasn't working with try/catch because the
initializer block gets broken up with a maybe-throw after every instruction that
can throw. These maybe-throws can then get turned into gotos by
PruneMaybeThrows, so LeaveSSA has to handle both cases.
The new logic scans from the start of the init block until reaching the end, and
creates declarations for all StoreLocals. Note that we don't yet support
maybe-throw in value blocks — that's already a todo — so the change here simply
allows us to compile farther until reaching that other todo. But i've
double-checked the HIR and it looks correct for this case, so it should just
work once we fix that todo. I've also added a comment to help us remember (and
of course, we'd have to add a snap fixture too)
---
(This came out of running a sync and observing hundreds of bailouts due from
this validation)
Reading `fnType` from environment overgeneralizes, as inner functions are
usually not the type of the outer react function.
```
// Component type
function Component() {
// not Component type
const helper = () => {...};
}
```
Let's attach fnType to `HIRFunction` and use that for our inference +
validations
Fixture from T175283039, a reassignment within an expression can sometimes
generate a StoreLocal within a value block. Depending on the case this can end
up as the last instruction of the block, which then hits an invariant.
Adds a todo in HIRBuilder, before we prune unreachable code we check if there
were any function expressions. Realistically that's only going to occur for
hoisted functions, so this lets us target a todo rather than hit an invariant.
For T175282529. We already detect hoisted functions and have a specific todo for
them, but in this case the function is in unreachable code that gets pruned
during BuildHIR. The later check for hoisted functions doesn't find it.
One of our visitors wasn't visiting TryTerminal's handlerBinding, which meant
that we missed renaming those identifiers in RenameVariables. I also updated the
printers to print this binding.
Within a try/catch, every instruction is followed by a maybe-throw terminal.
This currently breaks the logic in BuildReactiveFunction which tries to
reassemble the value block, since it isn't expecting the maybe-throw.
Conceptually the logic should just ignore it — we could even flatten away
maybe-throw terminals before this pass — but for now since this pattern is rare
we can just make it a todo.
For T181507827 — adds an invariant in codegen when emitting identifiers to
ensure that we only create babel Identifier nodes for nodes that the compiler
has explicitly promoted to valid, named identifiers. This means that we'll fail
for unnamed temporaries (previously caught), as well as promoted temporaries
that somehow didn't get renamed by RenameVariables (newly caught).
Adds a visitor to collect all the globals that are referenced within the
function, and then uses this list to avoid synthesizing variables with
conflicting names. This is used in both RenameVariables (for promoted
temporaries) and Codegen (for `$` and change variables only, so far, but this
can be extended in follow-ups).
This is a key part of avoiding generating conflicting names in our output. To
start, RenameVariables now returns a Set of the unique identifier names that
exist in the function. Codegen uses this to avoid generating duplicate names for
change variables and for the `$` useMemoCache variable. Rather than always emit
`$` or `c_N`, codegen checks that this name would not conflict and appends an
incrementing suffix until it finds a unique name.
Note that it's still possible for us to generate conflicts with global
variables, both during RenameVariable and Codegen. The next step will be to
avoid conflicts with globals.
Another title for this PR could be "Yet another reason for HIR-everywhere"
ReactiveFunctionVisitor doesn't traverse into HIRFunctions from
FunctionExpression and ObjectMethod values. This means that
PromoteUsedTemporaries and RenameVariables also weren't traversing into such
functions, and those values weren't getting promoted and renamed correctly.
This PR updates ReactiveFunctionVisitor with a method that can optionally be
invoked to traverse an HIRFunction and call the appropriate visitor methods.
PromoteUsedTemporaries and RenameVariables invoke this to ensure they visit all
places, even in nested HIRFunctions.
Mofei Zhang
Joe Savona
Sathya Gunasekaran
Jan Kassens
Lauren Tan