"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.
Infer if a function is a component or hook when we're deciding to compile a
function and store that in the environment.
This is used in passes like InferReferenceEffects rather than having to re-parse
the name in each pass.
Previously, Forget would throw if _any_ of the arguments to a component are
modified. This isn't quite right as a ref argument can be modified.
This PR assumes the second argument of a component to be a ref and allows it to
be mutable.
A future PR will add types to this argument so the validateRefAccessDuringRender
can catch if ref is mutated in render. This PR contains a todo test for this.
Rather than force scopes to be created for primitives within
InferReactiveScopeVariables, here we move the creation of scopes for these
instructions to a later pass. Later in the pipeline we have more context, such
as whether e.g. a primitive or propertyload is being accessed within a scope or
not, and whether it therefore needs its own scope or not.
Currently we allocate all reactive scopes during a single pass,
InferReactiveScopeVariables, using a local incrementing number to assign
ScopeIds. This means we can't easily create additional scopes later since we
don't know the next available scope id.
Here we add `Environment.nextScopeId` and use that to synthesize scope ids.
Looking up certain properties on a hook is a common pattern for logging.
It's non-ideal but it's not a bug to do this.
This updates Forget to not error on this pattern.
filepath
Internal rollout currently has a good number of test failures.
`enableEmitInstrumentForget` can help developers understand which functions /
files they should look at:
```
// input
function Foo() {
userCode();
// ...
}
// output
function Foo() {
if (__DEV__ && inE2eTestMode) {
logRender("Foo", "/path/to/filename.js");
}
const $ = useMemoCache(...);
userCode();
}
```
The code for value block handling assumes a small set of terminal kinds, but
try/catch causes the entire body to get wrapped in MaybeThrow terminals. We need
to skip over these and delegate to the inner content.
Joe Savona
Jan Kassens
Lauren Tan
Mofei Zhang
Michael Vitousek
Jordan Brown
Sathya Gunasekaran