diff --git a/packages/react-doctor/src/oxlint-config.ts b/packages/react-doctor/src/oxlint-config.ts index 962e664..23b19c2 100644 --- a/packages/react-doctor/src/oxlint-config.ts +++ b/packages/react-doctor/src/oxlint-config.ts @@ -381,24 +381,50 @@ export const ALL_REACT_DOCTOR_RULE_KEYS: ReadonlySet = new Set([ // HACK: single source of truth for which rules are gated behind the // project's detected React major. Adding a new version-gated rule means -// touching just this map. `null` reactMajorVersion (couldn't detect) -// keeps every rule enabled so we never silently swallow real findings. -const VERSION_GATED_RULE_IDS: ReadonlyMap = new Map([ - ["react-doctor/no-react19-deprecated-apis", REACT_19_DEPRECATION_MIN_MAJOR], - ["react-doctor/no-default-props", REACT_19_DEPRECATION_MIN_MAJOR], - ["react-doctor/no-react-dom-deprecated-apis", REACT_DOM_LEGACY_API_MIN_MAJOR], - ["react-doctor/prefer-use-effect-event", USE_EFFECT_EVENT_MIN_MAJOR], +// touching just this map. +// +// `mode` controls behavior when version detection FAILS (null): +// - "prefer-newer-api": the rule recommends an API that ONLY exists at +// or above `minMajor` (e.g. `useEffectEvent`). Suggesting it on a +// project where we can't prove the API exists is noise — fail closed. +// - "deprecation-warning": the rule flags patterns that BREAK at or +// above `minMajor` (e.g. `defaultProps` removal in React 19). Useful +// even on projects we can't version-detect, because the user may be +// mid-migration. Fail open. +type VersionGateMode = "prefer-newer-api" | "deprecation-warning"; +interface VersionGate { + minMajor: number; + mode: VersionGateMode; +} +const VERSION_GATED_RULE_IDS: ReadonlyMap = new Map([ + [ + "react-doctor/no-react19-deprecated-apis", + { minMajor: REACT_19_DEPRECATION_MIN_MAJOR, mode: "deprecation-warning" }, + ], + [ + "react-doctor/no-default-props", + { minMajor: REACT_19_DEPRECATION_MIN_MAJOR, mode: "deprecation-warning" }, + ], + [ + "react-doctor/no-react-dom-deprecated-apis", + { minMajor: REACT_DOM_LEGACY_API_MIN_MAJOR, mode: "deprecation-warning" }, + ], + [ + "react-doctor/prefer-use-effect-event", + { minMajor: USE_EFFECT_EVENT_MIN_MAJOR, mode: "prefer-newer-api" }, + ], ]); const filterRulesByReactMajor = ( rules: Record, reactMajorVersion: number | null, ): Record => { - if (reactMajorVersion === null) return rules; return Object.fromEntries( Object.entries(rules).filter(([ruleKey]) => { - const minMajor = VERSION_GATED_RULE_IDS.get(ruleKey); - return minMajor === undefined || reactMajorVersion >= minMajor; + const gate = VERSION_GATED_RULE_IDS.get(ruleKey); + if (gate === undefined) return true; + if (reactMajorVersion === null) return gate.mode === "deprecation-warning"; + return reactMajorVersion >= gate.minMajor; }), ); }; diff --git a/packages/react-doctor/src/plugin/constants.ts b/packages/react-doctor/src/plugin/constants.ts index 5f4c1ae..3ebd3b1 100644 --- a/packages/react-doctor/src/plugin/constants.ts +++ b/packages/react-doctor/src/plugin/constants.ts @@ -568,13 +568,19 @@ export const EXTERNAL_SYNC_OBSERVER_CONSTRUCTORS = new Set([ // almost always denotes a fire-and-forget user-action effect." // Layered on top of `FETCH_CALLEE_NAMES` so adding a new HTTP client // shorthand in one place propagates to every detector that recognizes it. +// +// HACK: ambiguous generic verbs (`track`, `logEvent`, `del`) used to +// live here too. They produced FPs on user-defined helpers +// (`track(progress)`, `del(item)`) that have nothing to do with +// network/analytics side effects. Detection still works via the +// receiver-bound member-call shape (`analytics.track(...)`, +// `api.del(...)`) in `EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS`. export const EVENT_TRIGGERED_SIDE_EFFECT_CALLEES = new Set([ ...FETCH_CALLEE_NAMES, // Network shorthand verbs (article uses `post`) "post", "put", "patch", - "del", // Navigation "navigate", "navigateTo", @@ -584,8 +590,6 @@ export const EVENT_TRIGGERED_SIDE_EFFECT_CALLEES = new Set([ "alert", "confirm", // Analytics - "track", - "logEvent", "logVisit", "captureEvent", ]); diff --git a/packages/react-doctor/src/plugin/helpers.ts b/packages/react-doctor/src/plugin/helpers.ts index 1b1709d..a5e83af 100644 --- a/packages/react-doctor/src/plugin/helpers.ts +++ b/packages/react-doctor/src/plugin/helpers.ts @@ -19,6 +19,17 @@ interface ComponentPropStackTracker { visitors: RuleVisitors; } +interface ComponentBindingStackTrackerCallbacks { + onVariableDeclarator?: (node: EsTreeNode) => void; +} + +interface ComponentBindingStackTracker { + isInsideComponent: () => boolean; + isBoundName: (name: string) => boolean; + addBindingToCurrentFrame: (name: string) => void; + visitors: RuleVisitors; +} + // HACK: AST is acyclic except for `parent` back-references, which we skip. // Visitors may return `false` to prune the subtree below `node` (e.g. to // stop walking into nested functions when collecting `await` expressions @@ -476,3 +487,63 @@ export const createComponentPropStackTracker = ( return { isPropName, getCurrentPropNames, visitors }; }; + +// HACK: sibling of `createComponentPropStackTracker` for rules that need +// to track *binding* sets per component scope rather than the destructured +// prop set — e.g. `no-effect-event-in-deps` accumulates the names of +// `useEffectEvent` declarators while inside a component and then queries +// "is this dep-array identifier one of our useEffectEvent bindings?". +// +// Three rules previously reimplemented this push/pop bookkeeping inline. +// They now share the same scaffold; the per-rule predicate (e.g. "is the +// initializer a `useEffectEvent(...)` call?") lives in the +// `onVariableDeclarator` callback. +// +// The barrier semantic is intentionally simpler than the prop-stack +// tracker: the rule (e.g. `no-effect-event-in-deps`) only mutates the +// top frame for VariableDeclarators directly inside a component, and +// the stack only grows on FunctionDeclaration / VariableDeclarator +// component entries, so a closed-over name from an outer component +// can't leak in via a nested helper. +export const createComponentBindingStackTracker = ( + callbacks?: ComponentBindingStackTrackerCallbacks, +): ComponentBindingStackTracker => { + const componentBindingStack: Array> = []; + + const isInsideComponent = (): boolean => componentBindingStack.length > 0; + + const isBoundName = (name: string): boolean => { + for (let frameIndex = componentBindingStack.length - 1; frameIndex >= 0; frameIndex--) { + if (componentBindingStack[frameIndex].has(name)) return true; + } + return false; + }; + + const addBindingToCurrentFrame = (name: string): void => { + if (componentBindingStack.length === 0) return; + componentBindingStack[componentBindingStack.length - 1].add(name); + }; + + const visitors: RuleVisitors = { + FunctionDeclaration(node: EsTreeNode) { + if (!node.id?.name || !isUppercaseName(node.id.name)) return; + componentBindingStack.push(new Set()); + }, + "FunctionDeclaration:exit"(node: EsTreeNode) { + if (!node.id?.name || !isUppercaseName(node.id.name)) return; + componentBindingStack.pop(); + }, + VariableDeclarator(node: EsTreeNode) { + if (isComponentAssignment(node)) { + componentBindingStack.push(new Set()); + return; + } + callbacks?.onVariableDeclarator?.(node); + }, + "VariableDeclarator:exit"(node: EsTreeNode) { + if (isComponentAssignment(node)) componentBindingStack.pop(); + }, + }; + + return { isInsideComponent, isBoundName, addBindingToCurrentFrame, visitors }; +}; diff --git a/packages/react-doctor/src/plugin/rules/state-and-effects.ts b/packages/react-doctor/src/plugin/rules/state-and-effects.ts index 4e04ae7..8a96ee3 100644 --- a/packages/react-doctor/src/plugin/rules/state-and-effects.ts +++ b/packages/react-doctor/src/plugin/rules/state-and-effects.ts @@ -30,6 +30,7 @@ import { collectPatternNames, containsFetchCall, countSetStateCalls, + createComponentBindingStackTracker, createComponentPropStackTracker, getCallbackStatements, getEffectCallback, @@ -550,33 +551,34 @@ export const noPropCallbackInEffect: Rule = { const depsNode = node.arguments[1]; if (depsNode.type !== "ArrayExpression" || !depsNode.elements?.length) return; - const bodyStatements = getCallbackStatements(callback); - for (const bodyStatement of bodyStatements) { - let invokedPropName: string | null = null; - if ( - bodyStatement.type === "ExpressionStatement" && - bodyStatement.expression?.type === "CallExpression" && - bodyStatement.expression.callee?.type === "Identifier" && - propStackTracker.isPropName(bodyStatement.expression.callee.name) - ) { - invokedPropName = bodyStatement.expression.callee.name; - } - if (!invokedPropName) continue; - - // Only flag if at least one dep is a non-prop (state-shape) - // identifier — otherwise the effect is just adapting to prop - // changes (legit pattern). - const hasStateLikeDep = depsNode.elements.some( - (element: EsTreeNode) => - element?.type === "Identifier" && !propStackTracker.isPropName(element.name), - ); - if (!hasStateLikeDep) continue; - + // Only flag if at least one dep is a non-prop (state-shape) + // identifier — otherwise the effect is just adapting to prop + // changes (legit pattern). + const hasStateLikeDep = depsNode.elements.some( + (element: EsTreeNode) => + element?.type === "Identifier" && !propStackTracker.isPropName(element.name), + ); + if (!hasStateLikeDep) return; + + // HACK: walk control-flow descendants (`if`, `try`, `for`, + // `switch`) but stop at any nested function boundary so calls + // inside `setTimeout(() => onChange(state))` aren't conflated + // with the top-level `onChange(state)` shape — those belong to + // `prefer-use-effect-event` (sub-handler reads), not this rule + // (lift state via callback). + const reportedNodes = new Set(); + walkInsideStatementBlocks(callback.body, (child: EsTreeNode) => { + if (child.type !== "CallExpression") return; + if (child.callee?.type !== "Identifier") return; + const calleeName = child.callee.name; + if (!propStackTracker.isPropName(calleeName)) return; + if (reportedNodes.has(child)) return; + reportedNodes.add(child); context.report({ - node: bodyStatement, - message: `useEffect calls prop callback "${invokedPropName}" with local state in deps — this is the "lift state via callback" anti-pattern; lift state into a shared Provider so both sides read the same source`, + node: child, + message: `useEffect calls prop callback "${calleeName}" with local state in deps — this is the "lift state via callback" anti-pattern; lift state into a shared Provider so both sides read the same source`, }); - } + }); }, }; }, @@ -593,55 +595,27 @@ export const noPropCallbackInEffect: Rule = { // `onChange` in ComponentB in the same file. export const noEffectEventInDeps: Rule = { create: (context: RuleContext) => { - const componentBindingStack: Array> = []; - - const isEffectEventBinding = (name: string): boolean => { - for (let stackIndex = componentBindingStack.length - 1; stackIndex >= 0; stackIndex--) { - if (componentBindingStack[stackIndex].has(name)) return true; - } - return false; - }; - - const enterComponent = (): void => { - componentBindingStack.push(new Set()); - }; - const exitComponent = (): void => { - componentBindingStack.pop(); - }; + const componentBindings = createComponentBindingStackTracker({ + onVariableDeclarator: (declaratorNode: EsTreeNode) => { + if (declaratorNode.id?.type !== "Identifier") return; + const initializer = declaratorNode.init; + if (!initializer || initializer.type !== "CallExpression") return; + if (!isHookCall(initializer, "useEffectEvent")) return; + componentBindings.addBindingToCurrentFrame(declaratorNode.id.name); + }, + }); return { - FunctionDeclaration(node: EsTreeNode) { - if (!node.id?.name || !isUppercaseName(node.id.name)) return; - enterComponent(); - }, - "FunctionDeclaration:exit"(node: EsTreeNode) { - if (!node.id?.name || !isUppercaseName(node.id.name)) return; - exitComponent(); - }, - VariableDeclarator(node: EsTreeNode) { - if (isComponentAssignment(node)) { - enterComponent(); - return; - } - if (componentBindingStack.length === 0) return; - if (node.id?.type !== "Identifier") return; - const init = node.init; - if (!init || init.type !== "CallExpression") return; - if (!isHookCall(init, "useEffectEvent")) return; - componentBindingStack[componentBindingStack.length - 1].add(node.id.name); - }, - "VariableDeclarator:exit"(node: EsTreeNode) { - if (isComponentAssignment(node)) exitComponent(); - }, + ...componentBindings.visitors, CallExpression(node: EsTreeNode) { if (!isHookCall(node, HOOKS_WITH_DEPS) || node.arguments.length < 2) return; - if (componentBindingStack.length === 0) return; + if (!componentBindings.isInsideComponent()) return; const depsNode = node.arguments[1]; if (depsNode.type !== "ArrayExpression") return; for (const element of depsNode.elements ?? []) { if (element?.type !== "Identifier") continue; - if (isEffectEventBinding(element.name)) { + if (componentBindings.isBoundName(element.name)) { context.report({ node: element, message: `"${element.name}" is from useEffectEvent and must not be in the deps array — its identity is intentionally unstable; call it inside the effect without listing it`, @@ -1355,47 +1329,9 @@ const cleanupReleasesSubscription = ( ): boolean => { const lastStatement = effectBodyStatements[effectBodyStatements.length - 1]; if (lastStatement?.type !== "ReturnStatement") return false; - const returnedValue = lastStatement.argument; - if (!returnedValue) return false; - - if ( - boundUnsubscribeName && - returnedValue.type === "Identifier" && - returnedValue.name === boundUnsubscribeName - ) { - return true; - } - - if ( - returnedValue.type === "ArrowFunctionExpression" || - returnedValue.type === "FunctionExpression" - ) { - let didReleaseSubscription = false; - walkAst(returnedValue, (child: EsTreeNode) => { - if (didReleaseSubscription) return; - if (child.type !== "CallExpression") return; - - if ( - boundUnsubscribeName && - child.callee?.type === "Identifier" && - child.callee.name === boundUnsubscribeName - ) { - didReleaseSubscription = true; - return; - } - - if ( - child.callee?.type === "MemberExpression" && - child.callee.property?.type === "Identifier" && - UNSUBSCRIPTION_METHOD_NAMES.has(child.callee.property.name) - ) { - didReleaseSubscription = true; - } - }); - return didReleaseSubscription; - } - - return false; + const knownBoundReleaseNames = new Set(); + if (boundUnsubscribeName) knownBoundReleaseNames.add(boundUnsubscribeName); + return isCleanupReturn(lastStatement.argument, knownBoundReleaseNames); }; export const preferUseSyncExternalStore: Rule = { @@ -1736,12 +1672,19 @@ const collectDepIdentifierNames = (effectNode: EsTreeNode): Set => { return depNames; }; +// HACK: only count setter calls that actually run during the effect's +// synchronous body. A `setX` inside `setTimeout(() => setX(...))` or +// `.then(() => setX(...))` is a DEFERRED write — by the time it fires, +// the chain reader effect has already had its dep-update window. Treat +// only direct (non-nested-function) writes as chain triggers; that +// stops `noEffectChain` from over-flagging the dominant debounce / +// async-fetch shape that real codebases use. const collectWrittenStateNamesInEffect = ( effectCallback: EsTreeNode, setterToStateName: Map, ): Set => { const writtenStateNames = new Set(); - walkAst(effectCallback, (child: EsTreeNode) => { + walkInsideStatementBlocks(effectCallback.body, (child: EsTreeNode) => { if (child.type !== "CallExpression") return; if (child.callee?.type !== "Identifier") return; const stateName = setterToStateName.get(child.callee.name); @@ -1750,6 +1693,32 @@ const collectWrittenStateNamesInEffect = ( return writtenStateNames; }; +// HACK: a useEffect cleanup return value MUST be a function (or +// undefined). Anything else is either user error or "I'm using +// `return` for early-exit, not for cleanup". For the chain detector, +// we treat only function-shaped returns as "this effect owns an +// external resource" — bare literals (`return null`, `return 0`) and +// state reads (`return foo`) get ignored so they don't silently +// disable chain detection. +const isFunctionShapedReturn = (returnedValue: EsTreeNode): boolean => { + if ( + returnedValue.type === "ArrowFunctionExpression" || + returnedValue.type === "FunctionExpression" + ) { + return true; + } + // Returning a CallExpression result — most cleanup-returning + // primitives (subscribe, addEventListener helpers) return a + // function. Conservatively accept this shape. + if (returnedValue.type === "CallExpression") return true; + // Returning a bare Identifier — could be the unsub binding from a + // `const unsub = subscribe(...)` line. We can't statically prove + // it's function-typed without scope analysis, but in idiomatic React + // this is the dominant cleanup pattern. Accept. + if (returnedValue.type === "Identifier") return true; + return false; +}; + const isExternalSyncEffect = (effectCallback: EsTreeNode): boolean => { // A cleanup return is the strongest signal that the effect owns // an external resource — once we see one, we don't need to inspect @@ -1757,7 +1726,13 @@ const isExternalSyncEffect = (effectCallback: EsTreeNode): boolean => { if (effectCallback.body?.type === "BlockStatement") { const statements = effectCallback.body.body ?? []; for (const statement of statements) { - if (statement.type === "ReturnStatement" && statement.argument) return true; + if ( + statement.type === "ReturnStatement" && + statement.argument && + isFunctionShapedReturn(statement.argument) + ) { + return true; + } } } @@ -2070,6 +2045,96 @@ const isSubscribeLikeCallExpression = (node: EsTreeNode): boolean => { return SUBSCRIPTION_METHOD_NAMES.has(node.callee.property.name); }; +// HACK: variables bound to a subscribe-like or timer-like call inside +// an effect body are CLEANUP TARGETS — `return X` or `() => X()` / +// `() => clearTimeout(X)` releases the resource. Collecting them here +// lets the shared release predicate accept user-named bindings +// (`const unsub = ...; return unsub`) without falling back to the +// previous "any Identifier is fine" behavior. +const collectReleasableBindingNames = (effectCallback: EsTreeNode): Set => { + const releasableNames = new Set(); + if (effectCallback.body?.type !== "BlockStatement") return releasableNames; + for (const statement of effectCallback.body.body ?? []) { + if (statement.type !== "VariableDeclaration") continue; + for (const declarator of statement.declarations ?? []) { + if (declarator.id?.type !== "Identifier") continue; + const init = declarator.init; + if (!init || init.type !== "CallExpression") continue; + if (isSubscribeLikeCallExpression(init)) { + releasableNames.add(declarator.id.name); + continue; + } + if ( + init.callee?.type === "Identifier" && + TIMER_CALLEE_NAMES_REQUIRING_CLEANUP.has(init.callee.name) + ) { + releasableNames.add(declarator.id.name); + } + } + } + return releasableNames; +}; + +// Single source of truth for "does this CallExpression release a +// previously-acquired effect resource?". Used by both +// `effectNeedsCleanup` and `prefer-use-sync-external-store` so the +// two rules can never disagree on what a cleanup looks like. +const isReleaseLikeCall = ( + callNode: EsTreeNode, + knownBoundReleaseNames: ReadonlySet, +): boolean => { + if (callNode?.type !== "CallExpression") return false; + const callee = callNode.callee; + if (callee?.type === "Identifier") { + if (TIMER_CLEANUP_CALLEE_NAMES.has(callee.name)) return true; + if (CLEANUP_LIKE_RELEASE_CALLEE_NAMES.has(callee.name)) return true; + if (knownBoundReleaseNames.has(callee.name)) return true; + return false; + } + if (callee?.type === "MemberExpression" && callee.property?.type === "Identifier") { + return UNSUBSCRIPTION_METHOD_NAMES.has(callee.property.name); + } + return false; +}; + +const containsReleaseLikeCall = ( + node: EsTreeNode, + knownBoundReleaseNames: ReadonlySet, +): boolean => { + let didFindRelease = false; + walkAst(node, (child: EsTreeNode) => { + if (didFindRelease) return false; + if (isReleaseLikeCall(child, knownBoundReleaseNames)) { + didFindRelease = true; + return false; + } + }); + return didFindRelease; +}; + +// Recognizes the four cleanup-return shapes uniformly: +// return unsub → bound name match +// return store.subscribe(handler) → subscribe call IS the unsub +// return () => unsub() → closure releases via name +// return () => store.removeListener(...) → closure releases via verb +const isCleanupReturn = ( + returnedValue: EsTreeNode | null | undefined, + knownBoundReleaseNames: ReadonlySet, +): boolean => { + if (!returnedValue) return false; + if (returnedValue.type === "Identifier") { + return knownBoundReleaseNames.has(returnedValue.name); + } + if (isSubscribeLikeCallExpression(returnedValue)) return true; + if ( + returnedValue.type === "ArrowFunctionExpression" || + returnedValue.type === "FunctionExpression" + ) { + return containsReleaseLikeCall(returnedValue, knownBoundReleaseNames); + } + return false; +}; + const effectHasCleanupRelease = (callback: EsTreeNode): boolean => { // HACK: expression-body arrows are the dominant shape for trivial // subscribe-only effects: @@ -2087,48 +2152,8 @@ const effectHasCleanupRelease = (callback: EsTreeNode): boolean => { const statements = callback.body.body ?? []; const lastStatement = statements[statements.length - 1]; if (lastStatement?.type !== "ReturnStatement") return false; - const returnedValue = lastStatement.argument; - if (!returnedValue) return false; - - if (returnedValue.type === "Identifier") return true; - - if (isSubscribeLikeCallExpression(returnedValue)) return true; - - if ( - returnedValue.type !== "ArrowFunctionExpression" && - returnedValue.type !== "FunctionExpression" - ) { - return false; - } - - let didFindRelease = false; - walkAst(returnedValue, (child: EsTreeNode) => { - if (didFindRelease) return false; - if (child.type !== "CallExpression") return; - const callee = child.callee; - if (callee?.type === "Identifier" && TIMER_CLEANUP_CALLEE_NAMES.has(callee.name)) { - didFindRelease = true; - return; - } - if ( - callee?.type === "MemberExpression" && - callee.property?.type === "Identifier" && - UNSUBSCRIPTION_METHOD_NAMES.has(callee.property.name) - ) { - didFindRelease = true; - return; - } - // Direct call to an Identifier whose name SAYS "release this - // resource" (e.g. `return () => unsubscribe()` or - // `return () => cleanup()`). The allowlist lives in `constants.ts` - // as `CLEANUP_LIKE_RELEASE_CALLEE_NAMES` so it stays in lockstep - // with `UNSUBSCRIPTION_METHOD_NAMES` plus the generic teardown - // vocabulary (`cleanup` / `dispose` / `destroy` / `teardown`). - if (callee?.type === "Identifier" && CLEANUP_LIKE_RELEASE_CALLEE_NAMES.has(callee.name)) { - didFindRelease = true; - } - }); - return didFindRelease; + const knownBoundReleaseNames = collectReleasableBindingNames(callback); + return isCleanupReturn(lastStatement.argument, knownBoundReleaseNames); }; export const effectNeedsCleanup: Rule = { @@ -2212,9 +2237,16 @@ export const noMirrorPropEffect: Rule = { const depsNode = effectCall.arguments[1]; if (depsNode.type !== "ArrayExpression") continue; - if ((depsNode.elements?.length ?? 0) !== 1) continue; - const depElement = depsNode.elements[0]; - if (depElement?.type !== "Identifier") continue; + // HACK: previously required EXACTLY one dep, which silently + // missed the legitimate `useEffect(() => setX(value), [value, otherDep])` + // mirror shape. Now we accept any deps array as long as the + // prop root we mirror IS one of the deps — `otherDep` being + // unused inside the body is a separate (exhaustive-deps) concern. + const depIdentifierNames = new Set(); + for (const element of depsNode.elements ?? []) { + if (element?.type === "Identifier") depIdentifierNames.add(element.name); + } + if (depIdentifierNames.size === 0) continue; const callback = getEffectCallback(effectCall); if (!callback) continue; @@ -2232,7 +2264,7 @@ export const noMirrorPropEffect: Rule = { const matchedBinding = mirrorBindings.find( (binding) => binding.setterName === expression.callee.name && - binding.propRootName === depElement.name && + depIdentifierNames.has(binding.propRootName) && areExpressionsStructurallyEqual(binding.initializer, setterArgument), ); if (!matchedBinding) continue; @@ -2395,6 +2427,32 @@ const getSubHandlerCalleeName = (callExpression: EsTreeNode): string | null => { // for either a direct sub-handler argument position OR a const binding // whose Identifier appears as an argument to a sub-handler call later // in the same effect body. +// Resolve the enclosing function back to its local-binding name across +// the three idiomatic shapes: +// const handler = (e) => ... → VariableDeclarator binding +// function handler(e) { ... } → FunctionDeclaration self-binding +// let handler; handler = (e) => ... → AssignmentExpression binding +const getEnclosingFunctionBindingName = (enclosingFunction: EsTreeNode): string | null => { + if ( + enclosingFunction.type === "FunctionDeclaration" && + enclosingFunction.id?.type === "Identifier" + ) { + return enclosingFunction.id.name; + } + const directParent = enclosingFunction.parent; + if (directParent?.type === "VariableDeclarator" && directParent.id?.type === "Identifier") { + return directParent.id.name; + } + if ( + directParent?.type === "AssignmentExpression" && + directParent.right === enclosingFunction && + directParent.left?.type === "Identifier" + ) { + return directParent.left.name; + } + return null; +}; + const findSubHandlerForEnclosingFunction = ( enclosingFunction: EsTreeNode, effectCallback: EsTreeNode, @@ -2408,9 +2466,8 @@ const findSubHandlerForEnclosingFunction = ( return directParent; } - if (directParent?.type !== "VariableDeclarator") return null; - if (directParent.id?.type !== "Identifier") return null; - const localName: string = directParent.id.name; + const localName = getEnclosingFunctionBindingName(enclosingFunction); + if (localName === null) return null; let matchingSubHandlerCall: EsTreeNode | null = null; walkAst(effectCallback, (child: EsTreeNode) => { diff --git a/packages/react-doctor/tests/regressions/prop-stack-barrier.test.ts b/packages/react-doctor/tests/regressions/prop-stack-barrier.test.ts index 144ca83..4a30bf0 100644 --- a/packages/react-doctor/tests/regressions/prop-stack-barrier.test.ts +++ b/packages/react-doctor/tests/regressions/prop-stack-barrier.test.ts @@ -1,11 +1,12 @@ /** * Regression tests for the "empty-frame-as-barrier" semantic in the - * shared prop-stack scaffolding used by `no-prop-callback-in-effect` - * and `no-derived-useState`. The visitor pushes an empty `Set` when - * entering a non-component FunctionDeclaration / ArrowFunctionExpression - * so identifiers inside the helper don't resolve against an outer - * component's props (a closed-over `value` is NOT a prop of the - * helper). + * shared prop-stack scaffolding used by all four `createComponentPropStackTracker` + * consumers (`no-derived-useState`, `no-prop-callback-in-effect`, + * `no-mirror-prop-effect`, `prefer-use-effect-event`). The visitor pushes + * an empty `Set` when entering a non-component FunctionDeclaration / + * ArrowFunctionExpression so identifiers inside the helper don't resolve + * against an outer component's props (a closed-over `value` is NOT a + * prop of the helper). * * The original `isPropName` walked the entire stack without honoring * the barrier, so a useState / useEffect inside a nested helper would @@ -36,6 +37,7 @@ const collectRuleHits = async ( framework: "unknown", hasReactCompiler: false, hasTanStackQuery: false, + reactMajorVersion: 19, }); return diagnostics .filter((diagnostic) => diagnostic.rule === ruleId) @@ -112,6 +114,56 @@ export const Toggle = ({ onChange }: { onChange: (next: boolean) => void }) => { expect(hits[0].message).toContain("onChange"); }); + it("flags `if (didChange) onChange(state)` inside a control-flow descendant (deep-walk regression)", async () => { + // Regression: previously only top-level statements of the effect + // body were inspected. The "lift state via callback" anti-pattern + // frequently lives behind a guard — that case was a silent FN. + const projectDir = setupReactProject(tempRoot, "no-prop-callback-deep-walk", { + files: { + "src/Toggle.tsx": `import { useEffect, useState } from "react"; + +export const Toggle = ({ onChange }: { onChange: (next: boolean) => void }) => { + const [isOn, setIsOn] = useState(false); + useEffect(() => { + if (isOn) { + onChange(isOn); + } + }, [isOn, onChange]); + return ; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-prop-callback-in-effect"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("onChange"); + }); + + it("does NOT flag `setTimeout(() => onChange(state))` (sub-handler belongs to prefer-use-effect-event, not this rule)", async () => { + // Sub-handler reads are the domain of `prefer-use-effect-event`. + // The deep-walk MUST stop at function boundaries so they don't + // double-fire here. + const projectDir = setupReactProject(tempRoot, "no-prop-callback-no-subhandler", { + files: { + "src/Debounced.tsx": `import { useEffect, useState } from "react"; + +export const Debounced = ({ onChange }: { onChange: (next: string) => void }) => { + const [text, setText] = useState(""); + useEffect(() => { + const id = setTimeout(() => onChange(text), 300); + return () => clearTimeout(id); + }, [text, onChange]); + return setText(event.target.value)} />; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-prop-callback-in-effect"); + expect(hits).toHaveLength(0); + }); + it("does NOT flag `useEffect(() => onChange(state), [state, onChange])` inside a nested helper", async () => { // Same nested-helper shape — the outer component's `onChange` prop // must not leak into the helper's effect-callback check. @@ -138,3 +190,105 @@ export const Outer = ({ onChange }: { onChange: (next: boolean) => void }) => { expect(hits).toHaveLength(0); }); }); + +describe("no-mirror-prop-effect — empty-frame barrier", () => { + it("flags the canonical `useEffect(setDraft(value), [value])` shape", async () => { + const projectDir = setupReactProject(tempRoot, "no-mirror-prop-real-prop", { + files: { + "src/Form.tsx": `import { useEffect, useState } from "react"; + +export const Form = ({ value }: { value: string }) => { + const [draft, setDraft] = useState(value); + useEffect(() => { + setDraft(value); + }, [value]); + return setDraft(event.target.value)} />; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-mirror-prop-effect"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("value"); + }); + + it("does NOT flag the same mirror shape when it lives inside a nested helper", async () => { + // The barrier must hide `Outer`'s `value` prop from the inner + // helper. Without the barrier the closed-over `value` would + // resolve as a prop of the helper and the mirror check would + // false-positive. + const projectDir = setupReactProject(tempRoot, "no-mirror-prop-nested-helper", { + files: { + "src/Outer.tsx": `import { useEffect, useState } from "react"; + +export const Outer = ({ value }: { value: string }) => { + function inner() { + const [draft, setDraft] = useState(value); + useEffect(() => { + setDraft(value); + }, [value]); + void draft; + } + inner(); + return {value}; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-mirror-prop-effect"); + expect(hits).toHaveLength(0); + }); +}); + +describe("prefer-use-effect-event — empty-frame barrier", () => { + it("flags the canonical sub-handler-only prop read shape", async () => { + const projectDir = setupReactProject(tempRoot, "prefer-use-effect-event-real-prop", { + files: { + "src/Debounced.tsx": `import { useEffect, useState } from "react"; + +export const Debounced = ({ onChange }: { onChange: (value: string) => void }) => { + const [text, setText] = useState(""); + useEffect(() => { + const id = setTimeout(() => onChange(text), 300); + return () => clearTimeout(id); + }, [text, onChange]); + return setText(event.target.value)} />; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "prefer-use-effect-event"); + expect(hits.length).toBeGreaterThanOrEqual(1); + expect(hits.some((hit) => hit.message.includes("onChange"))).toBe(true); + }); + + it("does NOT flag the same shape when it lives inside a nested helper", async () => { + // Outer's `onChange` prop must not leak into the helper's + // dep-classification logic. + const projectDir = setupReactProject(tempRoot, "prefer-use-effect-event-nested-helper", { + files: { + "src/Outer.tsx": `import { useEffect, useState } from "react"; + +export const Outer = ({ onChange }: { onChange: (value: string) => void }) => { + function inner() { + const [text, setText] = useState(""); + useEffect(() => { + const id = setTimeout(() => onChange(text), 300); + return () => clearTimeout(id); + }, [text, onChange]); + void setText; + } + inner(); + return ; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "prefer-use-effect-event"); + expect(hits).toHaveLength(0); + }); +}); diff --git a/packages/react-doctor/tests/regressions/state-rules.test.ts b/packages/react-doctor/tests/regressions/state-rules.test.ts index 1d51fb4..331d644 100644 --- a/packages/react-doctor/tests/regressions/state-rules.test.ts +++ b/packages/react-doctor/tests/regressions/state-rules.test.ts @@ -12,10 +12,14 @@ afterAll(() => { fs.rmSync(tempRoot, { recursive: true, force: true }); }); +// HACK: `setupReactProject` defaults to React ^19.0.0 in the synthetic +// `package.json`, so the test plumbing must mirror what production +// detection would yield for that project. Explicit values (17 / 18 / +// null) still override at the call site. const collectRuleHits = async ( projectDir: string, ruleId: string, - reactMajorVersion: number | null = null, + reactMajorVersion: number | null = 19, ): Promise> => { const diagnostics = await runOxlint({ rootDirectory: projectDir, @@ -642,6 +646,45 @@ export const Leaky = () => { const hits = await collectRuleHits(projectDir, "prefer-use-sync-external-store"); expect(hits).toHaveLength(0); }); + + it("DOES flag a useSyncExternalStore reimplementation whose cleanup uses a generic teardown verb (`cleanup()`)", async () => { + // Regression: `cleanupReleasesSubscription` previously only accepted + // `UNSUBSCRIPTION_METHOD_NAMES` plus the literal bound-unsubscribe + // name. Generic teardown verbs from `CLEANUP_LIKE_RELEASE_CALLEE_NAMES` + // (`cleanup`, `dispose`, `destroy`, `teardown`) were silently ignored, + // so a complete useSyncExternalStore reimplementation with a + // generic-named cleanup slipped past detection — even though + // `effectNeedsCleanup` (which already shared the broader allowlist) + // recognized the same shape. Both rules now share the same + // `isReleaseLikeCall` primitive. + const projectDir = setupReactProject(tempRoot, "prefer-use-sync-external-store-cleanup-verb", { + files: { + "src/Cleaned.tsx": `import { useEffect, useState } from "react"; + +declare const store: { + subscribe: (listener: () => void) => () => void; + getSnapshot: () => number; +}; +declare const cleanup: () => void; + +export const Cleaned = () => { + const [snapshot, setSnapshot] = useState(store.getSnapshot()); + useEffect(() => { + store.subscribe(() => { + setSnapshot(store.getSnapshot()); + }); + return () => cleanup(); + }, []); + return
{snapshot}
; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "prefer-use-sync-external-store"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("useSyncExternalStore"); + }); }); describe("no-event-trigger-state", () => { @@ -890,6 +933,60 @@ export const Wizard = () => { expect(triggerHits.length).toBe(1); }); + it("does NOT misclassify a user-defined `track(progress)` helper as analytics", async () => { + // Regression: `track` and `logEvent` used to be in the direct-call + // allowlist. They're so common as user-helper names (game progress + // tracking, event tracking) that direct-call detection produced + // FPs. Detection still works via the receiver shape + // (`analytics.track(...)`), which is what real analytics SDKs use. + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-user-track", { + files: { + "src/Game.tsx": `import { useEffect, useState } from "react"; + +declare const track: (progress: number) => void; + +export const Game = () => { + const [progress, setProgress] = useState(null); + useEffect(() => { + if (progress !== null) { + track(progress); + } + }, [progress]); + return ; +}; +`, + }, + }); + + const triggerHits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(triggerHits.length).toBe(0); + }); + + it("DOES still flag `analytics.track(progress)` member-call as event-triggered analytics", async () => { + const projectDir = setupReactProject(tempRoot, "no-event-trigger-state-analytics-track", { + files: { + "src/Game.tsx": `import { useEffect, useState } from "react"; + +declare const analytics: { track: (progress: number) => void }; + +export const Game = () => { + const [progress, setProgress] = useState(null); + useEffect(() => { + if (progress !== null) { + analytics.track(progress); + } + }, [progress]); + return ; +}; +`, + }, + }); + + const triggerHits = await collectRuleHits(projectDir, "no-event-trigger-state"); + expect(triggerHits.length).toBe(1); + expect(triggerHits[0].message).toContain("analytics.track"); + }); + it("KEEPS no-effect-event-handler warning when state-typed dep has a non-allowlisted callee (Bugbot #155 round 3)", async () => { // Regression: round-2 deference was too eager — it skipped // no-effect-event-handler whenever the trigger was a useState @@ -1283,6 +1380,66 @@ export const Profile = ({ userId, theme }: { userId: string; theme: string }) => const hits = await collectRuleHits(projectDir, "no-effect-chain"); expect(hits).toHaveLength(0); }); + + it("does NOT flag a chain whose writer setter runs inside a deferred sub-handler (setTimeout)", async () => { + // Regression: `collectWrittenStateNamesInEffect` previously walked + // the ENTIRE callback (including nested function bodies). A `setX` + // inside `setTimeout(() => setX(...))` was attributed as a sync + // chain write, producing a noisy diagnostic on the dominant + // debounce / delayed-fetch pattern. + const projectDir = setupReactProject(tempRoot, "no-effect-chain-deferred-write", { + files: { + "src/Debounced.tsx": `import { useEffect, useState } from "react"; + +export const Debounced = ({ raw }: { raw: string }) => { + const [debounced, setDebounced] = useState(""); + const [upper, setUpper] = useState(""); + useEffect(() => { + const id = setTimeout(() => setDebounced(raw), 300); + return () => clearTimeout(id); + }, [raw]); + useEffect(() => { + setUpper(debounced.toUpperCase()); + }, [debounced]); + return {upper}; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-effect-chain"); + expect(hits).toHaveLength(0); + }); + + it("DOES flag a chain when the writer effect uses a non-function `return` (literal / state read)", async () => { + // Regression: previously ANY `return ` made the writer + // effect look "external sync" — so `return null` or `return foo` + // silently disabled chain detection. We now require the returned + // value to be function-shaped for the early-out. + const projectDir = setupReactProject(tempRoot, "no-effect-chain-return-literal", { + files: { + "src/Chain.tsx": `import { useEffect, useState } from "react"; + +export const Chain = ({ userId }: { userId: string }) => { + const [name, setName] = useState(""); + const [upper, setUpper] = useState(""); + useEffect(() => { + setName(userId); + return null; + }, [userId]); + useEffect(() => { + setUpper(name.toUpperCase()); + }, [name]); + return {upper}; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-effect-chain"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("name"); + }); }); describe("no-uncontrolled-input", () => { @@ -1671,6 +1828,57 @@ export const Form = ({ value }: { value: string }) => { expect(hits[0].message).toContain("value"); }); + it("does NOT flag a multi-dep mirror when the prop root is NOT one of the deps (FP guard for L1)", async () => { + // Regression / FP guard: L1 widened the deps check from + // "exactly one dep" to "any deps including the prop root". Without + // the `depIdentifierNames.has(propRootName)` clause this would + // false-positive on effects that mention the setter+value but + // are actually keyed off something else entirely. + const projectDir = setupReactProject(tempRoot, "no-mirror-prop-effect-prop-not-in-deps", { + files: { + "src/Form.tsx": `import { useEffect, useState } from "react"; + +export const Form = ({ value, theme }: { value: string; theme: string }) => { + const [draft, setDraft] = useState(value); + useEffect(() => { + setDraft(value); + }, [theme]); + return setDraft(event.target.value)} />; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-mirror-prop-effect"); + expect(hits).toHaveLength(0); + }); + + it("flags the multi-dep mirror shape `useEffect(setX(value), [value, otherDep])`", async () => { + // Regression: previously required EXACTLY one dep, missing the + // common case where the mirror effect lists additional deps for + // exhaustive-deps compliance. The mirror anti-pattern still + // applies — `value` is mirrored even if `otherDep` is co-listed. + const projectDir = setupReactProject(tempRoot, "no-mirror-prop-effect-multi-deps", { + files: { + "src/Form.tsx": `import { useEffect, useState } from "react"; + +export const Form = ({ value, theme }: { value: string; theme: string }) => { + const [draft, setDraft] = useState(value); + useEffect(() => { + setDraft(value); + }, [value, theme]); + return setDraft(event.target.value)} />; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "no-mirror-prop-effect"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("draft"); + expect(hits[0].message).toContain("value"); + }); + it("flags the MemberExpression variant `useState(prop.x) + setDraft(prop.x)`", async () => { const projectDir = setupReactProject(tempRoot, "no-mirror-prop-effect-member", { files: { @@ -2371,6 +2579,58 @@ export const Outer = ({ value }: { value: (q: string) => void }) => { expect(hits).toHaveLength(0); }); + it("flags a `function handler() {...}` declaration (FunctionDeclaration shape, not just `const handler = ...`)", async () => { + // Regression: `findSubHandlerForEnclosingFunction` previously only + // recognized `const handler = ...` (VariableDeclarator). The + // FunctionDeclaration shape was a silent FN. + const projectDir = setupReactProject(tempRoot, "prefer-use-effect-event-fn-decl", { + files: { + "src/Listener.tsx": `import { useEffect, useState } from "react"; + +export const Listener = ({ onKey }: { onKey: (key: string) => void }) => { + const [prefix, setPrefix] = useState(""); + useEffect(() => { + function handler(event: KeyboardEvent) { onKey(event.key + prefix); } + window.addEventListener("keydown", handler); + return () => window.removeEventListener("keydown", handler); + }, [prefix, onKey]); + return setPrefix(event.target.value)} />; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "prefer-use-effect-event"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("onKey"); + }); + + it("flags an `let h; h = (e) => ...` reassignment shape (AssignmentExpression binding)", async () => { + // Regression: the AssignmentExpression form was a silent FN + // alongside the FunctionDeclaration shape. + const projectDir = setupReactProject(tempRoot, "prefer-use-effect-event-assign", { + files: { + "src/Listener.tsx": `import { useEffect, useState } from "react"; + +export const Listener = ({ onKey }: { onKey: (key: string) => void }) => { + const [prefix, setPrefix] = useState(""); + useEffect(() => { + let handler: (event: KeyboardEvent) => void; + handler = (event) => onKey(event.key + prefix); + window.addEventListener("keydown", handler); + return () => window.removeEventListener("keydown", handler); + }, [prefix, onKey]); + return setPrefix(event.target.value)} />; +}; +`, + }, + }); + + const hits = await collectRuleHits(projectDir, "prefer-use-effect-event"); + expect(hits).toHaveLength(1); + expect(hits[0].message).toContain("onKey"); + }); + it("does NOT flag a scalar destructured prop only read inside a sub-handler (Bugbot #162)", async () => { // Regression: previously every destructured prop satisfied the // function-typed gate. A component like \`({ onSearch, prefix })\` @@ -2407,9 +2667,14 @@ export const SearchInput = ({ expect(hits[0].message).not.toContain("prefix"); }); - it("fires when reactMajorVersion is unknown (null) so we never silently swallow real findings", async () => { - // Matches the existing convention — null version detection keeps every - // rule enabled. See `filterRulesByReactMajor` in oxlint-config.ts. + it("does NOT fire when reactMajorVersion is unknown (null) — `prefer-X` rules need the API to exist", async () => { + // Directional gating: `prefer-use-effect-event` recommends a + // React-19-only API. Without proof the project HAS the API, + // every diagnostic is invalid. See `filterRulesByReactMajor` in + // oxlint-config.ts (`VersionGateMode = "prefer-newer-api"`). + // `deprecation-warning`-mode rules (`no-react19-deprecated-apis`, + // `no-default-props`, `no-react-dom-deprecated-apis`) keep firing + // on unknown versions because they're useful during migration. const projectDir = setupReactProject(tempRoot, "prefer-use-effect-event-unknown-version", { files: { "src/SearchInput.tsx": `import { useEffect, useState } from "react"; @@ -2427,6 +2692,6 @@ export const SearchInput = ({ onSearch }: { onSearch: (q: string) => void }) => }); const hits = await collectRuleHits(projectDir, "prefer-use-effect-event", null); - expect(hits).toHaveLength(1); + expect(hits).toHaveLength(0); }); }); diff --git a/packages/react-doctor/tests/run-oxlint.test.ts b/packages/react-doctor/tests/run-oxlint.test.ts index 92143dc..328c277 100644 --- a/packages/react-doctor/tests/run-oxlint.test.ts +++ b/packages/react-doctor/tests/run-oxlint.test.ts @@ -52,6 +52,7 @@ describe("runOxlint", () => { framework: "unknown", hasReactCompiler: false, hasTanStackQuery: true, + reactMajorVersion: 19, }); nextjsDiagnostics = await runOxlint({ rootDirectory: NEXTJS_APP_DIRECTORY, @@ -59,6 +60,7 @@ describe("runOxlint", () => { framework: "nextjs", hasReactCompiler: false, hasTanStackQuery: false, + reactMajorVersion: 19, }); tanstackStartDiagnostics = await runOxlint({ rootDirectory: TANSTACK_START_APP_DIRECTORY, @@ -66,6 +68,7 @@ describe("runOxlint", () => { framework: "tanstack-start", hasReactCompiler: false, hasTanStackQuery: false, + reactMajorVersion: 19, }); });