A few libraries are known to be incompatible with memoization, whether
manually via `useMemo()` or via React Compiler. This puts us in a tricky
situation. On the one hand, we understand that these libraries were
developed prior to our documenting the [Rules of
React](https://react.dev/reference/rules), and their designs were the
result of trying to deliver a great experience for their users and
balance multiple priorities around DX, performance, etc. At the same
time, using these libraries with memoization — and in particular with
automatic memoization via React Compiler — can break apps by causing the
components using these APIs not to update. Concretely, the APIs have in
common that they return a function which returns different values over
time, but where the function itself does not change. Memoizing the
result on the identity of the function will mean that the value never
changes. Developers reasonable interpret this as "React Compiler broke
my code".
Of course, the best solution is to work with developers of these
libraries to address the root cause, and we're doing that. We've
previously discussed this situation with both of the respective
libraries:
* React Hook Form:
https://github.com/react-hook-form/react-hook-form/issues/11910#issuecomment-2135608761
* TanStack Table:
https://github.com/facebook/react/issues/33057#issuecomment-2840600158
and https://github.com/TanStack/table/issues/5567
In the meantime we need to make sure that React Compiler can work out of
the box as much as possible. This means teaching it about popular
libraries that cannot be memoized. We also can't silently skip
compilation, as this confuses users, so we need these error messages to
be visible to users. To that end, this PR adds:
* A flag to mark functions/hooks as incompatible
* Validation against use of such functions
* A default type provider to provide declarations for two
known-incompatible libraries
Note that Mobx is also incompatible, but the `observable()` function is
called outside of the component itself, so the compiler cannot currently
detect it. We may add validation for such APIs in the future.
Again, we really empathize with the developers of these libraries. We've
tried to word the error message non-judgementally, because we get that
it's hard! We're open to feedback about the error message, please let us
know.
NOTE: this is a merged version of @mofeiZ's original PR along with my
edits per offline discussion. The description is updated to reflect the
latest approach.
The key problem we're trying to solve with this PR is to allow
developers more control over the compiler's various validations. The
idea is to have a number of rules targeting a specific category of
issues, such as enforcing immutability of props/state/etc or disallowing
access to refs during render. We don't want to have to run the compiler
again for every single rule, though, so @mofeiZ added an LRU cache that
caches the full compilation output of N most recent files. The first
rule to run on a given file will cause it to get cached, and then
subsequent rules can pull from the cache, with each rule filtering down
to its specific category of errors.
For the categories, I went through and assigned a category roughly 1:1
to existing validations, and then used my judgement on some places that
felt distinct enough to warrant a separate error. Every error in the
compiler now has to supply both a severity (for legacy reasons) and a
category (for ESLint). Each category corresponds 1:1 to a ESLint rule
definition, so that the set of rules is automatically populated based on
the defined categories.
Categories include a flag for whether they should be in the recommended
set or not.
Note that as with the original version of this PR, only
eslint-plugin-react-compiler is changed. We still have to update the
main lint rule.
## Test Plan
* Created a sample project using ESLint v9 and verified that the plugin
can be configured correctly and detects errors
* Edited `fixtures/eslint-v9` and introduced errors, verified that the w
latest config changes in that fixture it correctly detects the errors
* In the sample project, confirmed that the LRU caching is correctly
caching compiler output, ie compiling files just once.
Co-authored-by: Mofei Zhang <feifei0@meta.com>
The new mutation/aliasing model significantly expands on the idea of
FunctionEffect. The type (and its usage in HIRFunction.effects) was only
necessary for the now-deleted old inference model so we can clean up
this code now.
Allows assigning a ref-accessing function to an object so long as that
object is not subsequently transitively mutated. We should likely
rewrite the ref validation to use the new mutation/aliasing effects,
which would provide a more consistent behavior across instruction types
and require fewer special cases like this.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34026).
* #34027
* __->__ #34026
Fixes#30782
When developers do an `if (ref.current == null)` guard for lazy ref
initialization, the "safe" blocks should extend up to the if's
fallthrough. Previously we only allowed writing to the ref in the if
consequent, but this meant that you couldn't use a ternary, logical, etc
in the if body.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34024).
* #34027
* #34026
* #34025
* __->__ #34024
We infer render helpers as functions whose result is immediately
interpolated into jsx. This is a very conservative approximation, to
help with common cases like `<Foo>{props.renderItem(ref)}</Foo>`. The
idea is similar to hooks that it's ultimately on the developer to catch
ref-in-render validations (and the runtime detects them too), so we can
be a bit more relaxed since there are valid reasons to use this pattern.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34006).
* #34027
* #34026
* #34025
* #34024
* #34005
* __->__ #34006
* #34004
Two related changes:
* ValidateNoRefAccessInRender now allows the mergeRefs pattern, ie a
function that aggregates multiple refs into a new ref. This is the main
case where we have seen false positive no-ref-in-render errors.
* Behind `@enableTreatRefLikeIdentifiersAsRefs`, we infer values passed
as the `ref` prop to some JSX as refs.
The second change is potentially helpful for situations such as
```js
function Component({ref: parentRef}) {
const childRef = useRef(null);
const mergedRef = mergeRefs(parentRef, childRef);
useEffect(() => {
// generally accesses childRef, not mergedRef
}, []);
return <Foo ref={mergedRef} />;
}
```
Ie where you create a merged ref but don't access its `.current`
property. Without inferring `ref` props as refs, we'd fail to allow this
merge refs case.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34004).
* #34027
* #34026
* #34025
* #34024
* #34005
* #34006
* __->__ #34004
Much of the logic in the new validation pass is already implemented in
DropManualMemoization, so let's combine them. I opted to keep the
environment flag so we can more precisely control the rollout.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34001).
* #34022
* #34002
* __->__ #34001
Adds a new validation pass to validate against `useMemo`s that don't
return anything. This usually indicates some kind of "useEffect"-like
code that has side effects that need to be memoized to prevent
overfiring, and is an anti-pattern.
A follow up validation could also look at the return value of `useMemo`s
to see if they are being used.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33990).
* #34022
* #34002
* #34001
* __->__ #33990
* #33989
Uses the new diagnostic infrastructure for this validation, which lets
us provide a more targeted message on the text that we highlight (eg
"This dependency may be mutated later") separately from the overall
error message.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33759).
* #33981
* #33777
* #33767
* #33765
* #33760
* __->__ #33759
* #33758
This PR uses the new diagnostic type for most of the error messages
produced in our explicit validation passes (`Validation/` directory).
One of the validations produced multiple errors as a hack to showing
multiple related locations, which we can now consolidate into a single
diagnostic.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33758).
* #33981
* #33777
* #33767
* #33765
* #33760
* #33759
* __->__ #33758
We currently inline IIFEs by creating a temporary and a labeled block w
the original code. The original return statements turn into an
assignment to the temporary and break out of the label. However, many
cases of IIFEs are due to inlining of manual `useMemo()`, and these
cases often have only a single return statement. Here, the output is
cleaner if we avoid the temporary and label - so that's what we do in
this PR.
Note that the most complex part of the change is actually around
ValidatePreserveExistingMemo - we have some logic to track the IIFE
temporary reassignmetns which needs to be updated to handle the simpler
version of inlining.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33726).
* __->__ #33726
* #33725
Squashed, review-friendly version of the stack from
https://github.com/facebook/react/pull/33488.
This is new version of our mutability and inference model, designed to
replace the core algorithm for determining the sets of instructions
involved in constructing a given value or set of values. The new model
replaces InferReferenceEffects, InferMutableRanges (and all of its
subcomponents), and parts of AnalyzeFunctions. The new model does not
use per-Place effect values, but in order to make this drop-in the end
_result_ of the inference adds these per-Place effects.
I'll write up a larger document on the model, first i'm doing some
housekeeping to rebase the PR.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33494).
* #33571
* #33558
* #33547
* #33543
* #33533
* #33532
* #33530
* #33526
* #33522
* #33518
* #33514
* #33513
* #33512
* #33504
* #33500
* #33497
* #33496
* #33495
* __->__ #33494
* #33572
## Summary
While investigating the root cause of #33208, I noticed a clear typo for
one of the validation files.
## How did you test this change?
Inside `/react/compiler/packages/babel-plugin-react-compiler` I ran the
test script successfully:
<img width="415" alt="Screenshot at May 22 16-43-06"
src="https://github.com/user-attachments/assets/3fe8c5e1-37ce-4a31-b35e-7e323e57cd9d"
/>
This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal.
We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error.
This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default.
ghstack-source-id: 075a731444
Pull Request resolved: https://github.com/facebook/react/pull/33079
This pass didn't previously report the precise difference btw inferred/manual dependencies unless a debug flag was set. But the error message is really good (nice job mofeiz): the only catch is that in theory the inferred dep could be a temporary that can't trivially be reported to the user.
But the messages are really useful for quickly verifying why the compiler couldn't preserve memoization. So here we switch to outputting a detailed message about the discrepancy btw inferred/manual deps so long as the inferred dep root is a named variable. I also slightly adjusted the message to handle the case where there is no diagnostic, which can occur if there were no manual deps but the compiler inferred a dependency.
ghstack-source-id: 534f6f1fec
Pull Request resolved: https://github.com/facebook/react/pull/33095
Updates ~all of our validations to return a Result, and then updates callers to either unwrap() if they should bailout or else just log.
ghstack-source-id: 418b5f5aa2
Pull Request resolved: https://github.com/facebook/react/pull/32688
React uses function identity to determine whether a given JSX expression represents the same type of component and should reconcile (keep state, update props) or replace (teardown state, create a new instance). This PR adds off-by-default validation to check that developers are not dynamically creating components during render.
The check is local and intentionally conservative. We specifically look for the results of call expressions, new expressions, or function expressions that are then used directly (or aliased) as a JSX tag. This allows common sketchy but fine-in-practice cases like passing a reference to a component from a parent as props, but catches very obvious mistakes such as:
```js
function Example() {
const Component = createComponent();
return <Component />;
}
```
We could expand this to catch more cases, but this seems like a reasonable starting point. Note that I tried enabling the validation by default and the only fixtures that error are the new ones added here. I'll also test this internally. What i'm imagining is that we enable this in the linter but not the compiler.
ghstack-source-id: e7408c0a55
Pull Request resolved: https://github.com/facebook/react/pull/32683
Prior to this PR, our HIR represented property access with numeric
literals (e.g. `myVar[0]`) as ComputedLoads. This means that they were
subject to some deopts (most notably, not being easily dedupable /
hoistable as dependencies).
Now, `PropertyLoad`, `PropertyStore`, etc reference numeric and string
literals (although not yet string literals that aren't valid babel
identifiers). The difference between PropertyLoad and ComputedLoad is
fuzzy now (maybe we should rename these).
- PropertyLoad: property keys are string and numeric literals, only when
the string literals are valid babel identifiers
- ComputedLoad: non-valid babel identifier string literals (rare) and
other non-literal expressions
The biggest feature from this PR is that it trivially enables
array-indicing expressions as dependencies. The compiler can also
specify global and imported types for arrays (e.g. return value of
`useState`)
I'm happy to close this if it complicates more than it helps --
alternative options are to entirely rely on instruction reordering-based
approaches like ReactiveGraphIR or make dependency-specific parsing +
hoisting logic more robust.
For now we just reject all calls of impure functions, and the validation
is off by default. Going forward we can make this more precise and only
reject impure functions called during render.
Note that I was intentionally imprecise in the return type of these
functions in order to avoid changing output of existing code. We lie to
the compiler and say that Date.now, performance.now, and Math.random
return unknown mutable objects rather than primitives. Once the
validation is complete and vetted we can switch this to be more precise.
We report a false positive for the combination of a ref-accessing
function placed inside an array which is they type-cast. Here we teach
ref validation about type casts. I also tried other variants like
`return ref as const` but those already worked.
Closes#31864
Summary:
The official guidance for useRef notes an exception to the rule that refs cannot be accessed during render: to avoid recreating the ref's contents, you can test that the ref is uninitialized and then initialize it using an if statement:
```
if (ref.current == null) {
ref.current = SomeExpensiveOperation()
}
```
The compiler didn't recognize this exception, however, leading to code that obeyed all the official guidance for refs being rejected by the compiler. This PR fixes that, by extending the ref validation machinery with an awareness of guard operations that allow lazy initialization. We now understand `== null` and similar operations, when applied to a ref and consumed by an if terminal, as marking the consequent of the if as a block in which the ref can be safely written to. In order to do so we need to create a notion of ref ids, which link different usages of the same ref via both the ref and the ref value.
ghstack-source-id: d2729274f3
Pull Request resolved: https://github.com/facebook/react/pull/31188
Summary:
The fact that phis are identifiers rather than places is unfortunate in a few cases. In some later analyses, we might wish to know whether a phi is reactive, but we don't have an easy way to do that currently.
Most of the changes here is just replacing phi.id with phi.place.identifier and such. Interesting bits are EnterSSA (several functions now take places rather than identifiers, and InferReactivePlaces now needs to mark places as reactive explicitly.
ghstack-source-id: 5f4fb396cd
Pull Request resolved: https://github.com/facebook/react/pull/31171
Summary:
This brings the behavior of ref mutation within hook callbacks into alignment with the behavior of global mutations--that is, we allow all hooks to take callbacks that may mutate a ref. This is potentially unsafe if the hook eagerly calls its callback, but the alternative is excessively limiting (and inconsistent with other enforcement).
This also bans *directly* passing a ref.current value to a hook, which was previously allowed.
ghstack-source-id: e66ce7123e
Pull Request resolved: https://github.com/facebook/react/pull/30917
Summary:
This change expands our handling of refs to build an understanding of nested refs within objects and functions that may return refs. It builds a special-purpose type system within the ref analysis that gives a very lightweight structural type to objects and array expressions (merging the types of all their members), and then propagating those types throughout the analysis (e.g., if `ref` has type `Ref`, then `{ x: ref }` and `[ref]` have type `Structural(value=Ref)` and `{x: ref}.anything` and `[ref][anything]` have type `Ref`).
This allows us to support structures that contain refs, and functions that operate over them, being created and passed around during rendering without at runtime accessing a ref value.
The analysis here uses a fixpoint to allow types to be fully propagated through the system, and we defend against diverging by widening the type of a variable if it could grow infinitely: so, in something like
```
let x = ref;
while (condition) {
x = [x]
}
```
we end up giving `x` the type `Structural(value=Ref)`.
ghstack-source-id: afb0b0cb01
Pull Request resolved: https://github.com/facebook/react/pull/30902
At Meta we have a pattern of using tagged template literals for features that are compiled away:
```
// Relay:
graphql`...graphql text...`
```
In many cases these tags produce a primitive value, and we can get even more optimal output if we can tell the compiler about these types. The new moduleTypeProvider gives us the ability to declare such types, this PR extends the compiler to use this type information for TaggedTemplateExpression values.
ghstack-source-id: 3cd6511b7f
Pull Request resolved: https://github.com/facebook/react/pull/30869
To prevent any difference in behavior, we check that the optionality of the inferred deps exactly matches the optionality of the manual dependencies. This required a fix, I was incorrectly inferring optionality of manual deps (they're only optional if OptionalTerminal.optional is true) - for nested cases of mixed optional/non-optional.
ghstack-source-id: afd49e89cc
Pull Request resolved: https://github.com/facebook/react/pull/30840
If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation.
The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule.
ghstack-source-id: 9f7a99ee5f
Pull Request resolved: https://github.com/facebook/react/pull/30816
Branch terminals didn't have a fallthrough because they correspond to an outer terminal (optional, logical, etc) that has the "real" fallthrough. But understanding how branch terminals correspond to these outer terminals requires knowing the branch fallthrough. For example, `foo?.bar?.baz` creates terminals along the lines of:
```
bb0:
optional fallthrough=bb4
bb1:
optional fallthrough=bb3
bb2:
...
branch ... (fallthrough=bb3)
...
bb3:
...
branch ... (fallthrough=bb4)
...
bb4:
...
```
Without a fallthrough on `branch` terminals, it's unclear that the optional from bb0 has its branch node in bb3. With the fallthroughs, we can see look for a branch with the same fallthrough as the outer optional terminal to match them up.
ghstack-source-id: d48c623289
Pull Request resolved: https://github.com/facebook/react/pull/30814
Adds an `optional: boolean` property to each token in a DependencyPath, currently always set to false. Also updates the equality and printing logic for paths to account for this field.
Subsequent PRs will update our logic to determine which manual dependencies were optional, then we can start inferring optional deps as well.
ghstack-source-id: 66c2da2cfa
Pull Request resolved: https://github.com/facebook/react/pull/30813
Previously the path of a ReactiveScopeDependency was `Array<string>`. We need to track whether each property access is optional or not, so as a first step we change this to `Array<{property: string}>`, making space for an additional property in a subsequent PR.
ghstack-source-id: c5d38d72f6
Pull Request resolved: https://github.com/facebook/react/pull/30812
Summary:
This addresses the issue of the compiler being overly restrictive about refs escaping into object expressions. Rather than erroring whenever a ref flows into an object, we will now treat the object itself as a ref, and apply the same escape rules to it. Whenever we look up a property from a ref value, we now don't know whether that value is itself a ref or a ref value, so we assume it's both.
The same logic applies to ref-accessing functions--if such a function is stored in an object, we'll propagate that property to the object itself and any properties looked up from it.
ghstack-source-id: 5c6fcb895d
Pull Request resolved: https://github.com/facebook/react/pull/30821
This fixture bails out on ValidatePreserveExistingMemo but would ideally memoize since the original memoization is safe. It's trivial to make it pass by commenting out the commented line (`LogEvent.log(() => object)`). I would expect the compiler to infer this as possible mutation of `logData`, since `object` captures a reference to `logData`. But somehow `logData` is getting memoized successfully, but we still infer the callback, `setCurrentIndex`, as having a mutable range that extends to the `setCurrentIndex()` call after the useCallback.
ghstack-source-id: 4f82e34510
Pull Request resolved: https://github.com/facebook/react/pull/30764
Per comments on the new validation pass, this disallows creating JSX (expression/fragment) within a try statement. Developers sometimes use this pattern thinking that they can catch errors during the rendering of the element, without realizing that rendering is lazy. The validation allows us to teach developers about the error boundary pattern.
ghstack-source-id: 0bc722aeae
Pull Request resolved: https://github.com/facebook/react/pull/30725
Summary:
We previously were excessively strict about preventing functions that access refs from being returned--doing so is potentially valid for hooks, because the return value may only be used in an event or effect.
ghstack-source-id: cfa8bb1b54
Pull Request resolved: https://github.com/facebook/react/pull/30724
Summary:
Since we want to make ref-in-render errors enabled by default, we should position those errors at the location of the read. Not only will this be a better experience, but it also aligns the behavior of Forget and Flow.
This PR also cleans up the resulting error messages to not emit implementation details about place values.
ghstack-source-id: 1d11317068
Pull Request resolved: https://github.com/facebook/react/pull/30723
Summary:
Refs, as stable values that the rules of react around mutability do not apply to, currently are treated as having mutable ranges, and through aliasing, this can extend the mutable range for other values and disrupt good memoization for those values. This PR excludes refs and their .current values from having mutable ranges.
Note that this is unsafe if ref access is allowed in render: if a mutable value is assigned to ref.current and then ref.current is mutated later, we won't realize that the original mutable value's range extends.
ghstack-source-id: e8f36ac25e
Pull Request resolved: https://github.com/facebook/react/pull/30713
