Tidies up the implementation a bit, splitting the single function and class into
distinct computeDominatorTree() and computePostDominatorTree() functions and
helper classes.
React will retry or abort components that throw (depending on a few conditions),
so from React's perspective a `throw` statement is not a normal exit node. Thus
the Rules of Hooks really have a caveat: the set of hooks that are called _in an
execution that returns successfully_ must be consistent. Examples such as the
following are therefore allowed:
```javascript
function Component(props) {
if (props.cond) {
throw new Error(...);
}
useHook();
}
```
By modeling `throw` as an exit node, we rejected cases such as this. This diff
changes to not model throws as exit nodes. #1584 changes this to make it an
option, since some cases will want to consider throw as an exit node.
Incorporates the fixtures from eslint-plugin-react-hooks using a script, so that
we can easily update them in the future. For each fixture we run the compiler
with and without hooks validation first so that we know if the fixture is
expected to pass — we have some false positives and false negatives that i can
work through. For example we accidentally think that `userFetch()` is a hook,
oops. Fixtures that should pass but error, or that should error but pass, are
marked as `todo.<name>` or `todo.error.<name>`.
While i was here i added the ability to have fixtures in subdirectories for
grouping purposes.
See the code comments for more, but the basic idea here is that we use the post
dominator tree to find the set of basic blocks which are guaranteed reachable in
each function. Those are the only blocks where it is safe to call hooks, and we
error for hook calls in any other blocks.
Implements an efficient algorithm for computing the dominator (or post
dominator) tree of a CFG, following
https://www.cs.rice.edu/~keith/Embed/dom.pdf. This is used/tested in the next PR
to validate that hooks are called unconditionally.
note: I clean up the implementation quite a bit late in the stack in #1584
I noticed this while demoing Forget to React Org alum Christoph Nakazawa — in
array.map calls (and other APIs that take a lambda as input) we sometimes end up
memoizing the lambda. It's technically correct since the function _could_ return
the lambda, and then we'd need it to be memoized. It's tricky because array.map
is often called on nested objects, where even if we had type inference on the
outer value we wouldn't know for sure that the inner property is an Array and
not some other data type with a custom .map. For example in
`data.feedback.comments.edges.map(edge => ...)`, even if we knew that `data` was
an Object, we wouldn't know that data.feedback.comments.edges is an Array
without cross-file type knowledge.
But it's definitely wasteful to memoize these lambdas, so we should brainstorm
options. One option that stands out right away: if the lambda has zero
dependencies, then we could lift it out to module scope and refer to it by name.
Adds a validation pass to check that the only thing you can do with hooks is
call them. A follow-up PR (still early WIP) will check the other aspect of the
rules of hooks, that they are not called conditionally. That's a more involved
algorithm.
We previously disallowed OptionalMemberExpression inside a normal
MemberExpression, eg `(a?.b).c`. The new representation handles this case
correctly so we can remove the restriction.
Our previous lowering for OptionalMemberExpression reordered the evaluation of
properties, such that we had to restrict the allowed properties to those that
were safe for reordering. With the new representation we preserve order of
evaluation, so we can relax the restriction. This unblocks a few cases in an
internal product.
Now that _all_ optional expression types use the new representation, the
optionality of all PropertyLoad and ComputedLoad is modeled via control flow (in
HIR) and the structure of OptionalExpression (in ReactiveFunction). Thus we no
longer need the `optional` properties on these load instructions — they're
optional if they're part of an OptionalExpression.
Earlier PRs in the stack change the way we lower OptionalMemberExpression, but
only when they ultimately appear inside some OptionalCallExpression. This PR
ensures that _all_ OptionalMemberExpressions get the new lowering. Note that one
test case has what is arguably a regression, but the new behavior is also
reasonable: if we see both `a.b?.c` and `a.b.c.` as dependencies of a scope, we
previously inferred `a.b.c` as the dependency, but we now infer `a.b` as the
dependency. This isn't as optimal as what we had before, but it also seems good
enough for now. Also note that some cases are improved: `foo(a.b?.c)` would
previously have taken `a.b` as a dependency, we now take the full value of
`a.b?.c` as a dependency - more precise.
So overall i'm inclined to land and follow-up on the one regression, since the
overall model is more cohesive.
call
When we traverse an OptionalExpression in PropagateScopeDependencies, we
previously considered the entire value to be optional. With the changes in this
stack to more accurately model OptionalMemberExpression, the `object` portion of
an OptionalMemberExpression is now evaluated within the OptionalExpression. This
PR refines the handling of OptionalExpression accordingly, so that we only treat
the optional portion as conditional.
The previous OptionalCall terminal and reactive value kinds are now used not
just for optional calls, but for optional member expressions that appear within
an optional call. This PR renames those data types to OptionalTerminal and
OptionalExpression for clarity.
Extends the new modeling of the previous diff to OptionalMemberExpression. In an
example such as `a?.b?.c`, we now model that not only is the `.c` conditional,
but our control flow graph accurately reflects the fact that the `.c` is only
evaluated if `a.b` exists. Previously we knew it was conditional but the CFG
allowed a path from a being null through to evaluation of `.c`.
More accurately models nested OptionalCallExpression. Consider:
```javascript
a?.(b)?.(c)
```
Our previous representation modeled it such that we treated the second function
call as if it would be called regardless of whether `a` existed or not. We knew
that the second call was conditional, so our test output was correct, but the
control-flow graph didn't faithfully model the semantics. That bothered me.
The new representation correctly models the control flow, and the fact that if
`a` is null/undefined execution immediately aborts (not reaching the second call
at all, nor the evaluation of its args), and evaluates the whole outer
OptionalCallExpression to `undefined`.
Note that nested optional member expressions still have the previous model —
that's next to address.
A common idiom is to map over some possibly-missing list of items from a data
payload and fall back to an empty array:
```javascript
const renderedItems = data?.items?.map(renderItem) ?? [];
```
The way we were lowering OptionalCallExpression meant that in this case, we'd
end up with an OptionalCallTerminal as the terminal of the logical expression's
test block, which violates our internal invariant. Logical test blocks must end
in a Branch! This PR fixes the immediate issue, which is that the callee - in
this case `data?.items?.map` — was being lowered prior to the
OptionalCallTerminal instead of inside its test block. Changing that fixes the
shape of the IR and makes this example work.
As part of investigating this I realized that the way I originally handled
lowering of optional call isn't quite right. The difference isn't observable
unless we did more sophisticated DCE but we don't correctly model the fact that
if `data.items` is null that the `map()` call won't occur. That is technically
fine bc we do model the fact that the `map()` call is conditional, and notably
its arguments are only conditional dependencies. So it's good enough. But in a
follow-up I'll change to model the fact that `data.items` is null, that the map
call isn't reachable at all.
Fix the previous bug — this was a simple oversight, where FlattenScopesWithHooks
overrode `visitValue()` but failed to call `traverseValue()`. This meant that
when we reached compound expressions such as LogicalExpressions that we didn't
traverse into their nested values, and didn't see the hooks hidden there.
Repro of a bug in which we incorrect memoize hook calls that are inside logical
expressions (though the bug could occur for ternaries, optional calls, and
sequence expressions too).
This is an attempt to get down some of the principles and goals that we've had
partially written down, partially just thoroughly discussed amongst the team.
It's rough draft quality but better than nothing, and gives us someplace to add
to.
This can't be tested yet - we only support simple, safely re-orderable values as
case test values - but it will easy to overlook later so i'm adding now.
PropagateScopeDependencies is one of the few places we don't use the new visitor
infra for traversing ReactiveFunction. Or rather it _was_!
Note that there's a bit less value here than in other places since we have to
handle each terminal variant with custom logic, but at least it's more
consistent with the rest of the codebase now.
We previously didn't support ternaries whose value was unused, so we had an
extraneous temporary and console.log call to ensure the value counted as used.
We now special-case ternary/conditional expressions which are in an
ExpressionStatement to not prune them, so the temporary and log are now
unnecessary.
Defines common `console` methods to tell the compiler that they take readonly
args. This ensures that things like `console.log()` aren't accidentally viewed
as a mutation. Previously the pattern of "build object, then log it after
mutation is done" would have grouped the console.log as part of the mutation and
the log only would fire if the value got reconstructed. Now we know the log
isn't mutating, and the log will happen regardless of whether the value is
rebuilt or cached.
Updates two points in the compiler that were easy to miss when adding new
terminals:
* HIRBuilder's `removeUnreachableFallthroughs()` nulls out unreachable
fallthroughs, but this had a non-exhaustive `if` statement. It now uses a helper
function which internally has an exhaustive switch.
* LeaveSSA needs to schedule block fallthroughs, but had a non-exhaustive `if`
statement. It also uses a helper function which internally has an exhaustive
switch.
cc @poteto since you ran into this (ie the compiler not alerting you to update
these places) w your diffs.
Discovered this in a recent attempt at syncing Forget to Meta, it seems
that calling path.stop() is unsafe as it appears to have strange
behavior in plugins that come after. This resulted in `import type
{...}` not being compiled away in the post-babel output which isn't
valid JS syntax. Removing the `stop()` calls fixes it
Test plan: made these changes locally, synced my local changes to Meta and reran
- in simulator and observe that it now runs and doesn't throw a syntax error
This is a more general version of the change from #1521. That PR ensured that
LoadLocal temporaries accessed outside the instruction's scope are correctly
promoted. However, we have a similar pattern with PropertyLoad.
This PR adds a general mechanism for handling these type of indirections: any
LoadLocal/PropertyLoad temporary accessed when it's defining scope is not active
will be promoted to a declaration of the defining scope. Notably, we do this in
a way that ensures that the dependencies are preserved, ie that we correctly
view the operand of LoadLocal/PropertyLoad as a dependency of the current scope.
Supports EmptyStatement nodes by ignoring them. Note no tests because
format-on-save clears away any empty statements and there is, rather
frustratingly, no way to tell VSCode not to format on save for a specific file
via the file contents itself or project configuration (at least, not that i can
find).
Uses the new ExpressionStatement instruction to ensure that logical and
conditional expressions are never pruned. This addresses an issue where we were
unable to construct a ReactiveFunction for unused logical/conditional bc there
wasn't a single Identifier assigned in both branches. The ExpressionStatement
ensures that the result is used, that we don't prune the phi, and that both
branches have a single assignment target.
In theory we could be more sophisticated with DCE and still prune these
instructions if their operands are also safe to prune, but in practice you're
only like to have a logical/conditional as an expression statement (in the
source) if it's for side effects.
Adds an `ExpressionStatement` instruction variant to model values that are
otherwise "unused" but which we don't want to remove. The next diff changes
BuildHIR to use this where appropriate.
We previously represented JsxExpressions using builtin tags - `<div>`, `<b>` etc
- by lowering the tag name to a Primitive with the string name of the tag.
However, by lowering into an independent value, it was possible that the lowered
tag name could be grouped into a different memo slot, such that we ended up with
output like:
```javascript
let t0;
if (c_1) {
...
t0 = "div"
...
} else { ... }
return <t0>{children}</t0>
```
This is obviously wrong. It's also wrong to rename `t0` -> `T0`, because React
treats that as a custom component, not a builtin. The right thing is to
explicitly model builtin components, which this PR does by making
`JsxExpression.tag` be a union of Place | BuiltinTag.
Ensures that temporaries used in JsxExpression tags are named with a capital
letter so that they are treated as custom components rather than builtins.
Joe Savona
Mofei Zhang
Sathya Gunasekaran
Lauren Tan