CannotPreserveMemoization
We do need to fix the error location to point to the "callsite" rather
than the definition of the useMemo callback, but that aside, even if the
error message were perfect, it's not meant to be actionable to the user.
So let's change the severity to CannotPreserveMemoization. This
preserves the validation, but the eslint plugin won't report it.
ghstack-source-id: 722c88922884de05e89030a7b001bd93e0a2a114
Pull Request resolved: https://github.com/facebook/react-forget/pull/2825
This adds a new category of error where the compiler cannot preserve
memoization exactly how as it was originally authored. We're adding a
new category here because it's not an actionable error, and allows us to
more specifically control whether it's reportable or not.
ghstack-source-id: 9693cd42ca64b980248c6202091bdd4c827e1cd4
Pull Request resolved: https://github.com/facebook/react-forget/pull/2824
First attempt at making the linter work with advanced TypeScript syntax
Falls back to the babel parser for some advanced syntax like string template
syntax.
This is pretty hacky as it doesn't take in any parsing options that are
configured for the outer ESLint parser, not sure how that could be handled.
Implements support for use:
* Teaches InferReactivePlaces to treat use() result as reactive
* Teaches FlattenScopesWithHooks to also flatten scopes with use()
Handles both `use()` and `React.use()`.
This adds rollup to the runtime and adds a new plugin to add the license banner
+ inject the `"use no memo"` directive. We need to inject it there as rollup
currently strips out unknown directives during bundling.
For now this configures rollup to strip out comments in DEV builds and
whitespace. Unfortunately there's no easy way to do this in just terser alone or
other minifiers/manglers, so I had to add prettier as well to re-format the
minified code. This does make the build a little bit slower:
``` before: yarn build 118.96s user 12.38s system 185% cpu 1:10.81 total after:
yarn build 121.55s user 12.90s system 183% cpu 1:13.17 total ```
Eventually I would like to have a similar setup to React's rollup config where
we can have DEV and prod builds. After the repo merge we could probably share or
reuse bits of React's rollup config.
This PR makes all packages share the same typescript version and updates us to
latest versions of typescript, ts-node, typescript-eslint/eslint-plugin and
typescript-eslint/parser.
I also noticed that the tsconfig we were extending (node18-strictest) was
deprecated, so I switched us over to one that's more up to date.
Also had to make a couple of small changes to the playground so that continues
to build correctly.
Previously, we would drop directives inside a component or hook but this is
problematic with reanimated which uses `'worklet'` to mark components from
compilation.
This PR adds a directive to HIRFunction and ReactiveFunction and codegens the
directive add the end. No processing is done on the directives themselves.
Babel seems to store the directives on a BlockStatement, rather than on the
Function but I've stored it on the Function types because we only support
compiling functions and the spec defines directives as occuring in the initial
statement list of a function: > A Directive Prologue is the longest sequence of
ExpressionStatements > occurring as the initial StatementListItems or
ModuleItems of a > FunctionBody, a ScriptBody, or a ModuleBody and where each >
ExpressionStatement in the sequence consists entirely of a > StringLiteral token
followed by a semicolon.
This PR was the result of a long chain of ~yak-shaving~ debugging kicked off as
a result of fixing up invariants. Where this started was that i noticed some
cases of loops where the first instance we saw of a reactive scope was after its
starting instruction. Eg instruction N would have an operand with scope
Start:End, where Start was _before_ N. One of the cases involved a phi with a
backedge. Then i noticed that we assign scopes differently for phis with and
without backedges:
```
[1] let x0 = init;
[2] if (x0 < limit) {
[3] x1 += increment;
}
x2 = phi(x0, x1);
[4] x2;
```
The phi isn't mutated _or_ reassigned after its creation, so we don't assign a
mutable range to the phi or any of its operands. We also don't create a scope
for `x`.
But change the `if` to a `while` and now the phi moves - now there's a backedge:
```
[1] let x0 = init;
[2] while (x0 < limit) {
x2 = phi(x0, x2); // now this is "mutated" later!!!
[3] x2 += increment;
}
[4] x2;
```
What was happening here is that x2 has a mutable range which is "after" the phi
instruction, so it would appear that the phi was actually being mutated later.
Ie, this was treated equivalently to the original "if" version, but with a
mutation:
```
let x = [];
if (cond) {
x = {};
}
mutate(x); // later mutation of the phi
```
But these latter two cases are different! We only need to (should) create a
mutable range for a phi _if its value is actually mutated_. If it's just being
reassigned, well then it shouldn't matter if there are back edges or not.
So this PR implements that intuition: only create a mutable range for a phi if
it is actually _mutated_ later, ie don't assign a mutable range if it is only
_reassigned_ later. Concretely in InferMutableRanges:
* InferMutableLifetimes no longer has to initialize a range for phis during the
first pass (inferMutableRangesForStores=false). We wait to see if the phi is
mutated during the main fixpoint iteration of InferMutableRanges
* The main fixpoint iteration in InferMutableRanges already aliases phi operands
if the phi is later mutated, which will extend the end of the mutable range of
all the operands accordingly.
* Finally, InferMutableLifetimes's second run
(inferMutableRangesForStores=true), we ensure that any phis mutated later have a
valid mutable range, specifically setting the `start` of the range since the
fixpoint only updates the `end` value.
A dependency D from either an instruction or scope is poisoned if there may be a
(non-linear) jump instruction between it and the start of its immediate parent
scope. Poisoned dependencies are added as conditional dependencies to their
parent scope.
(done: reduce false positives in scopes that begin after return/throw) (done:
fix bugs in recording and joining exhaustive conditional deps) (done: flesh out
commit message, clean up PR, add more fixtures)
--- \## Bug details:
Take a simple example: ```js target: { instrA; if (...) { instrB;
break target; } else { instrC; } instrD; // ... } instrE; // ... ```
This diagram shows how we represent this program in the reactive IR. - Blocks
are represented as a list of nodes. - Green nodes show instructions and value
blocks (simplified as a single instruction). - Pink nodes show terminals, which
transfer control to a subtree of nodes. <img width="450" alt="image"
src="https://github.com/facebook/react-forget/assets/34200447/930789f2-39cd-4ea8-b12a-530042807b46">
Prior to this PR, PropagateReactiveScopeDeps was incorrect because it assumed
that a block's instructions are evaluated unconditionally (which is how HIR
basic blocks work). E.g. if a reactive scope enclosed `block 1`, we assume that
`instrA` and `instrD` both will evaluate unconditionally.
This failed to account for `jump` instructions like break, continue, return, and
throw. This may result in invalid hoisting of PropertyLoads (i.e. Forget output
may throw when source does not throw). Note that other terminals (e.g. if and
loops) are not affected as they are self contained subtrees that evaluate
sequentially.
With the changes in this PR, we mark `block 1` as poisoned upon encountering the
`break` instruction. While `block 1` is active and poisoned, it will determine
how visited dependencies are added.
Here, added solid lines show unconditional dependencies, dashed lines show
conditionally accessed dependencies: - dependencies from `instrB, instrC` are
conditional because they are within conditional subtrees - dependencies from
`instrD` are conditional because it is within a poisoned block within its parent
scope.
<img width="450" alt="image"
src="https://github.com/facebook/react-forget/assets/34200447/81980f68-7e65-4bd7-ba94-3f0c26550e5c">
--- Recapping an offline discussion with @josephsavona: this pass would really
benefit from operating on HIR. The minimal work needed for this pass to run on
HIR is to rewrite and reorder `AlignReactiveScopesToBlockScopes` to operate on
HIR.
The following diagram shows what HIR blocks look like for the same code.
Evaluating hoistable PropertyLoad dependencies for a scope enclosing
`instr{A-D}` is much simpler: just evaluate whether the PropertyLoad evaluates
for every path between `bb0` and `bb4`. <img width="250" alt="image"
src="https://github.com/facebook/react-forget/assets/34200447/44b38939-defb-4b29-878d-4445ec6ccc06">
---
conditionals
This change is needed for #2752. To minimize renaming `error.fixture` ->
`fixture` files, I'm reordering this PR to earlier in the stack.
Prior to the fix in #2752, we only expected unconditional accesses within
`depsInCurrentConditional`, which records instructions directly within a
conditional block (not including nested conditional blocks).
RFC: we can either retain break/continue target ids (instead of pruning them in
`buildReactiveFunction`) or re-implement the same logic in `propagateScopeDeps`
(ignore implicit breaks; match unlabeled break / continues to their closest loop
/ while parent terminal).
If we go ahead with this approach, I'll clean up this PR (add relevant types and
comments)
Lauren Tan
Jan Kassens
Joe Savona
Sathya Gunasekaran
Mofei Zhang