From 4ba9eb0f0ebaf84e9924605fdc54bceb0da13328 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 13 Aug 2024 22:35:41 -0400 Subject: [PATCH 1/2] [compiler] Off-by-default validation against setState directly in passive effect Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render. This validation is off by default, i'm putting this up for discussion and to experiment with it internally. [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 5 + .../src/HIR/Environment.ts | 6 + .../ValidateNoSetStateInPassiveEffects.ts | 151 ++++++++++++++++++ ...setState-in-useEffect-transitive.expect.md | 37 +++++ ...nvalid-setState-in-useEffect-transitive.js | 16 ++ ...or.invalid-setState-in-useEffect.expect.md | 31 ++++ .../error.invalid-setState-in-useEffect.js | 10 ++ ...in-useEffect-listener-transitive.expect.md | 60 +++++++ ...tState-in-useEffect-listener-transitive.js | 18 +++ ...d-setState-in-useEffect-listener.expect.md | 53 ++++++ .../valid-setState-in-useEffect-listener.js | 15 ++ 11 files changed, 402 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 8711b251ea168..c9f9b5fb353a8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -104,6 +104,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'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -244,6 +245,10 @@ function* runWithEnvironment( validateNoSetStateInRender(hir); } + if (env.config.validateNoSetStateInPassiveEffects) { + validateNoSetStateInPassiveEffects(hir); + } + inferReactivePlaces(hir); yield log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 73645a3c953bc..0715507cf17c2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -217,6 +217,12 @@ const EnvironmentConfigSchema = z.object({ */ validateNoSetStateInRender: z.boolean().default(true), + /** + * Validates that setState is not called directly within a passive effect (useEffect). + * Scheduling a setState (with an event listener, subscription, etc) is valid. + */ + validateNoSetStateInPassiveEffects: z.boolean().default(false), + /** * Validates that the dependencies of all effect hooks are memoized. This helps ensure * that Forget does not introduce infinite renders caused by a dependency changing, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts new file mode 100644 index 0000000000000..2c331e35b9d1d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts @@ -0,0 +1,151 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {CompilerError, ErrorSeverity} from '../CompilerError'; +import { + HIRFunction, + IdentifierId, + isSetStateType, + isUseEffectHookType, + Place, +} from '../HIR'; +import {eachInstructionValueOperand} from '../HIR/visitors'; +import {Err, Ok, Result} from '../Utils/Result'; + +/** + * Validates against calling setState in the body of a *passive* effect (useEffect), + * 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(fn: HIRFunction): void { + const setStateFunctions: Map = new Map(); + const errors = new CompilerError(); + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + switch (instr.value.kind) { + case 'LoadLocal': { + if (setStateFunctions.has(instr.value.place.identifier.id)) { + setStateFunctions.set( + instr.lvalue.identifier.id, + instr.value.place, + ); + } + break; + } + case 'StoreLocal': { + if (setStateFunctions.has(instr.value.value.identifier.id)) { + setStateFunctions.set( + instr.value.lvalue.place.identifier.id, + instr.value.value, + ); + setStateFunctions.set( + instr.lvalue.identifier.id, + instr.value.value, + ); + } + break; + } + case 'FunctionExpression': { + if ( + // faster-path to check if the function expression references a setState + [...eachInstructionValueOperand(instr.value)].some( + operand => + isSetStateType(operand.identifier) || + setStateFunctions.has(operand.identifier.id), + ) + ) { + const callee = getSetStateCall( + instr.value.loweredFunc.func, + setStateFunctions, + ); + if (callee !== null) { + setStateFunctions.set(instr.lvalue.identifier.id, callee); + } + } + break; + } + case 'MethodCall': + case 'CallExpression': { + const callee = + instr.value.kind === 'MethodCall' + ? instr.value.receiver + : instr.value.callee; + if (isUseEffectHookType(callee.identifier)) { + const arg = instr.value.args[0]; + if (arg !== undefined && arg.kind === 'Identifier') { + const setState = setStateFunctions.get(arg.identifier.id); + if (setState !== undefined) { + errors.push({ + 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: ErrorSeverity.InvalidReact, + loc: setState.loc, + suggestions: null, + }); + } + } + } + break; + } + } + } + } + + if (errors.hasErrors()) { + throw errors; + } +} + +function getSetStateCall( + fn: HIRFunction, + setStateFunctions: Map, +): Place | null { + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + switch (instr.value.kind) { + case 'LoadLocal': { + if (setStateFunctions.has(instr.value.place.identifier.id)) { + setStateFunctions.set( + instr.lvalue.identifier.id, + instr.value.place, + ); + } + break; + } + case 'StoreLocal': { + if (setStateFunctions.has(instr.value.value.identifier.id)) { + setStateFunctions.set( + instr.value.lvalue.place.identifier.id, + instr.value.value, + ); + setStateFunctions.set( + instr.lvalue.identifier.id, + instr.value.value, + ); + } + break; + } + case 'CallExpression': { + const callee = instr.value.callee; + if ( + isSetStateType(callee.identifier) || + setStateFunctions.has(callee.identifier.id) + ) { + // TODO: once we support multiple locations per error, we should link to the + // original Place in the case that setStateFunction.has(callee) + return callee; + } + } + } + } + } + return null; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.expect.md new file mode 100644 index 0000000000000..ab10aa1bae51f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.expect.md @@ -0,0 +1,37 @@ + +## Input + +```javascript +// @validateNoSetStateInPassiveEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [state, setState] = useState(0); + const f = () => { + setState(s => s + 1); + }; + const g = () => { + f(); + }; + useEffect(() => { + g(); + }); + return state; +} + +``` + + +## Error + +``` + 11 | }; + 12 | useEffect(() => { +> 13 | g(); + | ^ InvalidReact: 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) (13:13) + 14 | }); + 15 | return state; + 16 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.js new file mode 100644 index 0000000000000..099f0f7079bea --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect-transitive.js @@ -0,0 +1,16 @@ +// @validateNoSetStateInPassiveEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [state, setState] = useState(0); + const f = () => { + setState(s => s + 1); + }; + const g = () => { + f(); + }; + useEffect(() => { + g(); + }); + return state; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.expect.md new file mode 100644 index 0000000000000..83be2965b8ada --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.expect.md @@ -0,0 +1,31 @@ + +## Input + +```javascript +// @validateNoSetStateInPassiveEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [state, setState] = useState(0); + useEffect(() => { + setState(s => s + 1); + }); + return state; +} + +``` + + +## Error + +``` + 5 | const [state, setState] = useState(0); + 6 | useEffect(() => { +> 7 | setState(s => s + 1); + | ^^^^^^^^ InvalidReact: 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) (7:7) + 8 | }); + 9 | return state; + 10 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.js new file mode 100644 index 0000000000000..d7d8e4e8858fd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-setState-in-useEffect.js @@ -0,0 +1,10 @@ +// @validateNoSetStateInPassiveEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [state, setState] = useState(0); + useEffect(() => { + setState(s => s + 1); + }); + return state; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.expect.md new file mode 100644 index 0000000000000..dd48adcda7158 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.expect.md @@ -0,0 +1,60 @@ + +## Input + +```javascript +// @validateNoSetStateInPassiveEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [state, setState] = useState(0); + useEffect(() => { + const f = () => { + setState(); + }; + setTimeout(() => f(), 10); + }); + return state; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInPassiveEffects +import { useEffect, useState } from "react"; + +function Component() { + const $ = _c(1); + const [state, setState] = useState(0); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = () => { + const f = () => { + setState(); + }; + + setTimeout(() => f(), 10); + }; + $[0] = t0; + } else { + t0 = $[0]; + } + useEffect(t0); + return state; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) 0 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.js new file mode 100644 index 0000000000000..8b1e159071ef3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener-transitive.js @@ -0,0 +1,18 @@ +// @validateNoSetStateInPassiveEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [state, setState] = useState(0); + useEffect(() => { + const f = () => { + setState(); + }; + setTimeout(() => f(), 10); + }); + return state; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.expect.md new file mode 100644 index 0000000000000..7fdd01fd0a76e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.expect.md @@ -0,0 +1,53 @@ + +## Input + +```javascript +// @validateNoSetStateInPassiveEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [state, setState] = useState(0); + useEffect(() => { + setTimeout(setState, 10); + }); + return state; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInPassiveEffects +import { useEffect, useState } from "react"; + +function Component() { + const $ = _c(1); + const [state, setState] = useState(0); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = () => { + setTimeout(setState, 10); + }; + $[0] = t0; + } else { + t0 = $[0]; + } + useEffect(t0); + return state; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) 0 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.js new file mode 100644 index 0000000000000..ba9720cba9b70 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-setState-in-useEffect-listener.js @@ -0,0 +1,15 @@ +// @validateNoSetStateInPassiveEffects +import {useEffect, useState} from 'react'; + +function Component() { + const [state, setState] = useState(0); + useEffect(() => { + setTimeout(setState, 10); + }); + return state; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; From fa560ce9dc101d56fc0d1a73c9072722d6d638e7 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 13 Aug 2024 22:38:40 -0400 Subject: [PATCH 2/2] Update on "[compiler] Off-by-default validation against setState directly in passive effect" Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render. This validation is off by default, i'm putting this up for discussion and to experiment with it internally. Rationale: https://react.dev/learn/you-might-not-need-an-effect [ghstack-poisoned] --- .../src/Validation/ValidateNoSetStateInPassiveEffects.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts index 2c331e35b9d1d..2c6e7d8ac67cd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts @@ -14,7 +14,6 @@ import { Place, } from '../HIR'; import {eachInstructionValueOperand} from '../HIR/visitors'; -import {Err, Ok, Result} from '../Utils/Result'; /** * Validates against calling setState in the body of a *passive* effect (useEffect), @@ -139,8 +138,10 @@ function getSetStateCall( isSetStateType(callee.identifier) || setStateFunctions.has(callee.identifier.id) ) { - // TODO: once we support multiple locations per error, we should link to the - // original Place in the case that setStateFunction.has(callee) + /* + * TODO: once we support multiple locations per error, we should link to the + * original Place in the case that setStateFunction.has(callee) + */ return callee; } }