---
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
Babel doesn't attach Comment nodes to anything, so they dangle off of
the Program node while only specifying a range. This meant that
previously we first had to traverse all of the Program's comments to
find an eslint suppression of the rules of React, then during traversal
of the individual functions, we would check if there were any *global*
eslint suppressions, then bailout all components.
This PR updates our logic to determine if individual functions are
affected by an eslint suppression range:
- If an eslint suppression range falls within its body; or
- If an eslint suppression wraps the function
---
16 out of ~150 recently added sprout fixtures have exceptions that reflect
easy-to-miss mistakes in the fixture input, like forgetting to import
`useState`.
This PR adds snapshot files for sprout to prevent these mistakes (or catch them
at diff review time). This describes what it implements, but happy to take other
suggestions as well!
1. One sprout snapshot file for each input.
This significantly increases the number of files we have, but makes it clear
what the fixture is testing. I was hoping to expand on these snapshots to record
the result of multiple re-renders, or mounts (with different parameters), but
the tradeoff is that adding / changing fixtures will now require running `yarn
sprout --mode update`.
Some alternatives I've considered:
- Only record snapshots for fixtures that error (`result.kind == "exception"`).
This change would be pretty easy. This doesn't help sanity check sprout results
for general mistakes though ("am I testing what I want to test?)
- Warn loudly for fixtures that error -- seems like these are easy to ignore,
but.. worth it for the convenience?
- Some github action that runs and comments on your PR with sprout fixture
changes; i.e. never manually update sprout files?
2. Sprout and snap share a common snapshot file, with some parsing hackery.
Previously, the implementation added new files to a separate sprout snapshot
directory, assuming that devs don't need to read these often.
After feedback from @josephsavona, I updated to have snap and sprout write to
the same file for ease of reviewing / debugging (to be able to see the input /
output side by side).
- `snap --mode update` will update snap's part of the file.
- `sprout --mode update` will update sprout's part
- `snap` and `sprout` will both compare oldSnapshot (read from the file) with
`newSnapshot = merge(oldSnapshot, newData)` to determine if a test pass.
3. Some hacks 😅 Absolute paths to project directory (e.g. stack traces) are
replaced with `<project_root>` in a mocked `console.log`
---
jsdom and other libraries seem to cause jest workers to exit with
`forceExit:true`. Not sure what option I set (or global I've overwritten) but
console logs aren't flushed as a result, making debugging a bit confusing.
This PR:
- Moves some code from `eval(...)` to a typechecked real js function. I always
had trouble debugging the `eval`ed code, so smaller code snippet is better here.
- waits for jest workers to end before exiting
I ran the plugin with the extended version of ValidateNoSetStateInRender enabled
(incl. function expressions) and there are no false positives. Let's remove the
flag for the function expression case since the whole rule is working
accurately.
Repro from T169063835. This works, but since it came up it seems good to add a
test case for it just in case there's something funny here that we could regress
on w/o realizing it.
Fixes one category of bugs with const hoisting. The algorithm finds all consts
that need to be hoisted, then looks through the statements of a block to find
the first statement which references that const, delaying the emission of the
HoistedConst instruction until its actually used. To determine if a statement
references a const we find every identifier in the statement and check if its
binding is one of the hoisted bindings.
There's a very small bug here: when we resolve the binding of each identifier,
we need to resolve it in its own scope. We're currently resolving these
identifiers agains the outer block statement's scope, which can cause us to
misattribute identifiers when there is shadowing:
```
const items = props.items.map(x => x); // we scan this statement, resolve 'x' in
the block statement scope, and mis-attribute it to the outer x.
const x = 42; // (1) this x is a candidate for hoisting, so the binding is in
the set of hoisted consts
```
I had added a repro for this earlier but hadn't realized it was due to const
hoisting. Renaming this test to clarify what's causing the problem and to make
it easier to find.
This is non ideal but at least it's a step in the right direction.
Getting the correct error requires us to track every identifier and global,
which seems a bit excessive for now.
We can revisit and improve this error if this is starting to confuse folks.
---
We were throwing `InvalidReact` errors on valid inputs.
```js
// Input
function Foo() {
log("block0");
for (const _ of foo) {
log("loop");
}
useBar();
}
// IR
bb0:
// log("block0");
ForOf init=bb2 loop=bb3 fallthrough=bb1
bb2:
// init
Branch: then:bb3 else:bb1
bb3:
// log("loop")
Goto(Continue) bb2
bb1:
// useBar();
Return
```
We correctly compute post dominators here.
```js
// In validateUnconditionalHooks
console.log(dominators.debug());
/* Output:
(read x => y as x is the post-dominator for y (all paths from x to the exit must
go through y))
"bb4" => "bb4",
"bb1" => "bb4",
"bb2" => "bb1",
"bb3" => "bb2",
"bb0" => "bb2",
*/
```
However, `findBlocksWithBackEdges` prevented us from adding `bb1` to the
`unconditionalBlocks` set as `bb2` (its post dominator) has a back edge. I'm not
sure what the `findBlocksWithBackEdges` was doing previously, so I replaced it
with an invariant asserting that the loop terminates.
---
Snap should compile all fixtures to record changes in results, even `todo`
prefixed ones. Previously, they were skipped as we noted the correlation of `//
@skip` pragmas and file naming.
Now, no fixture should be skipped as our compiler pipeline should be able to
handle every kind of error.
Mofei Zhang
Joe Savona
Sathya Gunasekaran
Lauren Tan