refactor: split cfg-visitor.ts by control-flow construct#619
refactor: split cfg-visitor.ts by control-flow construct#619carlos-alm merged 3 commits intomainfrom
Conversation
Impact: 16 functions changed, 0 affected
|
Claude finished @carlos-alm's task —— View job PR Review: refactor: split cfg-visitor.ts by control-flow constructTodo List:
Phase 1: Root Cause Analysis ✅ The PR refactors a large 874 SLOC
This is a legitimate internal refactor within the ast-analysis domain with no external API changes. The approach is sound - splitting by logical concern rather than arbitrary size limits. Phase 2: Configuration Change Detection ✅ ✅ NO HIGH-RISK CHANGES DETECTED
Phase 3: Test Integrity ✅ ✅ NO TEST INTEGRITY ISSUES
Phase 4: Code Quality Analysis ✅ Architecture & Design ✅
Type Safety ✅
Code Patterns ✅
Minor Line Count Increase (944 vs 874)
This is acceptable overhead for a well-structured refactor that significantly improves maintainability. Phase 5: Backlog Compliance ✅ Root Cause AnalysisProblem Being Solved: Large 874 SLOC monolithic Why This Approach: Logical separation by control-flow construct (conditionals, loops, try-catch) is more maintainable than arbitrary size-based splitting. Preserves all functionality while improving structure. Risk Assessment: Low risk - purely internal refactor with no API changes, preserved test compatibility, and clean module boundaries. Backlog Compliance
Critical ConcernsNone identified. This is a well-executed internal refactor that improves code maintainability without introducing risks. Final Recommendation
|
Greptile SummaryThis PR splits the monolithic Key points from the review:
Confidence Score: 5/5Safe to merge — pure structural refactoring with no behavioral changes and both prior review concerns fully resolved. All logic is faithfully extracted from the original monolith. The two concerns raised in the prior review thread (ProcessStatementsFn placement and unnecessary exports) are confirmed fixed in HEAD. No new bugs were found during this review. The module boundaries are clean, imports are minimal and correct, and the public API surface of cfg-visitor.ts is unchanged. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
engine["engine.ts\n(external caller)"]
visitor["cfg-visitor.ts\norchestrator\nprocessStatements / processStatement\ncreatesCfgVisitor"]
shared["cfg-shared.ts\nshared types & utilities\nFuncState · CfgBlockInternal\nProcessStatementsFn · makeFuncState\ngetBodyStatements · registerLabelCtx\npredicates: isIfNode / isForNode / …"]
cond["cfg-conditionals.ts\nprocessIf\nprocessSwitch"]
loops["cfg-loops.ts\nprocessForLoop\nprocessWhileLoop\nprocessDoWhileLoop\nprocessInfiniteLoop"]
trycatch["cfg-try-catch.ts\nprocessTryCatch"]
engine -->|createCfgVisitor| visitor
visitor -->|imports| shared
visitor -->|imports| cond
visitor -->|imports| loops
visitor -->|imports| trycatch
cond -->|imports types+utils| shared
loops -->|imports types+utils| shared
trycatch -->|imports types+utils| shared
visitor -->|passes processStatements callback| cond
visitor -->|passes processStatements callback| loops
visitor -->|passes processStatements callback| trycatch
Reviews (2): Last reviewed commit: "refactor: move ProcessStatementsFn to cf..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export type ProcessStatementsFn = ( | ||
| stmts: TreeSitterNode[], | ||
| currentBlock: CfgBlockInternal, | ||
| S: FuncState, | ||
| cfgRules: AnyRules, | ||
| ) => CfgBlockInternal | null; |
There was a problem hiding this comment.
ProcessStatementsFn belongs in cfg-shared.ts, not cfg-loops.ts
This shared callback type is imported by both cfg-conditionals.ts (line 8) and cfg-try-catch.ts (line 2) purely to access this type — not for anything loop-specific. This creates an unintended module coupling: the conditionals and try-catch modules depend on the loops module as a side-effect of a shared type definition.
Moving ProcessStatementsFn to cfg-shared.ts (alongside the other shared types like FuncState, CfgBlockInternal, etc.) would eliminate this coupling and better reflect the PR's stated intent of focused modules with minimal cross-cutting dependencies.
There was a problem hiding this comment.
Fixed in 00cb63d. Moved ProcessStatementsFn from cfg-loops.ts to cfg-shared.ts alongside the other shared types. All three sub-modules now import it from cfg-shared.js, eliminating the unintended coupling to the loops module.
| export function findTryHandlers( | ||
| tryStmt: TreeSitterNode, | ||
| cfgRules: AnyRules, | ||
| ): { catchHandler: TreeSitterNode | null; finallyHandler: TreeSitterNode | null } { | ||
| let catchHandler: TreeSitterNode | null = null; | ||
| let finallyHandler: TreeSitterNode | null = null; | ||
| for (let i = 0; i < tryStmt.namedChildCount; i++) { | ||
| const child = nn(tryStmt.namedChild(i)); | ||
| if (cfgRules.catchNode && child.type === cfgRules.catchNode) catchHandler = child; | ||
| if (cfgRules.finallyNode && child.type === cfgRules.finallyNode) finallyHandler = child; | ||
| } | ||
| return { catchHandler, finallyHandler }; | ||
| } |
There was a problem hiding this comment.
findTryHandlers, processCatchHandler, and processFinallyOnly are only called within this file (all three are called exclusively from processTryCatch). Exporting them widens the public API surface beyond what this module actually requires.
The same applies to processAlternative, processElifSiblings, and extractCaseBody in cfg-conditionals.ts — each is only called within that file, yet all three are exported.
In the original monolithic cfg-visitor.ts, all of these were private functions. They should remain unexported unless needed by another module or for unit testing.
There was a problem hiding this comment.
Fixed in 00cb63d. Removed the export keyword from all six internal helpers:
cfg-try-catch.ts:findTryHandlers,processCatchHandler,processFinallyOnlycfg-conditionals.ts:processAlternative,processElifSiblings,extractCaseBody
Confirmed none of these are imported by any other module — they are only called within their respective files, matching their original private scope in the monolith.
…helpers Move the shared ProcessStatementsFn type from cfg-loops.ts to cfg-shared.ts to eliminate unnecessary cross-module coupling between conditionals/try-catch and the loops module. Un-export internal helpers that are only called within their own files: - cfg-try-catch.ts: findTryHandlers, processCatchHandler, processFinallyOnly - cfg-conditionals.ts: processAlternative, processElifSiblings, extractCaseBody Impact: 6 functions changed, 7 affected
1 participant
Summary
cfg-visitor.ts(874 SLOC) into focused modules by control-flow constructcfg-conditionals.ts,cfg-loops.ts,cfg-try-catch.ts,cfg-shared.tsengine.tsimports cfg-visitorTitan Audit Context
Changes
src/ast-analysis/visitors/cfg-visitor.ts-- reduced to orchestrator, delegates to sub-modulessrc/ast-analysis/visitors/cfg-conditionals.ts-- if/else/switch processingsrc/ast-analysis/visitors/cfg-loops.ts-- for/while/do-while processingsrc/ast-analysis/visitors/cfg-try-catch.ts-- try/catch/finally processingsrc/ast-analysis/visitors/cfg-shared.ts-- shared CFG builder utilitiesMetrics Impact
Test plan