Fixes for the previous PR. What was happening is that our inference was
inferring the correct mutable ranges and reactive scopes, but the inlining
process left the instructions from the IIFEs inside a separate block, with a
'label' terminal preceding it. When we converted to ReactiveFunction this was
preserved as a ReactiveLabelTerminal, which meant that the first instruction for
the mutable range could be nested inside one LabelTerminal, while more would be
in a subsequent LabelTerminal. But we close blocks based on the block scope!
This meant that we'd have leftover instructions (in the second LabelTerminal)
that got left out of the block.
Furthermore, because inlining was happening after EnterSSA we weren't creating
phis correctly. This PR fixes a bunch of these issues, and a subsequent PR
handles the remaining cases:
* We move DropManualMemo and InlineIIFEs before EnterSSA. This means we lose the
ability to use type information, but we ensure that we create proper SSA ids and
phis for any reassignments within the IIFE
* We also update PruneUnusedLabels to not just remove the unused labels, but to
actually remove LabelTerminals that don't need them.
We construct invalid mutable ranges in these cases because the range starts
within a labeled block. We need to run merge consecutive scopes and EnterSSA
after inlining so that the code is lifted out of the labeled block to the
correct scope, and so that we create phis for reassignments within the IIFE.
This has caused issues for people when things like Babel cause issues. It's not
actionable and it crashes eslint. Just like the Babel plugin, the eslint plugin
should never throw. Instead, let's log the error so the data isn't lost.
This combines our scripts and makes it so we no longer need to create a separate
commit to add the forget-feedback/dist directory into the react-forget repo. I
originally did that so I could run tests here, but now that the external repo is
created and has its test suite hooked up to CI, this is now unnecessary friction
to run a sync
All of these warnings go away: ``` ➜ playground git:(remove-prettier) ✗ yarn
dev yarn run v1.22.19 $ NODE_ENV=development && next dev ready - started server
on 0.0.0.0:3000, url: http://localhost:3000 warn - You have enabled
experimental feature (appDir) in next.config.js. warn - Experimental features
are not covered by semver, and may cause unexpected or broken application
behavior. Use at your own risk. info - Thank you for testing `appDir` please
leave your feedback at https://nextjs.link/app-feedback
event - compiled client and server successfully in 964 ms (199 modules) wait -
compiling /page (client and server)... warn -
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/index.js Critical dependency: require function is
used in a way in which dependencies cannot be statically extracted
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/third-party.js Critical dependency: the request of a
dependency is an expression wait - compiling... warn -
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/index.js Critical dependency: require function is
used in a way in which dependencies cannot be statically extracted
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/index.js Critical dependency: the request of a
dependency is an expression
../../node_modules/prettier/third-party.js Critical dependency: the request of a
dependency is an expression ```
Standalone prettier is meant to be used in the browser:
https://prettier.io/docs/en/browser
I don't know if it's possible to write a test for this as I can't seem to get
the codegen to change.
For the following testcase: ``` function useFoo(setOne) { let x; let y; if
(setOne) { x = 1; y = 3; } else { x = 2; y = 5; }
return { x, y }; } ```
The LeaveSSA changes from: ``` .... bb1 (block): predecessor blocks: bb2 bb3
x$36:TPrimitive: phi(bb2: x$19, bb3: x$19) y$21[8:14]:TPrimitive: phi(bb2:
y$21, bb3: y$21)
... ```
to ``` ... bb1 (block): predecessor blocks: bb2 bb3 x$36:TPrimitive:
phi(bb2: x$19, bb3: x$19) y$38:TPrimitive: phi(bb2: y$21, bb3: y$21) ... ```
Notice how `y`'s reassignment got skipped previously.
Previously if any operand was reactive, we transferred that reactivity to other
operands that had a mutable effect (capture, conditionally mutate, mutate, or
store). But a value can be captured without ever being modified again. This PR
updates the logic to only transfer reactivity among operands that are actually
mutable at the given instruction, based on the mutable range. This is strictly
more precise.
This PR adds one remaining feature to InferReactivePlaces: tracking indirections
like LoadLocal, PropertyLoad, and similar. Consider something like:
```
// INPUT
x.push(reactiveValue);
// HIR
t0 = LoadLocal 'x'
t1 = PropertyLoad t0, 'push'
t2 = LoadLocal 'reactiveValue' // reactive
t3 = CallExpression mutate t0 . read t1 ( read t2 )
```
Because a reactive value (`t2`) flows into `t0`, we want to record t0 as
reactive as well. But that's just the temporary for `LoadLocal 'x'` - what's
really happening is that from this point, `x` is reactive.
InferReactiveIdentifiers tracked this, and now that logic is ported into
InferReactivePlaces as well. That lets us remove all the actual inference from
InferReactiveIdentifiers.
Updates `InferReactivePlaces` to infer control dependencies. We build on the
formal definition of control dependencies, which is that statement S2 is
control-dependent on statement S1 if S1 is in the post-dominance-frontier of S2.
Intuitively, if S1 decides whether S2 is reached or not, then S1 is a control
dependency of S2. The post dominance frontier of a given statement S is the set
of statements which may or may not reach S, and captures the intuitive notion.
We take advantage of phis: phis are the point where a variable may have multiple
values depending on the path we took. If a phi is not already known to be
reactive from data dependencies we check for control dependencies. Specifically
we look at each phi operand. We check if the block that the operand came from
has any reactive control dependencies, and if so we mark the phi itself as
reactive.
The post-dominance-frontier (PDF) algorithm requires walking the post-dominator
tree a bunch, so we cache the PDF of blocks so that we don't have to recalculate
on subsequent iterations.
In addition, `InferReactiveIdentifiers` now uses the _union_ of its own
inference plus the new `InferReactivePlaces` output when deciding what
identifiers are reactive. This ensures that control dependencies are recorded
correctly, fixing the previous test cases. The next diff adds the remaining
features to InferReactivePlaces so that it can fully replace
InferReactiveIdentifiers.
See context from #2187 for background about control dependencies.
Our current `PruneNonReactiveIdentifiers` pass runs on ReactiveFunction, after
scope construction, and removes scope dependencies that aren't reactive. It
works by first building up a set of reactive identifiers in
`InferReactiveIdentifiers`, then walking the ReactiveFunction and pruning any
scope dependencies that aren't in that set.
The challenge is control variables, as demonstrated by the test cases in #2184.
`InferReactiveIdentifiers` runs against ReactiveFunction, and when we initially
wrote it we didn't consider control variables. To handle control variables we
really need to use precise control- & data-flow analysis, which is much easier
with HIR.
This PR adds the start of `InferReactivePlaces`, which annotates each `Place`
with whether it is reactive or not. This allows the annotation to survive
LeaveSSA, which swaps out the identifiers of places but leaves other properties
as-is. This version does _not_ yet handle control variables, but it's already
more precise than our existing inference. In our current inference, if `x` is
ever assigned a reactive value, then all `x`s are marked reactive. In our new
inference, each instance of `x` (each Place) gets a separate flag based on
whether x can actually be reactive at that point in the program.
There are two main next steps (in follow-up PRs):
* Update the mechanism by which we prune non-reactive dependencies from scopes.
* Handle control variables. I think we may be able to use dominator trees to
figure out the set of basic blocks whose reachability is gated by the control
variables. This should clearly work for if/else and switch, as for loops i'm not
sure but intuitively it seems right.
This is part of a stack for inferring variables which are reactive via *control
dependencies* as opposed to a data dependency. In compiler engineering, a
statement S2 is control-dependent on statement S1 if S1 is in the post-dominance
frontier of S2. Stated more intuitively: if S1 decides whether or not S2 is
reached, then S1 is a control dependency of S2.
As a start, we add `Place.reactive: boolean` so that individual places can track
whether they are reactive or not. This lets us do fine-grained reactivity
inference on the control-flow graph, even taking into account different SSA
instances of a variable, so that we can say that a particular SSA version of `x`
is reactive, while other "versions" of x (due to reassignment) are not.
Adds a toy next.js app which doesn't do anything interesting. It has a single
e2e test which can be run via `npm run test:e2e`, which checks if a `$` variable
was injected by the compiler, as a sanity check whenever we commit a new version
of the compiler to the external repo.
Not every consumer of Forget will be able to run an experimental version of
React. In the meantime before useMemoCache is stable, provide a way for OSS to
pass in a userspace impl.
Turns out we were only using this in PrintHIR and in the logger, so it should be
safe to inline and makes installing the plugin for external collaborators a
little easier.
I'm assuming inlining this is ok because we only need to defer to the host's
Babel version(s) when it affects parsing or the final codegen output, hence why
we continue to no-inline @babel/types – as it is responsible for creating AST
nodes during codegen
The flattening broke the shell script because the directory structure changed.
Instead of depending on a flaky shell script, this PR encodes the commands as
part of the github workflow.
Attempt to bundle the plugin with rollup instead of just tsc. I'm intentionally
not inlining babel related modules, as we should ideally rely on the host
environment's version instead
We can now fully remove prettier from babel-plugin-react-forget. I moved it to
devDependencies instead, to make the rollup build simpler and so we can continue
to prettify our internal source code.
This moves prettier formatting into fixture-test-utils instead, so we can remove
the dependency on prettier in the babel plugin. I want to do this because I
don't want to include prettier in the rolledup artifact when we build the babel
plugin.
Runs Forget on the playground, so we can dogfood Forget and experiment with
improvements to UX.
Opts out of compiling the layout page because thats run on the server which
doesn't seem to have access to useMemoCache.
Joe Savona
Sathya Gunasekaran
Lauren Tan
Mofei Zhang