feat(cfg): control-flow graph engine, formal-verification stack, and CFG-backed rules#892
Open
aidenybai wants to merge 13 commits into
Open
feat(cfg): control-flow graph engine, formal-verification stack, and CFG-backed rules#892aidenybai wants to merge 13 commits into
aidenybai wants to merge 13 commits into
Conversation
…xposed 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).
Member
Author
|
/rde parity |
|
❗ Parity changed: +1 added · -1 removed across 1 repo Baseline:
trace ToolJet/ToolJet (plugins/packages/restapi) — +1 / -1✨ Added in this PR (not present in baseline)
✅ Removed (fixed) in this PR (were in baseline)
…(truncated) |
commit: |
…; add cleanup-path verifier 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
…ing cases A redirect()/notFound() that throws inside a nested finally (or a finally-only inner try) which sits in an outer try body propagates out via finally-completion and is swallowed by the outer catch — a true positive, not the false positive it appears to be in isolation. Pin both the should-flag cases and the no-catch-anywhere quiet twin.
…tion/message The rule-metadata convention test bans em/en dashes in titles and recommendations; replace them with commas.
4 tasks
…rt oxc no-unreachable corpus (#902)
Member
Author
|
/rde parity |
The rule-metadata title-length guard fails CI because the title is 66 chars. Mirror the sibling effect-cleanup rule's headline style.
aidenybai
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Member
Author
|
/rde parity |
…se positives Two false-positive classes surfaced by running the rule across the OSS corpus: - Loop-carried state machines: an unlabeled `break` inside a `switch` nested in a loop was routed to the loop's exit instead of the switch's merge, severing the loop back-edge so a value written in a `case` and read at the top of the next iteration looked dead. The CFG builder now resolves an unlabeled `break` to the innermost enclosing loop OR switch via a monotonic `breakScopeSeq`, keeping the back-edge intact (repro: tldraw `reorderShapes`). - Values read only on an exceptional path: a value written inside a `try` body and read only in the enclosing `catch`/`finally` looked dead under the coarse throw model. The rule now guards with `isInsideTryBlock` / `hasWriteInsideTryBlock` (repros: bippy owner-stack, excalidraw localStorage).
…n test The stdio `beforeAll` spawns the server, initializes LSP, and waits up to 20s for the first full workspace scan, but hooks defaulted to a 10s budget — too tight for a cold Windows CI runner, which flaked with "Hook timed out in 10000ms". Set `hookTimeout` to match the existing 30s `testTimeout`.
…aph passes Each rule rebuilt scopes/CFG/SSA/definite-assignment per file; now one build is shared per Program via a WeakMap (the effect-rules pattern) and the single CFG is threaded into SSA + definite-assignment instead of each rebuilding it. CFG derived structures (dominator/post-dominator trees, loop membership, reachability, unconditional set, node order) are computed lazily and memoized. Loop detection drops O(V^3) self-reachability to an O(V+E) SCC pass; reachability/unconditional stop re-shifting their BFS queues; forEachChildNode drops a per-node Object.keys allocation; enclosingFunction is memoized; and simple-path enumeration gains a global visit budget. Behavior-preserving: parity tests pin the loop/unconditional rewrites against the originals, and differential testing over 186 adversarial edge cases plus the fixtures, in both rule orders, produced byte-identical diagnostics. Measured ~2x faster on the CFG analysis layer (1.96x, -49%), widening with file size. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nches Cursor Bugbot (PR #892) flagged a false negative: the rule ended its SSA check at the render function's last statement, so a captured `let` reassigned on a branch that returns BEFORE that statement was missed — yet the memo closure runs on that path and observes the new value. Add `ssa.isRedefinedAfter(fromNode, binding)` (a write reachable from the hook call on any path, with no endpoint constraint) and switch the rule to it, dropping the last-statement endpoint. Parity for the prior cases is kept; the early-return case is now flagged. Covered by new tests at both the SSA layer and the rule layer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rayhanadev
force-pushed
the
cfg-beefup-and-rule-fixes
branch
from
df6b2fe to
7f3c8f7
Compare
186 small snippets stressing tricky control flow — early returns, throw edges, try/finally, do-while / labeled / infinite loops, switch fallthrough, short-circuits, callbacks escaping loops, multi-write shadowing — across the 8 CFG/SSA/typestate-consuming rules. Originally the differential-testing corpus for the perf refactor; kept as a characterization suite so any future change to the shared CFG/SSA analysis that silently flips a control-flow shape fails for review. Every expectation was reconciled against the rule's documented intent. Two entries are annotated `knownGap` where current behavior is a tracked bug (a no-set-state-in-render-loop false negative on guarded setters in an infinite loop, and a no-dead-assignment false positive on a short-circuit conditional write); the assertion pins current behavior — flip it when the bug is fixed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 03da338. Configure here.
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) <noreply@anthropic.com>
…ntial 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) <noreply@anthropic.com>
Member
Performance: CFG analysis layer (~2× faster)Each rule rebuilt the CFG, SSA, and typestate analysis from scratch, and a few graph passes ran super-linear. This change shares one analysis per file and drops those passes to linear time, with identical diagnostics. Measured (before/after from git worktrees at the same base, re-parsing each iteration, median of 9 runs):
The microbench gain grows with function size (1.59× at 50 branches, 1.93× at 400), tracking the O(V³)→O(V+E) passes. Each change
Headline snippetsShare one analysis per file across all rules ( // before: every wrapped rule rebuilds its own
let cachedCfg = null;
const getCfg = () => (cachedCfg ??= analyzeControlFlow(programRoot));
// after: one build per Program, shared across all rules
const analysisCache = new WeakMap<EsTreeNode, SharedAnalysis>();
const getCfg = () => {
const shared = sharedAnalysisFor(programRoot);
return (shared.cfg ??= analyzeControlFlow(programRoot));
};Loop membership, O(V³) to O(V+E) ( // before: per-block self-reachability BFS
for (const start of cfg.blocks) {
// BFS over successors; mark start cyclic if it reaches itself
}
// after: one strongly-connected-components pass
// cyclic = blocks in an SCC of size > 1, plus self-loopsNo behavior change: every rule produced byte-identical diagnostics before and after, across an adversarial corpus and the fixtures. |
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
2 participants
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
What started as a CFG beef-up grew into a dedicated control-flow analysis engine. This PR extracts the per-function control-flow graph into a new internal
@react-doctor/cfgpackage, layers a small formal-verification stack on top of it (SSA, dataflow, typestate, path feasibility), and lands 6 new CFG-backed rules plus several false-positive fixes on existing ones. The package is pure-TS, bundled intooxlint-plugin-react-doctorat build time (published surface unchanged), and lazy — a rule that never reads a layer pays nothing.@react-doctor/cfg— the engineTerminalmodeled on the React Compiler HIR taxonomy (goto/if/switch/ loops /logical/ternary/optional/try/return/throw), withfallthroughjoin blocks and explicitgotolowering ofbreak/continue.&&/||/??(and&&=/||=/??=) right operands, and optional-chain links past each?.each get their own blocks — so a hook /setStateshort-circuited inside an expression is correctly seen as conditional.isInfiniteLoopStart, oxc-parity constant folding),try/catch/finallyfinalize/join semantics, and a GraphviztoDotexport.EnterSSAuses) plus redundant-φ elimination — a clean-room port (MIT attribution, no Babel). Query API:versionAt,reachingDefinition,isLiveValue,isRedefinedBetween,bindingOf.solveDataflow, a generic monotone worklist fixpoint over aLattice<Fact>, withanalyzeDefiniteAssignmentbuilt on it.verifyTypestate(cfg, { automaton, classifier }), a reusable resource-protocol automaton reporting illegal transitions and leaked resources.isPathFeasible+lowerGuard/pathConditionFacts) that refutes correlated-branch counterexamples via union-find congruence closure. It only ever suppresses a diagnostic when the path search is complete and every counterexample is provably infeasible, so it strictly removes false positives and is never unsound for bug-finding.no-unreachable,no-fallthrough,no-unsafe-finally,getter-return), ESLint code-path analysis, and React CompilerBuildHIR.New rules (CFG-backed)
correctness/no-unreachable-code(Bugs) — code that never runs because every path above returns / throws / breaks / continues / loops forever. ESLintno-unreachablecarve-outs (hoisted functions, type-only TS decls, barevar x;).correctness/no-dead-assignment(SSA) — a write to a reassignable local whose value is never read because every path overwrites it first. Quiet forconst, compound assignments, and closure-captured bindings.correctness/no-use-before-define(dataflow) — a block-scoped binding used lexically before its declaration in the same synchronous execution (a TDZReferenceError). Sound by construction: quiet for hoistedvar/functions, params, globals, and closure/class-body access.state-and-effects/effect-cleanup-not-on-every-path— a subscription/timer acquired in an effect whose returned cleanup is skipped on an early-return path. Complementseffect-needs-cleanup(which only checks a cleanup exists at all).state-and-effects/no-unreleased-resource(typestate) — a resource opened in an effect callback and released inline on some paths but leaked on an early return. Scoped touseEffect/useLayoutEffect/useInsertionEffect; the returned-cleanup contract stays owned byeffect-cleanup-not-on-every-path.state-and-effects/no-stale-closure-capture— auseMemo/useCallbackclosure capturing aletreassigned later in the same render. Quiet forconst, never-reassigned bindings, and the deferred effect hooks.state-and-effects/no-set-state-in-render-loop(Bugs) — auseStatesetter called inside a render-phase loop. Partitions cleanly withno-set-state-in-renderonisUnconditionalFromEntry(no double-reporting).Existing-rule fixes (consumer-visible)
nextjs-no-redirect-in-try-catch— stop mis-flaggingredirect()/notFound()in acatch, in afinally, or in atrywhose only companion isfinally(none swallow the navigation control-flow error). Replaced the try-depth counter with an ancestry walk.no-set-state-in-render— now flags any setter the CFG proves runs on every render path (const x = setCount(...), unconditional blocks), not just bare top-level statements; the guarded store-previous-render pattern stays quiet.no-mutating-reducer-state— drop remembered mutations that can't reach the same-reference return (e.g.for (…) { state.items.push(x); return { ...state } }with a trailingreturn stateon the no-match path).js-hoist-regexp/js-index-maps/js-set-map-lookups— loop-awareness keys off real CFG loop membership instead of lexical nesting depth, so work inside a callback that merely escapes a loop is no longer flagged.Investigated, deliberately not shipped
server-auth-actions—isUnconditionalFromEntrygating produced ERROR-severity false positives on idiomatic validation-before-auth.effect-needs-cleanup— CFG reachability regressed real patterns where the resource is created in aforEach/helper closure (cross-function). The positional heuristic is better there.Test plan
pnpm test(full monorepo) — all packages green, including the new@react-doctor/cfgsuites and ported parity corporapnpm typecheckcleanpnpm lintcleanpnpm format:checkcleanNote
Medium Risk
Large new analysis engine and several new global/path-sensitive rules can shift diagnostic volume; behavior is heavily tested via parity corpora and differential tests, but edge cases in CFG/SSA may still surface in the wild.
Overview
Introduces internal
@react-doctor/cfg(bundled into the plugin at build time): a per-function structural CFG with React Compiler–style terminals, expression-level lowering (&&/ ternary / optional chaining), dominance/reachability/loop primitives, Braun SSA, a generic dataflow solver, typestate verification, and bounded path-feasibility pruning. CFG/SSA/definite-assignment are shared per file via aProgram-keyed cache with lazy derived analyses (~2× faster for multi-rule scans).New rules use these layers:
no-unreachable-code,no-dead-assignment,no-use-before-define,effect-cleanup-not-on-every-path,no-unreleased-resource,no-stale-closure-capture,no-set-state-in-render-loop.Existing rules are tightened or de-noised: CFG-backed
no-set-state-in-renderand loop rules; redirect-in-try-catch, reducer mutation, and loop-aware JS rules lose false positives; CFG fixes include correctbreakinsideswitch-in-loop and try-body liveness for dead-assignment.Reviewed by Cursor Bugbot for commit df5a163. Bugbot is set up for automated code reviews on this repo. Configure here.