See the previous PR, interleaved mutation can cause values that were not
reactive to become reactive. I swear I had a case where this was observable, but
I came up with it before reordering the PRs in this stack. I think my repro
relied on an immutable reference to a mutable value, which is now handled in
InferReactivePlaces. So here i'm just adding fixtures, and allowing this case
since it's unobservable.
During PruneNonReactiveDependencies, we sometimes need to promote a value from
non-reactive to reactive if it ended up being grouped in the same reactive scope
as some other reactive value. This generally happens due to interleaving
mutations.
In this case all downstream usage of the promoted value need to also be
considered reactive. Fully propagating the reactivity requires re-running
InferReactivePlaces, to account for things like control reactivity. We can't yet
reuse that pass here though, because we haven't unified the pipeline on HIR yet.
For now, we propagate the reactivity through local variables and downstream
reactive scopes. See test fixtures for some examples that now correctly
propagate reactivity and some that need the full reactivity inference to run
correctly. The latter cases are handled in the next PR.
I found this by adding logic to reject inputs where reactivity gets newly
propagated in PruneNonReactiveDependencies. It's possible to create a readonly
alias to a mutable value such that we don't know the value is reactive yet when
the alias is created. Thus we need to do a fixpoint iteration even if there are
no loops in order to be able to revisit such aliases and reflow the reactivity
forward. Example:
```javascript
const x = [];
const y = x;
const z = [y]; // y isn't reactive yet when we first visit this, so z is
initially non-reactive
y.push(props.value); // then we realize y is reactive. we need a fixpoint to
propagate this back to z
const a = [z]; // need an indirection to get past the partial propagation in
PruneNonReactiveDependencies
let b = 0;
if (a[0][0]) {
b = 1;
}
return [b];
```
Existing fixtures don't change because the basic reactivity propagation in
PruneNonReactiveDependencies is enough to make common cases work. I confirmed
that the new fixture does not work on previous PR in the stack.
Fixes T175227223. When inferring reactivity, mutation of a value with a reactive
input marks the mutable value as reactive. However, we also need to account for
aliases:
```javascript
const x = [];
const y = x;
y.push(props.value);
```
Previously we would have only considered `y` reactive here, but `x` also becomes
reactive.
The implementation extracts out a helper from InferReactiveScopeVariables that
builds a `DisjointSet<Identifier>` of disjoint sets of mutably aliased values.
InferReactivePlaces then treats all instances of each mutable alias group as
equivalent for reactivity purposes.
In InferReactivePlaces, we already account for reactively controlled values:
where a value is never assigned a non-reactive value, but _which_ value is
assigned is based on a reactive condition (the test conditions of an if, switch,
loop, etc).
This PR extends that reactively-controlled inference to mutation that is
conditioned upon a reactive value. From the test case:
```javascript
let x = [];
if (props.cond) {
// This mutation has no reactive inputs.
// *But* the mutation conditionally occurs based on props.cond which is reactive
x.push(1);
}
let y = false;
if (x[0]) { // therefore the value observed here is reactive
y = true;
}
// so the value of y here is reactive via the reactive control dependency x[0]
return [y];
```
---
Previously, our logic was something like:
```js
fixed-point-loop {
foreach instruction {
mark referenced identifiers
// assume that usages are always visited before declarations
if (instruction is decl) {
prune(instruction);
}
}
foreach instruction {
if not referenced {
delete(instruction);
}
}
```
This contained a bug, as not all usages of a variable are guaranteed to be
visited before its declaration.
```js
// input
let x = 0;
while(x < 10) {
x += 2;
}
return x;
// hir
entry:
x$0 = 0
goto loop-test
loop-test:
x$1 = phi(x$0, x$2)
if ... goto loop-body else goto fallthrough
loop-body:
x$2 = x$1 ...
goto loop-test
fallthrough:
return x$1
```
In this example,`x$2` is defined by `loop-body` and used by `loop-test`.
Similarly, `x$1` is defined by `loop-test` and used by `loop-body`.
---
TODO: trying to come up with more test fixtures
Same babel identifier issue as #2510 but for HoistedConst
Not sure how we should best test this -- one possibility is using constant prop.
Currently, we have false positives for HoistedConst that prevent constant
propagation. I don't want to over-rotate on babel apis tests in our fixtures
(instead of semantically interesting ones)
```js
// input
function Component() {
{ x: 4 };
const x = 2;
return x;
}
// output
function Component() {
const $ = useMemoCache(1);
let x;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
x = 2;
$[0] = x;
} else {
x = $[0];
}
return x;
}
```
identifiers
---
A few fixes for finding context identifiers:
Previously, we counted every babel identifier as a reference. This is
problematic because babel counts every string symbol as an identifier.
```js
print(x); // x is an identifier as expected
obj.x // x is.. also an identifier here
{x: 2} // x is also an identifier here
```
This PR adds a check for `isReferencedIdentifier`. Note that only non-lval
references pass this check
```js
print(x); // isReferencedIdentifier(x) -> true
obj.x // isReferencedIdentifier(x) -> false
{x: 2} // isReferencedIdentifier(x) -> false
x = 2 // isReferencedIdentifier(x) -> false
```
Which brings us to change #2.
Previously, we counted assignments as references due to the identifier visiting
+ checking logic. The logic was roughly the following (from #1691)
```js
contextVars = intersection(reassigned, referencedByInnerFn);
```
Now that assignments (lvals) and references (rvals) are tracked separately, the
equivalent logic is this. Note that assignment to a context variable does not
need to be modeled as a read (`console.log(x = 5)` always will evaluates and
prints 5, regardless of the previous value of x).
```
contextVars = union(reassignedByInnerFn, intersection(reassigned,
referencedByInnerFn))
```
---
Note that variables that are never read do not need to be modeled as context
variables, but this is unlikely to be a common pattern.
```js
function fn() {
let x = 2;
const inner = () => {
x = 3;
}
}
```
I sincerely appreciate the effort to get test262 up and running. This was my
idea, it seemed like a really good way to test our correctness on edge cases of
JS. Unfortunately test262 relies heavily on a few specific features that we
don't support, like classes and `var`, which has meant that we never actually
use this as a test suite.
In the meantime we've created a pretty extensive test suite and have tools like
Sprout to test actual memoization behavior at runtime, which is the right place
to invest our energy. Let's remove?
Do we still use these? I'm happy to close this PR if we still want this but it
feels like these may have served their purpose and no longer be necessary.
Fixes the false positive in the previous PR. When we prune a scope because it's
values are non-escaping, we now also remove any `Memoize` instructions for that
scope. The intuition being that we're actively removing unnecessary memoization,
so we don't need to check that the memoization occurred anymore.
This demonstrates a false positive in validatePreserveExistingManualMemoization.
We prune memoization of non-escaping values, but the validation pass just sees
that the value "should" have a scope and that scope doesn't exist, and thinks we
failed to preserve memoization.
Interpolating values into the `reason` field of an error breaks our error
aggregation. This PR moves the offending function name into the `description`
field which isn't used for aggregation.
I had trouble checking out the repo using Sapling because the submodule couldn't
clone properly. I got
> Error: Permission denied (publickey)
In my branch I tested that the https URL format seems to work okay with Sapling.
Add frozen reason for props and hook arguments
Improves the error message when mutating props or hook arguments.
Previously, this would print a generic error about mutating global variables.
This was an oversight in the original definition of useContext (oops my bad).
Context values are owned by React and should not be modified. I found this
because some cases of existing useMemo were not preserved (tested via the
validatePreserveExistingManualMemo flag) due to function calls referencing
context being assumed to mutate.
This change will allow more memoization, it's also just more correct for the
rules of React. Note the new ValueReason variant so that we can provide a
precise error message about mutating context values.
It's starting to get complex just with a couple of extra
passes — we either need to substantially extend the HIR or (as i've done so far)
pass information from early passes to later ones. This PR changes things so that
very early in the babel plugin we fork into a separate mode. Forest has
its own `compileProgram()` equivalent, its own pipeline, its own codegen, etc.
> Update: this is now passing all tests. The approach is likely wrong, and even
if it's fine it needs some cleanup. Putting up for review as folks (esp
@gsathya) have time.
## Background
InferTypes was intended to infer types for phi identifiers, but by accident we
ended up storing the inferred type on `phi.type` instead of `phi.id.type`, which
is the type that usages of the phi will reference. Because of this, we weren't
actually inferring types for several cases, for example if both if/else branches
assign `x` to an array literal, we'd ideally like the corresponding phi id to be
typed as a BuiltInArray:
```javascript
let x;
let y = { ... };
if (cond) {
x = [];
} else {
x = [];
}
// x should be BuiltnArray here. We inferred that on Phi.type but the x here
wouldn't get that type previously
x.push(y);
```
## Circular Types
I started by removing the `Phi.type` property and updating inference to store
the result of phi unification on `phi.id.type` — but this revealed other issues.
First was this can create circular types when there are loops. The solution is
to basically allow circular types _for phis only_, and when we detect them we
remove the cycle. Basically whenever we have a situation where we have some type
variable X, and a type Y that is a (nested) phi type one of whose transitive
operands contains X, we remove X from the transitive type and attempt to
collapse the phi type upwards if all of its remaining operands are the same:
```
X=Type(1)
Y=Phi [
Type(2),
Type(3) = Phi [
Type(1), // <-- cycle but we can prune this
Type(2),
Type(2),
]
]
=>
X=Type(1)
Y=Phi [
Type(2),
Type(3) = Phi [ // all remaining operands are the same, we can prune this
Type(2),
Type(2),
]
]
=>
X=Type(1)
Y=Phi [ // all remaining operands are the same, we can prune this
Type(2),
Type(2),
]
=>
X=Type(1)
Y=Type(2)
```
We have to do this not just doing unify(), but also in `get()` since there are
cases where we don't know yet which type variables we can remove from a phi.
Without also doing the pruning in get, we get an infinite loop.
## Reactive Scope Alignment
The above fixed the circular types, but exposed some new cases that can occur in
terms of mutable ranges and ast structures: it wasn't possible before to have a
Store on a phi node in practice, since that relied on type information which we
didn't have for phis.
The new validation that all instructions for a scope are part of that scope
caught a couple issues, which were basically like this:
```
[1] Sequence
...
[9] StoreLocal x@0[9:28]
[10] ...
```
Note that scope 0 starts at instruction 9, but that instruction is not at the
block scope level. The first instruction at the block scope level that is within
the range of scope 0 is instruction 10, which is after the scope should have
started! So I also had to update AlignScopesToBlockScopes to handle the case of
logical, conditional, and sequence expressions: we sometime need to adjust a
scope start earlier in case they contain instructions that should start a scope.
Fixes the case from the previous PR by using a different sentinel for
uninitialized cache values and early returns. I confirmed with console.log that
the reactive scope for `x` only evaluates on the first execution, after which we
figure out that we don't need to execute it again.
RFC. This is a quick sketch of adding support to Sprout to render the same
component instance multiple times with different props. This doesn't test
memoization (though it forms a basis for testing it, more below), but does allow
us to test that the code properly reacts to inputs and doesn't get "stuck"
always returning the same output even when inputs change.
Possible extensions:
- Support calling non-component functions multiple times
- Test memoization by having the `toJSON()` helper track objects it has
encountered before, assign each object a unique id, and then emit subsequent
references to the same value as the id instead of the printed form of the
object.
For example if we call a memoized function with the same input twice in a row,
today we might get output like:
```
[{a: 1}],
[{a: 1}],
```
Which doesn't tell us if the object is equal. Instead we could emit output like:
```
[{a: 1}] #0,
#0,
```
Which allows verifying that memoization actually happened. Or we could automate
this and just assert that anything structurally equal has to be referentially
equal — though there are cases with conditionals that break this.
Adds support for early returns within reactive scopes, behind a new feature
flag. The flag is off by default, where this case continues to throw a Todo
bailout.
Since implementing a sketch of the codegen in the previous PR I realized that
it's easy enough to implement the more optimal output, so i've updated that
here. Rather than both the if and else branch of the reactive scope having an
"if the return value was not a sentinel return it" check, we instead make the
return temporary a proper declaration of the reactive scope. Then, since it's
actually an output it's available in the outer block scope, and we could do a
single if-return after the reactive scope.
Edit: see comment below for thoughts on test cases.
Implements codegen for reactive scopes with early returns, though we don't ever
construct such a case yet. See comments in the code. There is a slightly more
optimal output that would require a larger refactor (also described in code
comments), for now i'm starting with the simpler approach since this is
relatively rare so we don't need to optimize code size / runtime as much.
Adds a new `earlyReturnValue` property on ReactiveScope which will be set if the
scope had one or more early returns, with information about the temporary
identifier that the early return value will be assigned to, as well as the label
to be used for breaking (to simulate the early-return). The next PR shows the
intended codegen.
Adds a new compiler pass that will eventually actually handle early returns
within reactive scopes. For now it just detects them and throws a Todo error.
Adds back a mode to transitively freeze function expressions, independently from
the mode to preserve existing manual memoization. This lets us experiment with a
few variants:
* Preserve existing memoization
* Validate existing memoization with:
* `enableAssumeHooksFollowRulesOfReact` &&
`enableTransitivelyFreezeFunctionExpressions`
* `enableAssumeHooksFollowRulesOfReact` only
* neither of those flags
Note that `enableTransitivelyFreezeFunctionExpressions` alone probably doesn't
make sense, it's more aggressive than
`enableAssumeHooksFollowRulesOfReact` so we might as well try them together.
Adds a new mode which validates that existing manual memoization is preserved
_without_ using information from the manual memoization to affect compilation.
This gives us a way to try out the more aggressive version of Forget — ignoring
manual memoization — first and see how much code bails out and what patterns
cause this.
We can then proceed to enable the mode to actually _preserve_ existing memo
guarantees only where necessary.
Extends `@enablePreserveExistingMemoization` to validate that all of the
original values were actually memoized. This works nearly identically to how we
validate effect deps are memoized. We look for Memoize instructions whose values
need memoization but whose range extends past the memoize instruction, or where
the value isn't memoized at all.
Merges `@enableTransitivelyFreezeFunctionExpressions` into the new
`@enablePreserveExistingMemoizationGuarantees` mode, since they are both
motivated by the same use case of preserving effect behavior by preserving
existing memoization behavior.
The idea is that `useCallback` has an implicit assumption: that the variables
captured by the callback aren't subsequently modified. Previous PRs treated the
values directly captured by the callback as frozen. But if those variables were
themselves another function expression, and that expression captured a mutable
value, then we wouldn't consider the freeze to be transitive:
```javascript
const object = makeObject();
useHook(); // oops, hook call inside `object`'s mutable range, can't memoize
object, log, or onClick!
const log = () => { console.log(object) };
const onClick = useCallback(() => { log() });
maybeMutate(object);
```
However, the assumption of such code is that it _doesn't_ modify such
transitively captured values. So here we merge
`@enableTransitivelyFreezeFunctionExpressions` mode into the
memoization-preserving mode. Now, the memoize instructions emitted for
useCallback (and useMemo) will transitively freeze captured function
expressions, allowing us to memoize.
The flip side of this is that some code may be violating these rules. We'll rely
on runtime validation to detect such cases.
Joe Savona
Mofei Zhang
Jan Kassens
Sathya Gunasekaran