---
> If this operand is used in a scope, has a dynamic value, and was defined
before this scope, then its a dependency of the scope.
> (from current comments in PropagateScopeDependencies::visitDependency)
A reactive scope can take a dependency from a definition produced by an
incomplete parent scope. Our tests previously did not cover this, since most
object types aliased together and remained mutable throughout a ReactiveScope.
e.g. our tests did not have
```
scope @0 (deps=..., declarations=[x, y]) {
x = {};
// define a reactive, immutable value that is not aliased to become mutable
const immutableVal = ...;
scope @1 (deps=immutableVal, declarations=[y]) {
y = read(immutableVal)
}
mutateX(x, ...);
}
```
We should not add a dependency if it is produced in exactly the same scope as
the one it is used. It is safe (and correct) to depend on values produced by a
parent scope.
---
Note that we still should check for whether a defining scope is active to
determine whether it should be added as a output of that scope
([src](https://github.com/facebook/react-forget/blob/b608ab20d57229b528deeffa19f1ee08a4bad37a/forget/src/ReactiveScopes/PropagateScopeDependencies.ts#L469-L478)).
Access of an identifier produced by a parent scope (i.e. adding a variable
defined by a scope's parent as its own dependency) does not require adding that
identifier to the parent's `declarations`, since that identifier is already
valid to access via identifier binding rules.
---
Following #1216:
If a value is known to be immutable, then it doesn't need to be considered
'captured' since no mutation should occur.
Couldn't figure out a unit test in which this specific fix matters, but we need
this to fix test output of #1273
cc. @gsathya, would love some feedback / eyes on this. This makes sense for
Primitives in particular (which are always read / copied in rval position), but
I'm not as familiar with edge cases for other immutable values especially around
lambdas.
---
Our current compiler has specific logic for determining what can be a reactive
value / reactive dependency.
Currently, all of the following affect whether an identifier is a reactive:
- **alias analysis** (applicable to objects)
- **data + control flow** (whether any other reactive identifiers is used in
determining it)
- **reactive scopes** (we generalize and say anything produced by a block with
reactive dependencies must be non-stable and reactive)
- this is not true in the case of const primitives, but an overestimate is safe
- whether the **scope that declares this identifier** is ~~currently active~~
the same scope in which it is used (fixed by #1275)
(since a scope cannot be dependent on itself)
These conditions are complex. We end up inferring most identifiers as `mutable`
and `object` types, which have different stability and aliasing properties from
primitives. As a result, we're missing some cases in our existing test coverage.
Test case output is fixed by #1274 and #1275
---
(This can be separated from the stack below, which implements conditional
dependencies. Happy to merge that first and open this as a new stack if that
produces a significantly better Git PR history.)
---
See comment block in `PropagateScopeDependencies` and added test case
`reduce-reactive-conditional-dependencies` for correctness properties /
dependency merging logic.
---
See comment block in `PropagateScopeDependencies` and added test case
`reduce-reactive-unconditional-dependencies` for correctness properties /
dependency merging logic.
---
We never use the `Place` of a ReactiveScopeDependency, except for when we want
to access its identifier. Later PRs in this stack will convert
`ReactiveScopeDependency` to property access trees (and traverse over the tree).
This usually involves merging multiple Dependencies into trees (where each root
is a unique identifier). We then traverse over each tree to extract its
dependencies (e.g. unconditional leaves).
```
{place: {loc: 1, identifier: 'props'}, path: ['a', 'b']}
{place: {loc: 2, identifier: 'props'}, path: ['a']}
// merges into a single tree root, which should represent a single identifier
```
The `place` of each individual `ReactiveScopeDependency` will be lost during the
tree traversal, and it doesn't really make sense to recreate them using the
`Place` attached to the tree root.
---
Patch and simplify logic around merging overlapping reactive dependencies.
Added `reduce-reactive-unconditional-deps` test fixtures, which tries to cover
all cases of merging unconditional dependencies (to a minimal dependencies set).
Please let me know if I missed any
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)
Sathya Gunasekaran
Lauren Tan
Mofei Zhang
mofeiZ
Joe Savona