This allows the plugin to be configured to run on an allowlist, rather
than compiling all files helping with an incremental rollout plan.
The sources option takes both an array of path strings or a function
to be flexible.
For now I've left this be optional but we can make it required.
ghstack-source-id: 282a33dc8d08d47f699894692e0fcc813dff5b77
Pull Request resolved: https://github.com/facebook/react-forget/pull/2855
Fixes a tiny inconsistency with compiler options where one was all
uppercase and one all lowercase by normalizing to lowercase regardless
of the casing of the user's config.
ghstack-source-id: fe60a3259de89a1b3fdd7475950e16e96cc57f6b
Pull Request resolved: https://github.com/facebook/react-forget/pull/2832
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 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.
I addressed some of the cases that lead to this invariant but there were still
more. In this case, we have scopes like this:
```
scope @1 declarations=[t$0] {
let t$0 = ArrayExpression []
if (...) {
return null;
}
}
scope @2 deps=[t$0] declarations=[t$1] {
let t$1 = Jsx children=[t$0] ...
}
```
Because scope 1 has an early return, PropagateEarlyReturns wraps its contents in
a label and converts the returns to breaks:
```
scope @1 declarations=[t$0] earlyReturn={t$2} {
let t$2
bb0: {
let t$0 = ArrayExpression []
if (...) {
t$2 = null;
break bb0;
}
}
}
scope @2 deps=[t$0] declarations=[t$1] {
let t$1 = Jsx children=[t$0] ...
}
```
But then MergeReactiveScopesThatInvalidateTogether smushes them together:
```
scope @1 declarations=[t$1] earlyReturn={t$2} {
let t$2
bb0: {
let t$0 = ArrayExpression [] // <--- Oops! We're inside a block now
if (...) {
t$2 = null;
break bb0;
}
}
let t$1 = Jsx children=[t$0] ...
}
```
Note that the `t$0` binding is now created inside the labeled block, so it's no
longer accessible to the Jsx instruction which follows the labeled block. This
isn't an issue with promoting temporaries or propagating outputs, but a simple
issue of the labeled block (used for early return) introducing a new block
scope. The solution here is to simply reorder the passes so that we transform
for early returns after other optimizations. This means the jsx element will
basically move inside the labeled block, solving the scoping issue:
```
scope @1 declarations=[t$1] earlyReturn={t$2} {
let t$2
bb0: {
let t$0 = ArrayExpression [] // ok, same block scope as its use
if (...) {
t$2 = null;
break bb0;
}
let t$1 = Jsx children=[t$0] // note this moved inside the labeled block
}
}
```
Fixes T175282980. InferReactiveScopeVariables had logic to force assigning a
scope to MethodCall property lookups with the idea of forcing the method call
lookup to be in the same scope as the method call itself. But this doesn't work
if we never assign a scope to the method call! That can happen if we're able to
infer that the method call produces a primitive and doesn't need memoization.
This PR changes things so that:
* InferReactiveScopeVariables no longer assumes that MethodCall property values
need a scope
* We run a separate pass that ensures that _if_ a MethodCall has a scope, that
it's property is in the scope, and that otherwise its property doesn't get a
scope. This is similar to the existing passes that force a single scope for
related instructions like ObjectMethod+ObjectExpression and fbt operands/calls.
This is a key part of avoiding generating conflicting names in our output. To
start, RenameVariables now returns a Set of the unique identifier names that
exist in the function. Codegen uses this to avoid generating duplicate names for
change variables and for the `$` useMemoCache variable. Rather than always emit
`$` or `c_N`, codegen checks that this name would not conflict and appends an
incrementing suffix until it finds a unique name.
Note that it's still possible for us to generate conflicts with global
variables, both during RenameVariable and Codegen. The next step will be to
avoid conflicts with globals.
Implements the optimization described in the previous PR: if we know that a
scope's dependency will _always_ invalidate (it is not memoized and it is
guaranteed to be a new object if the instruction executes, such as an array or
object literal), then we can prune that scope. The invalidation is transitive:
we track always-invalidating types from within scopes, and if their scope gets
invalidated we prune downstream scope that depend on them.
## Test Plan
Tested via #2639 - see https://fburl.com/everpaste/3e3hjpjs. 91 files change
output due to reactive scopes which would always invalidate due to always
invalidating dependencies.
Infer if a function is a component or hook when we're deciding to compile a
function and store that in the environment.
This is used in passes like InferReferenceEffects rather than having to re-parse
the name in each pass.
filepath
Internal rollout currently has a good number of test failures.
`enableEmitInstrumentForget` can help developers understand which functions /
files they should look at:
```
// input
function Foo() {
userCode();
// ...
}
// output
function Foo() {
if (__DEV__ && inE2eTestMode) {
logRender("Foo", "/path/to/filename.js");
}
const $ = useMemoCache(...);
userCode();
}
```
Updates the compiler to understand Flow hook syntax. Like component syntax, in
infer mode hooks are compiled by default unless opted out.
Looking ahead, i can imagine splitting up our compilation modes as follows:
* Annotations: opt-in explicitly
* Declarations: annotations + component/hook declarations
* Infer: annotations, component/hook declarations, + component/hook-like
functions
This also suggest an alternative annotation strategy: "use react" (or "use
component" / "use hook") as a general way to tell the compiler that a function
is intended for React. Then opting out of memoization could do "use
react(nomemo)".
We want to start moving away from "Forget", so this PR adds support "use memo"
and "use no memo"
I've left "use forget" and "use no forget" directives unchanged for now, as we
need to migrate existing users first and then come back and delete support for
these directives.
Currently we only allow adding the directive to function bodies, but there may
be cases where we want to always opt out an entire module from being compiled by
Forget
Continuing on my quest to clean up our feature flags, the logic for merging
consecutive feature flags is stable. Let's remove
`@enableMergeConsecutiveScopes` since this is enabled everywhere.
Some components stop being components over time and are used as regular
functions instead, but they may have lingering hook calls. Those hook calls make
it so the capitalized function calling them do not error (they appear to be a
function to existing eslint rules), but they are nonetheless unsafe to memoize.
This diff adds a conservative option to bail out on all capitalized function
calls.
There are a handful of known-non-component capitalized functions, like
`Boolean`, `String`, and `Number`. This diff also adds the ability to supply
capitalized function names that should not be considered in this analysis.
I added three tests:
1. Ensure an error occurs in the obvious case
2. Ensure an error occurs when the value is aliased simply
3. Ensure the allowlist works
This is my first commit so please go hard on me. I was unsure about where this
code should live, so please nitpick.
These validations needs to be able to transitively check for violations within
function expressions, without immediately erroring. So the inner "-Impl" helpers
return a Result. But the outer, exported validate functions don't need to return
a Result, especially since TS has no Rust-style enforcement that return values
are actually used. Unwrapping within the validation means the caller can't
forget to do so and inadvertently silence the errors.
This pass doesn't really make sense in light of
`@enableTransitivelyFreezeFunctionExpressions`. The original idea of
ValidateFrozenLambdas was that trying to pass a "mutable" lambda to a frozen
value was invalid. But since then we've realized that the better heuristic is
that freezing a lambda is transitive.
Rewrites the validation to not rely on the mutable range of functions to
determine whether they are called or not, since the range can be extended for
other reasons (they happen to reference a mutable value that is mutated later,
even though the function isn't called during render).
Instead we use the same approach as validateNoSetStateInRender, explicitly
tracking references to function expressions that access refs, and checking if
those function expressions appear to be called. This can have false negatives,
as with the setState validation, but catches lots of obviously incorrect code
without false positives.
Validates that all references to a variable (pre-SSA) are consistently "local"
references or "context" references. Ie, if a variable is declared as
DeclareContext, any accesses must be eg LoadContext or StoreContext, not
LoadLocal/StoreLocal. This will help with the issue from #2577 (assuming that we
know a variable _is_ a context variable) but also provides a more precise
bailout for an existing case with destructuring assignment to a context
variable.
---
This change simply logs on every function we encounter with a `use no forget`
directive. A few nuances -- `compilationMode: "infer"` only compiles functions
we infer to be 'react functions'.
```js
// `add` would not be compiled, as it has no jsx, no hook calls,
// and is not named as a component or hook
function add(a, b) {
return a + b;
}
```
With this PR, we would report todos for functions that Forget wouldn't
ordinarily try to compile.
```js
// Todo: Skipped due to "use no forget" directive.
function add(a, b) {
"use no forget";
return a + b;
}
```
This seems fine to me as (1) it's a bit nonsensical to have a `use no forget`
direction on a non-react function, and (2) we're goalling on getting `use no
forget`s down to 0.
We're doing some internal benchmarking using a lightweight bundler that @pieterv
wrote for experimentation purposes. It's designed to fully preserve Flow type
annotations so we can experiment with type-driven compilation and test out what
benefits we might get from "cross-module" compilation more easily (ie by just
bundling together a few modules so we can see them all as one).
However, the bundler renames local variables and imports, so that a reference to
`useMemo()` might end up as `React$useMemo()` or similar. This PR adds a flag to
tell the compiler that builtin hooks might be prefixed and resolve them
appropriately.
The compiler bails out of compiling code that contains suppressions of the
official React ESLint rules. However, some apps may use additional rules that
they want to trigger bailouts for, or use the official rules under a different
name (we do this at Meta). This PR adds a compiler flag to specify a custom set
of line rule names, suppression of which should trigger a bailout.
Interpolating values into the `reason` field of an error breaks our error
aggregation. This PR moves the offending function name into the `description`
field which isn't used for aggregation.
It's starting to get complex just with a couple of extra
passes — we either need to substantially extend the HIR or (as i've done so far)
pass information from early passes to later ones. This PR changes things so that
very early in the babel plugin we fork into a separate mode. Forest has
its own `compileProgram()` equivalent, its own pipeline, its own codegen, etc.
Adds a new compiler pass that will eventually actually handle early returns
within reactive scopes. For now it just detects them and throws a Todo error.
Adds a new mode which validates that existing manual memoization is preserved
_without_ using information from the manual memoization to affect compilation.
This gives us a way to try out the more aggressive version of Forget — ignoring
manual memoization — first and see how much code bails out and what patterns
cause this.
We can then proceed to enable the mode to actually _preserve_ existing memo
guarantees only where necessary.
Extends `@enablePreserveExistingMemoization` to validate that all of the
original values were actually memoized. This works nearly identically to how we
validate effect deps are memoized. We look for Memoize instructions whose values
need memoization but whose range extends past the memoize instruction, or where
the value isn't memoized at all.
ObjectExpressions
---
Currently, we're removing all reactive scopes containing object methods. This
could produce incorrect output as object method instructions may still be
included in other reactive scopes (and will lose their dependencies).
In normal React certain operations don't allocate new objects (property loads,
binary expressions, etc) and therefore don't need a reactive scope in Forget.
For example, property loads only extract part of an existing value and don't
allocate something new, while binary expressions are known to produce primitive
values that don't allocate. We rely on the fact that whenever their inputs
change we will re-run the component/hook and propagate the result forward.
For Forest, the only way to propagate data is via reactive scopes: the component
code is equivalent to a "setup" function. This PR updates some of our passes to
ensure that we create (and don't prune) scopes for these types of operations. I
started with a conservative set for now.
For Forest, we previously converted reactive scopes into derived signals during
Codegen. I'm moving this to a separate pass primarily to keep codegen simple
since there's enough complexity just dealing with core JS semantics. Ideally
we'd do a similar setup even for regular Forget, ie lower reactive scopes just
prior to codegen.
At the same time i also reordered the forget passes to be just before codegen,
and cleaned things up a bit. For state lowering, we now just rewrite `useState`
-> `createState`, because we actually need to keep around the setter function to
trigger scheduling updates in addition to writing the signal value.
---
I modeled guards as try-finally blocks to be extremely explicit. An alternative
implementation could flatten all nested hooks and only set / restore hook guards
when entering / exiting a React function (i.e. hook or component) -- this
alternative approach would be the easiest to represent as a separate pass
```js
// source
function Foo() {
const result = useHook(useContext(Context));
...
}
// current output
function Foo() {
try {
pushHookGuard();
const result = (() => {
try {
pushEnableHook();
return useHook((() => {
try {
pushEnableHook();
return useContext(Context);
} finally {
popEnableHook();
}
})());
} finally {
popEnableHook();
};
})();
// ...
} finally {
popHookGuard();
}
}
// alternative output
function Foo() {
try {
// check current is not lazyDispatcher;
// save originalDispatcher, set lazyDispatcher
pushHookGuard();
allowHook(); // always set originalDispatcher
const t0 = useContext(Context);
disallowHook(); // always set LazyDispatcher
allowHook(); // always set originalDispatcher
const result = useHook(t0);
disallowHook(); // always set LazyDispatcher
// ...
} finally {
popHookGuard(); // restore originalDispatcher
}
}
```
Checked that IG Web works as expected
Unless I add a sneaky useState:
<img width="705" alt="Screenshot 2023-12-05 at 6 44 59 PM"
src="https://github.com/facebook/react-forget/assets/34200447/3790bd76-7d71-44b5-a62e-f53256fb5736">
Joe Savona
Sathya Gunsasekaran
Lauren Tan
Mofei Zhang
Jordan Brown
Mike Vitousek