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.
Add a `logger` option so compiler errors are surfaced in our metrics collection
pipeline.
We can probably later merge the global `log` function with it.
Adds a new `StoreLocal <kind> <place> = <value>` instruction which stores
<value> into <place>. With this change, Instruction.lvalue is _always_ a `const`
temporary, and never a named identifier (there's a new validation pass to assert
this). StoreLocal is the only way to declare or update a named identifier: the
instructionKind property says whether it's a const/let declaration or a
reassignment. Naturally a _lot_ of passes had to be updated to make this work,
but the existing Effect.Store variant that @gsathya added made this overall
straightforward.
Note that as of this PR several passes still have code to handle the possibility
of an instruction lvalue being something other than a temporary. When we clean
that up in a follow-up, there will be a lot less of the duplication that appears
here. For example, CodegenReactiveFunction has two places to handle variable
declarations in this PR. However, one of them is to handle lvalues, which should
now _always_ be temporaries and never emit a regular variable declaration.
Similarly, several passes have to build up a table of identifier -> identifier
(because of LoadLocal). Longer-term, we should update the Place abstraction so
that it directly specifies the instruction which created that temporary, so we
can look it up on demand instead of needing an extra mapping.
Adds support for DoWhileStatements. It's pretty similar to how we handle While,
except in the case where a test block is unreachable (for example, an early
unconditional `break` within the loop body). In this scenario we eliminate the
terminal altogether and replace it with a goto to the loop block.
Changes InstructionValue::Place to InstructionValue::LoadLocal for clarity, this
is intended as the only instruction where a variable can appear as an operand.
All other instructions operands will be temporaries.
Joe Savona
Lauren Tan
Sathya Gunasekaran
mofeiZ
Mofei Zhang
Tianyu Yao