From 5271db3984661991956711936ddb9ab99f1841bf Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Mon, 24 Nov 2025 16:03:08 -0500 Subject: [PATCH] [compiler] Add validator for extraneous effect deps --- .../src/Entrypoint/Pipeline.ts | 5 + .../src/HIR/Environment.ts | 6 + .../ValidateExtraneousEffectDependencies.ts | 356 ++++++++++++++++++ .../invalid-extraneous-effect-deps.expect.md | 127 +++++++ .../invalid-extraneous-effect-deps.js | 21 ++ .../valid-extraneous-effect-deps.expect.md | 88 +++++ .../compiler/valid-extraneous-effect-deps.js | 17 + 7 files changed, 620 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExtraneousEffectDependencies.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-extraneous-effect-deps.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-extraneous-effect-deps.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-extraneous-effect-deps.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-extraneous-effect-deps.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 1b76dfb43e1..c8a0e7f4f7b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -106,6 +106,7 @@ import {validateNoDerivedComputationsInEffects_exp} from '../Validation/Validate import {nameAnonymousFunctions} from '../Transform/NameAnonymousFunctions'; import {optimizeForSSR} from '../Optimization/OptimizeForSSR'; import {validateExhaustiveDependencies} from '../Validation/ValidateExhaustiveDependencies'; +import {validateExtraneousEffectDependencies} from '../Validation/ValidateExtraneousEffectDependencies'; import {validateSourceLocations} from '../Validation/ValidateSourceLocations'; export type CompilerPipelineValue = @@ -298,6 +299,10 @@ function runWithEnvironment( } validateNoFreezingKnownMutableFunctions(hir).unwrap(); + + if (env.config.validateExtraneousEffectDependencies) { + env.logErrors(validateExtraneousEffectDependencies(hir)); + } } inferReactivePlaces(hir); diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index fc5ba403817..b5f8889189e 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,12 @@ export const EnvironmentConfigSchema = z.object({ */ validateExhaustiveMemoizationDependencies: z.boolean().default(true), + /** + * Validate that dependencies supplied to effects (useEffect, useLayoutEffect, useInsertionEffect) + * do not include extraneous values that are not referenced within the effect. + */ + validateExtraneousEffectDependencies: 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/ValidateExtraneousEffectDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExtraneousEffectDependencies.ts new file mode 100644 index 00000000000..351874f95c7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExtraneousEffectDependencies.ts @@ -0,0 +1,356 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {CompilerDiagnostic, CompilerError, SourceLocation} from '..'; +import {ErrorCategory} from '../CompilerError'; +import { + areEqualPaths, + DependencyPath, + HIRFunction, + Identifier, + IdentifierId, + Instruction, + isStableType, + isSubPath, + isSubPathIgnoringOptionals, + isUseEffectHookType, + isUseInsertionEffectHookType, + isUseLayoutEffectHookType, + Place, +} from '../HIR'; +import {Result} from '../Utils/Result'; + +type EffectDependency = { + root: + | { + kind: 'NamedLocal'; + value: Place; + } + | {kind: 'Global'; identifierName: string}; + path: DependencyPath; + loc: SourceLocation; +}; + +/** + * Validates that effect dependency arrays do not include extraneous dependencies that + * are not referenced within the effect. + * + * Including extraneous dependencies can cause effects to re-run unnecessarily. + * This leads to people relying on effects as "watchers". + * + * Example of extraneous dependency: + * ```javascript + * useEffect(() => { + * console.log(a); + * }, [a, b]); // `b` is extraneous - it's not used in the effect + * ``` + */ +export function validateExtraneousEffectDependencies( + fn: HIRFunction, +): Result { + const error = new CompilerError(); + const functionExpressions = new Map(); + const arrayExpressions = new Map(); + + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + const {lvalue, value} = instr; + + if (value.kind === 'FunctionExpression') { + functionExpressions.set(lvalue.identifier.id, value.loweredFunc.func); + } + + if (value.kind === 'ArrayExpression') { + arrayExpressions.set(lvalue.identifier.id, instr); + } + + if ( + value.kind === 'CallExpression' && + value.args.length === 2 && + isEffectHook(value.callee.identifier) + ) { + const [callback, depsArg] = value.args; + + if (callback?.kind !== 'Identifier' || depsArg?.kind !== 'Identifier') { + continue; + } + + const callbackFunc = functionExpressions.get(callback.identifier.id); + const depsArray = arrayExpressions.get(depsArg.identifier.id); + + if (callbackFunc == null || depsArray == null) { + continue; + } + + if (depsArray.value.kind !== 'ArrayExpression') { + continue; + } + + const deps: Array = []; + for (const el of depsArray.value.elements) { + if (el.kind === 'Identifier') { + const resolved = resolveLoadLocal(fn, el); + deps.push({ + root: { + kind: 'NamedLocal' as const, + value: resolved.place, + }, + path: resolved.path, + loc: el.loc, + }); + } + } + + validateEffectDependencies( + callbackFunc, + deps, + depsArray.loc, + value.loc, + error, + ); + } + } + } + + return error.asResult(); +} + +/** + * Validates that all dependencies in the effect's dependency array are actually + * referenced within the effect callback. + */ +function validateEffectDependencies( + callbackFunc: HIRFunction, + manualDeps: Array, + depsLoc: SourceLocation, + effectLoc: SourceLocation, + error: CompilerError, +): void { + // Collect all identifiers referenced in the callback + const referencedIdentifiers = collectReferencedIdentifiers(callbackFunc); + + const extraneousDeps: Array = []; + for (const manualDep of manualDeps) { + if (manualDep.root.kind !== 'NamedLocal') { + continue; + } + + const depId = manualDep.root.value.identifier.id; + const isReferenced = referencedIdentifiers.some( + ref => + ref.identifier.id === depId && + (areEqualPaths(ref.path, manualDep.path) || + isSubPath(manualDep.path, ref.path) || + isSubPathIgnoringOptionals(manualDep.path, ref.path)), + ); + + if (!isReferenced) { + extraneousDeps.push(manualDep); + } + } + + if (extraneousDeps.length === 0) { + return; + } + + /** + * Filter out stable types (refs, setState) as they're often included + * as defensive dependencies even if not used + */ + const nonStableExtraneous = extraneousDeps.filter(dep => { + if (dep.root.kind === 'NamedLocal') { + return !isStableType(dep.root.value.identifier); + } + return true; + }); + + if (nonStableExtraneous.length > 0) { + const diagnostic = CompilerDiagnostic.create({ + category: ErrorCategory.EffectDependencies, + reason: 'Found unnecessary effect dependencies', + description: + 'Unnecessary dependencies can cause an effect to re-run more often than necessary, ' + + 'potentially causing performance issues', + }); + + diagnostic.withDetails({ + kind: 'error', + message: `Unnecessary dependencies: ${nonStableExtraneous.map(dep => `\`${printEffectDependency(dep)}\``).join(', ')}. These values are not referenced in the effect`, + loc: depsLoc ?? effectLoc, + }); + + error.pushDiagnostic(diagnostic); + } +} + +type ReferencedIdentifier = { + identifier: Identifier; + path: DependencyPath; +}; + +/** + * Collects all identifiers that are referenced (read) within a function. + * This includes: + * - LoadLocal/LoadContext instructions that reference variables from outer scope + * - Property loads on those variables + */ +function collectReferencedIdentifiers( + fn: HIRFunction, +): Array { + const referenced: Array = []; + const identifierPaths = new Map(); + const sourcePlaces = new Map(); + + // Track which identifiers are local to the function (params, local vars) + const localIdentifiers = new Set(); + for (const param of fn.params) { + const place = param.kind === 'Identifier' ? param : param.place; + localIdentifiers.add(place.identifier.id); + } + + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + const {lvalue, value} = instr; + + // Track local variable declarations + if ( + value.kind === 'DeclareLocal' || + value.kind === 'StoreLocal' || + value.kind === 'DeclareContext' || + value.kind === 'StoreContext' || + value.kind === 'Destructure' + ) { + localIdentifiers.add(lvalue.identifier.id); + } + + // Track LoadLocal and LoadContext - these reference variables from outer scope + if (value.kind === 'LoadLocal' || value.kind === 'LoadContext') { + const sourceId = value.place.identifier.id; + // Only track if it's not a local identifier (i.e., it's from outer scope) + if (!localIdentifiers.has(sourceId)) { + const path = identifierPaths.get(sourceId) ?? []; + const exists = referenced.some( + ref => + ref.identifier.id === sourceId && areEqualPaths(ref.path, path), + ); + if (!exists) { + referenced.push({ + identifier: value.place.identifier, + path, + }); + } + // Track that lvalue now represents this source identifier + identifierPaths.set(lvalue.identifier.id, path); + sourcePlaces.set(lvalue.identifier.id, value.place); + } else { + // It's a local identifier, track its path + const path = identifierPaths.get(sourceId) ?? []; + identifierPaths.set(lvalue.identifier.id, path); + sourcePlaces.set(lvalue.identifier.id, value.place); + } + } + + // Track property loads to build paths and mark outer references + if (value.kind === 'PropertyLoad') { + const objectId = value.object.identifier.id; + const basePath = identifierPaths.get(objectId) ?? []; + const fullPath = [ + ...basePath, + {property: value.property, optional: false}, + ]; + identifierPaths.set(lvalue.identifier.id, fullPath); + + // Resolve to the original source place (outer scope) for the object + const basePlace = sourcePlaces.get(objectId) ?? value.object; + const baseSourceId = basePlace.identifier.id; + if (!localIdentifiers.has(baseSourceId)) { + const exists = referenced.some( + ref => + ref.identifier.id === baseSourceId && + areEqualPaths(ref.path, fullPath), + ); + if (!exists) { + referenced.push({ + identifier: basePlace.identifier, + path: fullPath, + }); + } + } + } + } + } + + return referenced; +} + +/** + * Resolves LoadLocal chains to find the actual source identifier and path. + * For example, if we have: + * $31 = LoadLocal $20 + * $20 = LoadLocal a + * This will resolve $31 back to identifier `a`. + */ +function resolveLoadLocal( + fn: HIRFunction, + place: Place, +): {place: Place; path: DependencyPath} { + const identifierPaths = new Map< + IdentifierId, + {place: Place; path: DependencyPath} + >(); + + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + const {lvalue, value} = instr; + + // Track LoadLocal to resolve chains + if (value.kind === 'LoadLocal' || value.kind === 'LoadContext') { + const source = identifierPaths.get(value.place.identifier.id) ?? { + place: value.place, + path: [], + }; + identifierPaths.set(lvalue.identifier.id, source); + } + + // Track PropertyLoad to build paths + if (value.kind === 'PropertyLoad') { + const base = identifierPaths.get(value.object.identifier.id) ?? { + place: value.object, + path: [], + }; + identifierPaths.set(lvalue.identifier.id, { + place: base.place, + path: [...base.path, {property: value.property, optional: false}], + }); + } + } + } + + return identifierPaths.get(place.identifier.id) ?? {place, path: []}; +} + +function printEffectDependency(dep: EffectDependency): string { + let identifierName: string; + if (dep.root.kind === 'Global') { + identifierName = dep.root.identifierName; + } else { + const name = dep.root.value.identifier.name; + if (name == null || name.kind !== 'named') { + return ''; + } + identifierName = name.value; + } + return `${identifierName}${dep.path.map(p => (p.optional ? '?' : '') + '.' + p.property).join('')}`; +} + +function isEffectHook(identifier: Identifier): boolean { + return ( + isUseEffectHookType(identifier) || + isUseLayoutEffectHookType(identifier) || + isUseInsertionEffectHookType(identifier) + ); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-extraneous-effect-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-extraneous-effect-deps.expect.md new file mode 100644 index 00000000000..41cef2a412f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-extraneous-effect-deps.expect.md @@ -0,0 +1,127 @@ + +## Input + +```javascript +// @loggerTestOnly @validateExtraneousEffectDependencies +import {useEffect, useLayoutEffect} from 'react'; + +function Component({a, b, x, y}) { + // error: `b` is not used in the effect + useEffect(() => { + log(a); + }, [a, b]); + // error: `x` and `y` are not used in the effect + useEffect(() => { + log('hello'); + }, [x, y]); + // error: works with useLayoutEffect too + useLayoutEffect(() => { + log(a); + }, [a, b]); + // error: more precise dep + useEffect(() => { + log(a, b); + }, [a, b.c]); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateExtraneousEffectDependencies +import { useEffect, useLayoutEffect } from "react"; + +function Component(t0) { + const $ = _c(19); + const { a, b, x, y } = t0; + let t1; + if ($[0] !== a) { + t1 = () => { + log(a); + }; + $[0] = a; + $[1] = t1; + } else { + t1 = $[1]; + } + let t2; + if ($[2] !== a || $[3] !== b) { + t2 = [a, b]; + $[2] = a; + $[3] = b; + $[4] = t2; + } else { + t2 = $[4]; + } + useEffect(t1, t2); + let t3; + if ($[5] !== x || $[6] !== y) { + t3 = [x, y]; + $[5] = x; + $[6] = y; + $[7] = t3; + } else { + t3 = $[7]; + } + useEffect(_temp, t3); + let t4; + if ($[8] !== a) { + t4 = () => { + log(a); + }; + $[8] = a; + $[9] = t4; + } else { + t4 = $[9]; + } + let t5; + if ($[10] !== a || $[11] !== b) { + t5 = [a, b]; + $[10] = a; + $[11] = b; + $[12] = t5; + } else { + t5 = $[12]; + } + useLayoutEffect(t4, t5); + let t6; + if ($[13] !== a || $[14] !== b) { + t6 = () => { + log(a, b); + }; + $[13] = a; + $[14] = b; + $[15] = t6; + } else { + t6 = $[15]; + } + let t7; + if ($[16] !== a || $[17] !== b.c) { + t7 = [a, b.c]; + $[16] = a; + $[17] = b.c; + $[18] = t7; + } else { + t7 = $[18]; + } + useEffect(t6, t7); +} +function _temp() { + log("hello"); +} + +``` + +## Logs + +``` +{"kind":"CompileError","detail":{"options":{"category":"EffectDependencies","reason":"Found unnecessary effect dependencies","description":"Unnecessary dependencies can cause an effect to re-run more often than necessary, potentially causing performance issues","details":[{"kind":"error","message":"Unnecessary dependencies: `b`. These values are not referenced in the effect","loc":{"start":{"line":8,"column":5,"index":222},"end":{"line":8,"column":11,"index":228},"filename":"invalid-extraneous-effect-deps.ts"}}]}},"fnLoc":null} +{"kind":"CompileError","detail":{"options":{"category":"EffectDependencies","reason":"Found unnecessary effect dependencies","description":"Unnecessary dependencies can cause an effect to re-run more often than necessary, potentially causing performance issues","details":[{"kind":"error","message":"Unnecessary dependencies: `x`, `y`. These values are not referenced in the effect","loc":{"start":{"line":12,"column":5,"index":325},"end":{"line":12,"column":11,"index":331},"filename":"invalid-extraneous-effect-deps.ts"}}]}},"fnLoc":null} +{"kind":"CompileError","detail":{"options":{"category":"EffectDependencies","reason":"Found unnecessary effect dependencies","description":"Unnecessary dependencies can cause an effect to re-run more often than necessary, potentially causing performance issues","details":[{"kind":"error","message":"Unnecessary dependencies: `b`. These values are not referenced in the effect","loc":{"start":{"line":16,"column":5,"index":420},"end":{"line":16,"column":11,"index":426},"filename":"invalid-extraneous-effect-deps.ts"}}]}},"fnLoc":null} +{"kind":"CompileError","detail":{"options":{"category":"EffectDependencies","reason":"Found unnecessary effect dependencies","description":"Unnecessary dependencies can cause an effect to re-run more often than necessary, potentially causing performance issues","details":[{"kind":"error","message":"Unnecessary dependencies: `b.c`. These values are not referenced in the effect","loc":{"start":{"line":20,"column":5,"index":498},"end":{"line":20,"column":13,"index":506},"filename":"invalid-extraneous-effect-deps.ts"}}]}},"fnLoc":null} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":108},"end":{"line":21,"column":1,"index":510},"filename":"invalid-extraneous-effect-deps.ts"},"fnName":"Component","memoSlots":19,"memoBlocks":7,"memoValues":7,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### 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/invalid-extraneous-effect-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-extraneous-effect-deps.js new file mode 100644 index 00000000000..30aa8da8273 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-extraneous-effect-deps.js @@ -0,0 +1,21 @@ +// @loggerTestOnly @validateExtraneousEffectDependencies +import {useEffect, useLayoutEffect} from 'react'; + +function Component({a, b, x, y}) { + // error: `b` is not used in the effect + useEffect(() => { + log(a); + }, [a, b]); + // error: `x` and `y` are not used in the effect + useEffect(() => { + log('hello'); + }, [x, y]); + // error: works with useLayoutEffect too + useLayoutEffect(() => { + log(a); + }, [a, b]); + // error: more precise dep + useEffect(() => { + log(a, b); + }, [a, b.c]); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-extraneous-effect-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-extraneous-effect-deps.expect.md new file mode 100644 index 00000000000..d7e8c998d86 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-extraneous-effect-deps.expect.md @@ -0,0 +1,88 @@ + +## Input + +```javascript +// @loggerTestOnly @validateExtraneousEffectDependencies +import {useEffect, useLayoutEffect} from 'react'; + +function Component({a, b, x, y}) { + // ok: all dependencies are used + useEffect(() => { + log(a, b); + }, [a, b]); + // ok: all dependencies are used + useEffect(() => { + log(a, b, b.c); + }, [a, b, b.c]); + // ok: no dependencies + useEffect(() => { + log('no deps'); + }, []); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @loggerTestOnly @validateExtraneousEffectDependencies +import { useEffect, useLayoutEffect } from "react"; + +function Component(t0) { + const $ = _c(9); + const { a, b } = t0; + let t1; + let t2; + if ($[0] !== a || $[1] !== b) { + t1 = () => { + log(a, b); + }; + t2 = [a, b]; + $[0] = a; + $[1] = b; + $[2] = t1; + $[3] = t2; + } else { + t1 = $[2]; + t2 = $[3]; + } + useEffect(t1, t2); + let t3; + let t4; + if ($[4] !== a || $[5] !== b) { + t3 = () => { + log(a, b, b.c); + }; + t4 = [a, b, b.c]; + $[4] = a; + $[5] = b; + $[6] = t3; + $[7] = t4; + } else { + t3 = $[6]; + t4 = $[7]; + } + useEffect(t3, t4); + let t5; + if ($[8] === Symbol.for("react.memo_cache_sentinel")) { + t5 = []; + $[8] = t5; + } else { + t5 = $[8]; + } + useEffect(_temp, t5); +} +function _temp() { + log("no deps"); +} + +``` + +## Logs + +``` +{"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":108},"end":{"line":17,"column":1,"index":397},"filename":"valid-extraneous-effect-deps.ts"},"fnName":"Component","memoSlots":9,"memoBlocks":3,"memoValues":5,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### 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/valid-extraneous-effect-deps.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-extraneous-effect-deps.js new file mode 100644 index 00000000000..bd7735d4d25 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-extraneous-effect-deps.js @@ -0,0 +1,17 @@ +// @loggerTestOnly @validateExtraneousEffectDependencies +import {useEffect, useLayoutEffect} from 'react'; + +function Component({a, b, x, y}) { + // ok: all dependencies are used + useEffect(() => { + log(a, b); + }, [a, b]); + // ok: all dependencies are used + useEffect(() => { + log(a, b, b.c); + }, [a, b, b.c]); + // ok: no dependencies + useEffect(() => { + log('no deps'); + }, []); +}