Enables support for assignment expressions in value blocks (which includes in
loop init/test/update blocks). This was pretty straightforward, the main changes
are:
* During PropagateScopeDependencies, we currently record scope reassignments
based on `Identifier` object identity. In the case where a variable is
reassigned in multiple control-flow paths of a value block, however, there can
be multiple object identities. So we now de-dupe reassignments based on
identifier id.
* MergeOverlappingScopes now treats value blocks as regular blocks, allowing it
to correctly merge scopes from the value with other scopes from the outer block.
Otherwise this is mostly just lots of tests. Note that there is an outstanding
todo, which is that we currently error for ternaries and logicals whose value is
unused (eg `cond ? (x = 1) : null`). I'll address that in a follow-up.
This PR clarifies the logic for adjust mutable ranges of phis and their operands
during LeaveSSA. Previously we had logic in several places to determine
whether/how to extend the ranges of each phi and its operands: this occurred
while traversing reassignmentPhis (in 2+ places) and rewritePhis, as well as in
rewritePlace().
This was kind of a band-aid to make things work, but the logic was imprecise.
The actual rules are as follows:
If there is a back-edge, or the phi id is unnamed, then were extend the ranges
of the phi and its operands to min(starts) and max(ends). This ensures that the
operands are computed as one unit, ie put into a single reactive scope. For
loops this is necessary because...looping! For unnamed values this is necessary
because of the way we collapse logical and ternary expressions back to a
hierarchical ReactiveFunction — we need to make sure the final mutable range
extends from the start of the final instruction up to the end of the
logical/ternaries value blocks.
Otherwise this is a phi where operands come from predecessors and are named. If
the phi is mutated later, then we have to extend the end of each operand's range
to account for the fact that they can be mutated later. Else, we leave the
operands alone.
Behavior doesn't change, but we consolidate all of the mutable range logic in
one place.
Makes JSX memoized by default again, but adds an option to disable memoization
of JSX. Also adds a new test and fixtures directory to test the opt-in
no-jsx-memoization behavior.
---
Currently, we run type inference passes early in the pipeline and do not check
inference output in any tests, test fixtures, or verifier passes. In fact, the
only ways to view inferred types are (1) locally add a test fixture with`@only`
and inspect console logs or (2) scroll to the relevant section on a playground
example.
However, inferred types and effects significantly affect the output of later
passes (Alias / MutableRange analysis, InferReactiveIdentifiers, etc), and we
have already found some bugs due to incorrect inference (e.g. #1274).
This PR add the `typer-tests` fixture with the following goals
1. Record relevant current compiler type + effect inference output.
2. Have relatively stable output (with respect to changes in HIR and PrintHIR).
- we try to achieve this by annotating the source code.
---
Currently, we run type inference passes early in the pipeline and do not check
inference output in any tests, test fixtures, or verifier passes. In fact, the
only ways to view inferred types are (1) locally add a test fixture with`@only`
and inspect console logs or (2) scroll to the relevant section on a playground
example.
However, inferred types and effects significantly affect the output of later
passes (Alias / MutableRange analysis, InferReactiveIdentifiers, etc), and we
have already found some bugs due to incorrect inference (e.g. #1274).
This PR add the `typer-tests` fixture with the following goals
1. Record relevant current compiler type + effect inference output.
2. Have relatively stable output (with respect to changes in HIR and PrintHIR).
- we try to achieve this by annotating the source code.
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).
Joe Savona
Mofei Zhang
Lauren Tan
Sathya Gunasekaran
mofeiZ