Summary: In change-detection mode, we previously were spreading the contents of the computation block into the result twice. Other babel passes that cause in-place mutations of the AST would then be causing action at a distance and breaking the overall transform result. This pr creates clones of the nodes instead, so that mutations aren't reflected in both places where the block is used.
ghstack-source-id: b78def8d8d
Pull Request resolved: https://github.com/facebook/react/pull/30148
Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:
1. Build up a mapping of context variables in the outer component/hook
2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
3. Propagate aliases of those functions via StoreLocal/LoadLocal
4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.
Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.
ghstack-source-id: c2b22e3d62
Pull Request resolved: https://github.com/facebook/react/pull/30107
Currently, the `hookKind` for `useInsertionEffect` is set to
`useLayoutEffect`. This pull request fixes it by adding a new `hookKind`
for `useInsertionEffect`.
Double checked by syncing internally and verifying the # of `visitInstruction` calls with unique `InstructionId`s.
This is a bit of an awkward pattern though. A cleaner alternative might be to override `visitValue` and store its results in a sidemap (instead of returning)
ghstack-source-id: f6797d7652
Pull Request resolved: https://github.com/facebook/react/pull/30077
Adds Array.prototype methods that return primitives or other arrays -- naive type inference can be really helpful in reducing mutable ranges -> achieving higher quality memoization.
Also copies Array.prototype methods to our mixed read-only JSON-like object shape.
(Inspired after going through some suboptimal internal compilation outputs.)
ghstack-source-id: 0bfad11180
Pull Request resolved: https://github.com/facebook/react/pull/30075
Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g.
```js
// source
a();
if (...) {
b();
}
c();
// HIR
bb0:
a()
if test=... consequent=bb1 fallthrough=bb2
bb1:
b()
goto bb2
bb2:
c()
// AlignReactiveScopesToBlockScopesHIR nodes
Root node (maps to both bb0 and bb2)
|- bb1
|- ...
```
There are two issues with the existing implementation:
1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range.
```
\# This case gets handled correctly
┌──────────────┐
│ │
block start block end
scope start scope end
│ │
└───────────────┘
\# But not this one!
┌──────────────┐
│ │
block start block end
scope start scope end
│ │
└───────────────┘
```
2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details.
ghstack-source-id: 327dec5019
Pull Request resolved: https://github.com/facebook/react/pull/29891
ghstack-source-id: 04b1526c85
Pull Request resolved: https://github.com/facebook/react/pull/29878
The AlignReactiveScope bug should be simplest to fix, but it's also caught by an invariant assertion. I think a fix could be either keeping track of "active" block-fallthrough pairs (`retainWhere(pair => pair.range.end > current.instr[0].id)`) or following the approach in `assertValidBlockNesting`.
I'm tempted to pull the value-block aligning logic out into its own pass (using the current `node` tree traversal), then align to non-value blocks with the `assertValidBlockNesting` approach. Happy to hear feedback on this though!
The other two are likely bigger issues, as they're not caught by static invariants.
Update:
- removed bug-phi-reference-effect as it's been patched by @josephsavona
- added bug-array-concat-should-capture
When converting value blocks from HIR to ReactiveFunction, we have to drop StoreLocal assignments that represent the assignment of the phi, since ReactiveFunction supports compound expressions. These StoreLocals are only present to represent the conditional assignment of the value itself - but it's also possible for the expression to have contained an assignment expression. Before, in trying to strip the first category of StoreLocal we also accidentally stripped the second category. Now we check that the assignment is for a temporary, and don't strip otherwise.
ghstack-source-id: e7759c963b
Pull Request resolved: https://github.com/facebook/react/pull/30067
Adds a fixture based on internal case where our current output is quite a bit more verbose than the original memoization. See the comment in the fixture for more about the heuristic we can apply.
ghstack-source-id: e637a38140
Pull Request resolved: https://github.com/facebook/react/pull/29998
Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain.
This PR includes two changes:
* First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once
* Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks.
In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization.
This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this.
So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes.
I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures.
ghstack-source-id: ad3e516fa4
Pull Request resolved: https://github.com/facebook/react/pull/29882
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.
This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.
Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.
Partially addresses #29648.
ghstack-source-id: ecc61c9f0b
Pull Request resolved: https://github.com/facebook/react/pull/29997
If a component uses the `useRef` hook directly then we type it's return
value as a ref. But if it's wrapped in a custom hook then we lose out on
this type information as the compiler doesn't look at the hook
definition. This has resulted in some false positives in our analysis
like the ones reported in #29160 and #29196.
This PR will treat objects named as `ref` or if their names end with the
substring `Ref`, and contain a property named `current`, as React refs.
```
const ref = useMyRef();
const myRef = useMyRef2();
useEffect(() => {
ref.current = ...;
myRef.current = ...;
})
```
In the above example, `ref` and `myRef` will be treated as React refs.
Updated version of #29758 removing `useFormState` since that was the
previous name for `useActionState`.
---------
Co-authored-by: Hieu Do <hieudn.uh@gmail.com>
Adds fixtures for `macro.namespace(...)` style invocations which we use internally in some cases instead of just `macro(...)`. I tried every example i could think of that could possibly break it (including basing one off of another fixture where we hit an invariant related due to a temporary being emitted for a method call), and they all worked. I just had to fix an existing bug where we early return in some cases instead of continuing, which is a holdover from when this pass was originally written as a ReactiveFunction visitor.
ghstack-source-id: c01f45b3ef
Pull Request resolved: https://github.com/facebook/react/pull/29899
Adds a pass just after DCE to reorder safely reorderable instructions (jsx, primitives, globals) closer to where they are used, to allow other optimization passes to be more effective. Notably, the reordering allows scope merging to be more effective, since that pass relies on two scopes not having intervening instructions — in many cases we can now reorder such instructions out of the way and unlock merging, as demonstrated in the changed fixtures.
The algorithm itself is described in the docblock.
note: This is a cleaned up version of #29579 that is ready for review.
ghstack-source-id: c54a806cad
Pull Request resolved: https://github.com/facebook/react/pull/29863
Updates our scope merging pass to allow more types of instructions to intervene btw scopes. This includes all the non-allocating kinds of nodes that are considered reorderable in #29863. It's already safe to merge scopes with these instructions — we only merge if the lvalue is not used past the next scope. Additionally, without changing this pass reordering isn't very effective, since we would reorder to add these types of intervening instructions and then not be able to merge scopes.
Sequencing this first helps to see the win just from reordering alone.
ghstack-source-id: 79263576d8
Pull Request resolved: https://github.com/facebook/react/pull/29881
Summary: The change detection mode was unavailable in the playground because the pragma was not a boolean. This fixes that by special casing it in pragma parsing, similar to validateNoCapitalizedCalls
ghstack-source-id: 4a8c17d21a
Pull Request resolved: https://github.com/facebook/react/pull/29889
Our passes aren't sequenced such that we could observe this bug, but this retains the proper terminal kind for pruned-scopes in mapTerminalSuccessors.
ghstack-source-id: 1a03b40e45
Pull Request resolved: https://github.com/facebook/react/pull/29884
Summary: Minor change inspired by #29863: the BuildHIR pass ensures that Binary and UnaryOperator nodes only use a limited set of the operators that babel's operator types represent, which that pr relies on for safe reorderability, but the type of those HIR nodes admits the other operators. For example, even though you can't build an HIR UnaryOperator with `delete` as the operator, it is a valid HIR node--and if we made a mistaken change that let you build such a node, it would be unsafe to reorder.
This pr makes the typing of operators stricter to prevent that.
ghstack-source-id: 9bf3b1a37e
Pull Request resolved: https://github.com/facebook/react/pull/29880
Fixes a bug found by mofeiZ in #29878. When we merge queued states, if the new state does not introduce changes relative to the queued state we should use the queued state, not the new state.
ghstack-source-id: c59f69de15
Pull Request resolved: https://github.com/facebook/react/pull/29879
Summary: We now expect that candidate components that have Flow or TS type annotations on their first parameters have annotations that are potentially objects--this lets us reject compiling functions that explicitly take e.g. `number` as a parameter.
ghstack-source-id: e2c2334826
Pull Request resolved: https://github.com/facebook/react/pull/29866
Summary: We can tighten our criteria for what is a component by requiring that a component or hook contain JSX or hook calls directly within its body, excluding nested functions . Currently, if we see them within the body anywhere -- including nested functions -- we treat it as a component if the other requirements are met. This change makes this stricter.
We also now expect components (but not necessarily hooks) to have return statements, and those returns must be potential React nodes (we can reject functions that return function or object literals, for example).
ghstack-source-id: 4507cc3955
Pull Request resolved: https://github.com/facebook/react/pull/29865
Summary: Projects which have heavily adopted Flow component syntax may wish to enable the compiler only for components and hooks that use the syntax, rather than trying to guess which functions are components and hooks. This provides that option.
ghstack-source-id: 579ac9f0fa
Pull Request resolved: https://github.com/facebook/react/pull/29864
Per title, implements an HIR-based version of FlattenScopesWithHooksOrUse as part of our push to use HIR everywhere. This is the last pass to migrate before PropagateScopeDeps, which is blocking the fix for `bug.invalid-hoisting-functionexpr`, ie where we can infer incorrect dependencies for function expressions if the dependencies are accessed conditionally.
ghstack-source-id: 05c6e26b3b
Pull Request resolved: https://github.com/facebook/react/pull/29840
Pre the title, this implements an HIR-based version of FlattenReactiveLoops. Another step on the way to HIR-everywhere.
ghstack-source-id: e1d166352d
Pull Request resolved: https://github.com/facebook/react/pull/29838
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.
Before submitting a pull request, please make sure the following is
done:
1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
10. If you haven't already, complete the CLA.
Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->
## Summary
<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
The parsePluginOptions seemed to be duplicated within
[BabelPlugin.ts](f5af92d2c4/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts (L32))
and
[Program.ts](f5af92d2c4/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts (L220)).
Since the options already parsed in BabelPlugin.ts should have been
passed to compileProgram, in this PR we deleted parsePluginOptions in
compileProgram and used the options passed as arguments as they are.
I've done that.
## How did you test this change?
<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
If you leave this empty, your PR will very likely be closed.
-->
<img width="516" alt="image"
src="https://github.com/facebook/react/assets/87469023/2a70c6ea-0330-42a2-adff-48ae3e905790">
Adds a shape type for component props, which has one defined property: "ref". This means that if the ref property exists, we can type usage of `props.ref` (or via destructuring) the same as the result of `useRef()` and infer downstream usage similarly.
ghstack-source-id: 76cd07c5df
Pull Request resolved: https://github.com/facebook/react/pull/29834
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope:
* Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them.
* Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case.
I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement.
ghstack-source-id: 80610ddafa
Pull Request resolved: https://github.com/facebook/react/pull/29820
Adds additional information to the CompileSuccess LoggerEvent:
* `prunedMemoBlocks` is the number of reactive scopes that were pruned for some reason.
* `prunedMemoValues` is the number of unique _values_ produced by those scopes.
Both numbers exclude blocks that are just a hook call - ie although we create and prune a scope for eg `useState()`, that's just an artifact of the sequencing of our pipeline. So what this metric is counting is cases of _other_ values that go unmemoized. See the new fixture, which takes advantage of improvements in the snap runner to optionally emit the logger events in the .expect.md file if you include the "logger" pragma in a fixture.
ghstack-source-id: c2015bb556
Pull Request resolved: https://github.com/facebook/react/pull/29810
Mostly addresses the issue with non-reactive pruned scopes. Before, values from pruned scopes would not be memoized, but could still be depended upon by downstream scopes. However, those downstream scopes would assume the value could never change. This could allow the developer to observe two different versions of a value - the freshly created one (if observed outside a scope) or a cached one (if observed inside, or through) a scope which used the value but didn't depend on it.
The fix here is to consider the outputs of pruned reactive scopes as reactive. Note that this is a partial fix because of things like control variables — the full solution would be to mark these values as reactive, and then re-run InferReactivePlaces. We can do this once we've fully converted our pipeline to use HIR everywhere. For now, this should fix most issues in practice because PruneNonReactiveDependencies already does basic alias tracking (see new fixture).
ghstack-source-id: 364430bbec
Pull Request resolved: https://github.com/facebook/react/pull/29790
There's a category of bug currently where pruned reactive scopes whose outputs are non-reactive can have their code end up inlining into another scope, moving the location of the instruction. Any value that had a scope assigned has to have its order of evaluation preserved, despite the fact that it got pruned, so naively we could just force every pruned scope to have its declarations promoted to named variables.
However, that ends up assigning names to _tons_ of scope declarations that don't really need to be promoted. For example, a scope with just a hook call ends up with:
```
const x = useFoo();
=>
scope {
$t0 = Call read useFoo$ (...);
}
$t1 = StoreLocal 'x' = read $t0;
```
Where t0 doesn't need to be promoted since it's used immediately to assign to another value which is a non-temporary.
So the idea of this PR is that we can track outputs of pruned scopes which are directly referenced from inside a later scope. This fixes one of the two cases of the above pattern. We'll also likely have to consider values from pruned scopes as always reactive, i'll do that in the next PR.
ghstack-source-id: b37fb9a7cb
Pull Request resolved: https://github.com/facebook/react/pull/29789
There are a few places where we want to check whether a value actually got memoized, and we currently have to infer this based on values that "should" have a scope and whether a corresponding scope actually exists. This PR adds a new ReactiveStatement variant to model a reactive scope block that was pruned for some reason, and updates all the passes that prune scopes to instead produce this new variant.
ghstack-source-id: aea6dab469
Pull Request resolved: https://github.com/facebook/react/pull/29781
## Summary
See #29737
## How did you test this change?
As the feature requires module support and the test runner does
currently not support running tests as modules, I could only test it via
playground.
This PR makes it so we always emit a const VariableDeclaration for
compiled functions in gating mode. If the original declaration's parent
was an ExportDefaultDeclaration we'll also append a new
ExportDefaultDeclaration pointing to the new identifier. This allows
code that adds optional properties to the function declaration to still
work in gating mode
ghstack-source-id: 5705479135
Pull Request resolved: https://github.com/facebook/react/pull/29806
When gating is enabled, any function declaration properties that were
previously set (typically `Function.displayName`) would cause a crash
after compilation as the original identifier is no longer present.
ghstack-source-id: beb7e25856
Pull Request resolved: https://github.com/facebook/react/pull/29802
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to always allow (at leat within InferReferenceEffects) for refs to be mutated. This means we completely rely on ValidateNoRefAccessInRender to validate ref access and stop reporting false positives.
ghstack-source-id: 1a30609f5f
Pull Request resolved: https://github.com/facebook/react/pull/29733
Summary
The dispatch function from useReducer is stable, so it is also non-reactive.
the related PR: #29665
the related comment: #29674 (comment)
I am not sure if the location of the new test file is appropriate😅.
How did you test this change?
Added the specific test compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useReducer-returned-dispatcher-is-non-reactive.expect.md.
