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.
We were using `returnValueKind` from function signatures for CallExpression but
not MethodCall; this PR changes to use this signature information for both
instruction kinds.
---
Going to hold off on landing until after codefreeze, it's not urgent as we
already fixed playground in #2404. All other internal pipelines do error
handling through Entrypoint, which catches and creates UnexpectedErrors as
needed.
Instead of using the source location to check for hoisting, just stop checking
for a given component after we reach it's declaration.
By definition all references to it before are (potential) hoisting errors.
Note that there could be false positives but that's ok.
Extends the validation that effect deps are memoized to handle an additional
case that @gsathya pointed out: when a dependency has a reactive scope but that
scope ends up being pruned. We track reactive scopes which actually exist in the
ReactiveFunction, and reject useEffect deps that have an associated reactive
scope but where that scope does not exist (bc it got pruned).
This is one approach to testing whether useEffect dependencies are memoized. The
idea is based off the observation that the only reason dependencies wouldn't be
memoized (other than compiler bugs) is that they are mutated later. If they're
mutated later, then the dep array will have a mutable range which encompasses
the InstructionId of the useEffect call. So we look for that pattern and throw a
validation error.
The downside of this approach is that we might reject code that happens to be
valid: specifically, that the lack of memoization isn't a problem in practice
because the effect won't trigger a loop. But (per test plan) this doesn't seem
to introduce that many new bailouts on www. Rather than implement a complex
validation that checks whether we un-memoized something that was memoized in the
input, it seems more practical to:
1. Enable this more comprehensive validation against any form of un-memo'd
effect dependency
2. Flip the default for hooks (to assume they follow the rules), which will fix
the primary cause of Forget pessimistically not memoizing dependencies.
## Test Plan
Synced to www and checked output via the upgrade script: a few components stop
getting memoized bc they have un-memoized effect dependencies. Let's chat!
We were modifying the Babel AST as a shortcut to lowering function declarations,
instead we can explicitly lower them equivalently to a `let <id> =
<function-expression>`.
When you have your panic threshold set to "NONE" as we recommend, it's easy to
miss that your config is wrong (which makes everything not compile) because
those errors were being silenced. This made debugging FluentUI and the
forget-feedback testapp pretty difficult to figure out at first, and defeats the
purpose of having config validation in the first place.
This pr makes it so InvalidConfig errors always throw, regardless of the panic
threshold set. In general our plugin should never throw at build time due to
component bailouts, but because an InvalidConfig would bailout everything from
being compiled at all, it seems reasonable to throw here
Sathya Gunasekaran
Mofei Zhang
Joe Savona
Lauren Tan