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.
I realized that codegen still had a fallback for generating identifier nodes for
unnamed temporaries. This PR updates codegen to throw if it needs to generate an
identifier for a temporary, and updates earlier passes to promote temporaries to
named values in all the cases that were missed:
* BuildHIR needs to promote temporaries for temporaries in destructuring
bindings and catch clause bindings
* PromoteUsedTemporaries has to promote temporaries for destructured function
parameters or function params that are context variables.
Uses an enum for Identifier.name to distinguish originally named identifiers vs
promoted temporaries. An opaque type for the named identifier variant makes it
hard to accidentally create that type.
When the compiler promotes temporary values to named variables, we currently
eagerly assign a name using the temporary's IdentifierId. This means that we're
sort of stuck with this name later in compilation, and RenameVariables can't be
100% sure whether a 't0' variable is a temporary or not. As a result, the names
of these promoted temporaries is influenced by how many temporaries we happened
to create during compilation (and what the next available identifier id was),
making them fluctuate more as we iterate on the compiler.
This is an RFC for showing how we can stabilize these names. The key elements:
* Distinguish promoted temporaries from other named identifiers. Here we use a
hack, naming them starting with '#t' or '#T', since '#' isn't a valid identifier
starting point. This lets us keep all of our logic that looks for non-null
identifiers names to distinguish named/unnamed, while also distinguishing real
names from generated names (if this was Rust, we'd use an Enum and have a
"isNamed()" method on it that was true for real/temporary names and false
otherwise)
* In RenameVariables, detect generated names and fall back to generating the
next available `tN`-style name (or `TN` for JSX tags).
* To reduce thrash overall, RenameVariables no longer keeps a global "next id"
value that uses to distinguish all conflicting identifiers, instead we restart
at 0 whenever we find a conflict, and keep bumping until we find a free name.
Thus if both `foo` and `bar` had conflicts, we previously would end up with
`foo$0` and `bar$1` as the deduped names, but now will end up with `foo$0` and
`bar$0`.
## RFC
I'm open to feedback on the approach. Two main questions:
* How to annotate promoted temporaries. The most type-safe option is to change
`Identifier.name` to be a union of `{kind: 'named', value: string} | `{kind:
'promoted', value: string} | `{kind: 'temporary'}` though TS then wouldn't allow
`identifier.name.value` (even as nullable) since it doesn't exist on one of the
variants. Maybe we could type the temporary one as `{kind: 'temporary', value?:
null}` so the value has to be null but you can always access that property?
* ?? Other concerns about the approach? We could keep the global
auto-incrementing id rather than attempting to reset to 0 for each conflict.
The previous implementation used IdentifierId, but since this pass operates
after LeaveSSA the identifier ids are no longer distinct for different SSA
instances. Instead we use the Identifier instance, which preserves SSA
information (even ever LeaveSSA) and allows distinguishing between variables
whose value always changes vs variables that may be reassigned such that they
don't always invalidate.
In the future when we use HIR everywhere, this pass should use the HIR CFG to
understand that phi nodes whose operands all will always invalidate can also be
treated as always invalidating.
## Test Plan
Synced to www, 91 files have output changes
(https://fburl.com/everpaste/3e3hjpjs). I spot checked these and confirmed that
they are all from cases where there was already missing memoization of earlier
values, where we now can prune later reactive scopes that depend on the
un-memoized values.
Implements the optimization described in the previous PR: if we know that a
scope's dependency will _always_ invalidate (it is not memoized and it is
guaranteed to be a new object if the instruction executes, such as an array or
object literal), then we can prune that scope. The invalidation is transitive:
we track always-invalidating types from within scopes, and if their scope gets
invalidated we prune downstream scope that depend on them.
## Test Plan
Tested via #2639 - see https://fburl.com/everpaste/3e3hjpjs. 91 files change
output due to reactive scopes which would always invalidate due to always
invalidating dependencies.
These fixtures demonstrate how currently, even if the dependency of a scope
doesn't get memoized (ie the scope gets pruned), we don't remove later scopes
that depend on that value. Those later scopes will always invalidate, so we
might as well remove them.
Summary: Currently Forget bails on mutations to globals within any callback function. However, callbacks passed to useEffect should not bail and are not subject to the rules of react in the same way.
We allow this by instead of immediately raising errors when we see illegal writes, storing the error as part of the function. When the function is called, or passed to a position that could call it during rendering, we bail as before; but if it's passed to `useEffect`, we don't raise the errors.
Sathya Gunasekaran
Mofei Zhang
Jan Kassens
Joe Savona
Lauren Tan
Michael Vitousek
Jordan Brown