---
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.
As noted earlier in the stack and in chat, there are some cases where the output
of a reactive scope is not guaranteed to change just because its inputs did.
Consider a function `foo(x) { return x < 10 }`. If x was 0 and changes to 1, the
result of `foo(x)` won't change.
For code such as `[foo(x)]`, then, merging the scope for `t0 = foo(x)` and `t1 =
[t0]` into a single `t1 = [foo(x)]` could cause us to invalidate `t1`
unnecessarily. For example, x changing from 0 to 1 would allocate a new array of
`[true]` even though the value didn't meaningfully change. This is the second
category of merging, where we merge scopes A and B if the outputs of A are the
inputs to B.
With this change, we only do this type of merge if the outputs of the first
scope are known to invalidate whenever the input does. We're conservative about
this, and only consider function expressions, arrays, object, and jsx to always
invalidate. Function calls _may_ invalidate, but as w the `foo()` example here
they also may not.
Note that this is purely about optimization and not correctness. We could always
merge in this case (per earlier in the stack) but that might invalidate more
often than we would like.
This is the optimization mentioned in #2113. When we merge scopes, often the
declarations from the first scope become unnecessary, since those values are
only consumed by the subsequent, now-merged scope. There's no point emitting
those values as outputs of the merged scope since no one can consume them.
Thanks to the logic in #2116 we now know the last place each identifier is used.
We use that again here, to prune declarations that aren't used past the end of
the merged scope. This is a pretty dramatic win on cache slots used.
We can't merge scopes if the intervening instructions produce values that are
used later on. This PR improves the mechanism for detecting this case: first we
build up a mapping of the last time (max instruction id) each identifier is
used. Then when we're about to merge scopes we check if all the intervening
lvalues are last used at or before the scope. If so that means it's safe to
merge.
I can't think of any edge cases that are problematic in the old behavior, but
this version is more trivially correct and should allow us to extend to other
types of instructions more easily.
This is something we've wanted to do for a while, and which @sophiebits also
brought up. The idea is to merge consecutive reactive scopes that will always
invalidate together. There are two cases of this:
* Both scopes have the exact same inputs
* Or the inputs of the second scope are the outputs of the first
In both these cases it's pure overhead to keep the scopes separate. In the first
case where both scopes have the same inputs, merging allows us to check the
inputs once instead of 2+ times. In the second case, we know that the second
scope will invalidate when the first does so it's wasteful to recheck the
outputs of the first scope for changes.
This is already cutting down on memoization quite a bit in the fixtures. Note
that there's an additional optimization we can do after merging the second
category, which is to remove the first scope's declarations if they were only
referenced by the second scope. I'll add that in a follow-up.
---
(pasted from comment)
Instead of handling holey arrays, bail out with a TODO error.
Older versions of babel seem to have inconsistent handling of holey arrays, at
least when paired with HermesParser. When using these versions, we should bail
out instead of throwing a Babel validation error.
Issue:
The babel ast definition for array elements changed from Array<PatternLike> to
Array<PatternLike | null>. Older versions do not expect null in the ArrayPattern
ast and will throw a validation error during Codegen.
- HermesParser will parse [, b] into [NodePath<null>, NodePath<Identifier>]
- Forget will try to preserve this holey array when we codegen back to js
(e.g. we call a babel builder function arrayPattern([null, identifier]))
- Babel will fail with `TypeError: Property elements[0] of ArrayPattern
expected node to be of a type ["PatternLike"] but instead got null`
PR that changed the AST definition:
https://github.com/babel/babel/pull/10917/files#diff-19b555d2f3904c206af406540d9df200b1e16befedb83ff39ebfcbd876f7fa8aL52-R56
[ez] Add TODO bailout on non-backward compatible holey arrays
---
(pasted from comment)
Instead of handling holey arrays, bail out with a TODO error.
Older versions of babel seem to have inconsistent handling of holey arrays, at
least when paired with HermesParser. When using these versions, we should bail
out instead of throwing a Babel validation error.
Issue:
The babel ast definition for array elements changed from Array<PatternLike> to
Array<PatternLike | null>. Older versions do not expect null in the ArrayPattern
ast and will throw a validation error during Codegen.
- HermesParser will parse [, b] into [NodePath<null>, NodePath<Identifier>]
- Forget will try to preserve this holey array when we codegen back to js
(e.g. we call a babel builder function arrayPattern([null, identifier]))
- Babel will fail with `TypeError: Property elements[0] of ArrayPattern
expected node to be of a type ["PatternLike"] but instead got null`
PR that changed the AST definition:
https://github.com/babel/babel/pull/10917/files#diff-19b555d2f3904c206af406540d9df200b1e16befedb83ff39ebfcbd876f7fa8aL52-R56
This PR adds preliminary support for hoisting const variable declarations. We do
this via BuildHIR when lowering top level statements in a BlockStatement, by
first checking which bindings are in scope to be hoistable if referenced before
they are declared. The declarations are then hoisted to their earliest point
where they are referenced (ie the top level statement just before) as context
variables.
Later, prior to codegen, we restore the original source by removing the
DeclareContexts and transforming their associated StoreContexts back.
Support for hoisting other kinds of declarations will come in future PRs!
This is a redo of #1640 now that we've established the necessary infrastructure,
most notably `Effect.ConditionallyMutate` and `noAlias` from #2103 earlier in
this stack. We can now understand the semantics of hooks that return deeply
readonly values composed of primitives, arrays, or objects such that any
`.map()` or `.filter()` calls are guaranteed to be the corresponding array
methods. That further allows us to refine, since we know that the lambdas passed
to these calls can't alias, are conditionally mutable, etc. All in all this
should let us memoize less in practice.
Adds `noAlias` support for CallExpression, including hooks. Note that we treat
hook arguments as escaping by default — ie we assume that they don't just flow
into the hook return value, but are just outright escape points equivalent to a
return. A `noAlias` annotation on a hook definition disables both: this will
allow us to avoid memoizing the `graphql` tag arguments to `useFragment`, for
example.
Skips compilation of code that has a reference to `useMemoCache()`, as a
last-resort to avoid double-compilation of code. This is meant as a quick way to
unblock since we're still seeing some double compilation issues when syncing
internally.
This has been nagging at me for a _long_ time: we unnecessarily memoize function
callbacks passed to things like Array.prototype.map, even though we know these
functions can't escape. This PR fixes this as follows:
* Adds a `noAlias?: boolean` flag to builtin function signatures, defaulting to
false if not specified.
* Adds a feature flag, `enableNoAliasOptimizations`, to gate optimizations based
on the value of that new flag.
* When the feature is enabled, `PruneNonEscapingScopes` now looks up the
signature of method calls, and avoids memoizing the arguments if the signatures
specifies `noAlias: true`.
* Annotates Array.prototype.map and Array.prototype.filter as `noAlias`.
This does not mean we'll never memoize arguments to Array.prototype.map, it just
means that the argument itself won't be considered as escaping. If the function
still escapes by some other means it will get memoized:
```
function Component(props) {
const f = () => {}; // memoized!
const x = [].map(f); // not from here..
return [x, f]; // but bc it escapes here
}
```
Note: this delivers some of the wins from #1640. That PR tried to do a bunch of
things, part of which I already landed w the introduction of
ConditionallyMutate, which allowed us to type Array.prototype.map. This PR
further gives us the ability to understand functions that don't alias their
params at all. The remaining bit from #1640 is the idea of understanding that
key hooks such as `useFragment()` return transitively readonly, transitively
array/object/primitive values, and any `.map()` or `.filter()` calls must be on
arrays, allowing us to optimize them. Without that extra step, we'll still have
to memoize a lot of `array.map()` lambdas just because we aren't sure that the
receiver is an Array. But this PR helps with some cases, and lays the groundwork
for the rest of that PR.
Per the title, `<fbt:param>0</fbt:param>` is invalid FBT, you must wrap the text
in an expression container. But that's not all, `fbt:param` can only have a
single child, which means we have to strip out the text elements that occur from
the whitespace in the source.
Mofei Zhang
Joe Savona
Sathya Gunasekaran
Lauren Tan