Skip to content

[compiler] Enable additional lints by default #33752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 24, 2025
Merged

[compiler] Enable additional lints by default #33752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fbe88a
[compiler] Validate against setState in all effect types
josephsavona Jul 24, 2025
1517c63
[compiler] Enable additional lints by default
josephsavona Jul 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLoca
import {outlineFunctions} from '../Optimization/OutlineFunctions';
import {propagatePhiTypes} from '../TypeInference/PropagatePhiTypes';
import {lowerContextAccess} from '../Optimization/LowerContextAccess';
import {validateNoSetStateInPassiveEffects} from '../Validation/ValidateNoSetStateInPassiveEffects';
import {validateNoSetStateInEffects} from '../Validation/ValidateNoSetStateInEffects';
import {validateNoJSXInTryStatement} from '../Validation/ValidateNoJSXInTryStatement';
import {propagateScopeDependenciesHIR} from '../HIR/PropagateScopeDependenciesHIR';
import {outlineJSX} from '../Optimization/OutlineJsx';
Expand Down Expand Up @@ -292,8 +292,8 @@ function runWithEnvironment(
validateNoSetStateInRender(hir).unwrap();
}

if (env.config.validateNoSetStateInPassiveEffects) {
env.logErrors(validateNoSetStateInPassiveEffects(hir));
if (env.config.validateNoSetStateInEffects) {
env.logErrors(validateNoSetStateInEffects(hir));
}

if (env.config.validateNoJSXInTryStatements) {
Expand Down
4 changes: 2 additions & 2 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,10 +318,10 @@ export const EnvironmentConfigSchema = z.object({
validateNoSetStateInRender: z.boolean().default(true),

/**
* Validates that setState is not called directly within a passive effect (useEffect).
* Validates that setState is not called synchronously within an effect (useEffect and friends).
* Scheduling a setState (with an event listener, subscription, etc) is valid.
*/
validateNoSetStateInPassiveEffects: z.boolean().default(false),
validateNoSetStateInEffects: z.boolean().default(false),

/**
* Validates against creating JSX within a try block and recommends using an error boundary
Expand Down
12 changes: 9 additions & 3 deletions ...ion/ValidateNoSetStateInPassiveEffects.ts → ...Validation/ValidateNoSetStateInEffects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,22 @@ import {
IdentifierId,
isSetStateType,
isUseEffectHookType,
isUseInsertionEffectHookType,
isUseLayoutEffectHookType,
Place,
} from '../HIR';
import {eachInstructionValueOperand} from '../HIR/visitors';
import {Result} from '../Utils/Result';

/**
* Validates against calling setState in the body of a *passive* effect (useEffect),
* Validates against calling setState in the body of an effect (useEffect and friends),
* while allowing calling setState in callbacks scheduled by the effect.
*
* Calling setState during execution of a useEffect triggers a re-render, which is
* often bad for performance and frequently has more efficient and straightforward
* alternatives. See https://react.dev/learn/you-might-not-need-an-effect for examples.
*/
export function validateNoSetStateInPassiveEffects(
export function validateNoSetStateInEffects(
fn: HIRFunction,
): Result<void, CompilerError> {
const setStateFunctions: Map<IdentifierId, Place> = new Map();
Expand Down Expand Up @@ -79,7 +81,11 @@ export function validateNoSetStateInPassiveEffects(
instr.value.kind === 'MethodCall'
? instr.value.receiver
: instr.value.callee;
if (isUseEffectHookType(callee.identifier)) {
if (
isUseEffectHookType(callee.identifier) ||
isUseLayoutEffectHookType(callee.identifier) ||
isUseInsertionEffectHookType(callee.identifier)
) {
const arg = instr.value.args[0];
if (arg !== undefined && arg.kind === 'Identifier') {
const setState = setStateFunctions.get(arg.identifier.id);
Expand Down
8 changes: 4 additions & 4 deletions .../__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
## Input

```javascript
// @loggerTestOnly @validateNoSetStateInPassiveEffects
// @loggerTestOnly @validateNoSetStateInEffects
import {useEffect, useState} from 'react';

function Component() {
Expand All @@ -24,7 +24,7 @@ function Component() {
## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateNoSetStateInPassiveEffects
import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateNoSetStateInEffects
import { useEffect, useState } from "react";

function Component() {
Expand Down Expand Up @@ -65,8 +65,8 @@ function _temp(s) {
## Logs

```
{"kind":"CompileError","detail":{"options":{"reason":"Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","description":null,"severity":"InvalidReact","suggestions":null,"loc":{"start":{"line":13,"column":4,"index":272},"end":{"line":13,"column":5,"index":273},"filename":"invalid-setState-in-useEffect-transitive.ts","identifierName":"g"}}},"fnLoc":null}
{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":99},"end":{"line":16,"column":1,"index":300},"filename":"invalid-setState-in-useEffect-transitive.ts"},"fnName":"Component","memoSlots":2,"memoBlocks":2,"memoValues":2,"prunedMemoBlocks":0,"prunedMemoValues":0}
{"kind":"CompileError","detail":{"options":{"reason":"Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","description":null,"severity":"InvalidReact","suggestions":null,"loc":{"start":{"line":13,"column":4,"index":265},"end":{"line":13,"column":5,"index":266},"filename":"invalid-setState-in-useEffect-transitive.ts","identifierName":"g"}}},"fnLoc":null}
{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":92},"end":{"line":16,"column":1,"index":293},"filename":"invalid-setState-in-useEffect-transitive.ts"},"fnName":"Component","memoSlots":2,"memoBlocks":2,"memoValues":2,"prunedMemoBlocks":0,"prunedMemoValues":0}
```

### Eval output
Expand Down
2 changes: 1 addition & 1 deletion ...eact-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @loggerTestOnly @validateNoSetStateInPassiveEffects
// @loggerTestOnly @validateNoSetStateInEffects
import {useEffect, useState} from 'react';

function Component() {
Expand Down
8 changes: 4 additions & 4 deletions ...ompiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
## Input

```javascript
// @loggerTestOnly @validateNoSetStateInPassiveEffects
// @loggerTestOnly @validateNoSetStateInEffects
import {useEffect, useState} from 'react';

function Component() {
Expand All @@ -18,7 +18,7 @@ function Component() {
## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateNoSetStateInPassiveEffects
import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateNoSetStateInEffects
import { useEffect, useState } from "react";

function Component() {
Expand All @@ -45,8 +45,8 @@ function _temp(s) {
## Logs

```
{"kind":"CompileError","detail":{"options":{"reason":"Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","description":null,"severity":"InvalidReact","suggestions":null,"loc":{"start":{"line":7,"column":4,"index":187},"end":{"line":7,"column":12,"index":195},"filename":"invalid-setState-in-useEffect.ts","identifierName":"setState"}}},"fnLoc":null}
{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":99},"end":{"line":10,"column":1,"index":232},"filename":"invalid-setState-in-useEffect.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0}
{"kind":"CompileError","detail":{"options":{"reason":"Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","description":null,"severity":"InvalidReact","suggestions":null,"loc":{"start":{"line":7,"column":4,"index":180},"end":{"line":7,"column":12,"index":188},"filename":"invalid-setState-in-useEffect.ts","identifierName":"setState"}}},"fnLoc":null}
{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":92},"end":{"line":10,"column":1,"index":225},"filename":"invalid-setState-in-useEffect.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0}
```

### Eval output
Expand Down
2 changes: 1 addition & 1 deletion ...el-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @loggerTestOnly @validateNoSetStateInPassiveEffects
// @loggerTestOnly @validateNoSetStateInEffects
import {useEffect, useState} from 'react';

function Component() {
Expand Down
4 changes: 2 additions & 2 deletions ...s__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
## Input

```javascript
// @validateNoSetStateInPassiveEffects
// @validateNoSetStateInEffects
import {useEffect, useState} from 'react';

function Component() {
Expand All @@ -26,7 +26,7 @@ export const FIXTURE_ENTRYPOINT = {
## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInPassiveEffects
import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInEffects
import { useEffect, useState } from "react";

function Component() {
Expand Down
2 changes: 1 addition & 1 deletion ...mpiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @validateNoSetStateInPassiveEffects
// @validateNoSetStateInEffects
import {useEffect, useState} from 'react';

function Component() {
Expand Down
4 changes: 2 additions & 2 deletions .../src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
## Input

```javascript
// @validateNoSetStateInPassiveEffects
// @validateNoSetStateInEffects
import {useEffect, useState} from 'react';

function Component() {
Expand All @@ -23,7 +23,7 @@ export const FIXTURE_ENTRYPOINT = {
## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInPassiveEffects
import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInEffects
import { useEffect, useState } from "react";

function Component() {
Expand Down
2 changes: 1 addition & 1 deletion ...in-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @validateNoSetStateInPassiveEffects
// @validateNoSetStateInEffects
import {useEffect, useState} from 'react';

function Component() {
Expand Down
6 changes: 3 additions & 3 deletions compiler/packages/babel-plugin-react-compiler/src/__tests__/parseConfigPragma-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ describe('parseConfigPragmaForTests()', () => {
// Validate defaults first to make sure that the parser is getting the value from the pragma,
// and not just missing it and getting the default value
expect(defaultConfig.enableUseTypeAnnotations).toBe(false);
expect(defaultConfig.validateNoSetStateInPassiveEffects).toBe(false);
expect(defaultConfig.validateNoSetStateInEffects).toBe(false);
expect(defaultConfig.validateNoSetStateInRender).toBe(true);

const config = parseConfigPragmaForTests(
'@enableUseTypeAnnotations @validateNoSetStateInPassiveEffects:true @validateNoSetStateInRender:false',
'@enableUseTypeAnnotations @validateNoSetStateInEffects:true @validateNoSetStateInRender:false',
{compilationMode: defaultOptions.compilationMode},
);
expect(config).toEqual({
Expand All @@ -28,7 +28,7 @@ describe('parseConfigPragmaForTests()', () => {
environment: {
...defaultOptions.environment,
enableUseTypeAnnotations: true,
validateNoSetStateInPassiveEffects: true,
validateNoSetStateInEffects: true,
validateNoSetStateInRender: false,
enableResetCacheOnSourceFileChanges: false,
},
Expand Down
6 changes: 6 additions & 0 deletions compiler/packages/eslint-plugin-react-compiler/src/rules/ReactCompilerRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ const COMPILER_OPTIONS: Partial<PluginOptions> = {
flowSuppressions: false,
environment: validateEnvironmentConfig({
validateRefAccessDuringRender: false,
validateNoSetStateInRender: true,
validateNoSetStateInEffects: true,
validateNoJSXInTryStatements: true,
Copy link
Contributor

@mofeiZ mofeiZ Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, even for potentially valid code like the following, we can just direct devs to move the JSX creation out of the try statement

function Component();
  try {
    const value = tryGetData();
    return <Foo value={value} />;
  } catch {
    return <Fallback />;
  }
}

validateNoImpureFunctionsInRender: true,
validateStaticComponents: true,
validateNoFreezingKnownMutableFunctions: true,
}),
};

Expand Down
6 changes: 6 additions & 0 deletions packages/eslint-plugin-react-hooks/src/rules/ReactCompiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ const COMPILER_OPTIONS: Partial<PluginOptions> = {
flowSuppressions: false,
environment: validateEnvironmentConfig({
validateRefAccessDuringRender: false,
validateNoSetStateInRender: true,
validateNoSetStateInEffects: true,
validateNoJSXInTryStatements: true,
validateNoImpureFunctionsInRender: true,
validateStaticComponents: true,
validateNoFreezingKnownMutableFunctions: true,
}),
};

Expand Down
Loading