> 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.
Adds test cases per the previous PR for useCallback:
* callback that references another callback, which in turn references a
possibly-mutated value
* callback that references a ref
Improves `@enablePreserveExistingMemoizationGuarantees` for the useCallback
case. Similar to useMemo, we add an explicit `Memoize` instruction for the
callback function itself _and_ for its dependencies. This means we'll assume the
callback doesn't mutate any captured variables.
TODO: check this with cases involving refs (should be allowed, but also not
accidentally freeze the ref) and reassignment of locals (should be disallowed,
though that might just be a validation we're missing today)
The previous PR introduced `memoize` instructions whose lvalues aren't used, but
which can't be pruned by DCE due to pipeline ordering. Here we change to make
memoize an instruction intended for its side effects only, and prune during
codegen.
See discussion on #2448 for full context. In the new
`@enablePreserveExistingMemoizationGuarantees` mode, the goal is to preserve the
existing referential equality guarantees from the original code. #2448 lays the
groundwork by explicitly marking the _output_ of each useMemo block as memoized,
hinting to the compiler that the value cannot subsequently change. This ensures
the mutable range doesn't extend _later_, possibly overlapping a hook call and
causing memoization to gett pruned.
This PR fixes the other direction. There are cases where free variables
referenced in the useMemo block could have been inferred as mutated, which could
then extend the _start_ of the range earlier past a hook:
```javascript
const foo = createObject();
useBar();
const baz = useMemo(() => {
const baz = createObject();
maybeMutate(foo, baz);
return baz;
}, [foo]);
```
Here the compiler would infer that both `baz` and `foo` are mutable at the
`maybeMutate()` call, grouping them in the same scope. But that scope would span
the `useBar()` call, and be pruned, meaning that `baz` went unmemoized.
However, useMemo blocks shouldn't be mutating free variables. Only variables
newly created within the useMemo block should be mutable. So this PR extends the
feature to treat all free variables referenced in a useMemo block as frozen as
of the block itself.
Adds an option to preserve existing memoization guarantees for values produced
with useMemo and useCallback. We still discard the calls to these hooks, but we
preserve the information that the value is frozen at that point in the program.
Because these values are produced solely within the useMemo/useCallback
callback, their mutation cannot have any interspersed hook calls. This means
that the values mutable range will never span a hook and end at the point of the
useMemo, ensuring that they are memoized at the same point.
The main things that can change (relative to the orignal code) are:
* Forget will infer a precise set of dependencies, ignoring the user-provided
values. In practice this should only occur if the original code had a lint
violation, which Forget would bail out on. So in practice this shouldn't happen
unless the code doesn't use the React linter.
* Forget may start the memoization block earlier than the developer did if other
values are mutated along with the value being produced. This can cause
memoization to fail, but only in situations where it would have failed
previously:
```javascript
const a = [];
useFoo();
const b = useMemo(() => {
const c = a;
c.push(1);
return c;
}, [a]);
```
In this example (sans Forget) the useMemo will invalidate on every render
because `a` will always be a new array and its listed as a dependency of the
useMemo. Forget would correctly determine that the memoization would have to
work as follows:
```javascript
let c;
if (...) {
const a = []
useFoo(); // OOPS we made a hook call conditional
const t0 = a;
t0.push(1);
c = t0;
...
} else {
c = $[...]
}
```
Because this is invalid, Forget would (later in the pipeline) strip out this
memoization block and (as with the original) leave `c` un-memoized.
In this same example, removing the hook would cause Forget to be able to memoize
a value that wasn't memoized before:
```javascript
const a = [];
const b = useMemo(() => {
const c = a;
c.push(1);
return c;
}, [a]);
```
This invalidates every render without Forget, but would memoize correctly with
Forget (it would expand the memoization block to include the declaration of
`a`).
Adds a fixture for our existing behavior that reactive scope dependencies
exclude values which are non-reactive. The idea is that regardless of whether
the value may actually get recreated over time or not, a "nonreactive" value
cannot semantically change and therefore we can ignore changes in its pointer
address.
After running the latest hook validation internally, I found some cases where
there was a violation but the error message was not ideal. For example on this
code:
```javascript
usePossiblyNullHook?.();
```
We reported a "hooks can't be used as normal values" violation, when we'd
ideally report a "hooks can't be called conditionally" violation. The solution
in this PR is to track errors by source location, and upgrade the former
violation to the latter, more serious violation. See fixtures for examples.
ObjectExpressions
---
Currently, we're removing all reactive scopes containing object methods. This
could produce incorrect output as object method instructions may still be
included in other reactive scopes (and will lose their dependencies).
Builds on the utilities added previously to infer types from type annotations on
variable declarations. This is a limited form, where currently we only infer for
local identifiers (not function parameters) and only infer a type for the
variable initializer and not subsequent reassignments.
This PR uses the information from type cast expressions (`as` or `(variable:
type)`) to inform type inference. BuildHIR converts the type annotation to our
internal type format where possible, falling back to the generic `makeType()`.
This is then used in InferTypes to help set the value's type.
Extends the previous analysis to work for PropertyLoad and ComputedLoad, so that
if the object is a prop we track the resulting value as a signal. We also
disallow PropertyLoad/ComputedLoad outside of a reactive scope where the object
is a signal (since that would drop reactivity).
Previously the only way to replace a value was to override transformInstruction
and transformTerminal, and to be careful to find nested values. This PR adds
`ReactiveFunctionTransform#transformValue()` which allows returning an optional
new value, which if present will replace the value in whatever context it
appeared. ReactiveFunctionTransform now reimplements all the methods necessary
to replace any value anywhere in the AST. See the next PR for an example usage.
I realized this while working on Forest. When computing the dependencies of a
reactive scope we can omit setState functions in the general case (exception
described below). Currently that's implemented in PruneNonReactiveDependencies.
However, this causes us to miss some optimizations — a value isn't reactive if
its only dependency is a setState, and that may allow further downstreams values
to become non-reactive. We lose out on that by only filtering out setStates in
PruneNonReactiveDependencies — this logic really belongs in InferReactivePlaces.
So this PR moves the check for setState types to that pass. The updated fixtures
show that this already uncovers some wins. The _new_ fixtures covers the
exception. It's possible for a value to be typed as being a setState function,
but to still be reactive: if its a local that is conditionally assigned
different setState function values. Currently this test happens to work because
our phi type inference is incomplete (see #2296). I'm adding the test now though
to prevent regressions when we fix phi type inference.
In normal React certain operations don't allocate new objects (property loads,
binary expressions, etc) and therefore don't need a reactive scope in Forget.
For example, property loads only extract part of an existing value and don't
allocate something new, while binary expressions are known to produce primitive
values that don't allocate. We rely on the fact that whenever their inputs
change we will re-run the component/hook and propagate the result forward.
For Forest, the only way to propagate data is via reactive scopes: the component
code is equivalent to a "setup" function. This PR updates some of our passes to
ensure that we create (and don't prune) scopes for these types of operations. I
started with a conservative set for now.
The previous PR converts reactive scopes to normal instructions, so that Forest
mode won't have any scopes left by the time we reach codegen. This PR removes
the now-unused codegen logic for forest.
For Forest, we previously converted reactive scopes into derived signals during
Codegen. I'm moving this to a separate pass primarily to keep codegen simple
since there's enough complexity just dealing with core JS semantics. Ideally
we'd do a similar setup even for regular Forget, ie lower reactive scopes just
prior to codegen.
At the same time i also reordered the forget passes to be just before codegen,
and cleaned things up a bit. For state lowering, we now just rewrite `useState`
-> `createState`, because we actually need to keep around the setter function to
trigger scheduling updates in addition to writing the signal value.
Found from eslint validator on www after doing a local sync + RunForget test of
#2432
P898168203
> New Errors:
> no-undef:'VARIABLE_NAME' is not defined.
Joe Savona
Sathya Gunasekaran
Jan Kassens
Mofei Zhang