---
(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.
This invariant interpolated values into the `reason` which prevent our internal
tooling from grouping related errors. This PR updates to make the reason static
and interpolate the description.
Fixes T173101142 — we previously computed incorrect function expression
dependencies for JSXMemberExpressions. This PR applies similar logic to
JSXMemberExpression as we use for MemberExpression.
## Test Plan
Synced internally, only one file changes output. I manually investigated to
confirm — the change is that a function expression's dependencies are more
precise and correct. See https://fburl.com/everpaste/4dqewxqv
---
No changes to snap or sprout's functionality.
Tweaks to consolidate sprout into snap while keeping its simple interface and
most developer patterns.
- to keep `filter` mode fast, we do not run sprout in filter mode
- sprout is run in non-filter mode for both test and update
~~Small qol improvement: `--watch` will start you in `filter` mode~~
### Cost of this change
`performance.now()` is quite noisy due to background processes and ThreadPool
logic (especially with asymmetric task distribution), so I used
`process.cpuUsage` which reports time spent in user-space. This was much less
noisy (1-4% standard dev / mean)
Running all tests becomes slower by ~50%. Initial runs are slower because they
load in Forget's `require` chains.
- 23.9s previous initial run
- 34.6s current initial run
- 11.5s previous subsequent runs
- 15.4s current subsequent runs
Running filtered tests remains very fast (~100ms on the average case)
---
Additional modes or commands could be added as needed (e.g. run tests in filter
mode, with sprout output)
Adds some test cases for hook calls in object methods. Initially we didn't catch
these because InferTypes doesn't actually visit ObjectMethod bodies. Once we fix
that we correctly reject these examples.
> Don’t call Hooks inside loops, conditions, or nested functions
Per https://react.dev/warnings/invalid-hook-call-warning#breaking-rules-of-hooks
it is invalid to call hooks inside function expressions. We now validate this by
default, i'll verify internally before landing.
Note the validation is somewhat more conservative and we only disallow known
hook calls here, this seems like a reasonable tradeoff but i'm open to
suggestions. We could reuse the same known/potential hook mechanism here but it
would take some more refactoring.
Updates the compiler to understand Flow hook syntax. Like component syntax, in
infer mode hooks are compiled by default unless opted out.
Looking ahead, i can imagine splitting up our compilation modes as follows:
* Annotations: opt-in explicitly
* Declarations: annotations + component/hook declarations
* Infer: annotations, component/hook declarations, + component/hook-like
functions
This also suggest an alternative annotation strategy: "use react" (or "use
component" / "use hook") as a general way to tell the compiler that a function
is intended for React. Then opting out of memoization could do "use
react(nomemo)".
Updates LowerReactiveScopes to rewrite to a ReactiveFunctionValue
(ReactiveFunction-based) instead of a FunctionExpression (HIR-based). This lets
us include terminals and even nested reactive scopes in the result.
Mofei Zhang
Joe Savona
Michael Vitousek
Jordan Brown
Sathya Gunasekaran