refactor(cfg): extract control-flow graph into @react-doctor/cfg + port oxc no-unreachable corpus#902
Merged
Merged
Conversation
Move the per-function CFG builder + dominance/reachability analyses out of oxlint-plugin-react-doctor into a new self-contained, internal @react-doctor/cfg package (owns its minimal ESTree glue so it has no dependency back on the plugin). The plugin bundles it at build time (devDependency), so the published surface is unchanged. CFG test suites and the run-cfg harness move with it. Pure extraction — no behavior change.
Replace the partial hand-port with a full port of oxc's eslint/no-unreachable pass/fail corpus (37 cases) asserted directly against the graph's isUnreachable via dead()/live() markers — our CFG matches oxc's reachability semantics across the whole suite. Add a README documenting the analysis API, the modeled terminal taxonomy (statement- and expression-level), the deliberate non-goals, and fixture provenance.
commit: |
Builds Layers A-D on the CFG/SSA engine, all pure-TS, lazy, run once per scan: - dataflow framework (solveDataflow) + analyzeDefiniteAssignment (Layer A) - typestate protocol engine, verifyTypestate (Layer C) - bounded path-feasibility checker, isPathFeasible (Layer D), wired into typestate + definite-assignment to prune provably-infeasible counterexamples New rules consuming them: - no-use-before-define: sound lexical Temporal Dead Zone detection - no-stale-closure-capture: SSA-verified stale render-closure captures - no-unreleased-resource: inline effect-resource leak on some paths - no-dead-assignment, no-unreachable-code, no-set-state-in-render-loop RDE-validated against the OSS corpus and hardened: - no-use-before-define rewritten from definite-assignment to lexical TDZ (162 -> 0 false positives); fixes TS interface-member keys being recorded as references (scope-analysis) and treats class bodies as deferred scopes - no-unreleased-resource narrowed to React effect callbacks (useEffect/useLayoutEffect/useInsertionEffect + React.* member form), dropping class-lifecycle and non-React-framework noise (20 -> 0) - shared node-start util dedups the structural byte-offset read
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Two Bugbot findings on the CFG builder for labeled iteration: - `continue <label>` now adds a back-edge to the labeled loop's header. Label entries were pushed with `header: null` (the `break` merge is known up front, the continue target is not until the loop builds its header), so labeled continue resolved to a header-less entry and the builder emitted no back-edge. Link enclosing `LabeledStatement` ancestors to the loop header as each loop is built. - `while` / `do-while` / `for` loop tests now map to the loop header rather than the pre-header block. The condition instruction already lived on the header, but `mapDescendantsToBlock` attributed the test's nodes to `current`, so side effects in the test (`while (q.pop())`) read as outside the loop, skewing `blockOf`, `isInsideLoop`, and SSA placement. The for-loop init stays on the pre-header (runs once).
Bugbot: the `for` update clause was mapped onto the loop header (where the test lives), so the first entry from `init` modeled `update` as running before the body — wrong; `update` runs only after the body on the back-edge. Give it its own latch block (body → latch → header) and route `continue` through the latch so the update still runs on an explicit `continue`. A loop with no update collapses the latch onto the header (identical to a plain back-edge). Fixes skewed SSA placement and per-block read/write order for loop-carried bindings.
500-repo RDE pass surfaced false positives, all inside TypeScript type space (no runtime, so a TDZ ReferenceError is impossible): - The scope analyzer descended into `TSInterfaceDeclaration` / `TSTypeAliasDeclaration` bodies and recorded their identifiers as value references — interface heritage (`interface X extends Y`), index- signature parameters (`[tabId: string]`), and computed property keys (`[ORIGINAL_INDEX_KEY]`). These bodies are erased at compile time, so stop walking into them (namespaces / `import =` still recurse since they bind real values). - Skip ambient declaration files (`.d.ts` / `.d.mts` / `.d.cts`) in the rule via a shared `isDeclarationFile` helper (also dedups the same check in `parse-source-file`). Real runtime TDZ bugs in `.jsx`/`.js`/`.ts` are still reported (verified on ToolJet: interface/`.d.ts` hits 28 -> 0, the genuine `useState(x)` before-`const x` crashes retained).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 981293b. Configure here.
…cleanup 500-repo RDE pass surfaced false positives where an effect both releases inline on some branch AND returns a cleanup function (the defensive remove-then-add idiom, or a conditional add with an unmount cleanup). The inline release made `closedResources` non-empty so the rule proceeded, but the returned cleanup — which actually releases the resource — sits in a nested closure the collector never descends into, so the typestate saw a false leak. Defer entirely when the effect returns a cleanup; that contract is owned by `effect-cleanup-not-on-every-path`. Verified on ToolJet useShowPopover + appsmith AudioRecorder (both 1-2 -> 0).
…back 500-repo RDE pass surfaced false positives on `useEffect` callbacks that capture a render-scope `let` assigned later in the body (the common `let bounds; useEffect(() => use(bounds)); bounds = compute()` shape). Effects run AFTER the synchronous render, so observing the final value is the intended pattern, not a stale capture. Narrow the rule to the render-phase producers `useMemo` / `useCallback`, where the deps array signals snapshot intent and a later reassignment is a genuine staleness bug. Verified on ToolJet (effect captures -> 0) + appsmith PlainTextCell (the useMemo staleness warning retained).
Confirmation RDE pass flagged one residual false positive: a type-only
re-export `export type { Foo as Bar } from "mod"` where a local class
`Foo` is declared later. An export specifier is a hoisted binding
re-export resolved at module-link time, not an expression-position read,
so it can never throw a TDZ ReferenceError. Skip it (covers value
`export { Foo }` before its declaration too). Verified on AFFiNE
icon-picker (1 -> 0).
Member
Author
|
/rde parity |
|
❌ Parity failed — trace |
Only strict `===`/`!==` lower to an identity fact. Loose `==`/`!=` does not imply value identity (`[] == 0`, `null == undefined`), so emitting an equality fact for it let the checker "prove" a satisfiable path infeasible and suppress a real diagnostic. Dropping the fact is sound — fewer facts only make a path look more feasible.
The plugin kept hand-synced copies of 8 AST helpers (EsTreeNode and friends, isNodeOfType, isFunctionLike, ...) that were byte-identical to the cfg package's own; collapse them into one-line re-exports from @react-doctor/cfg so the cross-package AST contract has a single source of truth and can't drift. Trim the cfg barrel to the surface its sole (private) consumer actually imports — the four analysis entry points, their result types, and the shared AST helpers. The IR, dataflow framework, path-feasibility primitives, and dominator utilities were exported but never consumed externally; they remain reachable via deep paths from inside the package and its tests. Also reuse the existing walkAst util in no-unreleased-resource instead of a duplicated subtree walk + double cast.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extracts React Doctor's control-flow graph into a dedicated, self-contained internal package and deepens its test parity with oxc. Stacked on #892 (base:
cfg-beefup-and-rule-fixes).@react-doctor/cfgpackage — the per-function CFG builder + dominance/reachability analyses (isUnconditionalFromEntry,isReachable,dominates,postDominates,isInsideLoop,isUnreachable) now live on their own. It owns a minimal copy of the generic ESTree glue it needs, so it has no dependency back on the plugin. The plugin bundles it at build time (devDependency), so the published surface is unchanged.wrap-with-semantic-context,rule-context, and the testrun-ruleharness import@react-doctor/cfg; rules keep usingcontext.cfguntouched. Cross-packageEsTreeNodeis structurally identical, so nodes pass the boundary without a cast.eslint/no-unreachablepass/failcorpus (37 cases) asserted directly againstisUnreachableviadead()/live()markers. Our CFG matches oxc's reachability across the whole suite.Pure extraction + tests/docs — no behavior change to any rule.
Test plan
pnpm --filter @react-doctor/cfg build/typecheck/test(100 tests)pnpm typecheck(13 tasks) — cross-packageEsTreeNodecompatibility holdspnpm test(full turbo suite, 11 tasks) — plugin 7295 tests still greenpnpm lint(clean),pnpm format:check(clean), plugingen:check(registry in sync)Note
Medium Risk
Large internal refactor of lint infrastructure with broad parity tests; published plugin surface is unchanged but CFG semantics now gate many rules.
Overview
Introduces the private
@react-doctor/cfgpackage and moves the per-function CFG builder plus dominance/reachability APIs (analyzeControlFlow,isUnreachable,isInsideLoop, etc.) out of the oxlint plugin. The plugin keepscontext.cfgand bundles the package at build time so published behavior stays the same.The engine is reorganized into
ir/,build/, andanalysis/modules with React Compiler HIR–style terminals, Cooper–Harvey–Kennedy dominators, and optional SSA, dataflow, typestate, and path-feasibility layers for future rules. Tests and fixtures live underpackages/cfg/tests, including a full port of oxc’seslint/no-unreachablepass/fail corpus asserted viaisUnreachable, plus ESLint code-path, React Compiler shape, and regression suites. A README documents the API and fixture provenance.Reviewed by Cursor Bugbot for commit 89fce67. Bugbot is set up for automated code reviews on this repo. Configure here.