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.
This kept throwing warnings in our playground build because prettier uses node
APIs and is not meant to be bundled and run on the browser.
There's a separate standalone version that runs on the browser. A follow on PR
will make the playground use that but this PR is a first step towards using a
standalone prettier.
Currently DCE can remove variable declarations that are unused, ie where all
control-flow paths to usage of the variable are overwritten by a reassignment.
We then have to reconstruct the original variable declaration at the appropriate
block scope during LeaveSSA, which is complex and can actually be incorrect in
some cases.
This PR updates to ensure that DCE will not remove the original variable
declaration for any variable that is used (even in the case of always being
reassigned before use). The main changes are:
* DCE retains variable declarations, but if a variable declaration is always
shadowed by reassignments then DCE will rewrite StoreLocal -> DeclareLocal so
that it can DCE the unused initial value.
* BuildHIR now has to change its handling for reassignment destructure
instructions with nesting. Nesting uses a temporary which would appear as a
declaration of a new variable, which is incompatible with other reassignments.
See comments in the file.
* LeaveSSA is quite a bit simpler now, since we never need to reconstruct a
declaration.
Adds test cases for all the cases of control dependencies that I can think of.
We don't currently handle control dependencies correctly in any of these cases.
There's also another test case which demonstrates why reactive dependency
inference needs to be fixpoint, even for non-control dependencies.
Reactive scopes are currently preceded by individual variable declarations, one
for each of the scope's dependencies. Only after checking independently if each
of these dependencies has changed do we then do an "or" to check if we should
actually recompute the body of the scope.
But that's wasteful: it's more efficient to rely on `||` short-circuiting, and
recompute as soon as any input changes.
The main reason I can see not to do this is debugging. Having the change
variables makes it easy to see what changed. But at this point I think it makes
sense to optimize for code size and performance; we can always add back a
dev-only mode that uses change variables if that turns out to help debugging.
Rewrites the core logic of MergeConsecutiveScopes to be easier to follow and fix
bugs. We now do a two-pass approach:
* First we iterate block instructions to identify scopes which can be merged,
without actually merging the instructions themselves.
* Then we iterate again, copying instructions from the block either into the new
output block, or into their merged scope, as appropriate.
I think the simplicity here is worth the performance cost, and we can always
revisit later as necessary.
