diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rule-registry.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rule-registry.ts index 3c304487e..a43f47b66 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rule-registry.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rule-registry.ts @@ -189,6 +189,7 @@ import { noSecretsInClientCode } from "./rules/security/no-secrets-in-client-cod import { noSetState } from "./rules/react-builtins/no-set-state.js"; import { noSetStateInRender } from "./rules/state-and-effects/no-set-state-in-render.js"; import { noSideTabBorder } from "./rules/design/no-side-tab-border.js"; +import { noStaleClosure } from "./rules/state-and-effects/no-stale-closure.js"; import { noStaticElementInteractions } from "./rules/a11y/no-static-element-interactions.js"; import { noStringRefs } from "./rules/react-builtins/no-string-refs.js"; import { noThisInSfc } from "./rules/react-builtins/no-this-in-sfc.js"; @@ -2830,6 +2831,20 @@ export const reactDoctorRules = [ category: "Architecture", }, }, + { + key: "react-doctor/no-stale-closure", + id: "no-stale-closure", + source: "react-doctor", + originallyExternal: false, + framework: "global", + category: "State & Effects", + severity: "warn", + rule: { + ...noStaleClosure, + framework: "global", + category: "State & Effects", + }, + }, { key: "react-doctor/no-static-element-interactions", id: "no-static-element-interactions", diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-stale-closure.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-stale-closure.test.ts new file mode 100644 index 000000000..dc62e1dc2 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-stale-closure.test.ts @@ -0,0 +1,489 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runRule } from "../../../test-utils/run-rule.js"; +import { noStaleClosure } from "./no-stale-closure.js"; + +describe("no-stale-closure", () => { + // ── useCallback with empty deps ────────────────────────────────── + + describe("useCallback — fail cases (stale closures)", () => { + it("flags useCallback with empty deps capturing a prop", () => { + const code = ` + const SearchInput = ({ onSearch }) => { + const handler = useCallback(() => { + onSearch("query"); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("onSearch"); + expect(result.diagnostics[0].message).toContain("stale"); + }); + + it("flags useCallback with empty deps capturing useState value", () => { + const code = ` + const Counter = () => { + const [count, setCount] = useState(0); + const log = useCallback(() => { + console.log(count); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("count"); + }); + + it("flags useCallback with empty deps capturing useContext value", () => { + const code = ` + const ThemedButton = () => { + const theme = useContext(ThemeContext); + const getColor = useCallback(() => { + return theme.primaryColor; + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("theme"); + }); + + it("flags useCallback with empty deps capturing multiple reactive values", () => { + const code = ` + const Form = ({ onSubmit }) => { + const [name, setName] = useState(""); + const [email, setEmail] = useState(""); + const handleSubmit = useCallback(() => { + onSubmit({ name, email }); + }, []); + return
; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("email"); + expect(result.diagnostics[0].message).toContain("name"); + expect(result.diagnostics[0].message).toContain("onSubmit"); + }); + + it("flags useCallback with empty deps capturing useReducer state", () => { + const code = ` + const App = () => { + const [state, dispatch] = useReducer(reducer, initialState); + const logState = useCallback(() => { + console.log(state); + }, []); + return
; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("state"); + }); + + it("flags useCallback in a function-declaration component", () => { + const code = ` + function Dashboard({ userId }) { + const [data, setData] = useState(null); + const refresh = useCallback(() => { + fetchData(userId, data); + }, []); + return ; + } + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("data"); + expect(result.diagnostics[0].message).toContain("userId"); + }); + }); + + // ── useCallback — pass cases (no stale closures) ───────────────── + + describe("useCallback — pass cases (not stale)", () => { + it("passes when useCallback has correct deps", () => { + const code = ` + const App = ({ value }) => { + const handler = useCallback(() => { + console.log(value); + }, [value]); + return
; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("passes when useCallback with empty deps only uses stable values", () => { + const code = ` + const App = () => { + const [count, setCount] = useState(0); + const increment = useCallback(() => { + setCount(42); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("passes when useCallback with empty deps only uses refs", () => { + const code = ` + const App = () => { + const inputRef = useRef(null); + const focusInput = useCallback(() => { + inputRef.current.focus(); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("passes when useCallback with empty deps has no captures", () => { + const code = ` + const App = () => { + const noop = useCallback(() => { + console.log("hello"); + }, []); + return
; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("passes when useCallback has no deps array", () => { + const code = ` + const App = ({ value }) => { + const handler = useCallback(() => { + console.log(value); + }); + return
; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("passes when useCallback with empty deps only captures useReducer dispatch", () => { + const code = ` + const App = () => { + const [state, dispatch] = useReducer(reducer, initialState); + const reset = useCallback(() => { + dispatch({ type: "RESET" }); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("passes when useCallback with empty deps only uses setter pattern names", () => { + const code = ` + const App = () => { + const [count, setCount] = useState(0); + const [name, setName] = useState(""); + const resetAll = useCallback(() => { + setCount(0); + setName(""); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("analytics"); + }); + + it("flags the interval callback pattern (stale state in ref)", () => { + const code = ` + const Poller = ({ endpoint }) => { + const [token, setToken] = useState(null); + const pollRef = useRef(() => { + fetch(endpoint, { headers: { Authorization: token } }); + }); + useEffect(() => { + const id = setInterval(pollRef.current, 5000); + return () => clearInterval(id); + }, []); + return
; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("endpoint"); + }); + + it("passes the correctly-updated ref pattern (useInsertionEffect)", () => { + const code = ` + const App = ({ onChange }) => { + const ref = useRef(onChange); + ref.current = onChange; + const stableCallback = useCallback((...args) => { + ref.current(...args); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("passes the zustand selector pattern (no reactive capture)", () => { + const code = ` + const App = () => { + const selectCount = useCallback((state) => state.count, []); + return
; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("passes the event handler with only setter calls", () => { + const code = ` + const ToggleButton = () => { + const [isOpen, setIsOpen] = useState(false); + const toggle = useCallback(() => { + setIsOpen((prev) => !prev); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("passes the navigation handler with stable dispatch", () => { + const code = ` + const NavButton = () => { + const [state, dispatch] = useReducer(reducer, initialState); + const goHome = useCallback(() => { + dispatch({ type: "NAVIGATE", payload: "/home" }); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + + it("flags callback that reads destructured prop fields", () => { + const code = ` + const Profile = ({ user }) => { + const greet = useCallback(() => { + alert("Hello, " + user.name); + }, []); + return ; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("user"); + }); + + it("does not flag non-component functions", () => { + const code = ` + const useFoo = () => { + const handler = useCallback(() => { + console.log("hook"); + }, []); + return handler; + }; + `; + const result = runRule(noStaleClosure, code); + expect(result.diagnostics).toHaveLength(0); + }); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-stale-closure.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-stale-closure.ts new file mode 100644 index 000000000..acbc01c1f --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-stale-closure.ts @@ -0,0 +1,347 @@ +import { SETTER_PATTERN, HOOKS_WITH_DEPS } from "../../constants/react.js"; +import { createComponentPropStackTracker } from "../../utils/create-component-prop-stack-tracker.js"; +import { collectPatternNames } from "../../utils/collect-pattern-names.js"; +import { defineRule } from "../../utils/define-rule.js"; +import { isHookCall } from "../../utils/is-hook-call.js"; +import { walkAst } from "../../utils/walk-ast.js"; +import type { EsTreeNode } from "../../utils/es-tree-node.js"; +import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; +import type { Rule } from "../../utils/rule.js"; +import type { RuleContext } from "../../utils/rule-context.js"; +import { isNodeOfType } from "../../utils/is-node-of-type.js"; + +const STABLE_HOOK_RETURN_NAMES = new Set([ + "useRef", + "useEffectEvent", + "experimental_useEffectEvent", +]); + +const REACTIVE_HOOK_STATE_NAMES = new Set(["useState", "useReducer"]); + +const REACTIVE_HOOK_RETURN_NAMES = new Set(["useContext"]); + +const collectReactiveAndStableBindings = ( + componentBody: EsTreeNode, + propNames: Set, +): { reactiveNames: Set; stableNames: Set } => { + const reactiveNames = new Set(propNames); + const stableNames = new Set(); + + if (!isNodeOfType(componentBody, "BlockStatement")) { + return { reactiveNames, stableNames }; + } + + for (const statement of componentBody.body ?? []) { + if (!isNodeOfType(statement, "VariableDeclaration")) continue; + for (const declarator of statement.declarations ?? []) { + if (!isNodeOfType(declarator.init, "CallExpression")) continue; + + const calleeName = getSimpleCalleeName(declarator.init); + if (!calleeName) continue; + + if (REACTIVE_HOOK_STATE_NAMES.has(calleeName)) { + if (isNodeOfType(declarator.id, "ArrayPattern")) { + const elements = declarator.id.elements ?? []; + const stateValue = elements[0]; + const stateSetter = elements[1]; + if (isNodeOfType(stateValue, "Identifier")) { + reactiveNames.add(stateValue.name); + } + if (isNodeOfType(stateSetter, "Identifier")) { + stableNames.add(stateSetter.name); + } + } + continue; + } + + if (REACTIVE_HOOK_RETURN_NAMES.has(calleeName)) { + if (isNodeOfType(declarator.id, "Identifier")) { + reactiveNames.add(declarator.id.name); + } + continue; + } + + if (STABLE_HOOK_RETURN_NAMES.has(calleeName)) { + if (isNodeOfType(declarator.id, "Identifier")) { + stableNames.add(declarator.id.name); + } + continue; + } + + if (calleeName === "useCallback" || calleeName === "useMemo") { + if (isNodeOfType(declarator.id, "Identifier")) { + stableNames.add(declarator.id.name); + } + continue; + } + } + } + + return { reactiveNames, stableNames }; +}; + +const getSimpleCalleeName = (node: EsTreeNode): string | null => { + if (!isNodeOfType(node, "CallExpression")) return null; + if (isNodeOfType(node.callee, "Identifier")) return node.callee.name; + return null; +}; + +const collectLocalBindings = (callbackNode: EsTreeNode): Set => { + const localNames = new Set(); + + const params = (callbackNode as { params?: EsTreeNode[] }).params ?? []; + for (const param of params) { + collectPatternNames(param, localNames); + } + + walkAst(callbackNode, (child: EsTreeNode) => { + if ( + isNodeOfType(child, "ArrowFunctionExpression") || + isNodeOfType(child, "FunctionExpression") || + isNodeOfType(child, "FunctionDeclaration") + ) { + if (child !== callbackNode) return false; + } + + if (isNodeOfType(child, "VariableDeclarator") && isNodeOfType(child.id, "Identifier")) { + localNames.add(child.id.name); + } + if (isNodeOfType(child, "VariableDeclarator")) { + collectPatternNames(child.id, localNames); + } + }); + + return localNames; +}; + +const isPropertyAccessPosition = (identifier: EsTreeNode): boolean => { + const parent = identifier.parent; + if ( + isNodeOfType(parent, "MemberExpression") && + !parent.computed && + parent.property === identifier + ) { + return true; + } + if ( + isNodeOfType(parent, "Property") && + !parent.computed && + !parent.shorthand && + parent.key === identifier + ) { + return true; + } + return false; +}; + +const isInsideNestedFunction = (identifier: EsTreeNode, callbackNode: EsTreeNode): boolean => { + let cursor: EsTreeNode | null = identifier.parent ?? null; + while (cursor && cursor !== callbackNode) { + if ( + isNodeOfType(cursor, "ArrowFunctionExpression") || + isNodeOfType(cursor, "FunctionExpression") || + isNodeOfType(cursor, "FunctionDeclaration") + ) { + return true; + } + cursor = cursor.parent ?? null; + } + return false; +}; + +interface StaleCaptureResult { + capturedReactiveNames: Set; +} + +const findStaleCapturesInCallback = ( + callbackNode: EsTreeNode, + reactiveNames: Set, + stableNames: Set, +): StaleCaptureResult => { + const capturedReactiveNames = new Set(); + const localBindings = collectLocalBindings(callbackNode); + + walkAst(callbackNode, (child: EsTreeNode) => { + if (!isNodeOfType(child, "Identifier")) return; + const identifierName = child.name; + + if (localBindings.has(identifierName)) return; + if (stableNames.has(identifierName)) return; + if (SETTER_PATTERN.test(identifierName)) return; + if (isPropertyAccessPosition(child)) return; + if (isInsideNestedFunction(child, callbackNode)) return; + + if (reactiveNames.has(identifierName)) { + capturedReactiveNames.add(identifierName); + } + }); + + return { capturedReactiveNames }; +}; + +const isEmptyDepsArray = (node: EsTreeNode): boolean => + isNodeOfType(node, "ArrayExpression") && (node.elements?.length ?? 0) === 0; + +const isFunctionExpression = (node: EsTreeNode | null | undefined): boolean => + Boolean(node) && + (isNodeOfType(node, "ArrowFunctionExpression") || isNodeOfType(node, "FunctionExpression")); + +const doesComponentBodyReassignRefCurrent = ( + componentBody: EsTreeNode, + refBindingName: string, +): boolean => { + let hasReassignment = false; + walkAst(componentBody, (child: EsTreeNode) => { + if (hasReassignment) return false; + + if (!isNodeOfType(child, "AssignmentExpression")) return; + const left = child.left; + if ( + isNodeOfType(left, "MemberExpression") && + isNodeOfType(left.object, "Identifier") && + left.object.name === refBindingName && + isNodeOfType(left.property, "Identifier") && + left.property.name === "current" + ) { + hasReassignment = true; + return false; + } + }); + return hasReassignment; +}; + +const formatCapturedNames = (names: Set): string => { + const sortedNames = [...names].sort(); + if (sortedNames.length === 1) return `\`${sortedNames[0]}\``; + if (sortedNames.length === 2) return `\`${sortedNames[0]}\` and \`${sortedNames[1]}\``; + const lastElement = sortedNames.pop(); + return `${sortedNames.map((name) => `\`${name}\``).join(", ")}, and \`${lastElement}\``; +}; + +export const noStaleClosure = defineRule({ + id: "no-stale-closure", + severity: "warn", + tags: ["test-noise"], + recommendation: + "Wrap the callback with `useEffectEvent(callback)` (React 19+) so it always reads the latest values without being a reactive dependency, or use a `useNonReactiveCallback` helper that stores the latest callback in a ref via useInsertionEffect. See https://react.dev/learn/separating-events-from-effects", + create: (context: RuleContext) => { + const checkComponent = (componentBody: EsTreeNode | undefined): void => { + if (!componentBody || !isNodeOfType(componentBody, "BlockStatement")) return; + + const currentPropNames = propStackTracker.getCurrentPropNames(); + const { reactiveNames, stableNames } = collectReactiveAndStableBindings( + componentBody, + currentPropNames, + ); + + for (const statement of componentBody.body ?? []) { + if (!isNodeOfType(statement, "VariableDeclaration")) continue; + + for (const declarator of statement.declarations ?? []) { + if (!isNodeOfType(declarator.init, "CallExpression")) continue; + + const calleeName = getSimpleCalleeName(declarator.init); + if (!calleeName) continue; + + if (calleeName === "useCallback") { + checkUseCallbackWithEmptyDeps(declarator, reactiveNames, stableNames, context); + continue; + } + + if (calleeName === "useRef") { + checkUseRefWithStaleCallback( + declarator, + componentBody, + reactiveNames, + stableNames, + context, + ); + continue; + } + } + } + }; + + const propStackTracker = createComponentPropStackTracker({ + onComponentEnter: checkComponent, + }); + + return propStackTracker.visitors; + }, +}); + +const checkUseCallbackWithEmptyDeps = ( + declarator: EsTreeNodeOfType<"VariableDeclarator">, + reactiveNames: Set, + stableNames: Set, + context: RuleContext, +): void => { + const callExpression = declarator.init; + if (!isNodeOfType(callExpression, "CallExpression")) return; + + const callArguments = callExpression.arguments ?? []; + if (callArguments.length < 2) return; + + const depsNode = callArguments[1]; + if (!isEmptyDepsArray(depsNode)) return; + + const callbackNode = callArguments[0]; + if (!isFunctionExpression(callbackNode)) return; + + const { capturedReactiveNames } = findStaleCapturesInCallback( + callbackNode, + reactiveNames, + stableNames, + ); + + if (capturedReactiveNames.size === 0) return; + + const bindingName = isNodeOfType(declarator.id, "Identifier") ? declarator.id.name : "callback"; + const capturedLabel = formatCapturedNames(capturedReactiveNames); + + context.report({ + node: callExpression, + message: `"${bindingName}" is a useCallback with empty deps but captures reactive ${ + capturedReactiveNames.size > 1 ? "values" : "value" + } ${capturedLabel} — ${capturedLabel} will be stale after the first render. Use useEffectEvent or useNonReactiveCallback to always read the latest values with a stable identity`, + }); +}; + +const checkUseRefWithStaleCallback = ( + declarator: EsTreeNodeOfType<"VariableDeclarator">, + componentBody: EsTreeNode, + reactiveNames: Set, + stableNames: Set, + context: RuleContext, +): void => { + const callExpression = declarator.init; + if (!isNodeOfType(callExpression, "CallExpression")) return; + + const callArguments = callExpression.arguments ?? []; + if (callArguments.length < 1) return; + + const initializer = callArguments[0]; + if (!isFunctionExpression(initializer)) return; + + const refBindingName = isNodeOfType(declarator.id, "Identifier") ? declarator.id.name : null; + if (!refBindingName) return; + + if (doesComponentBodyReassignRefCurrent(componentBody, refBindingName)) return; + + const { capturedReactiveNames } = findStaleCapturesInCallback( + initializer, + reactiveNames, + stableNames, + ); + + if (capturedReactiveNames.size === 0) return; + + const capturedLabel = formatCapturedNames(capturedReactiveNames); + + context.report({ + node: callExpression, + message: `useRef stores a callback capturing reactive ${ + capturedReactiveNames.size > 1 ? "values" : "value" + } ${capturedLabel} but "${refBindingName}.current" is never reassigned — ${capturedLabel} will be stale. Use useEffectEvent or useNonReactiveCallback instead`, + }); +};