diff --git a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts index c7c8d6a161e..a61967ef4a0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts @@ -601,7 +601,8 @@ function printErrorSummary(category: ErrorCategory, message: string): string { case ErrorCategory.Syntax: case ErrorCategory.UseMemo: case ErrorCategory.VoidUseMemo: - case ErrorCategory.MemoDependencies: { + case ErrorCategory.MemoDependencies: + case ErrorCategory.EffectExhaustiveDependencies: { heading = 'Error'; break; } @@ -683,6 +684,10 @@ export enum ErrorCategory { * Checks for memoized effect deps */ EffectDependencies = 'EffectDependencies', + /** + * Checks for exhaustive and extraneous effect dependencies + */ + EffectExhaustiveDependencies = 'EffectExhaustiveDependencies', /** * Checks for no setState in effect bodies */ @@ -838,6 +843,16 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule { preset: LintRulePreset.Off, }; } + case ErrorCategory.EffectExhaustiveDependencies: { + return { + category, + severity: ErrorSeverity.Error, + name: 'exhaustive-effect-dependencies', + description: + 'Validates that effect dependencies are exhaustive and without extraneous values', + preset: LintRulePreset.Off, + }; + } case ErrorCategory.EffectDerivationsOfState: { return { category, @@ -854,7 +869,9 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule { severity: ErrorSeverity.Error, name: 'set-state-in-effect', description: - 'Validates against calling setState synchronously in an effect, which can lead to re-renders that degrade performance', + 'Validates against calling setState synchronously in an effect. ' + + 'This can indicate non-local derived data, a derived event pattern, or ' + + 'improper external data synchronization.', preset: LintRulePreset.Recommended, }; } 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 1b76dfb43e1..ab5ac590d90 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -304,7 +304,10 @@ function runWithEnvironment( log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); if (env.enableValidations) { - if (env.config.validateExhaustiveMemoizationDependencies) { + if ( + env.config.validateExhaustiveMemoizationDependencies || + env.config.validateExhaustiveEffectDependencies + ) { // NOTE: this relies on reactivity inference running first validateExhaustiveDependencies(hir).unwrap(); } 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 17dd53adf56..05105ae5973 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -223,6 +223,11 @@ export const EnvironmentConfigSchema = z.object({ */ validateExhaustiveMemoizationDependencies: z.boolean().default(true), + /** + * Validate that dependencies supplied to effect hooks are exhaustive. + */ + validateExhaustiveEffectDependencies: z.boolean().default(false), + /** * When this is true, rather than pruning existing manual memoization but ensuring or validating * that the memoized values remain memoized, the compiler will simply not prune existing calls to @@ -695,6 +700,16 @@ export const EnvironmentConfigSchema = z.object({ */ enableAllowSetStateFromRefsInEffects: z.boolean().default(true), + /** + * When enabled, provides verbose error messages for setState calls within effects, + * presenting multiple possible fixes to the user/agent since we cannot statically + * determine which specific use-case applies: + * 1. Non-local derived data - requires restructuring state ownership + * 2. Derived event pattern - detecting when a prop changes + * 3. Force update / external sync - should use useSyncExternalStore + */ + enableVerboseNoSetStateInEffect: z.boolean().default(false), + /** * Enables inference of event handler types for JSX props on built-in DOM elements. * When enabled, functions passed to event handler props (props starting with "on") diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts index 2e6217bb51a..e364c025f38 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts @@ -10,6 +10,7 @@ import { CompilerDiagnostic, CompilerError, CompilerSuggestionOperation, + Effect, SourceLocation, } from '..'; import {CompilerSuggestion, ErrorCategory} from '../CompilerError'; @@ -18,10 +19,12 @@ import { BlockId, DependencyPath, FinishMemoize, + GeneratedSource, HIRFunction, Identifier, IdentifierId, InstructionKind, + isEffectEventFunctionType, isPrimitiveType, isStableType, isSubPath, @@ -40,6 +43,7 @@ import { } from '../HIR/visitors'; import {Result} from '../Utils/Result'; import {retainWhere} from '../Utils/utils'; +import {isEffectHook} from './ValidateMemoizedEffectDependencies'; const DEBUG = false; @@ -85,6 +89,7 @@ const DEBUG = false; export function validateExhaustiveDependencies( fn: HIRFunction, ): Result { + const env = fn.env; const reactive = collectReactiveIdentifiersHIR(fn); const temporaries: Map = new Map(); @@ -126,270 +131,344 @@ export function validateExhaustiveDependencies( loc: value.loc, }, ); - visitCandidateDependency(value.decl, temporaries, dependencies, locals); - const inferred: Array = Array.from(dependencies); - // Sort dependencies by name and path, with shorter/non-optional paths first - inferred.sort((a, b) => { - if (a.kind === 'Global' && b.kind == 'Global') { - return a.binding.name.localeCompare(b.binding.name); - } else if (a.kind == 'Local' && b.kind == 'Local') { - CompilerError.simpleInvariant( - a.identifier.name != null && - a.identifier.name.kind === 'named' && - b.identifier.name != null && - b.identifier.name.kind === 'named', - { - reason: 'Expected dependencies to be named variables', - loc: a.loc, - }, - ); - if (a.identifier.id !== b.identifier.id) { - return a.identifier.name.value.localeCompare(b.identifier.name.value); + if (env.config.validateExhaustiveMemoizationDependencies) { + visitCandidateDependency(value.decl, temporaries, dependencies, locals); + const inferred: Array = Array.from(dependencies); + + const diagnostic = validateDependencies( + inferred, + startMemo.deps ?? [], + reactive, + startMemo.depsLoc, + ErrorCategory.MemoDependencies, + ); + if (diagnostic != null) { + error.pushDiagnostic(diagnostic); + } + } + + dependencies.clear(); + locals.clear(); + startMemo = null; + } + + collectDependencies( + fn, + temporaries, + { + onStartMemoize, + onFinishMemoize, + onEffect: (inferred, manual, manualMemoLoc) => { + if (env.config.validateExhaustiveEffectDependencies === false) { + return; } - if (a.path.length !== b.path.length) { - // if a's path is shorter this returns a negative, sorting a first - return a.path.length - b.path.length; + if (DEBUG) { + console.log(Array.from(inferred, printInferredDependency)); + console.log(Array.from(manual, printInferredDependency)); } - for (let i = 0; i < a.path.length; i++) { - const aProperty = a.path[i]; - const bProperty = b.path[i]; - const aOptional = aProperty.optional ? 0 : 1; - const bOptional = bProperty.optional ? 0 : 1; - if (aOptional !== bOptional) { - // sort non-optionals first - return aOptional - bOptional; - } else if (aProperty.property !== bProperty.property) { - return String(aProperty.property).localeCompare( - String(bProperty.property), - ); + const manualDeps: Array = []; + for (const dep of manual) { + if (dep.kind === 'Local') { + manualDeps.push({ + root: { + kind: 'NamedLocal', + constant: false, + value: { + effect: Effect.Read, + identifier: dep.identifier, + kind: 'Identifier', + loc: dep.loc, + reactive: reactive.has(dep.identifier.id), + }, + }, + path: dep.path, + loc: dep.loc, + }); + } else { + manualDeps.push({ + root: { + kind: 'Global', + identifierName: dep.binding.name, + }, + path: [], + loc: GeneratedSource, + }); } } - return 0; - } else { - const aName = - a.kind === 'Global' ? a.binding.name : a.identifier.name?.value; - const bName = - b.kind === 'Global' ? b.binding.name : b.identifier.name?.value; - if (aName != null && bName != null) { - return aName.localeCompare(bName); - } - return 0; - } - }); - // remove redundant inferred dependencies - retainWhere(inferred, (dep, ix) => { - const match = inferred.findIndex(prevDep => { - return ( - isEqualTemporary(prevDep, dep) || - (prevDep.kind === 'Local' && - dep.kind === 'Local' && - prevDep.identifier.id === dep.identifier.id && - isSubPath(prevDep.path, dep.path)) + const diagnostic = validateDependencies( + Array.from(inferred), + manualDeps, + reactive, + manualMemoLoc, + ErrorCategory.EffectExhaustiveDependencies, ); - }); - // only retain entries that don't have a prior match - return match === -1 || match >= ix; - }); - // Validate that all manual dependencies belong there - if (DEBUG) { - console.log('manual'); - console.log( - (startMemo.deps ?? []) - .map(x => ' ' + printManualMemoDependency(x)) - .join('\n'), - ); - console.log('inferred'); - console.log( - inferred.map(x => ' ' + printInferredDependency(x)).join('\n'), + if (diagnostic != null) { + error.pushDiagnostic(diagnostic); + } + }, + }, + false, // isFunctionExpression + ); + return error.asResult(); +} + +function validateDependencies( + inferred: Array, + manualDependencies: Array, + reactive: Set, + manualMemoLoc: SourceLocation | null, + category: + | ErrorCategory.MemoDependencies + | ErrorCategory.EffectExhaustiveDependencies, +): CompilerDiagnostic | null { + // Sort dependencies by name and path, with shorter/non-optional paths first + inferred.sort((a, b) => { + if (a.kind === 'Global' && b.kind == 'Global') { + return a.binding.name.localeCompare(b.binding.name); + } else if (a.kind == 'Local' && b.kind == 'Local') { + CompilerError.simpleInvariant( + a.identifier.name != null && + a.identifier.name.kind === 'named' && + b.identifier.name != null && + b.identifier.name.kind === 'named', + { + reason: 'Expected dependencies to be named variables', + loc: a.loc, + }, ); - } - const manualDependencies = startMemo.deps ?? []; - const matched: Set = new Set(); - const missing: Array> = []; - const extra: Array = []; - for (const inferredDependency of inferred) { - if (inferredDependency.kind === 'Global') { - for (const manualDependency of manualDependencies) { - if ( - manualDependency.root.kind === 'Global' && - manualDependency.root.identifierName === - inferredDependency.binding.name - ) { - matched.add(manualDependency); - extra.push(manualDependency); - } + if (a.identifier.id !== b.identifier.id) { + return a.identifier.name.value.localeCompare(b.identifier.name.value); + } + if (a.path.length !== b.path.length) { + // if a's path is shorter this returns a negative, sorting a first + return a.path.length - b.path.length; + } + for (let i = 0; i < a.path.length; i++) { + const aProperty = a.path[i]; + const bProperty = b.path[i]; + const aOptional = aProperty.optional ? 0 : 1; + const bOptional = bProperty.optional ? 0 : 1; + if (aOptional !== bOptional) { + // sort non-optionals first + return aOptional - bOptional; + } else if (aProperty.property !== bProperty.property) { + return String(aProperty.property).localeCompare( + String(bProperty.property), + ); } - continue; } - CompilerError.simpleInvariant(inferredDependency.kind === 'Local', { - reason: 'Unexpected function dependency', - loc: value.loc, - }); - let hasMatchingManualDependency = false; + return 0; + } else { + const aName = + a.kind === 'Global' ? a.binding.name : a.identifier.name?.value; + const bName = + b.kind === 'Global' ? b.binding.name : b.identifier.name?.value; + if (aName != null && bName != null) { + return aName.localeCompare(bName); + } + return 0; + } + }); + // remove redundant inferred dependencies + retainWhere(inferred, (dep, ix) => { + const match = inferred.findIndex(prevDep => { + return ( + isEqualTemporary(prevDep, dep) || + (prevDep.kind === 'Local' && + dep.kind === 'Local' && + prevDep.identifier.id === dep.identifier.id && + isSubPath(prevDep.path, dep.path)) + ); + }); + // only retain entries that don't have a prior match + return match === -1 || match >= ix; + }); + // Validate that all manual dependencies belong there + if (DEBUG) { + console.log('manual'); + console.log( + manualDependencies + .map(x => ' ' + printManualMemoDependency(x)) + .join('\n'), + ); + console.log('inferred'); + console.log( + inferred.map(x => ' ' + printInferredDependency(x)).join('\n'), + ); + } + const matched: Set = new Set(); + const missing: Array> = []; + const extra: Array = []; + for (const inferredDependency of inferred) { + if (inferredDependency.kind === 'Global') { for (const manualDependency of manualDependencies) { if ( - manualDependency.root.kind === 'NamedLocal' && - manualDependency.root.value.identifier.id === - inferredDependency.identifier.id && - (areEqualPaths(manualDependency.path, inferredDependency.path) || - isSubPathIgnoringOptionals( - manualDependency.path, - inferredDependency.path, - )) + manualDependency.root.kind === 'Global' && + manualDependency.root.identifierName === + inferredDependency.binding.name ) { - hasMatchingManualDependency = true; matched.add(manualDependency); + extra.push(manualDependency); } } + continue; + } + CompilerError.simpleInvariant(inferredDependency.kind === 'Local', { + reason: 'Unexpected function dependency', + loc: inferredDependency.loc, + }); + /** + * Skip effect event functions as they are not valid dependencies + */ + if (isEffectEventFunctionType(inferredDependency.identifier)) { + continue; + } + let hasMatchingManualDependency = false; + for (const manualDependency of manualDependencies) { if ( - hasMatchingManualDependency || - isOptionalDependency(inferredDependency, reactive) + manualDependency.root.kind === 'NamedLocal' && + manualDependency.root.value.identifier.id === + inferredDependency.identifier.id && + (areEqualPaths(manualDependency.path, inferredDependency.path) || + isSubPathIgnoringOptionals( + manualDependency.path, + inferredDependency.path, + )) ) { - continue; + hasMatchingManualDependency = true; + matched.add(manualDependency); } - missing.push(inferredDependency); + } + if ( + hasMatchingManualDependency || + isOptionalDependency(inferredDependency, reactive) + ) { + continue; } - for (const dep of startMemo.deps ?? []) { - if (matched.has(dep)) { - continue; - } - if (dep.root.kind === 'NamedLocal' && dep.root.constant) { - CompilerError.simpleInvariant( - !dep.root.value.reactive && - isPrimitiveType(dep.root.value.identifier), - { - reason: 'Expected constant-folded dependency to be non-reactive', - loc: dep.root.value.loc, - }, - ); - /* - * Constant primitives can get constant-folded, which means we won't - * see a LoadLocal for the value within the memo function. - */ - continue; - } - extra.push(dep); + missing.push(inferredDependency); + } + + for (const dep of manualDependencies) { + if (matched.has(dep)) { + continue; + } + if (dep.root.kind === 'NamedLocal' && dep.root.constant) { + CompilerError.simpleInvariant( + !dep.root.value.reactive && isPrimitiveType(dep.root.value.identifier), + { + reason: 'Expected constant-folded dependency to be non-reactive', + loc: dep.root.value.loc, + }, + ); + /* + * Constant primitives can get constant-folded, which means we won't + * see a LoadLocal for the value within the memo function. + */ + continue; } + extra.push(dep); + } - if (missing.length !== 0 || extra.length !== 0) { - let suggestion: CompilerSuggestion | null = null; - if (startMemo.depsLoc != null && typeof startMemo.depsLoc !== 'symbol') { - suggestion = { - description: 'Update dependencies', - range: [startMemo.depsLoc.start.index, startMemo.depsLoc.end.index], - op: CompilerSuggestionOperation.Replace, - text: `[${inferred - .filter( - dep => - dep.kind === 'Local' && !isOptionalDependency(dep, reactive), - ) - .map(printInferredDependency) - .join(', ')}]`, - }; + if (missing.length !== 0 || extra.length !== 0) { + let suggestion: CompilerSuggestion | null = null; + if (manualMemoLoc != null && typeof manualMemoLoc !== 'symbol') { + suggestion = { + description: 'Update dependencies', + range: [manualMemoLoc.start.index, manualMemoLoc.end.index], + op: CompilerSuggestionOperation.Replace, + text: `[${inferred + .filter( + dep => + dep.kind === 'Local' && + !isOptionalDependency(dep, reactive) && + !isEffectEventFunctionType(dep.identifier), + ) + .map(printInferredDependency) + .join(', ')}]`, + }; + } + const diagnostic = createDiagnostic(category, missing, extra, suggestion); + for (const dep of missing) { + let reactiveStableValueHint = ''; + if (isStableType(dep.identifier)) { + reactiveStableValueHint = + '. Refs, setState functions, and other "stable" values generally do not need to be added ' + + 'as dependencies, but this variable may change over time to point to different values'; } - const diagnostic = CompilerDiagnostic.create({ - category: ErrorCategory.MemoDependencies, - reason: 'Found missing/extra memoization dependencies', - description: [ - missing.length !== 0 - ? 'Missing dependencies can cause a value to update less often than it should, ' + - 'resulting in stale UI' - : null, - extra.length !== 0 - ? 'Extra dependencies can cause a value to update more often than it should, ' + - 'resulting in performance problems such as excessive renders or effects firing too often' - : null, - ] - .filter(Boolean) - .join('. '), - suggestions: suggestion != null ? [suggestion] : null, + diagnostic.withDetails({ + kind: 'error', + message: `Missing dependency \`${printInferredDependency(dep)}\`${reactiveStableValueHint}`, + loc: dep.loc, }); - for (const dep of missing) { - let reactiveStableValueHint = ''; - if (isStableType(dep.identifier)) { - reactiveStableValueHint = - '. Refs, setState functions, and other "stable" values generally do not need to be added ' + - 'as dependencies, but this variable may change over time to point to different values'; - } + } + for (const dep of extra) { + if (dep.root.kind === 'Global') { diagnostic.withDetails({ kind: 'error', - message: `Missing dependency \`${printInferredDependency(dep)}\`${reactiveStableValueHint}`, - loc: dep.loc, + message: + `Unnecessary dependency \`${printManualMemoDependency(dep)}\`. ` + + 'Values declared outside of a component/hook should not be listed as ' + + 'dependencies as the component will not re-render if they change', + loc: dep.loc ?? manualMemoLoc, }); - } - for (const dep of extra) { - if (dep.root.kind === 'Global') { + } else { + const root = dep.root.value; + const matchingInferred = inferred.find( + ( + inferredDep, + ): inferredDep is Extract => { + return ( + inferredDep.kind === 'Local' && + inferredDep.identifier.id === root.identifier.id && + isSubPathIgnoringOptionals(inferredDep.path, dep.path) + ); + }, + ); + if ( + matchingInferred != null && + isEffectEventFunctionType(matchingInferred.identifier) + ) { diagnostic.withDetails({ kind: 'error', message: - `Unnecessary dependency \`${printManualMemoDependency(dep)}\`. ` + - 'Values declared outside of a component/hook should not be listed as ' + - 'dependencies as the component will not re-render if they change', - loc: dep.loc ?? startMemo.depsLoc ?? value.loc, + `Functions returned from \`useEffectEvent\` must not be included in the dependency array. ` + + `Remove \`${printManualMemoDependency(dep)}\` from the dependencies.`, + loc: dep.loc ?? manualMemoLoc, + }); + } else if ( + matchingInferred != null && + !isOptionalDependency(matchingInferred, reactive) + ) { + diagnostic.withDetails({ + kind: 'error', + message: + `Overly precise dependency \`${printManualMemoDependency(dep)}\`, ` + + `use \`${printInferredDependency(matchingInferred)}\` instead`, + loc: dep.loc ?? manualMemoLoc, }); - error.pushDiagnostic(diagnostic); } else { - const root = dep.root.value; - const matchingInferred = inferred.find( - ( - inferredDep, - ): inferredDep is Extract => { - return ( - inferredDep.kind === 'Local' && - inferredDep.identifier.id === root.identifier.id && - isSubPathIgnoringOptionals(inferredDep.path, dep.path) - ); - }, - ); - if ( - matchingInferred != null && - !isOptionalDependency(matchingInferred, reactive) - ) { - diagnostic.withDetails({ - kind: 'error', - message: - `Overly precise dependency \`${printManualMemoDependency(dep)}\`, ` + - `use \`${printInferredDependency(matchingInferred)}\` instead`, - loc: dep.loc ?? startMemo.depsLoc ?? value.loc, - }); - } else { - /** - * Else this dependency doesn't correspond to anything referenced in the memo function, - * or is an optional dependency so we don't want to suggest adding it - */ - diagnostic.withDetails({ - kind: 'error', - message: `Unnecessary dependency \`${printManualMemoDependency(dep)}\``, - loc: dep.loc ?? startMemo.depsLoc ?? value.loc, - }); - } + /** + * Else this dependency doesn't correspond to anything referenced in the memo function, + * or is an optional dependency so we don't want to suggest adding it + */ + diagnostic.withDetails({ + kind: 'error', + message: `Unnecessary dependency \`${printManualMemoDependency(dep)}\``, + loc: dep.loc ?? manualMemoLoc, + }); } } - if (suggestion != null) { - diagnostic.withDetails({ - kind: 'hint', - message: `Inferred dependencies: \`${suggestion.text}\``, - }); - } - error.pushDiagnostic(diagnostic); } - - dependencies.clear(); - locals.clear(); - startMemo = null; + if (suggestion != null) { + diagnostic.withDetails({ + kind: 'hint', + message: `Inferred dependencies: \`${suggestion.text}\``, + }); + } + return diagnostic; } - - collectDependencies( - fn, - temporaries, - { - onStartMemoize, - onFinishMemoize, - }, - false, // isFunctionExpression - ); - return error.asResult(); + return null; } function addDependency( @@ -397,7 +476,7 @@ function addDependency( dependencies: Set, locals: Set, ): void { - if (dep.kind === 'Function') { + if (dep.kind === 'Aggregate') { for (const x of dep.dependencies) { addDependency(x, dependencies, locals); } @@ -480,9 +559,14 @@ function collectDependencies( dependencies: Set, locals: Set, ) => void; + onEffect: ( + inferred: Set, + manual: Set, + manualMemoLoc: SourceLocation | null, + ) => void; } | null, isFunctionExpression: boolean, -): Extract { +): Extract { const optionals = findOptionalPlaces(fn); if (DEBUG) { console.log(prettyFormat(optionals)); @@ -501,25 +585,25 @@ function collectDependencies( } for (const block of fn.body.blocks.values()) { for (const phi of block.phis) { - let deps: Array | null = null; + const deps: Array = []; for (const operand of phi.operands.values()) { const dep = temporaries.get(operand.identifier.id); if (dep == null) { continue; } - if (deps == null) { - deps = [dep]; + if (dep.kind === 'Aggregate') { + deps.push(...dep.dependencies); } else { deps.push(dep); } } - if (deps == null) { + if (deps.length === 0) { continue; } else if (deps.length === 1) { temporaries.set(phi.place.identifier.id, deps[0]!); } else { temporaries.set(phi.place.identifier.id, { - kind: 'Function', + kind: 'Aggregate', dependencies: new Set(deps), }); } @@ -537,9 +621,6 @@ function collectDependencies( } case 'LoadContext': case 'LoadLocal': { - if (locals.has(value.place.identifier.id)) { - break; - } const temp = temporaries.get(value.place.identifier.id); if (temp != null) { if (temp.kind === 'Local') { @@ -548,6 +629,9 @@ function collectDependencies( } else { temporaries.set(lvalue.identifier.id, temp); } + if (locals.has(value.place.identifier.id)) { + locals.add(lvalue.identifier.id); + } } break; } @@ -683,10 +767,55 @@ function collectDependencies( } break; } + case 'ArrayExpression': { + const arrayDeps: Set = new Set(); + for (const item of value.elements) { + if (item.kind === 'Hole') { + continue; + } + const place = item.kind === 'Identifier' ? item : item.place; + // Visit with alternative deps/locals to record manual dependencies + visitCandidateDependency(place, temporaries, arrayDeps, new Set()); + // Visit normally to propagate inferred dependencies upward + visit(place); + } + temporaries.set(lvalue.identifier.id, { + kind: 'Aggregate', + dependencies: arrayDeps, + loc: value.loc, + }); + break; + } + case 'CallExpression': case 'MethodCall': { + const receiver = + value.kind === 'CallExpression' ? value.callee : value.property; + + const onEffect = callbacks?.onEffect; + if (onEffect != null && isEffectHook(receiver.identifier)) { + const [fn, deps] = value.args; + if (fn?.kind === 'Identifier' && deps?.kind === 'Identifier') { + const fnDeps = temporaries.get(fn.identifier.id); + const manualDeps = temporaries.get(deps.identifier.id); + if ( + fnDeps?.kind === 'Aggregate' && + manualDeps?.kind === 'Aggregate' + ) { + onEffect( + fnDeps.dependencies, + manualDeps.dependencies, + manualDeps.loc ?? null, + ); + } + } + } + // Ignore the method itself for (const operand of eachInstructionValueOperand(value)) { - if (operand.identifier.id === value.property.identifier.id) { + if ( + value.kind === 'MethodCall' && + operand.identifier.id === value.property.identifier.id + ) { continue; } visit(operand); @@ -710,7 +839,7 @@ function collectDependencies( visit(operand); } } - return {kind: 'Function', dependencies}; + return {kind: 'Aggregate', dependencies}; } function printInferredDependency(dep: InferredDependency): string { @@ -748,7 +877,7 @@ function printManualMemoDependency(dep: ManualMemoDependency): string { function isEqualTemporary(a: Temporary, b: Temporary): boolean { switch (a.kind) { - case 'Function': { + case 'Aggregate': { return false; } case 'Global': { @@ -773,7 +902,11 @@ type Temporary = context: boolean; loc: SourceLocation; } - | {kind: 'Function'; dependencies: Set}; + | { + kind: 'Aggregate'; + dependencies: Set; + loc?: SourceLocation; + }; type InferredDependency = Extract; function collectReactiveIdentifiersHIR(fn: HIRFunction): Set { @@ -888,3 +1021,64 @@ function isOptionalDependency( isPrimitiveType(inferredDependency.identifier)) ); } + +function createDiagnostic( + category: + | ErrorCategory.MemoDependencies + | ErrorCategory.EffectExhaustiveDependencies, + missing: Array, + extra: Array, + suggestion: CompilerSuggestion | null, +): CompilerDiagnostic { + let reason: string; + let description: string; + + function joinMissingExtraDetail( + missingString: string, + extraString: string, + joinStr: string, + ): string { + return [ + missing.length !== 0 ? missingString : null, + extra.length !== 0 ? extraString : null, + ] + .filter(Boolean) + .join(joinStr); + } + + switch (category) { + case ErrorCategory.MemoDependencies: { + reason = `Found ${joinMissingExtraDetail('missing', 'extra', '/')} memoization dependencies`; + description = joinMissingExtraDetail( + 'Missing dependencies can cause a value to update less often than it should, resulting in stale UI', + 'Extra dependencies can cause a value to update more often than it should, resulting in performance' + + ' problems such as excessive renders or effects firing too often', + '. ', + ); + break; + } + case ErrorCategory.EffectExhaustiveDependencies: { + reason = `Found ${joinMissingExtraDetail('missing', 'extra', '/')} effect dependencies`; + description = joinMissingExtraDetail( + 'Missing dependencies can cause an effect to fire less often than it should', + 'Extra dependencies can cause an effect to fire more often than it should, resulting' + + ' in performance problems such as excessive renders and side effects', + '. ', + ); + break; + } + default: { + CompilerError.simpleInvariant(false, { + reason: `Unexpected error category: ${category}`, + loc: GeneratedSource, + }); + } + } + + return CompilerDiagnostic.create({ + category, + reason, + description, + suggestions: suggestion != null ? [suggestion] : null, + }); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts index 86070e2973e..91de8f20671 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts @@ -121,26 +121,58 @@ export function validateNoSetStateInEffects( if (arg !== undefined && arg.kind === 'Identifier') { const setState = setStateFunctions.get(arg.identifier.id); if (setState !== undefined) { - errors.pushDiagnostic( - CompilerDiagnostic.create({ - category: ErrorCategory.EffectSetState, - reason: - 'Calling setState synchronously within an effect can trigger cascading renders', - description: - '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:\n' + - '* Update external systems with the latest state from React.\n' + - '* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\n' + - '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)', - suggestions: null, - }).withDetails({ - kind: 'error', - loc: setState.loc, - message: - 'Avoid calling setState() directly within an effect', - }), - ); + const enableVerbose = + env.config.enableVerboseNoSetStateInEffect; + if (enableVerbose) { + errors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.EffectSetState, + reason: + 'Calling setState synchronously within an effect can trigger cascading renders', + description: + 'Effects are intended to synchronize state between React and external systems. ' + + 'Calling setState synchronously causes cascading renders that hurt performance.\n\n' + + 'This pattern may indicate one of several issues:\n\n' + + '**1. Non-local derived data**: If the value being set could be computed from props/state ' + + 'but requires data from a parent component, consider restructuring state ownership so the ' + + 'derivation can happen during render in the component that owns the relevant state.\n\n' + + "**2. Derived event pattern**: If you're detecting when a prop changes (e.g., `isPlaying` " + + 'transitioning from false to true), this often indicates the parent should provide an event ' + + 'callback (like `onPlay`) instead of just the current state. Request access to the original event.\n\n' + + "**3. Force update / external sync**: If you're forcing a re-render to sync with an external " + + 'data source (mutable values outside React), use `useSyncExternalStore` to properly subscribe ' + + 'to external state changes.\n\n' + + 'See: https://react.dev/learn/you-might-not-need-an-effect', + suggestions: null, + }).withDetails({ + kind: 'error', + loc: setState.loc, + message: + 'Avoid calling setState() directly within an effect', + }), + ); + } else { + errors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.EffectSetState, + reason: + 'Calling setState synchronously within an effect can trigger cascading renders', + description: + '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:\n' + + '* Update external systems with the latest state from React.\n' + + '* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\n' + + '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)', + suggestions: null, + }).withDetails({ + kind: 'error', + loc: setState.loc, + message: + 'Avoid calling setState() directly within an effect', + }), + ); + } } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.exhaustive-deps-effect-events.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.exhaustive-deps-effect-events.expect.md new file mode 100644 index 00000000000..e2ab53dd05f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.exhaustive-deps-effect-events.expect.md @@ -0,0 +1,86 @@ + +## Input + +```javascript +// @validateExhaustiveEffectDependencies +import {useEffect, useEffectEvent} from 'react'; + +function Component({x, y, z}) { + const effectEvent = useEffectEvent(() => { + log(x); + }); + + const effectEvent2 = useEffectEvent(z => { + log(y, z); + }); + + // error - do not include effect event in deps + useEffect(() => { + effectEvent(); + }, [effectEvent]); + + // error - do not include effect event in deps + useEffect(() => { + effectEvent2(z); + }, [effectEvent2, z]); + + // error - do not include effect event captured values in deps + useEffect(() => { + effectEvent2(z); + }, [y, z]); +} + +``` + + +## Error + +``` +Found 3 errors: + +Error: Found extra effect dependencies + +Extra dependencies can cause an effect to fire more often than it should, resulting in performance problems such as excessive renders and side effects. + +error.exhaustive-deps-effect-events.ts:16:6 + 14 | useEffect(() => { + 15 | effectEvent(); +> 16 | }, [effectEvent]); + | ^^^^^^^^^^^ Functions returned from `useEffectEvent` must not be included in the dependency array. Remove `effectEvent` from the dependencies. + 17 | + 18 | // error - do not include effect event in deps + 19 | useEffect(() => { + +Inferred dependencies: `[]` + +Error: Found extra effect dependencies + +Extra dependencies can cause an effect to fire more often than it should, resulting in performance problems such as excessive renders and side effects. + +error.exhaustive-deps-effect-events.ts:21:6 + 19 | useEffect(() => { + 20 | effectEvent2(z); +> 21 | }, [effectEvent2, z]); + | ^^^^^^^^^^^^ Functions returned from `useEffectEvent` must not be included in the dependency array. Remove `effectEvent2` from the dependencies. + 22 | + 23 | // error - do not include effect event captured values in deps + 24 | useEffect(() => { + +Inferred dependencies: `[z]` + +Error: Found extra effect dependencies + +Extra dependencies can cause an effect to fire more often than it should, resulting in performance problems such as excessive renders and side effects. + +error.exhaustive-deps-effect-events.ts:26:6 + 24 | useEffect(() => { + 25 | effectEvent2(z); +> 26 | }, [y, z]); + | ^ Unnecessary dependency `y` + 27 | } + 28 | + +Inferred dependencies: `[z]` +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.exhaustive-deps-effect-events.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.exhaustive-deps-effect-events.js new file mode 100644 index 00000000000..f8e9c8d700e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.exhaustive-deps-effect-events.js @@ -0,0 +1,27 @@ +// @validateExhaustiveEffectDependencies +import {useEffect, useEffectEvent} from 'react'; + +function Component({x, y, z}) { + const effectEvent = useEffectEvent(() => { + log(x); + }); + + const effectEvent2 = useEffectEvent(z => { + log(y, z); + }); + + // error - do not include effect event in deps + useEffect(() => { + effectEvent(); + }, [effectEvent]); + + // error - do not include effect event in deps + useEffect(() => { + effectEvent2(z); + }, [effectEvent2, z]); + + // error - do not include effect event captured values in deps + useEffect(() => { + effectEvent2(z); + }, [y, z]); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-dep-on-ref-current-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-dep-on-ref-current-value.expect.md index ab9b4080e44..cba2cb4b4ae 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-dep-on-ref-current-value.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-dep-on-ref-current-value.expect.md @@ -21,7 +21,7 @@ function Component() { ``` Found 1 error: -Error: Found missing/extra memoization dependencies +Error: Found extra memoization dependencies Extra dependencies can cause a value to update more often than it should, resulting in performance problems such as excessive renders or effects firing too often. diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-deps-disallow-unused-stable-types.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-deps-disallow-unused-stable-types.expect.md index 30ab558918d..6c1bddae85f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-deps-disallow-unused-stable-types.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-deps-disallow-unused-stable-types.expect.md @@ -25,7 +25,7 @@ function Component() { ``` Found 1 error: -Error: Found missing/extra memoization dependencies +Error: Found extra memoization dependencies Extra dependencies can cause a value to update more often than it should, resulting in performance problems such as excessive renders or effects firing too often. diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-deps.expect.md index 7f94c039039..2c864f56aff 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-deps.expect.md @@ -51,7 +51,7 @@ function Component({x, y, z}) { ## Error ``` -Found 5 errors: +Found 4 errors: Error: Found missing/extra memoization dependencies @@ -101,49 +101,7 @@ error.invalid-exhaustive-deps.ts:17:6 Inferred dependencies: `[x?.y.z.a?.b]` -Error: Found missing/extra memoization dependencies - -Extra dependencies can cause a value to update more often than it should, resulting in performance problems such as excessive renders or effects firing too often. - -error.invalid-exhaustive-deps.ts:31:6 - 29 | return []; - 30 | // error: unnecessary -> 31 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); - | ^ Unnecessary dependency `x` - 32 | const ref1 = useRef(null); - 33 | const ref2 = useRef(null); - 34 | const ref = z ? ref1 : ref2; - -error.invalid-exhaustive-deps.ts:31:9 - 29 | return []; - 30 | // error: unnecessary -> 31 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); - | ^^^ Unnecessary dependency `y.z` - 32 | const ref1 = useRef(null); - 33 | const ref2 = useRef(null); - 34 | const ref = z ? ref1 : ref2; - -error.invalid-exhaustive-deps.ts:31:14 - 29 | return []; - 30 | // error: unnecessary -> 31 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); - | ^^^^^^^ Unnecessary dependency `z?.y?.a` - 32 | const ref1 = useRef(null); - 33 | const ref2 = useRef(null); - 34 | const ref = z ? ref1 : ref2; - -error.invalid-exhaustive-deps.ts:31:23 - 29 | return []; - 30 | // error: unnecessary -> 31 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]); - | ^^^^^^^^^^^^^ Unnecessary dependency `UNUSED_GLOBAL`. Values declared outside of a component/hook should not be listed as dependencies as the component will not re-render if they change - 32 | const ref1 = useRef(null); - 33 | const ref2 = useRef(null); - 34 | const ref = z ? ref1 : ref2; - -Inferred dependencies: `[]` - -Error: Found missing/extra memoization dependencies +Error: Found extra memoization dependencies Extra dependencies can cause a value to update more often than it should, resulting in performance problems such as excessive renders or effects firing too often. @@ -185,7 +143,7 @@ error.invalid-exhaustive-deps.ts:31:23 Inferred dependencies: `[]` -Error: Found missing/extra memoization dependencies +Error: Found missing memoization dependencies Missing dependencies can cause a value to update less often than it should, resulting in stale UI. diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-effect-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-effect-deps.expect.md new file mode 100644 index 00000000000..78332c7071b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-effect-deps.expect.md @@ -0,0 +1,116 @@ + +## Input + +```javascript +// @validateExhaustiveEffectDependencies +import {useEffect} from 'react'; + +function Component({x, y, z}) { + // error: missing dep - x + useEffect(() => { + log(x); + }, []); + + // error: extra dep - y + useEffect(() => { + log(x); + }, [x, y]); + + // error: missing dep - z; extra dep - y + useEffect(() => { + log(x, z); + }, [x, y]); + + // error: missing dep x + useEffect(() => { + log(x); + }, [x.y]); +} + +``` + + +## Error + +``` +Found 4 errors: + +Error: Found missing effect dependencies + +Missing dependencies can cause an effect to fire less often than it should. + +error.invalid-exhaustive-effect-deps.ts:7:8 + 5 | // error: missing dep - x + 6 | useEffect(() => { +> 7 | log(x); + | ^ Missing dependency `x` + 8 | }, []); + 9 | + 10 | // error: extra dep - y + +Inferred dependencies: `[x]` + +Error: Found extra effect dependencies + +Extra dependencies can cause an effect to fire more often than it should, resulting in performance problems such as excessive renders and side effects. + +error.invalid-exhaustive-effect-deps.ts:13:9 + 11 | useEffect(() => { + 12 | log(x); +> 13 | }, [x, y]); + | ^ Unnecessary dependency `y` + 14 | + 15 | // error: missing dep - z; extra dep - y + 16 | useEffect(() => { + +Inferred dependencies: `[x]` + +Error: Found missing/extra effect dependencies + +Missing dependencies can cause an effect to fire less often than it should. Extra dependencies can cause an effect to fire more often than it should, resulting in performance problems such as excessive renders and side effects. + +error.invalid-exhaustive-effect-deps.ts:17:11 + 15 | // error: missing dep - z; extra dep - y + 16 | useEffect(() => { +> 17 | log(x, z); + | ^ Missing dependency `z` + 18 | }, [x, y]); + 19 | + 20 | // error: missing dep x + +error.invalid-exhaustive-effect-deps.ts:18:9 + 16 | useEffect(() => { + 17 | log(x, z); +> 18 | }, [x, y]); + | ^ Unnecessary dependency `y` + 19 | + 20 | // error: missing dep x + 21 | useEffect(() => { + +Inferred dependencies: `[x, z]` + +Error: Found missing/extra effect dependencies + +Missing dependencies can cause an effect to fire less often than it should. Extra dependencies can cause an effect to fire more often than it should, resulting in performance problems such as excessive renders and side effects. + +error.invalid-exhaustive-effect-deps.ts:22:8 + 20 | // error: missing dep x + 21 | useEffect(() => { +> 22 | log(x); + | ^ Missing dependency `x` + 23 | }, [x.y]); + 24 | } + 25 | + +error.invalid-exhaustive-effect-deps.ts:23:6 + 21 | useEffect(() => { + 22 | log(x); +> 23 | }, [x.y]); + | ^^^ Overly precise dependency `x.y`, use `x` instead + 24 | } + 25 | + +Inferred dependencies: `[x]` +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-effect-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-effect-deps.js new file mode 100644 index 00000000000..b508117acb0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-exhaustive-effect-deps.js @@ -0,0 +1,24 @@ +// @validateExhaustiveEffectDependencies +import {useEffect} from 'react'; + +function Component({x, y, z}) { + // error: missing dep - x + useEffect(() => { + log(x); + }, []); + + // error: extra dep - y + useEffect(() => { + log(x); + }, [x, y]); + + // error: missing dep - z; extra dep - y + useEffect(() => { + log(x, z); + }, [x, y]); + + // error: missing dep x + useEffect(() => { + log(x); + }, [x.y]); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep-inner-function.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep-inner-function.expect.md index 6d03fb42351..2693d3bd05a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep-inner-function.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep-inner-function.expect.md @@ -26,7 +26,7 @@ function useHook() { ``` Found 1 error: -Error: Found missing/extra memoization dependencies +Error: Found missing memoization dependencies Missing dependencies can cause a value to update less often than it should, resulting in stale UI. diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep-unmemoized.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep-unmemoized.expect.md index a6868ef325c..bb991d17dad 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep-unmemoized.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep-unmemoized.expect.md @@ -24,7 +24,7 @@ function useHook() { ``` Found 1 error: -Error: Found missing/extra memoization dependencies +Error: Found missing memoization dependencies Missing dependencies can cause a value to update less often than it should, resulting in stale UI. diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep.expect.md index f3423495097..1bd8e1ba56f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.invalid-missing-nonreactive-dep.expect.md @@ -21,7 +21,7 @@ function useHook() { ``` Found 1 error: -Error: Found missing/extra memoization dependencies +Error: Found missing memoization dependencies Missing dependencies can cause a value to update less often than it should, resulting in stale UI. diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.sketchy-code-exhaustive-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.sketchy-code-exhaustive-deps.expect.md index 0422a07cd87..70be9d35d78 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.sketchy-code-exhaustive-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/error.sketchy-code-exhaustive-deps.expect.md @@ -25,7 +25,7 @@ function Component() { ``` Found 1 error: -Error: Found missing/extra memoization dependencies +Error: Found missing memoization dependencies Missing dependencies can cause a value to update less often than it should, resulting in stale UI. diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.expect.md index 5b6319ed7cf..2c449ddc5e5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.expect.md @@ -2,7 +2,7 @@ ## Input ```javascript -// @validateExhaustiveMemoizationDependencies +// @validateExhaustiveMemoizationDependencies @validateExhaustiveEffectDependencies import { useCallback, useTransition, @@ -11,6 +11,7 @@ import { useActionState, useRef, useReducer, + useEffect, } from 'react'; function useFoo() { @@ -21,6 +22,24 @@ function useFoo() { const [v, dispatch] = useReducer(() => {}, null); const [isPending, dispatchAction] = useActionState(() => {}, null); + useEffect(() => { + dispatch(); + startTransition(() => {}); + addOptimistic(); + setState(null); + dispatchAction(); + ref.current = true; + }, [ + // intentionally adding unnecessary deps on nonreactive stable values + // to check that they're allowed + dispatch, + startTransition, + addOptimistic, + setState, + dispatchAction, + ref, + ]); + return useCallback(() => { dispatch(); startTransition(() => {}); @@ -50,7 +69,7 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies +import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies @validateExhaustiveEffectDependencies import { useCallback, useTransition, @@ -59,10 +78,11 @@ import { useActionState, useRef, useReducer, + useEffect, } from "react"; function useFoo() { - const $ = _c(1); + const $ = _c(3); const [, setState] = useState(); const ref = useRef(null); const [, startTransition] = useTransition(); @@ -70,6 +90,7 @@ function useFoo() { const [, dispatch] = useReducer(_temp, null); const [, dispatchAction] = useActionState(_temp2, null); let t0; + let t1; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t0 = () => { dispatch(); @@ -79,12 +100,38 @@ function useFoo() { dispatchAction(); ref.current = true; }; + t1 = [ + dispatch, + startTransition, + addOptimistic, + setState, + dispatchAction, + ref, + ]; $[0] = t0; + $[1] = t1; } else { t0 = $[0]; + t1 = $[1]; + } + useEffect(t0, t1); + let t2; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t2 = () => { + dispatch(); + startTransition(_temp4); + addOptimistic(); + setState(null); + dispatchAction(); + ref.current = true; + }; + $[2] = t2; + } else { + t2 = $[2]; } - return t0; + return t2; } +function _temp4() {} function _temp3() {} function _temp2() {} function _temp() {} @@ -97,4 +144,5 @@ export const FIXTURE_ENTRYPOINT = { ``` ### Eval output -(kind: ok) "[[ function params=0 ]]" \ No newline at end of file +(kind: ok) "[[ function params=0 ]]" +logs: ['An optimistic state update occurred outside a transition or action. To fix, move the update to an action, or wrap with startTransition.'] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.js index 71000afcc42..721832eac1b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.js @@ -1,4 +1,4 @@ -// @validateExhaustiveMemoizationDependencies +// @validateExhaustiveMemoizationDependencies @validateExhaustiveEffectDependencies import { useCallback, useTransition, @@ -7,6 +7,7 @@ import { useActionState, useRef, useReducer, + useEffect, } from 'react'; function useFoo() { @@ -17,6 +18,24 @@ function useFoo() { const [v, dispatch] = useReducer(() => {}, null); const [isPending, dispatchAction] = useActionState(() => {}, null); + useEffect(() => { + dispatch(); + startTransition(() => {}); + addOptimistic(); + setState(null); + dispatchAction(); + ref.current = true; + }, [ + // intentionally adding unnecessary deps on nonreactive stable values + // to check that they're allowed + dispatch, + startTransition, + addOptimistic, + setState, + dispatchAction, + ref, + ]); + return useCallback(() => { dispatch(); startTransition(() => {}); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-effect-events.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-effect-events.expect.md new file mode 100644 index 00000000000..3b0e696911a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-effect-events.expect.md @@ -0,0 +1,104 @@ + +## Input + +```javascript +// @validateExhaustiveEffectDependencies +import {useEffect, useEffectEvent} from 'react'; + +function Component({x, y, z}) { + const effectEvent = useEffectEvent(() => { + log(x); + }); + + const effectEvent2 = useEffectEvent(z => { + log(y, z); + }); + + // ok - effectEvent not included in deps + useEffect(() => { + effectEvent(); + }, []); + + // ok - effectEvent2 not included in deps, z included + useEffect(() => { + effectEvent2(z); + }, [z]); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveEffectDependencies +import { useEffect, useEffectEvent } from "react"; + +function Component(t0) { + const $ = _c(12); + const { x, y, z } = t0; + let t1; + if ($[0] !== x) { + t1 = () => { + log(x); + }; + $[0] = x; + $[1] = t1; + } else { + t1 = $[1]; + } + const effectEvent = useEffectEvent(t1); + let t2; + if ($[2] !== y) { + t2 = (z_0) => { + log(y, z_0); + }; + $[2] = y; + $[3] = t2; + } else { + t2 = $[3]; + } + const effectEvent2 = useEffectEvent(t2); + let t3; + if ($[4] !== effectEvent) { + t3 = () => { + effectEvent(); + }; + $[4] = effectEvent; + $[5] = t3; + } else { + t3 = $[5]; + } + let t4; + if ($[6] === Symbol.for("react.memo_cache_sentinel")) { + t4 = []; + $[6] = t4; + } else { + t4 = $[6]; + } + useEffect(t3, t4); + let t5; + if ($[7] !== effectEvent2 || $[8] !== z) { + t5 = () => { + effectEvent2(z); + }; + $[7] = effectEvent2; + $[8] = z; + $[9] = t5; + } else { + t5 = $[9]; + } + let t6; + if ($[10] !== z) { + t6 = [z]; + $[10] = z; + $[11] = t6; + } else { + t6 = $[11]; + } + useEffect(t5, t6); +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-effect-events.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-effect-events.js new file mode 100644 index 00000000000..1c8c710d6cb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps/exhaustive-deps-effect-events.js @@ -0,0 +1,22 @@ +// @validateExhaustiveEffectDependencies +import {useEffect, useEffectEvent} from 'react'; + +function Component({x, y, z}) { + const effectEvent = useEffectEvent(() => { + log(x); + }); + + const effectEvent2 = useEffectEvent(z => { + log(y, z); + }); + + // ok - effectEvent not included in deps + useEffect(() => { + effectEvent(); + }, []); + + // ok - effectEvent2 not included in deps, z included + useEffect(() => { + effectEvent2(z); + }, [z]); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-set-state-in-effect-verbose-derived-event.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-set-state-in-effect-verbose-derived-event.expect.md new file mode 100644 index 00000000000..60479b8c230 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-set-state-in-effect-verbose-derived-event.expect.md @@ -0,0 +1,74 @@ + +## Input + +```javascript +// @validateNoSetStateInEffects @enableVerboseNoSetStateInEffect +import {useState, useEffect} from 'react'; + +function VideoPlayer({isPlaying}) { + const [wasPlaying, setWasPlaying] = useState(isPlaying); + useEffect(() => { + if (isPlaying !== wasPlaying) { + setWasPlaying(isPlaying); + console.log('Play state changed!'); + } + }, [isPlaying, wasPlaying]); + return