We were treating Destructuring as if it could never allocate and therefore
didn't have to be memoized. That's only true if there are no rest spreads
though. This PR teaches the compiler to treat rest spreads differently for
scoping and memoization
purposes, fixing the newly added test case and some existing bugs.
Adds a new pass that uses escape analysis and React-specific heuristics to tune
the amount of memoization applied. Specifically, the pass ensures that we only
memoize:
* Values which escape (are directly returned or transitively aliased by a
returned value)
* ...and that are not JSX elements
* OR values which are _dependencies_ of scopes that produce an escaping value.
The latter case is necessary to avoid breaking memoization of an escaping value
bc a scope happened to have a non-escaping dependency.
## Algorithm
1. First we build up a graph, a mapping of IdentifierId to a node describing all
the scopes and inputs involved in creating that identifier. Individual nodes are
marked as definitely aliased, conditionally aliased, or unaliased:
a. Arrays, objects, function calls all produce a new value and are always marked
as aliased
b. Conditional and logical expressions (and a few others) are conditinally
aliased, depending on whether their result value is aliased.
c. JSX is always unaliased (though its props children may be)
2. The same pass which builds the graph also stores the set of returned
identifiers
3. We traverse the graph starting from the returned identifiers and mark
reachable dependencies as escaping, based on the combination of the parent
node's type and its children (eg a conditional node with an aliased dep promotes
to aliased).
4. Finally we prune scopes whose outputs weren't marked.
This was the actual bug. When EliminateRedundantPhis eliminates a phi, it has to
rewrite downstream usages of the phi id to the single operand id. We were
correctly doing that in all but one place. When we iterate _downstream phis_, we
were looking up the operands against the rewrite table, but not updating the phi
operands themselves to the rewritten value.
This fixes the bug, and incidentally fixes a test that has been broken for a
while and nagging at me.
Found while debugging the previous issue: `mapInstructionOperands()` should not
look at lvalues. The previous version was causing us to create extra phi nodes,
which interestingly weren't the actual problem behind the "SSA" bug, but sure
looked like it at first.
Refactors the representation of ObjectExpression properties from a Map to an
`Array<ObjectProperty>` to prepare for the next diff which adds spread element
support.
BuildHIR currently propagates UnsupportedNodes for collection types where the
element itself can fail (for example object expressions where the key may not be
valid). However, given that we currently abort compilation after the first
failing pass (and will probably do so for quite a while) I think we can simplify
and just always return the collection. Note that I already did this for
destructuring. I'm open to leaving the code as-is if you prefer, though.
This PR starts to clean up our handling of lvalues and rvalues by adding new
`eachInstructionLValues()` and `mapInstructionLValues()` helpers. Now,
`eachInstructionOperand()` and `mapInstructionOperands()` only visit true
rvalues, and the new passes must be used to visit lvalues. This allows us to
remove the special-casing for StoreLocal and Destructure in most of the passes.
Instead of replacing original function with compiled code, this adds an option
to append the code and switch between the two based on an `isForgetEnabled` test
condition that's imported from the specified gatingModule.
---
**This PR slightly changes the semantics of ReactiveScopeDependencies**.
Previously, reading a ReactiveScopeDependency is guaranteed to preserve the
`nullthrows` semantics of its own declarations (not that of its inner scopes).
This does not affect the overall correctness properties, since we already hoist
reading of conditional dependencies (and thus may throw earlier than the
original source).
E.g. we already do not preserve *where* the nullthrows occurs.
```javascript
function Component(props) {
// throws here, before print(x)
const c_0 = props.a.b !== $[0];
let x;
if (c_0) {
x = {};
print(x);
if (...) mutate1(x, props.a.b);
mutate2(x, props.a.b);
// ...
```
### Summary
This is an optimization, not a correctness property.
When propagating reactive dependencies of an inner scope up to its parent, we
want to *retain information about conditional dependencies* -- not the derived
unconditional dependencies. This helps us produce more granular dependencies in
the parent scope.
Current implementation:
```javascript
const innerScopeDeps = innerScope.depTree.deriveMinimalUnconditionalDeps();
for (const dep of innerScopeDeps) {
currentScope.depTree.addDep(dep);
}
```
New implementation:
```javascript
// union of a tree takes union of each node
currentScope.depTree = currentScope.depTree.union(innerScope.depTree);
```
### Example
In the below example:
- `scope @1` has a conditional dependency of `props.a.b`, but that reduces to
the unconditional dependency `props`
- `scope @0` itself has a unconditional dependency of `props.a.b`
- Currently, Forget joins the derived / reduced dependencies of inner scopes,
which adds `props` as unconditional dependency of `scope @0`
- With this change, Forget joins the property trees and retains info about
conditional deps, which adds `props.a.b` as a conditional dep of `scope @0`.
```javascript
// scope @0 (deps=[???] decls=[x, y])
let y = {};
// scope @1 (deps=[props] decls=[x])
let x = {};
if (foo) mutate1(x, props.a.b);
mutate2(y, props.a.b);
```
### Followup
We currently keep track of properties unconditionally accessed per
ReactiveBlock. Eventually we want to keep track of properties unconditionally
accessed across blocks (as according to control flow).
Consider the following code, in which sibling scopes 0 and 1 are sequentially
executed. In this case, we can safely add props.a.b as a dependency of scope 1.
```javascript
// scope@0 (deps=[props.a.b], decls=[x])
let x = { a: foo(props.a.b) };
// scope@1 (deps=[???], decls=[y])
let y = {};
if (...) {
mutate(y, props.a.b);
}
```
---
Implementation details summarized in comments.
Overall, we want to calculate a `ReactiveDependencyTree` for every conditional
block. If we know that conditional blocks are exhaustive (e.g. all CFG paths
calculates a tree), we can take `intersection(depsFromEachBlock)` and add this
to the parent Reactive + conditional scope `parentDeps = union(parentDeps,
intersection(...))`.
We use trees instead of individual deps here because we can still derive
unconditional accesses.
e.g.
```
let x = {};
// props.a is an unconditional access here
if (foo(other)) {
x.a = props.a.b;
} else {
x.b = props.a.c;
}
```
---
Small refactor of reactive dependency logic, no behavioral change.
- Moves ReactiveDependencyTree logic into `DeriveMinimalDependencies`.
- this moves hides most helper functions + types 🥳
- made `ReactiveDependencyTree` a class
- Changes `#dependencies` type: `Set<ReactiveScopeDep>` ->
`ReactiveDependencyTree`
- instead of collecting all dependencies into a tree in the end, we now eagerly
join dependencies into the tree on `visitDep`
- this is needed for the next PR in the stack, which relies on incremental
merging
In #1287 i implemented basic handling for destructuring in DCE: if any of the
pattern values are used, we retained the whole instruction as-is. However,
ideally we could prune out unused elements from the pattern. There are pretty
simple rules:
* ArrayPattern we can eliminate unused elements from the end.
* ObjectPattern we can eliminate any unused element, but only if there is no
rest element.
This is more followup toward deleting Instruction.lvalue. The previous PR
ensured that all Instruction.lvalue identifiers are only ever assigned to once.
Now we ensure that `lowerExpression()` is _only_ called via
`lowerExpressionToTemporary()`, ie we now always lower every single expression
to a temporary.
This will make it easier to make lvalue a part of the InstructionValue instead
of the instruction itself, in follow-up PRs.
As part of removing Instruction.lvalue we need to ensure that it is only used to
represent that instruction's value — the InstructionKind should always be Const.
The one place where we violated this was for value blocks, specifically
ConditionalExpression and LogicalExpression. For both of those, we generate a
single temporary place to represent the expression result. Then the consequent
and alternate branch ended in a `LoadLocal` that reassigned that temporary (in
the lvalue) to the result of that branch.
This PR changes to use StoreLocal instead, and updates the recently added
validation pass to ensure that all identifiers that appear in an
Instruction.lvalue are only ever assigned once.
Changes to explicitly model destructuring (array and object patterns), expanding
support to include rest elements and preserving destructuring through the
output. The new "Destructure" instruction is similar to "StoreLocal" but has a
pattern instead of a place. For now each level of nested array/object patterns
creates a separate destructure instruction, which ensures we have a temporary
Place to talk about the intermediate array/object and its type/effects etc.
Example:
```
// INPUT
const [x, {y}, ...z] = a; // yay rest elements work now!
// HIR
[1] <unknown> $2 = LoadLocal a$1
[2] <unknown> $6 = Destructure Const [ <unknown> x$3, <unknown> $4, ...<unknown>
z$5 ] = <unknown> $2
[3] <unknown> $8 = Destructure Const { y: <unknown> y$8 } = <unknown> $4
// OUTPUT
const [x, t0, ...z] = a;
const {y} = t0;
```
Note that we can still collapse to a single destructure statement during
codegen, independently of whether we have separate instructions internally. For
now i'm going w the simple approach of emitting multiple statements in codegen
(the code will very likely get further rewritten by downstream babel passes
anyway).
Also, I don't love the "if StoreLocal/Destructure else ..." pattern that the
StoreLocal created and that this PR entrenches. As discussed w @gsathya offline,
the long-term direction will be to add a separate visitor, roughly
`eachLValue()` and `eachOperand()` so that we can treat all instructions the
same. Existing Instruction.lvalue will go away and become a property of the
other types of instructions.
InferMutableRangesForAlias is about extending the mutable ranges, not for
updating the alias sets. Let's refactor this into a separate pass.
InferMutableRangesForAlias was iterating over alias sets and not the HIR so this
refactor isn't costing us any additional perf cost (in terms of an extra
iteration over the HIR).
Identifiers don't need skipping anyways, so this doesn't affect the existing
behavior.
In the future, we will special case handling of LHS of AssignmentExpression
which will require us to not skip the RHS.
The mutable range difference for assignment is just 1 which is something we
usually don't track as we care about mutation and not assignment.
But this isn't true for context refs whose (re) assignment is actually a
mutation.
Having undefined as the initial value makes this a primitive. There's a separate
bug where we need to remove type inference for captured refs that get mutated
but that's secondary -- we're currently not even marking the Identifier LHS as a
captured ref. Fix the test to repro this bug for now.
Lauren Tan
Joe Savona
Sathya Gunasekaran
mofeiZ
Mofei Zhang