This PR changes BuildHIR to lower all operands to temporaries. Example:
```javascript
// Input
a + b;
// Previous Lowering
Const t0 = BinaryOperation Place(a) "+" Place(b)
// New Lowering
Const t0 = Place(a);
Const t1 = Place(b);
BinaryOperation Place(t0) "+" Place(t1)
```
This is necessary to ensure we're always referring to the correct version of a
variable, even in the case of reassignment mid-expression. For example, we
previously evaluated `let x=1; x + (x = 2) + x` incorrectly to 6 because we
lowered the `x = 2` prior to the binary operators. We now lowers each instance
of x to a temporary, ensuring they refer to the correct SSA version of the
variable, and produce the correct result (5).
Note that with this change, the _only_ place a variable can appear as an
operator is when the InstructionValue is a raw identifier. This was already the
case for globals (as of the LoadGlobal instruction). All other instruction value
variants will only ever receive temporaries as arguments.
This necessitated a few changes to our inference:
* The logic to extend the range of phi operands (if the phi is mutated) was
previously in LeaveSSA, but that was actually too late. The introduction of
lowering to temporaries help discover failing cases, which I fixed earlier in
the stack by moving the logic to extend the range of phi operands into the
InferMutableRanges fixpoint loop.
* PropagateScopeDependencies now has to track variable reassignments in addition
to tracking property accesses
* AnalyzeFunctions now has to track variable reassignments in addition to
tracking property accesses
* InferReactiveIdentifiers now needs a fixpoint iteration, because identifiers
don't directly appear together in the same instruction anymore (such that we can
directly propagate the reactivity between them). Instead, we'll first see that
the temporaries are reactive, and have to propagate that back to the identifiers
the temporaries were loaded from.
Overall while this does introduce a bit more complexity, it also makes the
compiler more robust. As with the phi example illustrates, there are legitimate
inputs that can create similar indirections to that introduced by lowering
identifiers to temporaries.
Note that there’s a theme to the changes here: several analysis passes need to
map an operand back to its identifier value. Ideally our HIR structure would
directly support looking up the value for a temporary. For example, if operands
were references to eg the index of the instruction that produced them. Because
we don’t have such a representation yet (it would fall out naturally if we were
writing in Rust), we have to do some bookkeeping. The key takeaway here is that
this bookkeeping is incidental complexity given our current representation, not
fundamental complexity of the algorithm.
I found this while working to ensure that we always lower all operands to
temporaries. This works:
```javascript
// the whole computation of x is memoized in one block, bc of the mutation after
the phi
let x;
if (cond) {
x = someObj();
} else {
x = someObj();
}
mutate(x);
```
However, if you alias either of the operands, we lose the mutation:
```javascript
let x;
if (cond) {
const y = someObj(); // OOPS this gets independently memoized
x = y;
} else {
x = someObj();
}
mutate(x);
```
The core issue is that InferMutableRanges does not take into account mutation of
phis. ~~My first thought is that we need an additional, outer fixpoint iteration
loop to flow mutation back "up" to phi operands~~
edit: there was a much easier fix, we need to alias phi operands and phi id
within the existing fixpoint iteration. See follow-up PR which fixes.
This is a precursor to validating that all identifiers are defined - we need to
know about gobals and module declarations, so this PR adds the ability to
configure a Set<string> of defined globals. The default list is inspired by the
globals that prepack defines, which just comes from the spec definition.
Updates BuildHIR to produce LoadGlobal instructions for references to globals.
Note that this breaks our previous strategy of finding hook calls: that relied
on looking at the callee of a CallExpression and checking its name, which relied
on the callee not being lowered to a temporary. By lowering the name (eg
`useState`) to a temporary first, we now no longer see the name at the callsite.
Thankfully @gsathya solved this for us already by teaching type inference about
hooks, and more generally implementing type inference. I updated this so that we
infer the type of a LoadGlobal if the name is a hook: the type inference picks
this up and propagates the type forward correctly. So now, all places that
needed to check for a hook can just look at the type and everything works.
This is much more robust than before - you can now reassign a hook to a local
variable and we'll still detect that when you call it, you're calling a hook.
Adds a new `LoadGlobal` InstructionValue variant which will be used to represent
identifiers that refer to globals. We don't construct this value type yet.
Updates the babel plugin so that environment options — including custom hook
definitions — can be passed in through the plugin:
* Renames `CompilerFlags` => `PluginOptions` since they are specific to the
babel plugin, and are no longer just flags.
* Moves the definition of `useFreeze()` out of the builtin hook list and instead
passes it when our unit tests configure the plugin.
InferReferenceEffects needs to be able to pass around the function's
Environment, but there is already a local class with that name. It's confusing
to have two "environment" concepts in one file, so this PR renames that local
class to the more appropriate `InferenceState` and renames local variables and
updates comments accordingly.
While reviewing @poteto's PR I noticed that there were some cases of missing
dependencies. I tracked it down to a bug I introduced
[here](https://github.com/facebook/react-forget/commit/5b827eb85ce0b09a72e620449d1d676071c2e0b9#r100646304).
Decl.id is meant to be the id of the instruction that declares the variable. We
then test to see if a dependency is later than that. If the Decl.id is
incorrectly too high, then we miss some dependencies thinking they aren't
defined yet.
This is to help prep for @poteto's renaming PR. To make that PR work we
generally need to use IdentifierId to distinguish "the same identifier" rather
than Identifier object identity.
InferReactiveIdentifiers has some extra logic to find identifiers declared in
the same scope, and promote non-reactive identifiers to reactive if they appear
inside a reactive scope (reactive scope == scope with one or more (reactive)
dependencies). Even though the identifier alone might not be technically
reactive (have no reactive inputs), it can get re-recreated if the scope
re-evaluates.
We can now do this during PruneNonReactiveDependencies as we exit out of each
scope.
I removed fixpoint iteration and all tests pass, which matches my intuition that
it's really that we need strictly two passes. Removing to simplify and for
performance (avoid unnecessary extra visits of the ast)
The fact that InferReactiveIdentifiers is integrated directly into
PropagateScopeDependencies has made the latter pretty tricky to debug at times.
If a dependency is missing, we have to introspect and figure out if that's
because it was somehow inferred as non-reactive. This PR creates a new
PruneNonReactiveDependencies pass to separate out these phases.
Optimizes dead code elimination. Currently it keeps iterating the control flow
graph until no new usages have been discovered, which accounts for usages across
loops. However, when there are no loops it's sufficient to iterate the CFG
exactly once.
Lauren Tan
mofeiZ
Joe Savona
Sathya Gunasekaran