diff --git a/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts b/compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts index c7c8d6a161e..a8869fdb8f0 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, 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 fc5ba403817..c9ef6d3e426 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 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/__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/preserve-memo-validation/error.useMemo-unrelated-mutation-in-depslist.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.useMemo-unrelated-mutation-in-depslist.expect.md index 7831aeac15d..fe0bf6c22f6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.useMemo-unrelated-mutation-in-depslist.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.useMemo-unrelated-mutation-in-depslist.expect.md @@ -32,7 +32,7 @@ function useFoo(input1) { ``` 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.