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.
---
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.
---
Currently on main, rollup does not inline source files
```js
// in packages/babel-plugin-react-forget
// $yarn build
// output
var CompilerError_1 = require("./CompilerError");
Object.defineProperty(exports, "CompilerError", { enumerable: true, get:
function () { return CompilerError_1.CompilerError; } });
// ...
```
I debugged a bit but not familiar with node or rollup.
- It seems that rollup fails to recognize source file imports with this setting,
as resolveId no longer gets called
- current `module` option defaults to `ESNext`, which works for some reason.
Let's revert for now to unblock syncs.
Sanity checked my repro by reinstalling node-modules and cleaning rollup cache.
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.
Joe Savona
Sathya Gunasekaran
Jan Kassens
Mofei Zhang