Uses a simple algorithm for detecting writes of refs during render, since the ref _read_ during render is now handled as part of ValidateNoImpureValuesInRender.
Passing `--nodebug` will disable debug mode, even if only a single fixture is filtered. This can help when running tests one by one and checking if their output is correct - debug mode causes tests to fail because there is extra/different error output.
Rewrites ValidateNoRefAccessInRender to use a simpler single-pass algorithm
that only validates ref mutations (reads are now handled separately by
ValidateNoImpureValuesInRender).
The new approach:
- Tracks refs and ref values through the function
- Identifies functions that mutate refs (directly or transitively)
- Only errors when ref-mutating functions are called at the top level
- Supports null-guard exception: mutations inside `if (ref.current == null)`
are allowed for the initialization pattern
This reduces ~700 lines of complex fixpoint iteration to ~400 lines of
straightforward forward data-flow analysis.
Updates to guard against *reading* refs during render via the improved validateNoImpureValuesInRender() pass. InferMutationAliasingEffects generates `Impure` effects for ref reads, and then this pass validates that those accesses don't flow into `Render` effects. We now call the impure value validation first so that it takes priority over validateNoRefAccessInRender - the latter still has all the existing logic for now, but we can dramatically simplify it in a follow-up PR so that it only has to validate against ref writes.
note: This implements the idea discussed in https://github.com/reactwg/react/discussions/389#discussioncomment-14252280
Currently we create `Impure` effects for impure functions like `Date.now()` or `Math.random()`, and then throw if the effect is reachable during render. However, impurity is a property of the resulting value: if the value isn't accessed during render then it's okay: maybe you're console-logging the time while debugging (fine), or storing the impure value into a ref and only accessing it in an effect or event handler (totally ok).
This PR updates to validate that impure values are not transitively consumed during render, building on the `Render` effect. The validation currently uses an algorithm very similar to that of InferReactivePlaces - building a set of known impure values, starting with `Impure` effects as the sources and then flowing outward via data flow and mutations. If a transitively impure value reaches a `Render` effect, it's an error. We record both the source of the impure value and where it gets rendered to make it easier to understand and fix errors:
```
Error: Cannot access impure value during render
Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent).
error.invalid-impure-functions-in-render-via-render-helper.ts:10:25
8 | const array = makeArray(now);
9 | const hasDate = identity(array);
> 10 | return <Bar hasDate={hasDate} />;
| ^^^^^^^ Cannot access impure value during render
11 | };
12 | return <Foo renderItem={renderItem} />;
13 | }
error.invalid-impure-functions-in-render-via-render-helper.ts:6:14
4 |
5 | function Component() {
> 6 | const now = Date.now();
| ^^^^^^^^^^ `Date.now` is an impure function.
7 | const renderItem = () => {
8 | const array = makeArray(now);
9 | const hasDate = identity(array);
```
Impure values are allowed to flow into refs, meaning that we now allow `useRef(Date.now())` or `useRef(localFunctionThatReturnsMathDotRandom())` which would have errored previously. The next PR reuses this improved impurity tracking to validate ref access in render as well.
`react-hooks/exhaustive-effect-dependencies` from
`ValidateExhaustiveDeps` reports errors for both missing and extra
effect deps. We already have `react-hooks/exhaustive-deps` that errors
on missing dependencies. In the future we'd like to consolidate this all
to the compiler based error, but for now there's a lot of overlap. Let's
enable testing the extra dep warning by splitting out reporting modes.
This PR
- Creates `on`, `off`, `missing-only`, and `extra-only` reporting modes
for the effect dep validation flag
- Temporarily enables the new rule with `extra-only` in
`eslint-plugin-react-hooks`
- Adds additional null checking to `manualMemoLoc` to fix a bug found
when running against the fixture
Summary:
These validations are not essential for compilation, with this we only
run that logic when outputMode is 'lint'
Test Plan:
Update fixtures and run tests
Putting up https://github.com/facebook/react/pull/35129 again
Reverted in https://github.com/facebook/react/pull/35346 after breaking
main before security patch
This change impacts output formatting in a lot of snaps, so is very
sensitive to additions in main to the fixtures resulting in broken tests
after merging, so we should try merge quickly after rebasing or do a
fast follow to the merge with a snap update.
### What
Fixes source locations for VariableDeclarator in the generated AST.
Fixes a number of the errors in the snapshot I added yesterday in the
source loc validator PR https://github.com/facebook/react/pull/35109
I'm not entirely sure why, but a side effect of the fix has resulted in
a ton of snaps needing updating, with some empty lines no longer present
in the generated output. I broke the change up into 2 separate commits.
The [first
commit](f4e4dc0f44)
has the core change and the update to the missing source locations test
expectation, and the [second
commit](cd4d9e944c)
has the rest of the snapshot updates.
### How
- Add location for variable declarators in ast codegen.
- We don't actually have the location preserved in HIR, since when we
lower the declarations we pass through the location for the
VariableDeclaration. Since VariableDeclarator is just a container for
each of the assignments, the start of the `id` and end of the `init` can
be used to accurately reconstruct it when generating the AST.
- Add source locations for object/array patterns for destructuring
assignment source location support
Was bumped to a canary in https://github.com/facebook/react/pull/34499/
which got never released as stable.
Presumeably to use `Activity` which only made it into Activity in later
Next.js releases. However, `Activity` never ended up being used due to
incompatibilities with Monaco Editor. Downgrading should be safe.
Downgrading to fix
https://github.com/vercel/next.js/security/advisories/GHSA-9qr9-h5gf-34mp.
This will allow new deploys since Vercel is currently blocking new
deploys of unsafe version
---------
Co-authored-by: Eugene Choi <4eugenechoi@gmail.com>
The current `validateNoSetStateInEffects` error has potential false
positives because
we cannot fully statically detect patterns where calling setState in an
effect is
actually valid. This flag `enableVerboseNoSetStateInEffect` adds a
verbose error mode that presents multiple possible
use-cases, allowing an agent to reason about which fix is appropriate
before acting:
1. Non-local derived data - suggests restructuring state ownership
2. Derived event pattern - suggests requesting an event callback from
parent
3. Force update / external sync - suggests using `useSyncExternalStore`
This gives agents the context needed to make informed decisions rather
than
blindly applying a fix that may not be correct for the specific
situation.
Alternative approach to #35282 for validating effect deps in the
compiler that builds on the machinery in ValidateExhaustiveDependencies.
Key changes to that pass:
* Refactor to track the dependencies of array expressions as temporaries
so we can look them up later if they appear as effect deps.
* Instead of not storing temporaries for LoadLocals of locally created
variables, we store the temporary but also propagate the local-ness
through. This allows us to record deps at the top level, necessary for
effect deps. Previously the pass was only ever concerned with tracking
deps within function expressions.
* Refactor the bulk of the dependency-checking logic from
`onFinishMemoize()` into a standalone helper to use it for the new
`onEffect()` helper as well.
* Add a new ErrorCategory for effect deps, use it for errors on
effects
* Put the effect dep validation behind a feature flag
* Adjust the error reason for effect errors
---------
Co-authored-by: Jack Pope <jackpope1@gmail.com>
Fixes an edge case where a function expression would fail to take a
dependency if it referenced a hoisted `const` inferred as a primitive
value. We were incorrectly skipping primitve-typed operands when
determing scopes for merging in InferReactiveScopeVariables.
This was super tricky to debug, for posterity the trick is that Context
variables (StoreContext etc) are modeled just like a mutable object,
where assignment to the variable is equivalent to `object.value = ...`
and reading the variable is equivalent to `object.value` property
access. Comparing to an equivalent version of the repro case replaced
with an object and property read/writes showed that everything was
exactly right, except that InferReactiveScopeVariables wasn't merging
the scopes of the function and the context variable, which led me right
to the problematic line.
Closes#35122
Adds a new `enableUseKeyedState` compiler flag that changes the error
message for unconditional setState calls during render.
When `enableUseKeyedState` is enabled, the error recommends using
`useKeyedState(initialState, key)` to reset state when dependencies
change. When disabled (the default), it links to the React docs for the
manual pattern of storing previous values in state.
Both error messages now include helpful bullet points explaining the two
main alternatives:
1. Use useKeyedState (or manual pattern) to reset state when other
state/props change
2. Compute derived data directly during render without using state
ValidateNoSetStateInEffects already supports transitive setter
functions. This PR marks any synchonous state setter useEffectEvent
function so we can validate that uEE isn't being used only as
misdirection to avoid the validation within an effect body.
The error points to the call of the effect event.
Example:
```js
export default function MyApp() {
const [count, setCount] = useState(0)
const effectEvent = useEffectEvent(() => {
setCount(10)
})
useEffect(() => {
effectEvent()
}, [])
return <div>{count}</div>;
```
```
Found 1 error:
Error: Calling setState synchronously within an effect can trigger cascading renders
Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:
* Update external systems with the latest state from React.
* Subscribe for updates from some external system, calling setState in a callback function when external state changes.
Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect).
5 | })
6 | useEffect(() => {
> 7 | effectEvent()
| ^^^^^^^^^^^ Avoid calling setState() directly within an effect
8 | }, [])
9 | return <div>{count}</div>;
10 | }
```
Fixes some issues i ran into w my recent snap changes:
* Correctly match against patterns that contain subdirectories, eg
`fbt/fbt-call`
* When checking if the input pattern has an extension, only prune known
supported extensions. Our convention of `error.<name>` for fixtures that
error makes the rest of the test name look like an extension to
`path.extname()`.
Tested with lots of different patterns including `error.` examples at
the top level and in nested directories, etc.
First, this adds some more tests and organizes them into an
`exhaustive-deps/` subdirectory.
Second, the diagnostics are overhauled. For each memo block we now
report a single diagnostic which summarizes the issue, plus individual
errors for each missing/extra dependency. Within the extra deps, we
distinguish whether it's truly extra vs whether its just a more (too)
precise version of an inferred dep. For example, if you depend on
`x.y.z` but the inferred dep was `x.y`. Finally, we print the full
inferred deps at the end as a hint (it's also a suggestion, but this
makes it more clear what would be suggested).
Enables `@validateExhaustiveMemoizationDependencies` feature flag by
default, and disables it in select tests that failed due to the change.
Some of our tests intentionally use incorrect memo dependencies in order
to test edge cases.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35201).
* #35213
* __->__ #35201
In ValidateExhaustiveDependencies, I previously changed to allow
extraneous dependencies as long as they were non-reactive. Here we make
that more precise, and distinguish between values that are definitely
referenced in the memo function but optional as dependencies vs values
that are not even referenced in the memo function. The latter now error
as extraneous even if they're non-reactive. This also turned up a case
where constant-folded primitives could show up as false positives of the
latter category, so now we track manual deps which quality for constant
folding and don't error on them.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35204).
* #35213
* #35201
* __->__ #35204
Similar to ValidateHookUsage, we implement this check in the compiler
for safety but (for now) continue to rely on the existing rule for
actually reporting errors to users.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35192).
* #35201
* #35202
* __->__ #35192
The existing exhaustive-deps rule allows omitting non-reactive
dependencies, even if they're not memoized. Conceptually, if a value is
non-reactive then it cannot semantically change. Even if the value is a
new object, that object represents the exact same value and doesn't
necessitate redoing downstream computation. Thus its fine to exclude
nonreactive dependencies, whether they're a stable type or not.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35190).
* #35201
* #35202
* #35192
* __->__ #35190
Since adding this validation we've already changed our inference to use
knowledge from manual memoization to inform when values are frozen and
which values are non-nullable. To align with that, if the user chooses
to use different optionality btw the deps and the memo block/callback,
that's fine. The key is that eg `x?.y` will invalidate whenever `x.y`
would, so from a memoization correctness perspective its fine. It's not
our job to be a type checker: if a value is potentially nullable, it
should likely use a nullable property access in both places but
TypeScript/Flow can check that.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35186).
* #35201
* #35202
* #35192
* #35190
* __->__ #35186
When checking ValidateExhaustiveDeps internally, this seems to be the
most common case that it flags. The current exhaustive-deps rule allows
extraneous deps if they are a set of stable types. So here we reuse our
existing isStableType() util in the compiler to allow this case.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35185).
* #35201
* #35202
* #35192
* #35190
* #35186
* __->__ #35185
With `ValidateExhaustiveMemoDependencies` we can now check exhaustive
dependencies for useMemo and useCallback within the compiler, without
relying on the separate exhaustive-deps rule. Until now we've bailed out
of any component/hook that suppresses this rule, since the suppression
_might_ affect a memoization value. Compiling code with incorrect memo
deps can change behavior so this wasn't safe. The downside was that a
suppression within a useEffect could prevent memoization, even though
non-exhaustive deps for effects do not cause problems for memoization
specifically.
So here, we change to ignore ESLint suppressions if we have both the
compiler's hooks validation and memo deps validations enabled.
Now we just have to test out the new validation and refine before we can
enable this by default.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35184).
* #35201
* #35202
* #35192
* #35190
* #35186
* #35185
* __->__ #35184
Records more information in DropManualMemoization so that we know the
full span of the manual dependencies array (if present). This allows
ValidateExhaustiveDeps to include a suggestion with the correct deps.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34471).
* #34472
* __->__ #34471
The compiler currently drops manual memoization and rewrites it using
its own inference. If the existing manual memo dependencies has missing
or extra dependencies, compilation can change behavior by running the
computation more often (if deps were missing) or less often (if there
were extra deps). We currently address this by relying on the developer
to use the ESLint plugin and have `eslint-disable-next-line
react-hooks/exhaustive-deps` suppressions in their code. If a
suppression exists, we skip compilation.
But not everyone is using the linter! Relying on the linter is also
imprecise since it forces us to bail out on exhaustive-deps checks that
only effect (ahem) effects — and while it isn't good to have incorrect
deps on effects, it isn't a problem for compilation.
So this PR is a rough sketch of validating manual memoization
dependencies in the compiler. Long-term we could use this to also check
effect deps and replace the ExhaustiveDeps lint rule, but for now I'm
focused specifically on manual memoization use-cases. If this works, we
can stop bailing out on ESLint suppressions, since the compiler will
implement all the appropriate checks (we already check rules of hooks).
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34394).
* #34472
* #34471
* __->__ #34394
This deprecates the `noEmit: boolean` flag and adds `outputMode:
'client' | 'client-no-memo' | 'ssr' | 'lint'` as the replacement.
OutputMode defaults to null and takes precedence if specified, otherwise
we use 'client' mode for noEmit=false and 'lint' mode for noEmit=true.
Key points:
* Retrying failed compilation switches from 'client' mode to
'client-no-memo'
* Validations are enabled behind
Environment.proto.shouldEnableValidations, enabled for all modes except
'client-no-memo'. Similar for dropping manual memoization.
* OptimizeSSR is now gated by the outputMode==='ssr', not a feature flag
* Creation of reactive scopes, and related codegen logic, is now gated
by outputMode==='client'
Just a quick poc:
* Inline useState when the initializer is known to not be a function.
The heuristic could be improved but will handle a large number of cases
already.
* Prune effects
* Prune useRef if the ref is unused, by pruning 'ref' props on primitive
components. Then DCE does the rest of the work - with a small change to
allow `useRef()` calls to be dropped since function calls aren't
normally eligible for dropping.
* Prune event handlers, by pruning props whose names start w "on" from
primitive components. Then DCE removes the functions themselves.
Per the fixture, this gets pretty far.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35102).
* #35112
* __->__ #35102
Summary:
I missed this conditional messing things up for undefined useState()
calls. We should be tracking them.
I also missed a test that expect an error was not throwing.
Test Plan:
Update broken test
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35174).
* __->__ #35174
* #35173
Summary:
The operands of a function expression are the elements passed as
context. This means that it doesn't make sense to record mutations for
them.
The relevant mutations will happen in the function body, so we need to
prevent FunctionExpression type instruction from running the logic for
effect mutations.
This was also causing some values to depend on themselves in some cases
triggering an infinite loop. Also added n invariant to prevent this
issue
Test Plan:
Added fixture test
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35173).
* #35174
* __->__ #35173
I've been trying out LLM agents for compiler development, and one thing
i found is that the agent naturally wants to run `yarn snap <pattern>`
to test a specific fixture, and I want to be able to tell it (directly
or in rules/skills) to do this in order to get the debug output from all
the compiler passes. Agents can figure out our current testfilter.txt
file system but that's just tedious. So here we add support for `yarn
snap -p <pattern>`. If you pass in a pattern with an extension, we
target that extension specifically. If you pass in a .expect.md file, we
look at that specific fixture. And if the pattern doesn't have
extensions, we search for `<pattern>{.js,.jsx,.ts,.tsx}`. When patterns
are enabled we automatically log as in debug mode (if there is a single
match), and disable watch mode.
Open to feedback!
Conditionally calling setState in an effect is sometimes necessary, but
should generally follow the pattern of using a "previous vaue" ref to
manually compare and ensure that the setState is idempotent. See fixture
for an example.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35147).
* #35148
* __->__ #35147
Destructing statements that start off as declarations can end up
becoming reassignments if the variable is a scope declaration, so we
have existing logic to handle cases where some parts of a destructure
need to be converted into new locals, with a reassignment to the hoisted
scope variable afterwards. However, there is an edge case where all of
the values are reassigned, in which case we don't need to rewrite and
can just set the instruction kind to reassign.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35144).
* #35148
* #35147
* #35146
* __->__ #35144
Fix for the repro from the previous PR. A `Capture x -> y` effect should
downgrade to `ImmutableCapture` when the source value is maybe-frozen.
MaybeFrozen represents the union of a frozen value with a non-frozen
value.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35140).
* __->__ #35140
* #35139
## Summary
Fixes#35040. The React compiler incorrectly flags ref access within
event handlers as ref access at render time. For example, this code
would fail to compile with error "Cannot access refs during render":
```tsx
const onSubmit = async (data) => {
const file = ref.current?.toFile(); // Incorrectly flagged as error
};
<form onSubmit={handleSubmit(onSubmit)}>
```
This is a false positive because any built-in DOM event handler is
guaranteed not to run at render time. This PR only supports built-in
event handlers because there are no guarantees that user-made event
handlers will not run at render time.
## How did you test this change?
I created 4 test fixtures which validate this change:
* allow-ref-access-in-event-handler-wrapper.tsx - Sync handler test
input
* allow-ref-access-in-event-handler-wrapper.expect.md - Sync handler
expected output
* allow-ref-access-in-async-event-handler-wrapper.tsx - Async handler
test input
* allow-ref-access-in-async-event-handler-wrapper.expect.md - Async
handler expected output
All linters and test suites also pass.
Summary:
This only matters when enableTreatSetIdentifiersAsStateSetters=true
This pattern is still bad. But Right now the validation can only
recommend to move stuff to "calculate in render"
A global setState should not be moved to render, not even conditionally
and you can't remove state without crossing Component boundaries, which
makes this a different kind of fix.
So while we are only suggesting "calculate in render" as a fix we should
disallow the lint from throwing in this case IMO
Test Plan:
Added a fixture
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35135).
* __->__ #35135
* #35134
