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.
Summary: Using the change detection code to debug codebases that violate the rules of react is a lot easier when we have a source location corresponding to the value that has changed inappropriately. I didn't see an easy way to track that information in the existing data structures at the point of codegen, so this PR adds locations to identifiers and reactive scopes (the location of a reactive scope is the range of the locations of its included identifiers).
I'm interested if there's a better way to do this that I missed!
ghstack-source-id: aed5f7edda
Pull Request resolved: https://github.com/facebook/react/pull/29658
Summary: This PR expands the analysis from the previous in the stack in order to also capture when a value can incorrectly change within a single render, rather than just changing between two renders. In the case where dependencies have changed and so a new value is being computed, we now compute the value twice and compare the results. This would, for example, catch when we call Math.random() in render.
The generated code is a little convoluted, because we don't want to have to traverse the generated code and substitute variable names with new ones. Instead, we save the initial value to the cache as normal, then run the computation block again and compare the resulting values to the cached ones. Then, to make sure that the cached values are identical to the computed ones, we reassign the cached values into the output variables.
ghstack-source-id: d0f11a4cb2
Pull Request resolved: https://github.com/facebook/react/pull/29657
Summary: jmbrown215 recently had an observation that the arguments to useState/useRef are only used when a component renders for the first time, and never afterwards. We can skip more computation that we previously could, with reactive blocks that previously recomputed values when inputs changed now only ever computing them on the first render.
ghstack-source-id: 5d044ef787
Pull Request resolved: https://github.com/facebook/react/pull/29653
Summary: The essential assumption of the compiler is that if the inputs to a computation have not changed, then the output should not change either--computation that the compiler optimizes is idempotent.
This is, of course, known to be false in practice, because this property rests on requirements (the Rules of React) that are loosely enforced at best. When rolling out the compiler to a codebase that might have rules of react violations, how should developers debug any issues that arise?
This diff attempts one approach to that: when the option is set, rather than simply skipping computation when dependencies haven't changed, we will *still perform the computation*, but will then use a runtime function to compare the original value and the resultant value. The runtime function can be customized, but the idea is that it will perform a structural equality check on the values, and if the values aren't structurally equal, we can report an error, including information about what file and what variable was to blame.
This assists in debugging by narrowing down what specific computation is responsible for a difference in behavior between the uncompiled code and the program after compilation.
ghstack-source-id: 50dad3dacf
Pull Request resolved: https://github.com/facebook/react/pull/29656
Summary: This adds a debugging mode to the compiler that simply adds a `|| true` to the guard on all memoization blocks, which results in the generated code never using memoized values and always recomputing them. This is designed as a validation tool for the compiler's correctness--every program *should* behave exactly the same with this option enabled as it would with it disabled, and so any difference in behavior should be investigated as either a compiler bug or a pipeline issue.
(We add `|| true` rather than dropping the conditional block entirely because we still want to exercise the guard tests, in case the guards themselves are the source of an error, like reading a property from undefined in a guard.)
ghstack-source-id: 955a47ec16
Pull Request resolved: https://github.com/facebook/react/pull/29655
Summary: This adds a compiler option to not drop existing manual memoization and leaving useMemo/useCallback in the generated source. Why do we need this, given that we also have options to validate or ensure that existing memoization is preserved? It's because later diffs on this stack are designed to alter the behavior of the memoization that the compiler emits, in order to detect rules of react violations and debug issues. We don't want to change the behavior of user-level memoization, however, since doing so would be altering the semantics of the user's program in an unacceptable way.
ghstack-source-id: 89dccdec9c
Pull Request resolved: https://github.com/facebook/react/pull/29654
## Summary
Resolves#29622
## How did you test this change?
I verified the implementation using the test.
Note:
This PR was done without waiting for approval in #29622, so feel free to
just close it.
We were missing a check that ObjectMethods are not getters or setters. In our experience this is pretty rare within React components and hooks themselves, so let's start with a todo.
Closes#29586
ghstack-source-id: 03c6cce9a9
Pull Request resolved: https://github.com/facebook/react/pull/29592
Fixes https://x.com/raibima/status/1794395807216738792
The issue is that if you pass a global-modifying function as prop to JSX, we currently report that it's invalid to modify a global during rendering. The problem is that we don't really know when/if the child component will actually call that function prop. It would be against the rules to call the function during render, but it's totally fine to call it during an event handler or from a useEffect.
Since we don't know at the call-site how the child will use the function, we should allow such calls. In the future we could improve this in a few ways:
* For all functions that modify globals, codegen an assertion or warning into the function that fires if it's called "during render". We'd have to precisely define what "during render" is, but this would at least help developers catch this dynamically.
* Use the type system to distinguish "event/effect" and "render" functions to help developers avoid accidentally mutating globals during render.
ghstack-source-id: 4aba4e6d21
Pull Request resolved: https://github.com/facebook/react/pull/29591
user's pipeline
When the user app has a babel.config file that is missing the compiler,
strange things happen as babel does some strange merging of options from
the user's config and in various callsites like in our eslint rule and
healthcheck script. To minimize odd behavior, we default to not reading
the user's babel.config
Fixes#29135
ghstack-source-id: d6fdc43c5c
Pull Request resolved: https://github.com/facebook/react/pull/29211
After this is merged, I'll add it to .git-blame-ignore-revs. I can't do
it now as the hash will change after ghstack lands this stack.
ghstack-source-id: 054ca869b7
Pull Request resolved: https://github.com/facebook/react/pull/29214
When I added new builtin types for jsx and functions, i forget to add a shape definition. This meant that attempting to accesss a property or method on these types would cause an internal error with an unresolved shape. That wasn't obvious because we rarely call methods on these types.
I confirmed that the new fixtures here fail without the fix.
ghstack-source-id: aa8f8d75a3
Pull Request resolved: https://github.com/facebook/react/pull/29624
We currently don't report an error if the code attempts to reassign a const. Our thinking has been that we're not trying to catch all possible mistakes you could make in JavaScript — that's what ESLint, TypeScript, and Flow are for — and that we want to focus on React errors. However, accidentally reassigning a const is easy to catch and doesn't get in the way of other analysis so let's implement it.
Note that React Compiler's ESLint plugin won't report these errors by default, but they will show up in playground.
Fixes#29598
ghstack-source-id: a0af8b9a48
Pull Request resolved: https://github.com/facebook/react/pull/29619
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc.
I tried to write a proper fixture test to test that we react to changes to a custom setState setter function, but I think there may be an issue with snap and how it handles re-renders from effects. I think the tests are good here but open to feedback if we want to go down the rabbit hole of figuring out a proper snap test for this.
ghstack-source-id: 5e9a8f6e0d
Pull Request resolved: https://github.com/facebook/react/pull/29190
No-op refactor to make Environment#getGlobalDeclaration() take a NonLocalBinding instead of just a name. The idea is that in subsequent PRs we can use information about the binding to resolve a type more accurately. For example, we can resolve `Array` differently if its an import or local and not the global Array. Similar for resolving local `useState` differently than the one from React.
ghstack-source-id: c8063e6fb8
Pull Request resolved: https://github.com/facebook/react/pull/29189
We currently use `LoadGlobal` and `StoreGlobal` to represent any read (or write) of a variable defined outside the component or hook that is being compiled. This is mostly fine, but for a lot of things we want to do going forward (resolving types across modules, for example) it helps to understand the actual source of a variable.
This PR is an incremental step in that direction. We continue to use LoadGlobal/StoreGlobal, but LoadGlobal now has a `binding:NonLocalBinding` instead of just the name of the global. The NonLocalBinding type tells us whether it was an import (and which kind, the source module name etc), a module-local binding, or a true global. By keeping the LoadGlobal/StoreGlobal instructions, most code that deals with "anything not declared locally" doesn't have to care about the difference. However, code that _does_ want to know the source of the value can figure it out.
ghstack-source-id: e701d4ebc0
Pull Request resolved: https://github.com/facebook/react/pull/29188
Repro of a case where we should ideally merge consecutive scopes, but where intermediate temporaries prevent the scopes from merging.
We'd need to reorder instructions in order to merge these.
ghstack-source-id: 4f05672604
Pull Request resolved: https://github.com/facebook/react/pull/29197
In MergeReactiveScopesThatInvalidateTogether when deciding which scopes were eligible for mergin at all, we looked specifically at the instructions whose lvalue produces the declaration. So if a scope declaration was `t0`, we'd love for the instruction where `t0` was the lvalue and look at the instruction type to decide if it is eligible for merging.
Here, we use the inferred type instead (now that the inferred types support the same set of types of instructions we looked at before). This allows us to find more cases where scopes can be merged.
ghstack-source-id: 0e3e05f24e
Pull Request resolved: https://github.com/facebook/react/pull/29157
Improves merging of consecutive scopes so that we now merge two scopes if the dependencies of the second scope are a subset of the previous scope's output *and* that dependency has a type that will always produce a new value (array, object, jsx, function) if it is re-evaluated.
To make this easier, we extend the set of builtin types to include ones for function expressions and JSX and to infer these types in InferTypes. This allows using the already inferred types in MergeReactiveScopesThatInvalidateTogether.
ghstack-source-id: e9119fc4e0
Pull Request resolved: https://github.com/facebook/react/pull/29156
