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.
This morning @mofeiZ reminded me that our codegen doesn't really have any guards
against reordering, since temporaries are lazily emitted. We're relying on the
fact that our lowering and memoization carefully preserves order of evaluation,
such that delaying the instructions in codegen doesn't change semantics.
To help catch any mistakes with this, I had previously added code that reset the
codegen context's temporaries before/after exiting a reactive scope. That
ensured that temporaries from within the scope weren't accessible outside it.
This PR extends that approach to _all_ blocks, so that temporaries created
within a block aren't accessible outside it.
I'm also going to explore more actively resetting temporaries after they
"should" be used. There are a couple cases where temporaries are reused, though,
which we have to change first.
So far we've been preserving JSX whitespace all the way through to codegen. But
JSX has clear rules around whitespace handling, which allows us to trim
whitespace in the input in lots of cases. For the most part this doesn't change
our output, but I think that’s generally because of prettier. This PR should
make a big difference when debugging the compiler, by removing all the
whitespace JsxText values.
But in some edge-cases it really makes a difference in the output since we can
avoid memo slots for strings like `"\n "`..
## Test Plan
* Experimented with our internal tool to verify transform output to confirm that
JSXText whitespace does not impact fbt transform results.
* Synced and tested profile page, looks fine
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.
This is a repro for a bug that occurs when `enableMergeConsecutiveScopes` is
enabled. Reminder that this feature is off by default and not enabled yet
internally.
I found this via #2121, where eliminating extraneous whitespace JSXText
instructions meant that MergeConsecutiveScopes started merging a fixture
differently, revealing a bug. This PR reproduces that case by keeping the
identical structure, but using plain objects to represent the JSX elements
instead of JSX syntax.
This PR completes the refactor. We now do the following sequence:
* ValidateUseMemo. This is a new pass that extracts just the validation logic
from the existing InlineUseMemo. This was always being run before, so this pass
also always runs.
* DropManualMemoization. As before, this converts useMemo calls into an IIFE
(immediately invoked function expression).
* InlineImmediatelyInvokedFunctionExpressions (prev InlineUseMemo). This pass
now inlines _all_ IIFEs, including both useMemo calls that were dropped as well
as IIFEs that the user wrote.
The motivation for this change is that some codebases use IIFEs as a workaround
for lack of if expressions, but we're unable to optimize within function
expressions. This is the reason we originally added inlining for useMemo, but
given that IIFEs are common it makes sense to generalize the inlining.
## Test Plan
* Manually checked changes in output
* Synced internally and tested on profile page, no issues observed. Also
spot-checked some of the changes in ouput and it looks as expected.
The goal of this stack is to generalize `InlineUseMemo` into a pass that inlines
all immediately invoked function expressions (IIFEs). Rather than specialize
just useMemo calls, we'll rely on DropManualMemoization running first and
turning useMemo calls into IIFEs. Then the generalized inlining pass can handle
those IIFEs as well as others present in the source.
For now, moving the order of the pass makes the output closer to what it will
eventually be after this stack is complete.
#2127 introduced a special type for the result of `useContext()` that was sort
of ref-like. The intent was to allow code like this:
```
function Foo() {
const cx = useContext(...);
function onEvent() {
cx.foo = true;
};
return <Bar onEvent={onEvent} />;
}
```
However, that code actually is allowed by the compiler by default. It's only a
bailout when `@validateFrozenLambdas` is enabled. The "fix" in #2127 therefore
wasn't strictly necessary to unblock rollout, and it's also flawed in a few
ways:
* First, `useContext(FooContext)` should have equivalent behavior to a custom
hooks which does the same thing, ie `function useFooContext() { return
useContext(FooContext) }`. Specializing the type of useContext makes the
behavior different.
* Second, it meant that even readonly accesses of the context inside a callback
marked the function as capturing, which in turn prevented those callbacks from
being memoized.
So i'm reverting this and we'll have to think a bit more about this case.
---
Patch for original patch of injecting `useMemoCache` logic 😅 This is failing on
a good number React components internally (not in our current experiments)
The playground now uses the new pragma parser so it's guaranteed to use the
right defaults and have consistent parsing with snap/sprout. In addition, we now
emit a debug event from the compiler which contains pretty-printed environment
config, making it easy to check which settings are being applied in playground.
<img width="3008" alt="Screenshot 2023-10-05 at 11 05 58 AM"
src="https://github.com/facebook/react-forget/assets/6425824/2417f40c-1320-4c39-a661-a4e34e3d69c4">
Updates Snap and Sprout to use the new pragma parser, which also means they will
always use the same default flags as the compiler itself sets. A side benefit of
this is that you no longer need to rebuild snap/sprout to update their flags,
since they will take flags from the version of the compiler being executed.
Adds a helper function for parsing pragma strings to the compiler itself, and
exports it. This will be used in follow-ups to make Snap, Sprout, and Playground
all use the same pragma parser. The helper also starts from the default values,
so adopting this will also make it easy for all those places to have the same
defaults automatically.
Adds support for empty catch clauses in a try/catch. We add a new block kind
`catch` which prevents the empty catch block from being merged with other types
of blocks, preserving the block structure within the HIR and allowing us to
reconstruct the empty catch.
---
Implements popular feature request ✨ per feedback from a majority of snap users.
**Add `@debug` to the first line of your `testfilter.txt` file to opt into
implicit debug mode**, in which debug logging is enabled anytime filter mode is
on + only one fixture file is found.
- live edits to testfilter.txt are reflected in watch mode, so you can add /
remove `@debug` to `testfilter.txt` in the middle of a watch session
- I personally don't use debug mode all the time (I often a single file filtered
+ a lot of console log traces), but it should be easy to add `@debug` to the top
of `testfilter.txt` and leave it there forever.
This is a (hopefully) better approach at printing ReactiveFunction. The nesting
wasn't always clear in the previous version, this should help. See playground to
experiment.
Updates `Environment` to store all feature flags on a single `config` object. We
now also define an object with all the default config values, and use this to
populate defaults for any missing values in the user-provided config.
Most of our feature flags are accessed via the `Environment`, but a few cases
have slipped in where we look at the `config` object directly. The problem is
that the config object doesn't set defaults, so the check is effectively
encoding what the default is.
This PR moves to always accessing flags off of the environment, and adds a few
flags that weren't yet defined there.
Previously, we stored a global count variable that was updated every time we
added a property to the `arg` object. This was added to prevent collisions, and
make sure we do actually mutate the object.
But the count value was shared by the forget compiled and uncompiled versions,
so the same object mutated in either versions would result in having different
properties leading to potential test failures.
Instead, let's make count local and attempt to incrementally mutate the object
with different keys.
InferReferenceEffects uses object identity to merge states, which breaks when we
create a new object to model `undefined`.
Two value objects representing `undefined` are not equal due to referential
equality.
Instead, let's use a singleton to represent `undefined` value.
Handles an edge-case from earlier in the stack. When looking up a property on a
shape, if the property is defined we return it. But if it isn't defined, and the
property name is a hook, we treat it like a default custom hook.
Allows using hooks/methods off of the `React` namespace, for example
`React.useState(sathya)`. Thanks to the previous PR we correctly handle things
like validation of hooks called via propertyload syntax. The main change here is
to teach the compiler about the `React` namespace. This is a bit of a hack since
we treat it as a global, but we're transforming React code so this seems
reasonable (?).
There are a few additional touch-ups which I'll do in subsequent PRs to make
review easier. For example, we need to teach our useMemo/useCallback flattening
logic to also handle the case of `React.useMemo()` etc.
Hooks can be called via method call syntax, eg `Foo.useBar(sathya)`. This PR
teaches the compiler about this form of hooks for things like flattening scopes
with hooks, validating conditional hooks, etc.
Note that we still disallow calls on the React namespace, so things like
`React.useState()` continue to error. That's the next PR in the stack!
Adds a new type for representing context values, which is transitive. So
`useContext(a).b.c` also gets inferred as a context type. This allows us to
refine our inference, and allow passing callbacks that modify context where a
"frozen" lambda is exepcted.
This is a distilled version of the duplicate declaration @mofeiZ and I saw when
trying to sync latest Forget internally, plus a fix to avoid the duplicate
instruction.
Fixes an issue with incorrect spacing where spaces were getting dropped, despite
an explicit `{" "}` in the input. The issue is that we didn't maintain JSXText
all the way through compilation. BuildHIR distinguishes string literals (such as
the above, inside an expressioncontainer) from JSXText, and we propagate this
distinction all the way through to codegen.
But then codegen stores temporary values as `t.Expression` nodes, which means we
have to convert the JSXText nodes to StringLiteral and we lose the distinction.
This PR updates codegen to save temporaries as `t.Expression | t.JSXText` so
that we can preserve the difference. In most places we just coerce the value to
an expression, but the code for emitting JSX child items looks at the raw value
so it can distinguish them. JSXText is emitted as-is, while StringLiterals are
always wrapped in an expression container.
See the new test case which demonstrates the expression being preserved.
Object methods must not be cached independently, so this PR flattens the
reactive scope to prevent memoization.
In the future, we can combine the FlattenScopesWithObjectMethods and
FlattedScopesWithHooks passes by making them more modular. But this works for
now.
Object methods are lowered to functions and added to ObjectExpression. The
codegen is interesting because we shouldn't emit code that lowers the object
method into a separate statement and then stores it into an object expression.
An shorthand object method has different semantics than an object method using
the function syntax, so we need to preserve the shorthand object method syntax
in the generated code.
To do this, we don't immediately generate an AST node for the ObjectMethod but
instead store it in a side table during codegen. Only when emitting code for an
ObjectExpression, we lookup this side table and emit the object method inline in
the body.
Adds support for lowering rest element parameters to spreads. We eagerly create
a temporary, similar to the approach for destructuring. In theory we could do
something more optimal if you have a `...foo` (rest element where the argument
is an Identifier) but it doesn't seem worth optimizing yet.
