I realized that we can use our value block system to fix _most_ of the remaining
order-of-evaluation issues we had with sequence expressions. This PR adds a new
SequenceTerminal to HIR; there is already a ReactiveFunction equivalent
(ReactiveSequenceValue) that the next PR will convert this terminal into.
Handles some edge-cases where we previously flattened away some of the structure
of a labeled block, instead ensuring that we retain the original shape. See the
output.
## Test Plan
Tested on the internal app we're focused on (w useMemo inlining enabled), it
works fine.
This is part of a stack to fix some edge cases in inlining of useMemo closures.
In this first step, I'm disabling `shrink()` in order to retain more information
about the data flow. For example,
```
label: if (cond) {
break label;
}
return foo;
```
Would previously have shrunk away the if body, making the IfTerminal.consequent
point directly to the fallthrough block (w the return). Now we retain a separate
block.
I noticed some instances of this error when running forget on an internal
product. I previously fixed the case if a logical/conditional used only for side
effects (not assigned to a variable) but the new cases were assigned to an
unused variable. I double-checked and we’ve actually fixed all the steps after
these invariants so we can just remove them and support these cases.
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.
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.
NOTE: See background in #1476.
Updates BuildHIR to use the new LabelTerminal for LabeledStatements, and adds
support for HIR->ReactiveFunction transformation and codegen. Note that we
sometimes produce an extraneous block wrapper if it turns out the label wasn't
necessary, that seems...fine?
Adds a new `LabelTerminal` which will be used to represent LabeledStatements
that contain a statement other than a loop. What we do for these cases is
basically break the containing block in two, with a goto after the inner
statement to the fallthrough. This allows us to model the label, and any `break`
to it, in the HIR. However this fails in codegen because we can't find the
fallthrough branch — we need a high level terminal that knows about this
structure.
Hence LabelTerminal. Now, instead of just a continuation block and a goto, we
have a structured terminal. The LabelTerminal expresses the block for the
labeled statement and the continuation, and we can use this to put it back
together when constructing a ReactiveFunction. Note that this PR is just the
scaffolding for LabelTerminal, the next PR is the interesting bits.
Adds support for `for` statements with an empty or unreachable update
expression. In both cases, reversePostorderBlocks() will remove the
empty/unreachable update block, leaving the ForTerminal.update pointing to a
non-existent block. We explicitly rewrite this (much like we null out
unreachable fallthroughs after shrink). When transforming to ReactiveFunction,
we emit the update block as null if it was the same as the test block.
Implements HIR->ReactiveFunction conversion and Codegen for optional calls. We
add a new OptionalCall variant of ReactiveValue, which is a SequenceExpression
that describes the evaluation of the args and the call itself. This is then
straightforward to codgen.
Adds a new `optional-call` terminal and sets up the appropriate handling in the
visitors, with lowering/reactivefunction/codegen as todos for now and
implemented in follow-ups.
If a logical or conditional expression is unused, then a phi node isn't created
for the identifier it assigns to. Then when we leave SSA form the two branches
will assign to separate values, and we aren't sure which identifier to use as
the lvalue of the resulting ReactiveInstruction (remember that
logicals/conditionals decompose into control flow in HIR, but are a single
compound instruction in ReactiveFunction). If the two sides don't assign to the
same location, it could be because of a bug in the compiler or because the value
wasn't used. Ideally we'd represent this explicitly, but for now i'm just making
this a TODO since most logicals/conditionals should have their value used.
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.
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.
There's a bug in the HIR->ReactiveFunction conversion for certain categories of
compound value blocks where we replace operands (which must be a Place) with a
ReactiveValue. This approach worked in practice for lots of cases so I thought a
type coercion was safe, but then I found a case where this assumption breaks
(see new test).
The updated logic fixes the bug and is simpler. When a value block gets split up
(because there was a nested value block), instead of replacing the earlier value
in the later instructions, we append the instructions together. This can result
in some extra nesting (which if we wanted we could flatten away) but ensures
that we maintain type-safety.
A SequenceExpression currently doesn't store the InstructionId that produced its
final `.value`. This PR adds that instruction id, which is then used in the next
PR as we compose SequenceExpressions.
Changes ReactiveWhileTerminal’s test to use the new value block representation.
This means logical and condition expressions will work as while test values now.
Support conditional expressions from AST -> HIR -> ReactiveFunction -> AST. This
also helps make the patterns for value block handling more clear, so i was able
to extract some reusable logic in the HIR -> ReactiveFunction conversion phase.
Implements the conversion from LogicalTerminal into a ReactiveLogicalValue (and
ReactiveSequenveValue if necessary). The implementation is a bit rough, i clean
it up in subsequent PRs which revealed parts of the logic that could be shared w
ternaries.
Changes the lowering for LogicalExpression to use the new 'logical' terminal.
Whereas before we tried to more directly model the semantics of `??` by
generating an `if (<lhs> != null)`, we now generate a branch terminal that looks
at the lhs.
The HIR -> ReactiveFunction construction for logical and branch terminals are
placeholders while I refactor the ReactiveFunction representation to support
value blocks.
As discussed, this repurposes OtherStatement as a catch all variant for
unsupported syntax or errors in the source. This also renames the previously
added ErrorTerminal to UnsupportedTerminal for consistency (plus makes it a
little bit less confusing that it's not an actual terminal representing an
Error).
Not loving the name but couldn't think of anything better, open to suggestions!
Previously when converting from HIR -> ReactiveFunction we elided break/continue
terminals in places where control would implicitly transfer to the
break/continue target and therefore nothing has to be emitted. The one downside
of this approach is that it makes scope analysis a bit trickier. We want to
close scopes once we see an instruction id past the end of the scope's range,
but these implicit breaks were causing us to miss some instruction ids. We
compensated for this, but it's helpful to keep the representation explicit and
discard these terminals later in codegen.
Collapses HIRTReeVisitor into BuildReactiveFunction, allowing us to remove the
generic interface and simplify the code. Note that because the visitor was
already not attempting to group instructions by scope anymore, the visitor code
was very straightforward. This is mostly replacing calls to `appendBlock(block,
instr)` with `block.push(instr)`.
The primary goal of this stack is to change HIRTreeVisitor to make it easier to
handle value blocks. That's complicated by the fact that the visitor is a
general-purpose visitor, used in several analysis passes including
BuildReactiveFunction (which translates HIR->ReactiveFunction while also
grouping instructions into scopes) and InferReactiveScopes (which is actually
two passes, one to align scopes to block boundaries, one to merge overlapping
scopes). The long-term goal then is as follows:
1. Make BuildReactiveFunction transform HIR->ReactiveFunction but _without_
reactive scopes.
2. Align scopes to block boundaries, but rewritten to operate on
ReactiveFunction
3. Merge overlapping scopes, again rewritten to operate on ReactiveFunction
4. Group statements within ReactiveFunction into ReactiveScopeBlocks (today this
occurs when constructing the ReactiveFunction).
This PR implements 1 and 4. Because the implementation is incomplete this would
break the whole compiler, so for now both versions are still around. By default
compilation uses the old pipeline, but if a feature flag is enabled we use the
new version. The plan is to incrementally fix up the new version of the passes
in this stack, and then cutover: removing the flag and the old version of the
passes.
Moves _some_ files from HIR into new top-level directories. To not make
@gsathya's life a pain I left the files he's touching alone, but I moved some
others. My intent is to have something like this:
* Babel/ - code for the Babel plugin, though ideally this actually gets split
into a separate package and the compiler itself is AST in, AST out w no Babel
dep.
* HIR/ - the core HIRFunction, HIR and related data types, plus the HIR
construction and printing, HIR visitors.
* Inference/ - the core inference passes that operate on the HIR, including type
inference, reference effects, alias analysis, mutable range analysis, etc. I'll
let @gsathya move these files when at a good stopping point.
* ReactiveScopes/ - inference relating to reactive scopes,
constructing/printing/codegenning ReactiveFunction
* SSA/ - enter/leave SSA and eliminate redundant phis
* Utils/ - every project needs a place to put stuff that doesn't fit into the
other categories, this is ours.
This leaves just index.ts at the top level, and overall feels pretty tidy. Not
too tedious to figure out where anything goes, hopefully.
Joe Savona
Lauren Tan