From 0b7a7dd7dc889e494df071d5984e16e017666d49 Mon Sep 17 00:00:00 2001 From: Aiden Bai Date: Thu, 18 Jun 2026 23:54:17 -0700 Subject: [PATCH 01/13] feat(oxlint-plugin): beef up the CFG and fix the false positives it exposed Expand the control-flow graph with reachability, dominance, post-dominance, loop-membership, and unreachable-code primitives; model loop back-edges and infinite loops; and give try/catch/finally proper finalize/join semantics (finally stays reachable through an abrupt try; post-try code is unreachable when no path completes normally). Port oxc's no-unreachable fixtures to lock the behavior in. Adopt the new primitives to fix false positives: - nextjs-no-redirect-in-try-catch: stop mis-flagging navigation calls in a catch/finally block or in a try whose only companion is finally. - no-mutating-reducer-state: drop mutations that can't reach the same-reference return (loop mutate-then-return-fresh with a trailing `return state`). - js-hoist-regexp / js-index-maps / js-set-map-lookups: loop-awareness now keys off real CFG loop membership instead of lexical nesting, so work inside a callback that merely escapes a loop is no longer flagged. server-auth-actions and effect-needs-cleanup were investigated and deliberately left unchanged (CFG gating introduced worse false positives in both). --- .changeset/cfg-beefup-and-rule-fixes.md | 11 + .../src/plugin/constants/js.ts | 8 - .../js-performance/js-hoist-regexp.test.ts | 34 ++ .../rules/js-performance/js-hoist-regexp.ts | 2 +- .../rules/js-performance/js-index-maps.ts | 2 +- .../js-performance/js-set-map-lookups.ts | 2 +- .../nextjs-no-redirect-in-try-catch.test.ts | 75 ++++ .../nextjs/nextjs-no-redirect-in-try-catch.ts | 51 ++- .../no-mutating-reducer-state.test.ts | 50 +++ .../no-mutating-reducer-state.ts | 15 +- ...control-flow-graph.oxc-unreachable.test.ts | 107 +++++ .../semantic/control-flow-graph.test.ts | 110 +++++ .../control-flow-graph.try-finally.test.ts | 106 +++++ .../src/plugin/semantic/control-flow-graph.ts | 409 ++++++++++++++++-- .../utils/create-loop-aware-visitors.ts | 25 +- .../utils/wrap-with-semantic-context.ts | 5 + 16 files changed, 935 insertions(+), 77 deletions(-) create mode 100644 .changeset/cfg-beefup-and-rule-fixes.md create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-hoist-regexp.test.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.test.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.oxc-unreachable.test.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.try-finally.test.ts diff --git a/.changeset/cfg-beefup-and-rule-fixes.md b/.changeset/cfg-beefup-and-rule-fixes.md new file mode 100644 index 000000000..25f920a1c --- /dev/null +++ b/.changeset/cfg-beefup-and-rule-fixes.md @@ -0,0 +1,11 @@ +--- +"oxlint-plugin-react-doctor": patch +--- + +Beef up the control-flow graph and fix the false positives it exposed. + +The internal CFG now exposes reachability, dominance, post-dominance, loop-membership, and unreachable-code primitives, models loop back-edges and infinite loops, and gives `try`/`catch`/`finally` proper finalize/join semantics (a `finally` body stays reachable even when the `try` returns; code after the try is unreachable when no path completes normally). Several rules adopt it: + +- `nextjs-no-redirect-in-try-catch` no longer mis-flags `redirect()` / `notFound()` in a `catch` block, in a `finally` block, or in a `try` that has only a `finally` (no `catch`) — none of those swallow the navigation control-flow error. +- `no-mutating-reducer-state` no longer reports a loop that mutates and then `return`s a fresh object (`for (…) { state.items.push(x); return { ...state } }`) when a trailing `return state` only runs on the no-match path. +- `js-hoist-regexp`, `js-index-maps`, and `js-set-map-lookups` no longer mis-flag work inside a callback that merely escapes a loop (the loop-aware check now uses real CFG loop membership instead of lexical nesting depth). diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/constants/js.ts b/packages/oxlint-plugin-react-doctor/src/plugin/constants/js.ts index d9558b54e..56999dc2b 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/constants/js.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/constants/js.ts @@ -1,11 +1,3 @@ -export const LOOP_TYPES = [ - "ForStatement", - "ForInStatement", - "ForOfStatement", - "WhileStatement", - "DoWhileStatement", -]; - // ESTree node type names for the three "function-like" syntactic // forms — declaration, expression, arrow. Used by the scope analyzer // (to bound function scopes) and by `rules-of-hooks` (to skip into diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-hoist-regexp.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-hoist-regexp.test.ts new file mode 100644 index 000000000..342fa2979 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-hoist-regexp.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runRule } from "../../../test-utils/run-rule.js"; +import { jsHoistRegexp } from "./js-hoist-regexp.js"; + +describe("js-hoist-regexp", () => { + it("flags `new RegExp(...)` built inside a loop body", () => { + const result = runRule( + jsHoistRegexp, + `function fn(rows) { for (const row of rows) { const re = new RegExp(row.pattern); test(re); } }`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); + + it("does not flag a `new RegExp(...)` outside any loop", () => { + const result = runRule(jsHoistRegexp, `const re = new RegExp(pattern);`); + expect(result.diagnostics).toHaveLength(0); + }); + + it("does not flag a `new RegExp(...)` inside a callback that merely escapes the loop", () => { + // Regression: the regexp is built per-click, not per-iteration — + // the click handler is a separate function with its own acyclic + // CFG, so cycle-based loop membership must report nothing. + const result = runRule( + jsHoistRegexp, + `function fn(rows) { + for (const row of rows) { + row.element.onclick = () => { const re = new RegExp(row.pattern); test(re); }; + } + }`, + ); + expect(result.diagnostics).toHaveLength(0); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-hoist-regexp.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-hoist-regexp.ts index 721c137cf..285df11d2 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-hoist-regexp.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-hoist-regexp.ts @@ -12,7 +12,7 @@ export const jsHoistRegexp = defineRule({ recommendation: "Move `new RegExp(...)` (or large regex literals) to a constant outside the loop so it isn't rebuilt on every pass", create: (context: RuleContext) => - createLoopAwareVisitors({ + createLoopAwareVisitors(context, { NewExpression(node: EsTreeNodeOfType<"NewExpression">) { if (isNodeOfType(node.callee, "Identifier") && node.callee.name === "RegExp") { context.report({ diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-index-maps.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-index-maps.ts index f8cf5a12b..a3d3913d9 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-index-maps.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-index-maps.ts @@ -32,7 +32,7 @@ export const jsIndexMaps = defineRule({ recommendation: "Build a `Map` once before the loop instead of calling `array.find(...)` inside it", create: (context: RuleContext) => - createLoopAwareVisitors({ + createLoopAwareVisitors(context, { CallExpression(node: EsTreeNodeOfType<"CallExpression">) { if ( !isNodeOfType(node.callee, "MemberExpression") || diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-set-map-lookups.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-set-map-lookups.ts index 987f93771..1688cad5e 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-set-map-lookups.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/js-set-map-lookups.ts @@ -280,7 +280,7 @@ export const jsSetMapLookups = defineRule({ recommendation: "Use a `Set` or `Map` when you check for the same items over and over. `Array.includes`/`find` scans the whole list each time", create: (context: RuleContext) => - createLoopAwareVisitors({ + createLoopAwareVisitors(context, { CallExpression(node: EsTreeNodeOfType<"CallExpression">) { if ( !isNodeOfType(node.callee, "MemberExpression") || diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.test.ts new file mode 100644 index 000000000..780c2ec41 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runRule } from "../../../test-utils/run-rule.js"; +import { nextjsNoRedirectInTryCatch } from "./nextjs-no-redirect-in-try-catch.js"; + +const CASES: ReadonlyArray<{ name: string; code: string; expectedDiagnosticCount: number }> = [ + { + name: "flags redirect() in a try body that has a catch handler", + code: `import { redirect } from "next/navigation"; + export const action = () => { try { redirect("/x"); } catch (e) {} };`, + expectedDiagnosticCount: 1, + }, + { + name: "ignores redirect() in the catch handler", + code: `import { redirect } from "next/navigation"; + export const action = () => { try {} catch { redirect("/x"); } };`, + expectedDiagnosticCount: 0, + }, + { + name: "ignores redirect() in the finally block", + code: `import { redirect } from "next/navigation"; + export const action = () => { try {} finally { redirect("/x"); } };`, + expectedDiagnosticCount: 0, + }, + { + name: "ignores redirect() in a try with only a finally (no catch)", + code: `import { redirect } from "next/navigation"; + export const action = () => { try { redirect("/x"); } finally {} };`, + expectedDiagnosticCount: 0, + }, + { + name: "flags at the inner try when the nested try is the one that catches", + code: `import { redirect } from "next/navigation"; + export const action = () => { try { try { redirect("/x"); } catch {} } catch {} };`, + expectedDiagnosticCount: 1, + }, + { + name: "flags an inner-catch redirect that the outer try body swallows", + code: `import { redirect } from "next/navigation"; + export const action = () => { try { try {} catch { redirect("/x"); } } catch {} };`, + expectedDiagnosticCount: 1, + }, + { + name: "ignores redirect() outside any try", + code: `import { redirect } from "next/navigation"; + export const action = () => { redirect("/x"); };`, + expectedDiagnosticCount: 0, + }, + { + name: "still flags redirect() nested in an if inside the try body", + code: `import { redirect } from "next/navigation"; + export const action = (x) => { try { if (x) redirect("/x"); } catch (e) {} };`, + expectedDiagnosticCount: 1, + }, + { + name: "flags notFound() in a try body that has a catch handler", + code: `import { notFound } from "next/navigation"; + export const action = () => { try { notFound(); } catch (e) {} };`, + expectedDiagnosticCount: 1, + }, + { + name: "flags permanentRedirect() in a try body that has a catch handler", + code: `import { permanentRedirect } from "next/navigation"; + export const action = () => { try { permanentRedirect("/x"); } catch (e) {} };`, + expectedDiagnosticCount: 1, + }, +]; + +describe("nextjs-no-redirect-in-try-catch", () => { + for (const testCase of CASES) { + it(testCase.name, () => { + const result = runRule(nextjsNoRedirectInTryCatch, testCase.code); + expect(result.diagnostics).toHaveLength(testCase.expectedDiagnosticCount); + }); + } +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.ts index 987d343e0..e255ff70e 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.ts @@ -2,8 +2,27 @@ import { NEXTJS_NAVIGATION_FUNCTIONS } from "../../constants/nextjs.js"; import { defineRule } from "../../utils/define-rule.js"; import type { RuleContext } from "../../utils/rule-context.js"; import { isNodeOfType } from "../../utils/is-node-of-type.js"; +import type { EsTreeNode } from "../../utils/es-tree-node.js"; import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; +// A navigation call's control-flow error is only swallowed when the call sits +// in the `try` BLOCK of a statement that has a `catch` handler — a call in the +// `catch`/`finally`, or in a `try` whose only companion is `finally`, escapes +// untouched. So we walk the ancestry for the nearest such enclosing try rather +// than counting raw try-nesting depth (which over-reports all three cases). +const isInsideSwallowingTry = (callExpression: EsTreeNode): boolean => { + let child: EsTreeNode = callExpression; + let ancestor = callExpression.parent ?? null; + while (ancestor) { + if (isNodeOfType(ancestor, "TryStatement") && ancestor.handler && ancestor.block === child) { + return true; + } + child = ancestor; + ancestor = ancestor.parent ?? null; + } + return false; +}; + export const nextjsNoRedirectInTryCatch = defineRule({ id: "nextjs-no-redirect-in-try-catch", title: "redirect() inside try-catch", @@ -12,26 +31,16 @@ export const nextjsNoRedirectInTryCatch = defineRule({ severity: "warn", recommendation: "Move `redirect()` or `notFound()` outside the try block, or rethrow in `catch`, because these APIs throw control-flow errors that catch blocks swallow.", - create: (context: RuleContext) => { - let tryCatchDepth = 0; - - return { - TryStatement() { - tryCatchDepth++; - }, - "TryStatement:exit"() { - tryCatchDepth--; - }, - CallExpression(node: EsTreeNodeOfType<"CallExpression">) { - if (tryCatchDepth === 0) return; - if (!isNodeOfType(node.callee, "Identifier")) return; - if (!NEXTJS_NAVIGATION_FUNCTIONS.has(node.callee.name)) return; + create: (context: RuleContext) => ({ + CallExpression(node: EsTreeNodeOfType<"CallExpression">) { + if (!isNodeOfType(node.callee, "Identifier")) return; + if (!NEXTJS_NAVIGATION_FUNCTIONS.has(node.callee.name)) return; + if (!isInsideSwallowingTry(node)) return; - context.report({ - node, - message: `${node.callee.name}() inside try-catch gets swallowed, so the redirect silently fails.`, - }); - }, - }; - }, + context.report({ + node, + message: `${node.callee.name}() inside try-catch gets swallowed, so the redirect silently fails.`, + }); + }, + }), }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-mutating-reducer-state.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-mutating-reducer-state.test.ts index f2815fed4..e79e54484 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-mutating-reducer-state.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-mutating-reducer-state.test.ts @@ -984,4 +984,54 @@ describe("no-mutating-reducer-state", () => { expect(result.parseErrors).toEqual([]); expect(Array.isArray(result.diagnostics)).toBe(true); }); + + it("does not flag a loop mutation that only precedes a fresh-object return", () => { + // The 2^N path walker treats the whole loop as one straight-line + // statement, so without CFG reachability it wrongly attributes the + // `push` to the trailing `return state`. But whenever the `push` + // runs, control hits `return { ...state }` (a NEW top-level object — + // React re-renders); `return state` is reached only when nothing + // matched, i.e. when `push` never ran. Mirrors the if-branch + // invariant that mutate-then-return-fresh is out of scope. + const result = runRule( + noMutatingReducerState, + ` + import { useReducer } from "react"; + + function reducer(state, action) { + for (const item of action.items) { + if (item.id !== action.targetId) continue; + state.items.push(item); + return { ...state, changed: true }; + } + return state; + } + + useReducer(reducer, { items: [] }); + `, + ); + + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("still flags a loop mutation that reaches a same-reference return", () => { + const result = runRule( + noMutatingReducerState, + ` + import { useReducer } from "react"; + + function reducer(state, action) { + for (const item of action.items) { + state.items.push(item); + } + return state; + } + + useReducer(reducer, { items: [] }); + `, + ); + + expect(result.diagnostics).toHaveLength(1); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-mutating-reducer-state.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-mutating-reducer-state.ts index ba144e79c..151351841 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-mutating-reducer-state.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-mutating-reducer-state.ts @@ -683,9 +683,20 @@ const analyzeReactUseReducerFunctionForStateMutation = ( statement, activeState, ); - const mutationsAtReturn = [...activeState.mutations, ...returnMutations]; if (canExpressionReturnOriginalReducerStateReference(statement.argument, activeState)) { - reportReducerStateMutations(mutationsAtReturn); + // The 2^N path walker treats a loop / try body as one straight-line + // statement, so it can attribute a remembered mutation to a later + // top-level return even when an inner return/break severs that flow + // (mutate-then-`return { ...state }` in a loop, with a fallback + // `return state` after it). The CFG knows whether the mutation can + // actually reach this return; drop the ones that can't. Cross-file + // reducer nodes live in another file's CFG, so skip the filter there. + const rememberedMutations = options.crossFileConsumerCallSite + ? activeState.mutations + : activeState.mutations.filter((mutation) => + context.cfg.isReachable(mutation.node, statement), + ); + reportReducerStateMutations([...rememberedMutations, ...returnMutations]); } continue; } diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.oxc-unreachable.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.oxc-unreachable.test.ts new file mode 100644 index 000000000..e0d6ce3e1 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.oxc-unreachable.test.ts @@ -0,0 +1,107 @@ +import { describe, expect, it } from "vite-plus/test"; +import { analyzeControlFlow } from "./control-flow-graph.js"; +import { attachParentReferences } from "../../test-utils/attach-parent-references.js"; +import { parseFixture } from "../../test-utils/parse-fixture.js"; +import type { EsTreeNode } from "../utils/es-tree-node.js"; + +// Control-flow fixtures ported from oxc's `eslint/no-unreachable` test +// suite (crates/oxc_linter/src/rules/eslint/no_unreachable.rs). oxc +// asserts these via the rule; here they exercise `cfg.isUnreachable` +// directly. Every case shares a `x = 2` marker statement so the +// assertion is uniform: in oxc's FAIL cases that statement is +// unreachable, in its PASS cases it is reachable. +// +// Cases where our CFG deliberately diverges from oxc are omitted with a +// note: `var`/function-declaration hoisting (a rule policy, not a CFG +// fact). try/catch/finally normal-completion is now modeled via +// Finalize/Join edges and is covered in control-flow-graph.try-finally.test.ts. + +const analyze = (code: string) => { + const parsed = parseFixture(code); + attachParentReferences(parsed.program); + return { ...analyzeControlFlow(parsed.program), program: parsed.program, errors: parsed.errors }; +}; + +// Find the `x = 2` assignment-expression node shared by every fixture. +const findMarker = (root: EsTreeNode): EsTreeNode | null => { + let found: EsTreeNode | null = null; + const visit = (node: EsTreeNode): void => { + if (found) return; + if ( + node.type === "AssignmentExpression" && + (node as { left: EsTreeNode }).left.type === "Identifier" && + (node as { left: { name: string } }).left.name === "x" && + (node as { right: EsTreeNode }).right.type === "Literal" && + (node as { right: { value: unknown } }).right.value === 2 + ) { + found = node; + return; + } + const record = node as unknown as Record; + for (const key of Object.keys(record)) { + if (key === "parent") continue; + const child = record[key]; + if (Array.isArray(child)) { + for (const item of child) { + if (item && typeof item === "object" && "type" in item) visit(item as EsTreeNode); + } + } else if (child && typeof child === "object" && "type" in (child as object)) { + visit(child as EsTreeNode); + } + } + }; + visit(root); + return found; +}; + +// oxc FAIL cases: the `x = 2` marker is unreachable. +const UNREACHABLE_FIXTURES: ReadonlyArray = [ + "function foo() { var x = 1; if (x) { return; } else { throw e; } x = 2; }", + "function foo() { var x = 1; if (x) return; else throw -1; x = 2; }", + "function foo() { var x = 1; try { return; } finally {} x = 2; }", + "function foo() { var x = 1; try { } finally { return; } x = 2; }", + "function foo() { var x = 1; do { return; } while (x); x = 2; }", + "function foo() { var x = 1; for (;;) { if (x) continue; } x = 2; }", + // The infinite-loop cases are why we port oxc's loop handling — with + // no `break`, code after the loop is unreachable: + "function foo() { var x = 1; while (true) { } x = 2; }", + "function foo() { var x = 1; do { } while (true); x = 2; }", +]; + +// oxc PASS cases: the `x = 2` marker is reachable. +const REACHABLE_FIXTURES: ReadonlyArray = [ + "function foo() { var x = 1; if (x) { return; } x = 2; }", + "function foo() { var x = 1; if (x) { } else { return; } x = 2; }", + "function foo() { var x = 1; switch (x) { case 0: break; default: return; } x = 2; }", + "function foo() { var x = 1; while (x) { return; } x = 2; }", + "function foo() { var x = 1; for (x in {}) { return; } x = 2; }", + // Infinite loop, but an explicit `break` lets control reach the marker. + "function foo() { var x = 1; for (;;) { if (x) break; } x = 2; }", + "function foo() { var x = 1; for (;x == 1;) { if (x) continue; } x = 2; }", +]; + +describe("control-flow-graph: oxc no-unreachable fixtures", () => { + describe("unreachable marker (oxc FAIL cases)", () => { + for (const fixture of UNREACHABLE_FIXTURES) { + it(fixture, () => { + const analysis = analyze(fixture); + expect(analysis.errors).toEqual([]); + const marker = findMarker(analysis.program); + expect(marker).not.toBeNull(); + expect(analysis.isUnreachable(marker!)).toBe(true); + }); + } + }); + + describe("reachable marker (oxc PASS cases)", () => { + for (const fixture of REACHABLE_FIXTURES) { + it(fixture, () => { + const analysis = analyze(fixture); + expect(analysis.errors).toEqual([]); + const marker = findMarker(analysis.program); + expect(marker).not.toBeNull(); + expect(analysis.isUnreachable(marker!)).toBe(false); + }); + } + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.test.ts index cb1d804a0..b46b1ce8c 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.test.ts @@ -239,4 +239,114 @@ describe("control-flow-graph", () => { expect(cfg!.exit).toBeDefined(); }); }); + + describe("isReachable", () => { + it("forward through straight-line code, not backward", () => { + const analysis = analyze(`function fn() { a(); b(); }`); + const a = findCalleeNode(analysis.program, "a")!; + const b = findCalleeNode(analysis.program, "b")!; + expect(analysis.isReachable(a, b)).toBe(true); + expect(analysis.isReachable(b, a)).toBe(false); + }); + + it("both directions inside a loop body (cycle)", () => { + const analysis = analyze(`function fn() { while (x) { a(); b(); } }`); + const a = findCalleeNode(analysis.program, "a")!; + const b = findCalleeNode(analysis.program, "b")!; + expect(analysis.isReachable(a, b)).toBe(true); + expect(analysis.isReachable(b, a)).toBe(true); + }); + + it("not reachable across mutually-exclusive branches", () => { + const analysis = analyze(`function fn() { if (x) { thenCall(); } else { elseCall(); } }`); + const thenCall = findCalleeNode(analysis.program, "thenCall")!; + const elseCall = findCalleeNode(analysis.program, "elseCall")!; + expect(analysis.isReachable(thenCall, elseCall)).toBe(false); + }); + + it("never reachable across function boundaries", () => { + const analysis = analyze(` + function outer() { outerCall(); const f = () => { innerCall(); }; } + `); + const outerCall = findCalleeNode(analysis.program, "outerCall")!; + const innerCall = findCalleeNode(analysis.program, "innerCall")!; + expect(analysis.isReachable(outerCall, innerCall)).toBe(false); + }); + }); + + describe("dominates", () => { + it("earlier straight-line statement dominates later", () => { + const analysis = analyze(`function fn() { a(); b(); }`); + const a = findCalleeNode(analysis.program, "a")!; + const b = findCalleeNode(analysis.program, "b")!; + expect(analysis.dominates(a, b)).toBe(true); + expect(analysis.dominates(b, a)).toBe(false); + }); + + it("a guard before a branch dominates the post-merge, branch bodies do not", () => { + const analysis = analyze(` + function fn() { guard(); if (x) { thenCall(); } after(); } + `); + const guard = findCalleeNode(analysis.program, "guard")!; + const thenCall = findCalleeNode(analysis.program, "thenCall")!; + const after = findCalleeNode(analysis.program, "after")!; + expect(analysis.dominates(guard, after)).toBe(true); + expect(analysis.dominates(thenCall, after)).toBe(false); + }); + }); + + describe("postDominates", () => { + it("a later straight-line statement post-dominates an earlier one", () => { + const analysis = analyze(`function fn() { a(); b(); }`); + const a = findCalleeNode(analysis.program, "a")!; + const b = findCalleeNode(analysis.program, "b")!; + expect(analysis.postDominates(b, a)).toBe(true); + expect(analysis.postDominates(a, b)).toBe(false); + }); + + it("post-merge code post-dominates a pre-branch statement, branch bodies do not", () => { + const analysis = analyze(` + function fn() { before(); if (x) { thenCall(); } after(); } + `); + const before = findCalleeNode(analysis.program, "before")!; + const thenCall = findCalleeNode(analysis.program, "thenCall")!; + const after = findCalleeNode(analysis.program, "after")!; + expect(analysis.postDominates(after, before)).toBe(true); + expect(analysis.postDominates(thenCall, before)).toBe(false); + }); + }); + + describe("isInsideLoop", () => { + it("true for a node in a loop body, false after the loop", () => { + const analysis = analyze(` + function fn() { for (const item of items) { inLoop(); } afterLoop(); } + `); + expect(analysis.isInsideLoop(findCalleeNode(analysis.program, "inLoop")!)).toBe(true); + expect(analysis.isInsideLoop(findCalleeNode(analysis.program, "afterLoop")!)).toBe(false); + }); + + it("false for a node inside a callback that merely escapes a loop", () => { + // The regression that motivated cycle-based membership: the + // callback is a separate function with its own acyclic CFG, so + // `escaped()` runs per-invocation, not per-iteration. + const analysis = analyze(` + function fn() { + for (const item of items) { + element.onclick = () => { escaped(); }; + } + } + `); + expect(analysis.isInsideLoop(findCalleeNode(analysis.program, "escaped")!)).toBe(false); + }); + }); + + describe("isUnreachable", () => { + it("true for code after an unconditional return, false otherwise", () => { + const analysis = analyze(` + function fn() { live(); return; dead(); } + `); + expect(analysis.isUnreachable(findCalleeNode(analysis.program, "live")!)).toBe(false); + expect(analysis.isUnreachable(findCalleeNode(analysis.program, "dead")!)).toBe(true); + }); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.try-finally.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.try-finally.test.ts new file mode 100644 index 000000000..c3aad8bf6 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.try-finally.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it } from "vite-plus/test"; +import { analyzeControlFlow } from "./control-flow-graph.js"; +import { attachParentReferences } from "../../test-utils/attach-parent-references.js"; +import { parseFixture } from "../../test-utils/parse-fixture.js"; +import type { EsTreeNode } from "../utils/es-tree-node.js"; + +// try / catch / finally control-flow, ported from oxc's `no-unreachable`, +// `no-unsafe-finally`, and `getter-return` suites. These exercise the +// Finalize / Join edge model: `finally` is reachable on every path (even +// when `try` returns), but code after the try is reachable only when the +// protected region can complete normally. +// +// DELIBERATE DIVERGENCE FROM OXC: for `try { return } catch { return } +// finally { … }`, oxc's builder uses an unconditional tail edge after the +// finally and reports the code after the try as *reachable* (a documented +// over-approximation). We gate the tail on normal completion, so we report +// it unreachable — which matches the real language semantics. + +const analyze = (code: string) => { + const parsed = parseFixture(code); + attachParentReferences(parsed.program); + return { ...analyzeControlFlow(parsed.program), program: parsed.program, errors: parsed.errors }; +}; + +const findMarker = (root: EsTreeNode): EsTreeNode | null => { + let found: EsTreeNode | null = null; + const visit = (node: EsTreeNode): void => { + if (found) return; + if ( + node.type === "AssignmentExpression" && + (node as { left: EsTreeNode }).left.type === "Identifier" && + (node as { left: { name: string } }).left.name === "x" && + (node as { right: EsTreeNode }).right.type === "Literal" && + (node as { right: { value: unknown } }).right.value === 2 + ) { + found = node; + return; + } + const record = node as unknown as Record; + for (const key of Object.keys(record)) { + if (key === "parent") continue; + const child = record[key]; + if (Array.isArray(child)) { + for (const item of child) { + if (item && typeof item === "object" && "type" in item) visit(item as EsTreeNode); + } + } else if (child && typeof child === "object" && "type" in (child as object)) { + visit(child as EsTreeNode); + } + } + }; + visit(root); + return found; +}; + +// The `x = 2` marker is reachable. +const REACHABLE_FIXTURES: ReadonlyArray = [ + // `finally` runs even though `try` returns — the marker is in the finally body. + "function foo() { try { return; } finally { x = 2; } }", + // try completes normally, finally completes normally → after-try reachable. + "function foo() { try { ok(); } finally { cleanup(); } x = 2; }", + // try and catch both complete normally → after-try reachable. + "function foo() { try { mayThrow(); } catch (e) { handle(e); } finally { cleanup(); } x = 2; }", + // throw is caught and the catch falls through → after-try reachable. + "function foo() { try { throw e; } catch (err) { b(); } x = 2; }", + // catch returns, but the try body can still complete normally → reachable. + "function foo() { try { mayThrow(); } catch (e) { return; } x = 2; }", +]; + +// The `x = 2` marker is unreachable. +const UNREACHABLE_FIXTURES: ReadonlyArray = [ + // try returns; finally completes normally but resumes the return → after unreachable. + "function foo() { try { return; } finally { cleanup(); } x = 2; }", + // finally itself returns → after unreachable. + "function foo() { try { ok(); } finally { return; } x = 2; }", + // try and catch both return; no normal path survives → after unreachable. + "function foo() { try { return; } catch (e) { return; } x = 2; }", + // try + catch both return, finally normal: precise (oxc over-approximates to reachable). + "function foo() { try { return; } catch (e) { return; } finally { cleanup(); } x = 2; }", +]; + +describe("control-flow-graph: try/catch/finally reachability", () => { + describe("reachable marker", () => { + for (const fixture of REACHABLE_FIXTURES) { + it(fixture, () => { + const analysis = analyze(fixture); + expect(analysis.errors).toEqual([]); + const marker = findMarker(analysis.program); + expect(marker).not.toBeNull(); + expect(analysis.isUnreachable(marker!)).toBe(false); + }); + } + }); + + describe("unreachable marker", () => { + for (const fixture of UNREACHABLE_FIXTURES) { + it(fixture, () => { + const analysis = analyze(fixture); + expect(analysis.errors).toEqual([]); + const marker = findMarker(analysis.program); + expect(marker).not.toBeNull(); + expect(analysis.isUnreachable(marker!)).toBe(true); + }); + } + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.ts b/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.ts index 3a86760d2..b1dc7a92a 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.ts @@ -7,13 +7,27 @@ import { isNodeOfType } from "../utils/is-node-of-type.js"; // "Is this AST node guaranteed to execute on every call to its // enclosing function?" (isUnconditionalFromEntry — used by rules-of-hooks) // -// Edges have just two kinds: uncond (sequential fall-through) and cond -// (any conditional branch — true / false / loop / case / etc.). The -// finer-grained edge taxonomy in oxc_cfg matters for analyses we don't -// run (e.g. dominator construction in the IR optimizer); for -// rules-of-hooks the binary distinction is sufficient. - -export type CfgEdgeKind = "uncond" | "cond" | "throw"; +// Edge kinds, mapped from oxc_cfg's richer `EdgeType` taxonomy to the +// distinctions our analyses actually consume: +// uncond — sequential fall-through (oxc `Normal`) +// cond — a conditional branch: true / false / loop-enter / case +// (oxc `Jump` + the `Normal` else-arm; we don't split them +// because reachability/dominance weight every edge equally) +// backedge — a loop's back-edge to its header (oxc `Backedge`); the +// sole creator of cycles, so loop detection keys off it +// throw — an exception path to a catch/finally or the function exit +// (oxc `Error`); excluded from "normal completion" reachability +// finalize — entry into a `finally` block, taken on every path through +// the protected region — even a `return`/`throw` in `try` +// (oxc `Finalize`). An abrupt completion can't sever it, so +// the `finally` body stays reachable. +// join — the normal continuation after a `finally` completes, added +// only when the protected region can itself complete normally +// (oxc `Join`). Its absence is what makes code after +// `try { return } finally { … }` unreachable. +// oxc's `NewFunction` edge is absent by construction: every function gets +// its own CFG here, so reachability never crosses a function boundary. +export type CfgEdgeKind = "uncond" | "cond" | "throw" | "backedge" | "finalize" | "join"; export interface CfgEdge { readonly from: BasicBlock; @@ -39,7 +53,28 @@ export interface FunctionCfg { export interface ControlFlowAnalysis { readonly cfgFor: (functionLike: EsTreeNode) => FunctionCfg | null; readonly enclosingFunction: (node: EsTreeNode) => EsTreeNode | null; + // On every path from the enclosing function's entry to its exit. readonly isUnconditionalFromEntry: (node: EsTreeNode) => boolean; + // Some control-flow path lets execution flow from `fromNode` to + // `toNode` within the same enclosing function. Cross-function pairs + // are never reachable. + readonly isReachable: (fromNode: EsTreeNode, toNode: EsTreeNode) => boolean; + // `aNode` executes on EVERY path that reaches `bNode` (graph + // dominance). A guard that dominates a sink runs before it on every + // path. + readonly dominates: (aNode: EsTreeNode, bNode: EsTreeNode) => boolean; + // `bNode` executes on EVERY path from `aNode` to the function exit + // (graph post-dominance). A cleanup that post-dominates a + // subscription always runs after it. + readonly postDominates: (bNode: EsTreeNode, aNode: EsTreeNode) => boolean; + // The node's basic block is part of a cycle in ITS OWN function's CFG + // — i.e. it executes once per iteration of an enclosing loop. A node + // inside a callback that merely escapes a loop is NOT inside the loop + // (the callback is a separate function with its own acyclic CFG). + readonly isInsideLoop: (node: EsTreeNode) => boolean; + // The node's block is not reachable from the function entry (dead + // code after an unconditional return / throw / break). + readonly isUnreachable: (node: EsTreeNode) => boolean; } interface CfgBuilder { @@ -154,6 +189,24 @@ const findLabel = ( return null; }; +// A loop whose test is a compile-time truthy constant (`while (true)`, +// `do … while (1)`) — or a `for (;;)` with no test at all — never exits +// through its condition. We model that by omitting the header→merge +// "cond" edge, so code after the loop is reachable only via an explicit +// `break` (matching oxc's infinite-loop handling, which is what makes +// `while (true) {} after();` flag `after()` as unreachable). +const isAlwaysTruthyLoopTest = (test: EsTreeNode | null | undefined): boolean => { + if (!test) return true; + if (isNodeOfType(test, "Literal")) { + const literalValue = (test as { value?: unknown }).value; + if (typeof literalValue === "boolean") return literalValue; + if (typeof literalValue === "number") return literalValue !== 0; + if (typeof literalValue === "string") return literalValue.length > 0; + if (typeof literalValue === "bigint") return literalValue !== BigInt(0); + } + return false; +}; + // Process a list of statements inside a block. Returns the block where // fall-through control flow ends up. Caller is responsible for // connecting that to the next block (e.g. exit, merge). @@ -251,7 +304,7 @@ const buildStatement = ( if (isNodeOfType(statement, "ContinueStatement")) { const targetLabel = statement.label ? statement.label.name : null; const target = findLabel(builder, targetLabel); - if (target?.header) addEdge(current, target.header, "uncond"); + if (target?.header) addEdge(current, target.header, "backedge"); return createBlock(builder); } @@ -277,6 +330,7 @@ const buildStatement = ( if (isNodeOfType(statement, "WhileStatement") || isNodeOfType(statement, "DoWhileStatement")) { const isDoWhile = isNodeOfType(statement, "DoWhileStatement"); mapDescendantsToBlock(builder, statement.test as EsTreeNode, current); + const isInfinite = isAlwaysTruthyLoopTest(statement.test as EsTreeNode); const header = createBlock(builder); const body = createBlock(builder); const merge = createBlock(builder); @@ -286,18 +340,18 @@ const buildStatement = ( } else { addEdge(current, header, "uncond"); addEdge(header, body, "cond"); - addEdge(header, merge, "cond"); + if (!isInfinite) addEdge(header, merge, "cond"); } builder.loopStack.push({ header, merge, label: null }); const bodyEnd = buildStatement(builder, statement.body as EsTreeNode, body); builder.loopStack.pop(); if (isDoWhile) { // After body, test is evaluated → loop back or merge. - addEdge(bodyEnd, header, "uncond"); + addEdge(bodyEnd, header, "backedge"); addEdge(header, body, "cond"); - addEdge(header, merge, "cond"); + if (!isInfinite) addEdge(header, merge, "cond"); } else { - addEdge(bodyEnd, header, "uncond"); + addEdge(bodyEnd, header, "backedge"); } return merge; } @@ -305,23 +359,27 @@ const buildStatement = ( if (isNodeOfType(statement, "ForStatement")) { if (statement.init) mapDescendantsToBlock(builder, statement.init as EsTreeNode, current); if (statement.test) mapDescendantsToBlock(builder, statement.test as EsTreeNode, current); + // `for (;;)` (no test) and `for (; true;)` never exit via condition. + const isInfinite = isAlwaysTruthyLoopTest(statement.test as EsTreeNode | null); const header = createBlock(builder); const body = createBlock(builder); const merge = createBlock(builder); addEdge(current, header, "uncond"); addEdge(header, body, "cond"); - addEdge(header, merge, "cond"); + if (!isInfinite) addEdge(header, merge, "cond"); builder.loopStack.push({ header, merge, label: null }); const bodyEnd = buildStatement(builder, statement.body as EsTreeNode, body); builder.loopStack.pop(); if (statement.update) mapDescendantsToBlock(builder, statement.update as EsTreeNode, header); - addEdge(bodyEnd, header, "uncond"); + addEdge(bodyEnd, header, "backedge"); return merge; } if (isNodeOfType(statement, "ForInStatement") || isNodeOfType(statement, "ForOfStatement")) { mapDescendantsToBlock(builder, statement.right as EsTreeNode, current); mapDescendantsToBlock(builder, statement.left as EsTreeNode, current); + // A for-in / for-of iterates a (finite) collection, so the loop can + // always complete — the header→merge edge stays. const header = createBlock(builder); const body = createBlock(builder); const merge = createBlock(builder); @@ -331,7 +389,7 @@ const buildStatement = ( builder.loopStack.push({ header, merge, label: null }); const bodyEnd = buildStatement(builder, statement.body as EsTreeNode, body); builder.loopStack.pop(); - addEdge(bodyEnd, header, "uncond"); + addEdge(bodyEnd, header, "backedge"); return merge; } @@ -363,11 +421,13 @@ const buildStatement = ( if (isNodeOfType(statement, "TryStatement")) { const tryBlock = createBlock(builder); const merge = createBlock(builder); - let catchBlock: BasicBlock | null = null; - let finallyBlock: BasicBlock | null = null; - if (statement.handler) catchBlock = createBlock(builder); - if (statement.finalizer) finallyBlock = createBlock(builder); + const catchBlock = statement.handler ? createBlock(builder) : null; + const finallyBlock = statement.finalizer ? createBlock(builder) : null; addEdge(current, tryBlock, "uncond"); + + // Try body. A throw anywhere inside is modeled by a "cond" edge to + // catch (conditionally reached, like any branch — keeps catch out of + // the "every normal path" set without making it dead). builder.tryStack.push({ catch: catchBlock, finally: finallyBlock }); const tryEnd = buildStatements( builder, @@ -375,25 +435,53 @@ const buildStatement = ( tryBlock, ); builder.tryStack.pop(); - // Try block can throw at any point — model with a cond edge to catch. if (catchBlock) addEdge(tryBlock, catchBlock, "cond"); + const tryCompletesNormally = completesNormally(tryBlock, tryEnd); + + let catchEnd: BasicBlock | null = null; if (statement.handler && catchBlock) { - const handlerBody = (statement.handler as { body: EsTreeNode }).body; - const catchEnd = buildStatement(builder, handlerBody, catchBlock); - if (finallyBlock) addEdge(catchEnd, finallyBlock, "uncond"); - else addEdge(catchEnd, merge, "uncond"); + catchEnd = buildStatement( + builder, + (statement.handler as { body: EsTreeNode }).body, + catchBlock, + ); } + const catchCompletesNormally = catchEnd !== null && completesNormally(catchBlock!, catchEnd); + + // The try STATEMENT can complete normally when its body does, or — + // if the throw is caught — when the catch body does. + const protectedCompletesNormally = catchBlock + ? tryCompletesNormally || catchCompletesNormally + : tryCompletesNormally; + if (finallyBlock && statement.finalizer) { - addEdge(tryEnd, finallyBlock, "uncond"); + // `finally` runs on every path through the region — wire it from + // the region entries so a `return`/`throw` in try/catch can't make + // it unreachable. Also connect the normal ends so dominance sees + // the in-order path. + addEdge(tryBlock, finallyBlock, "finalize"); + if (catchBlock) addEdge(catchBlock, finallyBlock, "finalize"); + if (tryCompletesNormally && tryEnd !== tryBlock) addEdge(tryEnd, finallyBlock, "finalize"); + if (catchEnd && catchCompletesNormally && catchEnd !== catchBlock) { + addEdge(catchEnd, finallyBlock, "finalize"); + } const finallyEnd = buildStatements( builder, (statement.finalizer as { body: ReadonlyArray }).body, finallyBlock, ); - addEdge(finallyEnd, merge, "uncond"); - } else { - addEdge(tryEnd, merge, "uncond"); + // Resume-after-finally: code after the try is reachable only if the + // protected region could complete normally. (If `finally` itself + // completes abruptly, `finallyEnd` is an orphan and this join is + // dead regardless.) + if (protectedCompletesNormally) addEdge(finallyEnd, merge, "join"); + return merge; } + + // No finally: after-try is reached from whichever of try / catch can + // complete normally. + if (tryCompletesNormally) addEdge(tryEnd, merge, "uncond"); + if (catchEnd && catchCompletesNormally) addEdge(catchEnd, merge, "uncond"); return merge; } @@ -494,9 +582,195 @@ const computeUnconditionalSet = (cfg: FunctionCfg): Set => { return unconditional; }; +// Blocks reachable from entry over EVERY edge kind (including catch +// edges). Used to answer `isUnreachable` — a block with no path from +// entry is dead code. +const computeReachableFromEntry = (cfg: FunctionCfg): Set => { + const visited = new Set(); + const queue: BasicBlock[] = [cfg.entry]; + while (queue.length > 0) { + const block = queue.shift()!; + if (visited.has(block)) continue; + visited.add(block); + for (const edge of block.successors) queue.push(edge.to); + } + return visited; +}; + +// Standard iterative dominator dataflow: dom(entry) = {entry}; +// dom(b) = {b} ∪ (⋂ dom(p) for predecessors p). `a` dominates `b` iff +// `a ∈ dom(b)`. O(blocks²) — fine for function-sized graphs. +const computeDominators = (cfg: FunctionCfg): Map> => { + const dominators = new Map>(); + for (const block of cfg.blocks) { + dominators.set(block, block === cfg.entry ? new Set([cfg.entry]) : new Set(cfg.blocks)); + } + let didChange = true; + while (didChange) { + didChange = false; + for (const block of cfg.blocks) { + if (block === cfg.entry) continue; + let intersection: Set | null = null; + for (const edge of block.predecessors) { + const predecessorDominators = dominators.get(edge.from)!; + if (intersection === null) { + intersection = new Set(predecessorDominators); + } else { + for (const candidate of intersection) { + if (!predecessorDominators.has(candidate)) intersection.delete(candidate); + } + } + } + const nextDominators = intersection ?? new Set(); + nextDominators.add(block); + const previousDominators = dominators.get(block)!; + if (!areSetsEqual(previousDominators, nextDominators)) { + dominators.set(block, nextDominators); + didChange = true; + } + } + } + return dominators; +}; + +// Post-dominators are dominators on the reversed graph: postDom(exit) = +// {exit}; postDom(b) = {b} ∪ (⋂ postDom(s) for successors s). `b` +// post-dominates `a` iff `b ∈ postDom(a)`. +const computePostDominators = (cfg: FunctionCfg): Map> => { + const postDominators = new Map>(); + for (const block of cfg.blocks) { + postDominators.set(block, block === cfg.exit ? new Set([cfg.exit]) : new Set(cfg.blocks)); + } + let didChange = true; + while (didChange) { + didChange = false; + for (const block of cfg.blocks) { + if (block === cfg.exit) continue; + let intersection: Set | null = null; + for (const edge of block.successors) { + const successorPostDominators = postDominators.get(edge.to)!; + if (intersection === null) { + intersection = new Set(successorPostDominators); + } else { + for (const candidate of intersection) { + if (!successorPostDominators.has(candidate)) intersection.delete(candidate); + } + } + } + const nextPostDominators = intersection ?? new Set(); + nextPostDominators.add(block); + const previousPostDominators = postDominators.get(block)!; + if (!areSetsEqual(previousPostDominators, nextPostDominators)) { + postDominators.set(block, nextPostDominators); + didChange = true; + } + } + } + return postDominators; +}; + +// A block is on a cycle iff it can reach itself by following non-throw +// successor edges (loop back-edges are normal "uncond" edges; a +// throw→catch edge is not a loop). +const computeCyclicBlocks = (cfg: FunctionCfg): Set => { + const cyclicBlocks = new Set(); + for (const startBlock of cfg.blocks) { + const visited = new Set(); + const queue: BasicBlock[] = []; + for (const edge of startBlock.successors) { + if (edge.kind !== "throw") queue.push(edge.to); + } + let isOnCycle = false; + while (queue.length > 0) { + const block = queue.shift()!; + if (block === startBlock) { + isOnCycle = true; + break; + } + if (visited.has(block)) continue; + visited.add(block); + for (const edge of block.successors) { + if (edge.kind !== "throw") queue.push(edge.to); + } + } + if (isOnCycle) cyclicBlocks.add(startBlock); + } + return cyclicBlocks; +}; + +// Source-order index for every node owned by this function (not +// descending into nested functions). Used to break ties for two nodes +// that share a basic block: within a straight-line block the earlier +// node dominates the later one. +const computeNodeOrder = (functionNode: EsTreeNode, body: EsTreeNode): Map => { + const nodeOrder = new Map(); + let nextOrder = 0; + const walk = (node: EsTreeNode): void => { + if (!nodeOrder.has(node)) nodeOrder.set(node, nextOrder++); + if (node !== functionNode && isFunctionLike(node)) return; + const record = node as unknown as Record; + for (const key of Object.keys(record)) { + if (key === "parent") continue; + const child = record[key]; + if (Array.isArray(child)) { + for (const item of child) if (isAstNode(item)) walk(item); + } else if (isAstNode(child)) { + walk(child); + } + } + }; + walk(body); + return nodeOrder; +}; + +const areSetsEqual = (first: Set, second: Set): boolean => { + if (first.size !== second.size) return false; + for (const value of first) if (!second.has(value)) return false; + return true; +}; + +const isBlockReachableFromBlock = ( + fromBlock: BasicBlock, + toBlock: BasicBlock, + includeEdge: (edge: CfgEdge) => boolean = () => true, +): boolean => { + const visited = new Set(); + const queue: BasicBlock[] = [fromBlock]; + while (queue.length > 0) { + const block = queue.shift()!; + for (const edge of block.successors) { + if (!includeEdge(edge)) continue; + if (edge.to === toBlock) return true; + if (!visited.has(edge.to)) { + visited.add(edge.to); + queue.push(edge.to); + } + } + } + return false; +}; + +// A protected region (the `try` body or a `catch` body) "completes +// normally" iff its end block is reachable from its entry without +// leaving via an exception (`throw`) or diverting into the `finally` +// (`finalize`). `return` / `throw` / `break` route elsewhere and strand +// the region end as an orphan, so it stays unreachable here. +const completesNormally = (regionEntry: BasicBlock, regionEnd: BasicBlock): boolean => + regionEntry === regionEnd || + isBlockReachableFromBlock( + regionEntry, + regionEnd, + (edge) => edge.kind !== "throw" && edge.kind !== "finalize", + ); + interface FunctionCfgEntry { cfg: FunctionCfg; unconditionalSet: Set; + dominators: Map>; + postDominators: Map>; + cyclicBlocks: Set; + reachableFromEntry: Set; + nodeOrder: Map; } // Walks the AST building a CFG for every function-like node + the @@ -511,6 +785,11 @@ export const analyzeControlFlow = (program: EsTreeNode): ControlFlowAnalysis => functionCfgs.set(functionNode, { cfg, unconditionalSet: computeUnconditionalSet(cfg), + dominators: computeDominators(cfg), + postDominators: computePostDominators(cfg), + cyclicBlocks: computeCyclicBlocks(cfg), + reachableFromEntry: computeReachableFromEntry(cfg), + nodeOrder: computeNodeOrder(functionNode, body), }); }; @@ -567,9 +846,83 @@ export const analyzeControlFlow = (program: EsTreeNode): ControlFlowAnalysis => return entry.unconditionalSet.has(block); }; + interface LocatedNode { + owner: EsTreeNode; + entry: FunctionCfgEntry; + block: BasicBlock; + } + + const locate = (node: EsTreeNode): LocatedNode | null => { + const owner = enclosingFunction(node); + if (!owner) return null; + const entry = functionCfgs.get(owner); + if (!entry) return null; + const block = entry.cfg.blockOf(node); + if (!block) return null; + return { owner, entry, block }; + }; + + const isReachable = (fromNode: EsTreeNode, toNode: EsTreeNode): boolean => { + const from = locate(fromNode); + const to = locate(toNode); + if (!from || !to || from.owner !== to.owner) return false; + if (from.block === to.block) { + if (from.entry.cyclicBlocks.has(from.block)) return true; + const fromOrder = from.entry.nodeOrder.get(fromNode) ?? 0; + const toOrder = to.entry.nodeOrder.get(toNode) ?? 0; + return fromOrder <= toOrder; + } + return isBlockReachableFromBlock(from.block, to.block); + }; + + const dominates = (aNode: EsTreeNode, bNode: EsTreeNode): boolean => { + const dominator = locate(aNode); + const dominated = locate(bNode); + if (!dominator || !dominated || dominator.owner !== dominated.owner) return false; + if (dominator.block === dominated.block) { + const aOrder = dominator.entry.nodeOrder.get(aNode) ?? 0; + const bOrder = dominated.entry.nodeOrder.get(bNode) ?? 0; + return aOrder <= bOrder; + } + return dominated.entry.dominators.get(dominated.block)?.has(dominator.block) ?? false; + }; + + const postDominates = (bNode: EsTreeNode, aNode: EsTreeNode): boolean => { + const postDominator = locate(bNode); + const postDominated = locate(aNode); + if (!postDominator || !postDominated || postDominator.owner !== postDominated.owner) { + return false; + } + if (postDominator.block === postDominated.block) { + const bOrder = postDominator.entry.nodeOrder.get(bNode) ?? 0; + const aOrder = postDominated.entry.nodeOrder.get(aNode) ?? 0; + return bOrder >= aOrder; + } + return ( + postDominated.entry.postDominators.get(postDominated.block)?.has(postDominator.block) ?? false + ); + }; + + const isInsideLoop = (node: EsTreeNode): boolean => { + const located = locate(node); + if (!located) return false; + return located.entry.cyclicBlocks.has(located.block); + }; + + const isUnreachable = (node: EsTreeNode): boolean => { + const located = locate(node); + if (!located) return false; + return !located.entry.reachableFromEntry.has(located.block); + }; + return { cfgFor, enclosingFunction, isUnconditionalFromEntry, + isReachable, + dominates, + postDominates, + isInsideLoop, + isUnreachable, }; }; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/create-loop-aware-visitors.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/create-loop-aware-visitors.ts index e7d7324db..e7b8b78a3 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/create-loop-aware-visitors.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/create-loop-aware-visitors.ts @@ -1,5 +1,5 @@ -import { LOOP_TYPES } from "../constants/js.js"; import type { EsTreeNode } from "./es-tree-node.js"; +import type { RuleContext } from "./rule-context.js"; import type { RuleVisitors } from "./rule-visitors.js"; // HACK: handlers accept narrower node types (e.g. `NewExpression`) than @@ -8,27 +8,22 @@ import type { RuleVisitors } from "./rule-visitors.js"; // the visitor type erase at the call site. type LoopVisitor = (node: never) => void; +// Forwards each inner visitor only when the visited node executes once +// per iteration of an enclosing loop in ITS OWN function. Uses the CFG's +// `isInsideLoop` (cycle membership) rather than a lexical loop-nesting +// counter, so a node inside a callback that merely escapes a loop +// (`for (...) { el.onclick = () => new RegExp(x); }`) is correctly NOT +// treated as in-loop — the callback is a separate function with its own +// acyclic CFG. export const createLoopAwareVisitors = ( + context: RuleContext, innerVisitors: Record, ): RuleVisitors => { - let loopDepth = 0; - const incrementLoopDepth = (): void => { - loopDepth++; - }; - const decrementLoopDepth = (): void => { - loopDepth--; - }; - const visitors: RuleVisitors = {}; - for (const loopType of LOOP_TYPES) { - visitors[loopType] = incrementLoopDepth; - visitors[`${loopType}:exit`] = decrementLoopDepth; - } - for (const [nodeType, handler] of Object.entries(innerVisitors)) { visitors[nodeType] = (node: EsTreeNode) => { - if (loopDepth > 0) (handler as (input: EsTreeNode) => void)(node); + if (context.cfg.isInsideLoop(node)) (handler as (input: EsTreeNode) => void)(node); }; } diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/wrap-with-semantic-context.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/wrap-with-semantic-context.ts index ea7f7addc..19f571f9e 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/wrap-with-semantic-context.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/wrap-with-semantic-context.ts @@ -52,6 +52,11 @@ const FALLBACK_CFG: ControlFlowAnalysis = { cfgFor: () => null, enclosingFunction: () => null, isUnconditionalFromEntry: () => false, + isReachable: () => false, + dominates: () => false, + postDominates: () => false, + isInsideLoop: () => false, + isUnreachable: () => false, }; export const wrapWithSemanticContext = (rule: Rule): HostRule => ({ From 9d55d8704924400c12cc6a171949c7d36e3ca87f Mon Sep 17 00:00:00 2001 From: Aiden Bai Date: Fri, 19 Jun 2026 02:07:01 -0700 Subject: [PATCH 02/13] feat(oxlint-plugin): lower expression-level control flow into the CFG; add cleanup-path verifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Model the React Compiler HIR's expression terminals — ternary, logical (&&/||/?? and &&=/||=/??=), and optional chaining — as basic blocks so a hook / setState / effect nested in a short-circuit reads as conditional. Rewires if-test, return/throw arguments, switch discriminant, plain statements, and arrow bodies through the new lowering, gated by a cheap containsExpressionControlFlow check to keep straight-line code on the fast path. - new rule effect-cleanup-not-on-every-path: flags a timer/subscribe acquisition whose cleanup is bypassed by an early return on some path, via cfg.isReachable - generalize no-set-state-in-render to any setter cfg.isUnconditionalFromEntry proves runs on every render path (assignments, blocks), scoped to the component body via walkInsideStatementBlocks --- .changeset/feat-cfg-react-verifier.md | 12 + .../src/plugin/rule-registry.ts | 13 + .../effect-cleanup-not-on-every-path.test.ts | 173 +++++++++++ .../effect-cleanup-not-on-every-path.ts | 123 ++++++++ .../no-set-state-in-render.test.ts | 110 +++++++ .../no-set-state-in-render.ts | 53 ++-- ...control-flow-graph.expression-flow.test.ts | 238 +++++++++++++++ .../control-flow-graph.regression.test.ts | 222 ++++++++++++++ .../src/plugin/semantic/control-flow-graph.ts | 279 ++++++++++++++++-- .../src/test-utils/run-cfg.ts | 162 ++++++++++ 10 files changed, 1338 insertions(+), 47 deletions(-) create mode 100644 .changeset/feat-cfg-react-verifier.md create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-cleanup-not-on-every-path.test.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-cleanup-not-on-every-path.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-set-state-in-render.test.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.expression-flow.test.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/semantic/control-flow-graph.regression.test.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/test-utils/run-cfg.ts diff --git a/.changeset/feat-cfg-react-verifier.md b/.changeset/feat-cfg-react-verifier.md new file mode 100644 index 000000000..bc9aa7c43 --- /dev/null +++ b/.changeset/feat-cfg-react-verifier.md @@ -0,0 +1,12 @@ +--- +"oxlint-plugin-react-doctor": minor +--- + +Turn the control-flow graph into a React verifier: model expression-level control flow and add a path-sensitive effect-leak rule. + +The CFG now lowers expression-level control flow the way the React Compiler's HIR does — a ternary's arms, a `&&` / `||` / `??` (and `&&=` / `||=` / `??=`) right operand, and an optional chain's links past each `?.` all get their own basic blocks. A hook / `setState` / effect short-circuited inside any of those is now correctly seen as conditional, which statement-level lowering alone could not see. + +Two rules use it as a verifier: + +- New `effect-cleanup-not-on-every-path`: flags a subscription/timer acquired in an effect whose cleanup is skipped on an early-return path (`const id = setInterval(…); if (skip) return; return () => clearInterval(id)` leaks on the `skip` path). This is a reachability question no AST shape can answer — it complements `effect-needs-cleanup` (which only checks a cleanup exists at all) and stays quiet when the guard runs before the acquisition or when every return path cleans up. +- `no-set-state-in-render` now flags any setter the CFG proves runs on every render path (`isUnconditionalFromEntry`), not just a bare top-level statement — so `const x = setCount(c + 1)` and unconditional blocks are caught, while the guarded store-previous-render fixed-point pattern (`if (prev !== count) setPrev(count)`) stays quiet. 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 081d204e6..ae3e834ba 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rule-registry.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rule-registry.ts @@ -48,6 +48,7 @@ import { noThreePeriodEllipsis } from "./rules/react-ui/no-three-period-ellipsis import { noVagueButtonLabel } from "./rules/react-ui/no-vague-button-label.js"; import { dialogHasAccessibleName } from "./rules/a11y/dialog-has-accessible-name.js"; import { displayName } from "./rules/react-builtins/display-name.js"; +import { effectCleanupNotOnEveryPath } from "./rules/state-and-effects/effect-cleanup-not-on-every-path.js"; import { effectNeedsCleanup } from "./rules/state-and-effects/effect-needs-cleanup.js"; import { exhaustiveDeps } from "./rules/react-builtins/exhaustive-deps.js"; import { expoNoNonInlinedEnv } from "./rules/react-native/expo-no-non-inlined-env.js"; @@ -876,6 +877,18 @@ export const reactDoctorRules = [ requires: [...new Set(["react", ...(displayName.requires ?? [])])], }, }, + { + key: "react-doctor/effect-cleanup-not-on-every-path", + id: "effect-cleanup-not-on-every-path", + source: "react-doctor", + originallyExternal: false, + rule: { + ...effectCleanupNotOnEveryPath, + framework: "global", + category: "Bugs", + requires: [...new Set(["react", ...(effectCleanupNotOnEveryPath.requires ?? [])])], + }, + }, { key: "react-doctor/effect-needs-cleanup", id: "effect-needs-cleanup", diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-cleanup-not-on-every-path.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-cleanup-not-on-every-path.test.ts new file mode 100644 index 000000000..f31937700 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-cleanup-not-on-every-path.test.ts @@ -0,0 +1,173 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runRule } from "../../../test-utils/run-rule.js"; +import { effectCleanupNotOnEveryPath } from "./effect-cleanup-not-on-every-path.js"; + +const run = (code: string) => + runRule(effectCleanupNotOnEveryPath, code, { filename: "fixture.tsx" }); + +describe("effect-cleanup-not-on-every-path", () => { + it("flags a listener acquired before an early bare return, with cleanup at the end", () => { + const result = run(` + import { useEffect } from "react"; + function Component({ url, skip }) { + useEffect(() => { + const socket = new WebSocket(url); + socket.addEventListener("message", handler); + if (skip) return; + return () => socket.removeEventListener("message", handler); + }); + return null; + } + `); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("addEventListener"); + }); + + it("flags a timer leaked on a conditional early return", () => { + const result = run(` + import { useEffect } from "react"; + function Component({ paused }) { + useEffect(() => { + const id = setInterval(tick, 1000); + if (paused) { + return; + } + return () => clearInterval(id); + }); + return null; + } + `); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("setInterval"); + }); + + it("flags when the early return is `return null`", () => { + const result = run(` + import { useEffect } from "react"; + function Component({ enabled }) { + useEffect(() => { + const sub = store.subscribe(onChange); + if (!enabled) return null; + return () => sub.unsubscribe(); + }); + return null; + } + `); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("subscribe"); + }); + + it("stays quiet when the guard runs BEFORE the acquisition (cleanup post-dominates)", () => { + const result = run(` + import { useEffect } from "react"; + function Component({ enabled }) { + useEffect(() => { + if (!enabled) return; + const id = setInterval(tick, 1000); + return () => clearInterval(id); + }); + return null; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("stays quiet when there is no early return at all", () => { + const result = run(` + import { useEffect } from "react"; + function Component({ url }) { + useEffect(() => { + const socket = new WebSocket(url); + socket.addEventListener("message", handler); + return () => socket.removeEventListener("message", handler); + }); + return null; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("stays quiet when there is no cleanup at all (effect-needs-cleanup's job)", () => { + const result = run(` + import { useEffect } from "react"; + function Component({ skip }) { + useEffect(() => { + const id = setInterval(tick, 1000); + if (skip) return; + }); + return null; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("stays quiet when every early return path itself returns a cleanup", () => { + const result = run(` + import { useEffect } from "react"; + function Component({ mode }) { + useEffect(() => { + const id = setInterval(tick, 1000); + if (mode) { + return () => clearInterval(id); + } + return () => clearInterval(id); + }); + return null; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("stays quiet when the acquisition is inside a nested handler (separate function)", () => { + const result = run(` + import { useEffect } from "react"; + function Component({ skip }) { + useEffect(() => { + button.onclick = () => { + const id = setInterval(tick, 1000); + }; + if (skip) return; + return () => teardown(); + }); + return null; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("ignores non-effect hooks", () => { + const result = run(` + import { useMemo } from "react"; + function Component({ skip }) { + const value = useMemo(() => { + const id = setInterval(tick, 1000); + if (skip) return; + return () => clearInterval(id); + }, [skip]); + return value; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("flags the acquisition leaked by a return nested in a branch after it", () => { + const result = run(` + import { useEffect } from "react"; + function Component({ a, b }) { + useEffect(() => { + window.addEventListener("resize", onResize); + if (a) { + if (b) { + return; + } + } + return () => window.removeEventListener("resize", onResize); + }); + return null; + } + `); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("addEventListener"); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-cleanup-not-on-every-path.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-cleanup-not-on-every-path.ts new file mode 100644 index 000000000..888e40470 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-cleanup-not-on-every-path.ts @@ -0,0 +1,123 @@ +import { TIMER_CALLEE_NAMES_REQUIRING_CLEANUP } from "../../constants/dom.js"; +import { EFFECT_HOOK_NAMES } from "../../constants/react.js"; +import { defineRule } from "../../utils/define-rule.js"; +import type { EsTreeNode } from "../../utils/es-tree-node.js"; +import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; +import { getEffectCallback } from "../../utils/get-effect-callback.js"; +import { isHookCall } from "../../utils/is-hook-call.js"; +import { isNodeOfType } from "../../utils/is-node-of-type.js"; +import type { RuleContext } from "../../utils/rule-context.js"; +import { walkInsideStatementBlocks } from "../../utils/walk-inside-statement-blocks.js"; +import { isSubscribeLikeCallExpression } from "./utils/is-subscribe-like-call-expression.js"; + +interface ResourceAcquisition { + readonly node: EsTreeNode; + readonly resourceName: string; +} + +const getTimerAcquisitionName = (node: EsTreeNode): string | null => { + if (!isNodeOfType(node, "CallExpression")) return null; + if (!isNodeOfType(node.callee, "Identifier")) return null; + return TIMER_CALLEE_NAMES_REQUIRING_CLEANUP.has(node.callee.name) ? node.callee.name : null; +}; + +const getSubscribeAcquisitionName = (node: EsTreeNode): string | null => { + if (!isSubscribeLikeCallExpression(node)) return null; + if (!isNodeOfType(node, "CallExpression")) return null; + if (!isNodeOfType(node.callee, "MemberExpression")) return null; + if (!isNodeOfType(node.callee.property, "Identifier")) return null; + return node.callee.property.name; +}; + +// Resources acquired synchronously in the effect body (a timer or a +// subscribe-like registration). Mirrors `effect-needs-cleanup`'s notion of +// an acquisition so the two rules agree on what must be cleaned up — this +// one only fires when a cleanup EXISTS but is bypassed on some path. +const collectAcquisitions = (effectBody: EsTreeNode): ResourceAcquisition[] => { + const acquisitions: ResourceAcquisition[] = []; + walkInsideStatementBlocks(effectBody, (child) => { + const timerName = getTimerAcquisitionName(child); + if (timerName) { + acquisitions.push({ node: child, resourceName: timerName }); + return; + } + const subscribeName = getSubscribeAcquisitionName(child); + if (subscribeName) acquisitions.push({ node: child, resourceName: subscribeName }); + }); + return acquisitions; +}; + +const isNullishLiteralOrIdentifier = (node: EsTreeNode): boolean => { + if (isNodeOfType(node, "Identifier")) return node.name === "undefined"; + if (isNodeOfType(node, "Literal")) return node.value === null; + return false; +}; + +// A `return;` / `return null` / `return undefined` that leaves the effect +// WITHOUT handing React a cleanup function — i.e. a path that skips cleanup. +const isCleanupSkippingReturn = (node: EsTreeNode): boolean => { + if (!isNodeOfType(node, "ReturnStatement")) return false; + if (!node.argument) return true; + return isNullishLiteralOrIdentifier(node.argument as EsTreeNode); +}; + +// A `return ` whose value is a cleanup the effect handed back. +// We treat any non-nullish returned value as a cleanup-bearing path (the +// conservative direction — we only accuse the empty early returns). +const isCleanupBearingReturn = (node: EsTreeNode): boolean => + isNodeOfType(node, "ReturnStatement") && + Boolean(node.argument) && + !isNullishLiteralOrIdentifier(node.argument as EsTreeNode); + +const collectOwnReturns = (effectBody: EsTreeNode): EsTreeNode[] => { + const returns: EsTreeNode[] = []; + walkInsideStatementBlocks(effectBody, (child) => { + if (isNodeOfType(child, "ReturnStatement")) returns.push(child); + }); + return returns; +}; + +export const effectCleanupNotOnEveryPath = defineRule({ + id: "effect-cleanup-not-on-every-path", + title: "Effect cleanup skipped on some paths", + severity: "warn", + tags: ["test-noise"], + recommendation: + "Acquire the resource AFTER any early `return`, or clean it up on every path. An early `return` that runs after the subscription/timer is created — but before the cleanup is returned — leaks it on that path.", + create: (context: RuleContext) => ({ + CallExpression(node: EsTreeNodeOfType<"CallExpression">) { + if (!isHookCall(node, EFFECT_HOOK_NAMES)) return; + const callback = getEffectCallback(node); + if (!callback) return; + if ( + !isNodeOfType(callback, "ArrowFunctionExpression") && + !isNodeOfType(callback, "FunctionExpression") + ) { + return; + } + if (!isNodeOfType(callback.body, "BlockStatement")) return; + + const acquisitions = collectAcquisitions(callback.body); + if (acquisitions.length === 0) return; + + const returns = collectOwnReturns(callback.body); + // Only fire when the effect DOES return a cleanup somewhere — a total + // absence of cleanup is `effect-needs-cleanup`'s job, not ours. + if (!returns.some(isCleanupBearingReturn)) return; + const cleanupSkippingReturns = returns.filter(isCleanupSkippingReturn); + if (cleanupSkippingReturns.length === 0) return; + + for (const acquisition of acquisitions) { + const leakingReturn = cleanupSkippingReturns.find((returnStatement) => + context.cfg.isReachable(acquisition.node, returnStatement), + ); + if (!leakingReturn) continue; + context.report({ + node: acquisition.node, + message: `\`${acquisition.resourceName}\` is created here, but a later \`return\` exits before the cleanup runs on some path — leaking it. Move the acquisition after that early return, or clean up on every path.`, + }); + return; + } + }, + }), +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-set-state-in-render.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-set-state-in-render.test.ts new file mode 100644 index 000000000..33ebb107c --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-set-state-in-render.test.ts @@ -0,0 +1,110 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runRule } from "../../../test-utils/run-rule.js"; +import { noSetStateInRender } from "./no-set-state-in-render.js"; + +const run = (code: string) => runRule(noSetStateInRender, code, { filename: "fixture.tsx" }); + +describe("no-set-state-in-render", () => { + it("flags a bare top-level setter call in render", () => { + const result = run(` + import { useState } from "react"; + function Component() { + const [count, setCount] = useState(0); + setCount(count + 1); + return null; + } + `); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("setCount()"); + }); + + it("flags an unconditional setter assigned to a variable (not a bare statement)", () => { + const result = run(` + import { useState } from "react"; + function Component() { + const [count, setCount] = useState(0); + const ignored = setCount(count + 1); + return null; + } + `); + expect(result.diagnostics).toHaveLength(1); + }); + + it("flags an unconditional setter nested in a plain block", () => { + const result = run(` + import { useState } from "react"; + function Component() { + const [count, setCount] = useState(0); + { + setCount(count + 1); + } + return null; + } + `); + expect(result.diagnostics).toHaveLength(1); + }); + + it("stays quiet on the guarded store-previous-render fixed-point pattern", () => { + const result = run(` + import { useState } from "react"; + function Component({ count }) { + const [prevCount, setPrevCount] = useState(count); + if (prevCount !== count) { + setPrevCount(count); + } + return null; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("stays quiet when the setter runs only after an early return", () => { + const result = run(` + import { useState } from "react"; + function Component({ ready }) { + const [count, setCount] = useState(0); + if (!ready) return null; + setCount(count + 1); + return null; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("stays quiet when the setter is inside an effect", () => { + const result = run(` + import { useState, useEffect } from "react"; + function Component() { + const [count, setCount] = useState(0); + useEffect(() => { + setCount(count + 1); + }, [count]); + return null; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("stays quiet when the setter is inside an event handler", () => { + const result = run(` + import { useState } from "react"; + function Component() { + const [count, setCount] = useState(0); + const onClick = () => setCount(count + 1); + return ;\n}\n\nexport default Counter;\n", + "expectedToFlag": false + } +] From d1b6230cf06ef05a38ce0492bb955fa90fd2e02c Mon Sep 17 00:00:00 2001 From: Rayhan Noufal Arayilakath Date: Sun, 21 Jun 2026 01:42:45 -0700 Subject: [PATCH 12/13] fix(no-use-before-define): flag self-referential initializer TDZ reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor Bugbot (PR #892), finding "Self init TDZ missed": the offset gate skipped any access at or after the declaration, so a read inside the binding's own initializer (`const x = x`) was missed — yet it always hits the TDZ. Flag a read that lies within its own declarator's initializer (still let through for a closure that runs later, e.g. `const x = () => x`, via the existing deferred-boundary check). Bugbot's sibling "typeof exempt from TDZ" finding is NOT a bug and is deliberately left as-is: `typeof` is exempt only for never-declared names; `typeof` of a let/const in the TDZ still throws a ReferenceError (verified), so flagging it is correct. Added tests pin both behaviors. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../correctness/no-use-before-define.test.ts | 49 +++++++++++++++++++ .../rules/correctness/no-use-before-define.ts | 38 +++++++++++++- 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-use-before-define.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-use-before-define.test.ts index 13ae32618..d4d79d8bc 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-use-before-define.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-use-before-define.test.ts @@ -234,4 +234,53 @@ describe("no-use-before-define", () => { `); expect(result.diagnostics).toHaveLength(1); }); + + it("flags a self-referential initializer read (`const x = x`)", () => { + // The initializer reads the binding while it is still in its own TDZ — its + // offset follows the declaration's, so the offset gate alone would miss it. + const result = run(` + function f() { + const x = x; + return x; + } + `); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].nodeType).toBe("Identifier"); + }); + + it("flags a `typeof` read of a block-scoped binding in its TDZ (typeof still throws)", () => { + // `typeof` is exempt from ReferenceError ONLY for never-declared names; a + // let/const in the TDZ still throws, so this is a true positive. + const result = run(` + function f() { + const probe = typeof value; + let value = 1; + return probe + value; + } + `); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); + + it("does NOT flag `typeof` of a never-declared name (safe, evaluates to 'undefined')", () => { + const result = run(` + function f() { + return typeof neverDeclaredAnywhere; + } + `); + expect(result.diagnostics).toEqual([]); + }); + + it("does NOT flag a closure inside an initializer that runs later (`const x = () => x`)", () => { + // The read sits in a nested arrow that executes after the binding is bound, + // so it never hits the TDZ — the deferred-boundary check must still let it through. + const result = run(` + function f() { + const x = () => x; + return x; + } + `); + expect(result.diagnostics).toEqual([]); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-use-before-define.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-use-before-define.ts index c30ba8354..f32174af5 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-use-before-define.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-use-before-define.ts @@ -1,6 +1,8 @@ import { defineRule } from "../../utils/define-rule.js"; +import type { EsTreeNode } from "../../utils/es-tree-node.js"; import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; import { isDeclarationFile } from "../../utils/is-declaration-file.js"; +import { isFunctionLike } from "../../utils/is-function-like.js"; import { isNodeOfType } from "../../utils/is-node-of-type.js"; import { nodeStart } from "../../utils/node-start.js"; import type { RuleContext } from "../../utils/rule-context.js"; @@ -53,6 +55,32 @@ const crossesDeferredBoundary = ( return false; }; +// True when `accessNode` reads the binding from inside the binding's OWN +// initializer (`const x = x`, `const x = x.y`). The byte offset of such a read +// is AFTER the declaration's, so the offset gate would skip it — but the +// binding is uninitialized until its initializer finishes evaluating, so the +// read always hits the Temporal Dead Zone. (A read in a closure inside the +// initializer — `const x = () => x` — is still let through here and filtered by +// the deferred-boundary check, since the closure runs later.) +const isWithinOwnInitializer = (accessNode: EsTreeNode, declarationNode: EsTreeNode): boolean => { + let current: EsTreeNode | null | undefined = declarationNode; + while (current && !isNodeOfType(current, "VariableDeclarator")) { + if (isNodeOfType(current, "VariableDeclaration") || isFunctionLike(current)) return false; + current = current.parent; + } + if (!current || !isNodeOfType(current, "VariableDeclarator")) return false; + const initializer = current.init as EsTreeNode | null | undefined; + if (!initializer) return false; + // oxc carries byte offsets as `start` / `end` (never `range`), so compare + // structurally via `nodeStart` and the matching `end`. + const initializerStart = nodeStart(initializer); + const initializerEnd = + "end" in initializer && typeof initializer.end === "number" ? initializer.end : -1; + const accessStart = nodeStart(accessNode); + if (initializerStart < 0 || initializerEnd < 0 || accessStart < 0) return false; + return accessStart >= initializerStart && accessStart < initializerEnd; +}; + export const noUseBeforeDefine = defineRule({ id: "no-use-before-define", title: "Variable used before its declaration (Temporal Dead Zone)", @@ -80,7 +108,15 @@ export const noUseBeforeDefine = defineRule({ const accessStart = nodeStart(node); const declarationStart = nodeStart(symbol.declarationNode); if (accessStart < 0 || declarationStart < 0) return; - if (accessStart >= declarationStart) return; + // A same-or-later access is normally fine, EXCEPT a self-referential + // read inside the binding's own initializer (`const x = x`), which + // always hits the TDZ even though its offset follows the declaration's. + if ( + accessStart >= declarationStart && + !isWithinOwnInitializer(node, symbol.declarationNode) + ) { + return; + } if (crossesDeferredBoundary(reference.scope, symbol.scope)) return; From df5a163c70efa98c3703159a7da679b8e30b4b5d Mon Sep 17 00:00:00 2001 From: Rayhan Noufal Arayilakath Date: Sun, 21 Jun 2026 01:42:59 -0700 Subject: [PATCH 13/13] fix(cfg): correct unconditional-set in infinite loops and self-referential stores Two adversarial-corpus findings on PR #892, both pre-existing (the perf refactor's differential test showed it changed neither): - no-set-state-in-render-loop false negative: in an exit-less `for (;;)` the exit is unreachable, so computeUnconditionalSet marked EVERY reachable block "unconditional from entry", and a guarded `setX()` was wrongly deferred to no-set-state-in-render. Treat the latch of an exit-less loop as a virtual completion so the cut test stays well-defined; when the exit is reachable there are no such latches, so reachable-exit behavior (and rules-of-hooks) is unchanged. - no-dead-assignment false positive: a plain `x = b || x` lowered the store into the pre-rhs block, so the short-circuit read of `x` resolved to the freshly-written version and the prior definition looked dead. Land the write target in the post-rhs join block where the value becomes available. Adds SSA/rule tests for both and flips the two corpus `knownGap` entries to their now-correct expectations. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/cfg/src/analysis/unconditional.ts | 96 ++++++++++++++----- packages/cfg/src/build/build-expression.ts | 17 ++++ packages/cfg/tests/ssa.test.ts | 16 ++++ .../correctness/no-dead-assignment.test.ts | 28 ++++++ .../no-set-state-in-render-loop.test.ts | 22 +++++ .../fixtures/cfg-adversarial-corpus.json | 6 +- 6 files changed, 156 insertions(+), 29 deletions(-) diff --git a/packages/cfg/src/analysis/unconditional.ts b/packages/cfg/src/analysis/unconditional.ts index 8f012c0ca..178109c4a 100644 --- a/packages/cfg/src/analysis/unconditional.ts +++ b/packages/cfg/src/analysis/unconditional.ts @@ -1,57 +1,103 @@ import type { BasicBlock, FunctionCfg } from "../ir/basic-block.js"; +import { computeDominatorTree } from "./dominators.js"; -// A block B is "unconditional from entry" iff every execution path -// from entry to exit passes through B. We compute this by, for each -// block B, asking: if we removed B from the graph, is exit still -// reachable from entry? If NO, B is on every path → unconditional. +// A block B is "unconditional from entry" iff every execution of the function +// passes through B. For a function that completes normally that means B lies on +// every entry→exit path; for one that only ever loops forever it means B runs +// on every iteration of the loop it gets stuck in. // -// Cost: O(|blocks|^2) — fine for function-sized CFGs (typically <100 -// blocks). Avoids needing a full dominator tree. +// We compute it with a cut test: B is unconditional iff, with B removed, +// execution can no longer reach a "completion" — the exit on a normal path, or +// the latch of an exit-less infinite loop, which we treat as a virtual edge to +// the exit so the test stays well-defined when the real exit is unreachable. +// Without those latch completions an exit-less `for (;;) { if (g) setX() }` +// would leave the exit unreachable and mark EVERY block unconditional, hiding +// the conditional `setX()` from a render-loop / hooks check. When the exit IS +// reachable there are no infinite latches, so this matches the plain +// "removing B disconnects exit from entry" test exactly. +// +// `throw` edges are skipped throughout: an uncaught throw is not a normal +// completion path, so `if (x) throw; useHook();` keeps `useHook` unconditional. export const computeUnconditionalSet = (cfg: FunctionCfg): Set => { - // Skip "throw" edges when computing reachability — uncaught throws - // don't represent a normal completion path. This makes - // `if (x) throw; useHook();` evaluate as unconditional (the - // `useHook` block is the only normal path to exit). - const reachableFromEntry = (excluded: BasicBlock | null): Set => { + // Blocks that can reach the exit over non-throw edges (backward walk). + const reachesExit = new Set([cfg.exit]); + const reachesExitQueue: BasicBlock[] = [cfg.exit]; + let reachesExitHead = 0; + while (reachesExitHead < reachesExitQueue.length) { + const block = reachesExitQueue[reachesExitHead++]!; + for (const edge of block.predecessors) { + if (edge.kind === "throw") continue; + if (!reachesExit.has(edge.from)) { + reachesExit.add(edge.from); + reachesExitQueue.push(edge.from); + } + } + } + + // Latches of exit-less loops: the source of a non-throw back-edge (its target + // dominates it) that cannot itself reach the exit. Each is a virtual + // completion below. A normal loop's latch reaches the exit, so it adds + // nothing and the reachable-exit case is unchanged. + const dominatorTree = computeDominatorTree(cfg.entry); + const infiniteLatches = new Set(); + for (const block of cfg.blocks) { + if (reachesExit.has(block)) continue; + for (const edge of block.successors) { + if (edge.kind === "throw") continue; + if (dominatorTree.dominates(edge.to, block)) { + infiniteLatches.add(block); + break; + } + } + } + + // Can entry still reach a completion (the exit, or an infinite-loop latch) + // once `excluded` is removed? + const reachesCompletion = (excluded: BasicBlock | null): boolean => { const visited = new Set(); const queue: BasicBlock[] = []; if (cfg.entry !== excluded) queue.push(cfg.entry); - // Index cursor instead of `queue.shift()` — the shift is O(V), which - // would make each traversal O(V^2); a head index keeps it O(V+E). let head = 0; while (head < queue.length) { const block = queue[head++]!; if (visited.has(block)) continue; visited.add(block); + if (block === cfg.exit || infiniteLatches.has(block)) return true; for (const edge of block.successors) { if (edge.kind === "throw") continue; if (edge.to === excluded) continue; queue.push(edge.to); } } - return visited; + return false; }; - // Whole-graph reachability: any block NOT in this set is dead code - // (e.g. statements after an unconditional `return;` / `throw;`). - // Dead-code blocks vacuously satisfy "unconditional from entry" - // because the call site is never reached at runtime — there's - // nothing to constrain. - const reachableFromEntryFull = reachableFromEntry(null); + // Whole-graph reachability (no exclusion): a block not reachable from entry + // over normal edges is dead code (after an unconditional `return` / `throw`) + // and vacuously unconditional — its call site never runs at all. + const reachableFromEntry = new Set(); + const reachableQueue: BasicBlock[] = [cfg.entry]; + let reachableHead = 0; + while (reachableHead < reachableQueue.length) { + const block = reachableQueue[reachableHead++]!; + if (reachableFromEntry.has(block)) continue; + reachableFromEntry.add(block); + for (const edge of block.successors) { + if (edge.kind === "throw") continue; + reachableQueue.push(edge.to); + } + } const unconditional = new Set(); - // Entry is trivially on every path. unconditional.add(cfg.entry); - // Exit is on every (terminating) path. unconditional.add(cfg.exit); for (const block of cfg.blocks) { if (unconditional.has(block)) continue; - if (!reachableFromEntryFull.has(block)) { + if (!reachableFromEntry.has(block)) { unconditional.add(block); continue; } - const stillReaches = reachableFromEntry(block).has(cfg.exit); - if (!stillReaches) unconditional.add(block); + if (!reachesCompletion(block)) unconditional.add(block); } return unconditional; }; diff --git a/packages/cfg/src/build/build-expression.ts b/packages/cfg/src/build/build-expression.ts index 784b35c21..06c24012d 100644 --- a/packages/cfg/src/build/build-expression.ts +++ b/packages/cfg/src/build/build-expression.ts @@ -110,6 +110,23 @@ export const buildExpression = ( return merge; } + if ( + isNodeOfType(node, "AssignmentExpression") && + (node as { operator: string }).operator === "=" && + isNodeOfType((node as { left: EsTreeNode }).left, "Identifier") + ) { + // A plain `x = ` STORES only after the rhs is evaluated. A generic + // left-to-right child walk would map the write target into the pre-rhs + // block, so for a self-referential store (`x = b || x`) the short-circuit + // read of `x` would resolve to the freshly-written version and the prior + // definition would look dead. Evaluate the rhs first, then land the target + // (and the node) in the block where the value becomes available. + const afterRight = buildExpression(builder, (node as { right: EsTreeNode }).right, current); + builder.nodeBlock.set((node as { left: EsTreeNode }).left, afterRight); + builder.nodeBlock.set(node, afterRight); + return afterRight; + } + // Generic expression: evaluate children left-to-right, threading the // block so a control-flow child splits the siblings that follow it. The // node itself completes in the final cursor block. diff --git a/packages/cfg/tests/ssa.test.ts b/packages/cfg/tests/ssa.test.ts index 206ed4f78..041c170fc 100644 --- a/packages/cfg/tests/ssa.test.ts +++ b/packages/cfg/tests/ssa.test.ts @@ -194,6 +194,22 @@ describe("ssa / value queries", () => { // Nothing is reassigned after the final use itself. expect(fixture.ssa.isRedefinedAfter(finalUse, binding!)).toBe(false); }); + + it("keeps the init live across a self-referential `x = b || x` store", () => { + // The short-circuit read of `x` in `b || x` must resolve to the prior + // definition (the init), not the assignment's own new version — otherwise + // the init has no reader and looks like a dead store. + const fixture = analyzeSsaFixture(` + function f(b) { + let chosen = read(); + chosen = b || chosen; + return chosen; + } + `); + const initValue = fixture.ssa.versionAt(fixture.identifier("chosen")); // let chosen = read() + expect(initValue).not.toBeNull(); + expect(fixture.ssa.isLiveValue(initValue!)).toBe(true); + }); }); describe("ssa / DOT rendering", () => { diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-dead-assignment.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-dead-assignment.test.ts index 7ffc98287..cfc38ed08 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-dead-assignment.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/correctness/no-dead-assignment.test.ts @@ -207,4 +207,32 @@ describe("no-dead-assignment", () => { `); expect(result.diagnostics).toHaveLength(1); }); + + it("does NOT flag the init of a self-referential `x = b || x` (the rhs reads the init)", () => { + // Regression: `chosen = b || chosen` reads chosen's current value (the + // init) on the short-circuit path before storing, so the init is live. A + // CFG block-mapping bug landed the store before the rhs read, collapsing + // the rhs read into the new version and making the init look dead. + const result = run(` + function pick(a, b) { + let chosen = 0; + (a > 0) && (chosen = a); + chosen = b || chosen; + return chosen; + } + `); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("does NOT flag a straight-line `x = fallback || x` idiom", () => { + const result = run(` + function pick(b) { + let chosen = 0; + chosen = b || chosen; + return chosen; + } + `); + expect(result.diagnostics).toEqual([]); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-set-state-in-render-loop.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-set-state-in-render-loop.test.ts index 8cf4e7661..09310a3b4 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-set-state-in-render-loop.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/no-set-state-in-render-loop.test.ts @@ -147,4 +147,26 @@ describe("no-set-state-in-render-loop", () => { `); expect(result.diagnostics).toEqual([]); }); + + it("flags a setter guarded inside an exit-less infinite loop", () => { + // Regression: the setter is conditional (behind `if (gate)`) inside a + // `for (;;)` that never exits. With the exit unreachable the CFG briefly + // marked the setter "unconditional from entry", so this rule deferred to + // no-set-state-in-render and dropped the warning. (The sibling + // `while (true)` test above proves the truly-unconditional case still defers.) + const result = run(` + import { useState } from "react"; + function Loop({ gate }) { + const [value, setValue] = useState(0); + for (;;) { + if (gate) { + setValue(1); + } + } + } + `); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("setValue"); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/tests/fixtures/cfg-adversarial-corpus.json b/packages/oxlint-plugin-react-doctor/tests/fixtures/cfg-adversarial-corpus.json index b294044d0..cae34ddd3 100644 --- a/packages/oxlint-plugin-react-doctor/tests/fixtures/cfg-adversarial-corpus.json +++ b/packages/oxlint-plugin-react-doctor/tests/fixtures/cfg-adversarial-corpus.json @@ -183,8 +183,7 @@ "rule": "no-dead-assignment", "name": "logical-shortcircuit-conditional-write", "code": "function pick(a: number, b: number) {\n let chosen = 0;\n (a > 0) && (chosen = a);\n chosen = b || chosen;\n return chosen;\n}\n", - "expectedToFlag": true, - "knownGap": "should be clean — medium rule-bug (see PR review)" + "expectedToFlag": false }, { "rule": "no-dead-assignment", @@ -442,8 +441,7 @@ "rule": "no-set-state-in-render-loop", "name": "for-infinite-conditional-setter-flag", "code": "import { useState } from \"react\";\nfunction Loop({ gate }) {\n const [value, setValue] = useState(0);\n for (;;) {\n if (gate) {\n setValue(1);\n }\n }\n}\n", - "expectedToFlag": false, - "knownGap": "should flag — medium rule-bug (see PR review)" + "expectedToFlag": true }, { "rule": "no-set-state-in-render-loop",