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.
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.
> 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.
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.
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.
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.
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.
---
I modeled guards as try-finally blocks to be extremely explicit. An alternative
implementation could flatten all nested hooks and only set / restore hook guards
when entering / exiting a React function (i.e. hook or component) -- this
alternative approach would be the easiest to represent as a separate pass
```js
// source
function Foo() {
const result = useHook(useContext(Context));
...
}
// current output
function Foo() {
try {
pushHookGuard();
const result = (() => {
try {
pushEnableHook();
return useHook((() => {
try {
pushEnableHook();
return useContext(Context);
} finally {
popEnableHook();
}
})());
} finally {
popEnableHook();
};
})();
// ...
} finally {
popHookGuard();
}
}
// alternative output
function Foo() {
try {
// check current is not lazyDispatcher;
// save originalDispatcher, set lazyDispatcher
pushHookGuard();
allowHook(); // always set originalDispatcher
const t0 = useContext(Context);
disallowHook(); // always set LazyDispatcher
allowHook(); // always set originalDispatcher
const result = useHook(t0);
disallowHook(); // always set LazyDispatcher
// ...
} finally {
popHookGuard(); // restore originalDispatcher
}
}
```
Checked that IG Web works as expected
Unless I add a sneaky useState:
<img width="705" alt="Screenshot 2023-12-05 at 6 44 59 PM"
src="https://github.com/facebook/react-forget/assets/34200447/3790bd76-7d71-44b5-a62e-f53256fb5736">
---
Prior to this PR, we were mutating functions after CodegenReactiveFunction
completes (in `Entrypoint/Program.ts`).
The reasoning for this separation was that we wanted to keep non-compiler logic
out of the core Pipeline. However, it made our code difficult to read and reason
about.
Open to other alternatives, like adding a pass after Codegen.
New approach to hooks validation per recent discussion. The idea is to avoid
false positives while still preventing serious violations. See the comments in
the file for more details about the approach. It uses a somewhat similar idea to
InferReferenceEffects in that we track a "Kind" for each IdentifierId, and
various instructions propagate or derive a result Kind from the operands. Kinds
form a lattice and can be joined, allowing us to be more precise about known vs
potential hooks, and known vs potential _sources_ of hooks.
The previous PR helped me realize we weren't handling Array#at correctly. If the
receiver is a mutable value its effect should be Capture and the lvalue effect
needs to be Store. This PR updates the definition for Array#at to make the
receiver Capture, and then updates inference to automatically set the lvalue
effect to Store if _any_ argument (or the receiver) was Capture.
There was one missing piece to the optimization from the previous PR: Array#map
can return an alias to the receiver in its output, which means that mutations of
the result have to be treated as mutations of the receiver. This means we need
to use a Capture effect on the receiver. If that doesn't get downgraded to a
Read bc the value was immutable, we then also need to make the lvalue effect a
Store (so that InferMutableRanges actually looks at it for aliasing).
Improves memoization for cases such as #2409:
```javascript
const x = [];
useEffect(...);
return <div>{x.map(item => <span>{item}</span>)}</div>;
```
We previously thought that the `x.map(...)` call mutated `x` since its kind was
Mutable. However, in this case we can determine that the map call cannot mutate
`x` (or anything else): the lambda does not mutate any free variables and does
not mutate its arguments.
This PR adds a new flag to function signatures, used for method calls only, that
checks for such cases. The idea is that if the receiver is the only thing that
is mutable — including that there are no args which are function expressions
which mutate their parameters — then we can infer the effect as a read. See
tests which confirm that function expressions which capture or mutate their
params bypass the optimization.
Distilled repro of an internal example we found. Forget determines a mutable
range for the array, but that mutable range spans a hook call, so the reactive
scope gets pruned. That's all working as expected.
What isn't ideal though is that if we know `x` is an array and `f` can't mutate
its arguments, then `x.map(f)` shouldn't count as a mutation of `x`, since
Array.prototype.map can only mutate the receiver via the callback (if the
callback mutates its args).
Improving on this example requires a) we have to know it's an Array, via type
information or bc we saw an array literal and b) being precise about which
functions could possibly mutate their parameters, which is tricky because of
indirect mutations via stores, etc.
We were using `returnValueKind` from function signatures for CallExpression but
not MethodCall; this PR changes to use this signature information for both
instruction kinds.
Joe Savona
Jan Kassens
Mofei Zhang