refactor(native): flatten and decompose extractor match arms#844
refactor(native): flatten and decompose extractor match arms#844carlos-alm merged 7 commits intomainfrom
Conversation
Extract hardcoded magic numbers to named constants in constants.rs: - Louvain: MAX_LEVELS=50, MAX_PASSES=20, MIN_GAIN=1e-12, DEFAULT_SEED=42 - Dataflow: TRUNCATION_LIMIT=120 - Build pipeline: FAST_PATH_MAX_CHANGED_FILES=5, FAST_PATH_MIN_EXISTING_FILES=20 Also extract DEFAULT_RANDOM_SEED=42 in TS louvain.ts.
|
Claude finished @carlos-alm's task —— View job PR Review: Extractor Match Arm FlatteningReviewing this native engine refactoring PR following the 5-phase analysis framework. Todo List
Phase 1: Root Cause Analysis ✓Problem Being Solved: High cyclomatic complexity and maintainability issues in native Rust extractors. The PR addresses deeply nested match statements (6-12 levels) that create cognitive overhead and maintenance burden. Evidence of Real Issues:
Approach Justification: The refactoring extracts per-node-kind handler functions, which is a standard complexity reduction technique. The transformation from deeply nested match blocks to clean dispatch is genuine complexity reduction. Phase 2: Configuration Change Detection ✓No High-Risk Configuration Changes Found:
Phase 3: Test Integrity ✓Test Coverage Appears Intact:
Phase 4: Code Quality Review ✓Excellent Refactoring Pattern:
Before/After Pattern (cpp.rs:385-397): // Clean dispatcher - no nested logic
fn match_cpp_node(node: &Node, source: &[u8], symbols: &mut FileSymbols, _depth: usize) {
match node.kind() {
"function_definition" => handle_cpp_function_definition(node, source, symbols),
"class_specifier" => handle_cpp_class_specifier(node, source, symbols),
// ... 8 clean dispatches
}
}Quality Indicators:
Phase 5: Backlog Compliance Assessment ✓Zero-dep: ✓ — No new runtime dependencies. Pure refactoring of existing Rust code using tree-sitter AST. Foundation-aligned: ✓ — Fully compliant with FOUNDATION.md:
Problem-fit: 3/5 — This is infrastructure improvement. Indirectly helps agents by:
Breaking: No — Pure internal refactoring. All extraction output remains identical. No API, CLI, or schema changes. Tier: 1 (zero-dep ✓ + foundation-aligned ✓) Root Cause AnalysisProblem Being Solved: Technical debt in native Rust extractors with deeply nested match arms creating maintenance burden and complexity metrics indicating bug-prone code. Why This Approach: Standard extract-function refactoring. Separates dispatch logic from business logic, improving readability and testability. Risk Assessment: Very low risk - pure code reorganization with preserved functionality. Backlog Compliance
Critical ConcernsNone. This is exemplary refactoring work. Final Recommendation
Minor observation: This is infrastructure work that doesn't directly solve the core "AI orientation" problem, but it's necessary foundation work for future accuracy improvements. |
Greptile SummaryThis PR is a pure structural refactoring of 12 Rust extractor files and Key changes:
Confidence Score: 5/5Pure structural refactoring with no behavioral changes; safe to merge All 12 extractor files follow an identical, mechanical flattening pattern — dispatch logic is preserved exactly and per-language edge cases are correctly retained in their per-language helpers. No new logic is introduced, only structural decomposition. No P0 or P1 findings. No files require special attention Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[walk_tree] --> B{match_*_node}
B -->|function_definition| C[handle_function_*]
B -->|class_declaration| D[handle_class_*]
B -->|interface_declaration| E[handle_interface_*]
B -->|enum_declaration| F[handle_enum_*]
B -->|import_declaration| G[handle_import_*]
B -->|call_expression| H[handle_call_*]
B -->|other kinds| I[handle_other_*]
C --> J[symbols.definitions.push]
D --> J
D --> K[extract_*_fields / extract_*_base_types]
K --> J
E --> J
F --> J
G --> L[symbols.imports.push]
H --> M[symbols.calls.push]
I --> J
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Wildcard or empty-names reexports | ||
| if re.wildcard_reexport || re.names.is_empty() { | ||
| if ctx.has_definition(re.source, symbol_name) { | ||
| return Some(re.source.to_string()); | ||
| } | ||
| let deeper = resolve_barrel_export(ctx, re.source, symbol_name, visited); | ||
| if deeper.is_some() { | ||
| return deeper; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new shared implementation silently alters the wildcard barrel resolution logic from both edge_builder.rs and import_edges.rs.
Old behavior (both callers, wildcard branch):
if re.wildcard_reexport || re.names.is_empty() {
if let Some(defs) = ctx.file_defs.get(re.source.as_str()) {
// Only enters here if the source IS in file_defs
if defs.contains(symbol_name) { return Some(...); }
let deeper = resolve_barrel_export(...);
if deeper.is_some() { return deeper; }
}
// If source not in file_defs: do nothing, continue to next reexport
}New behavior (this file, wildcard branch):
if re.wildcard_reexport || re.names.is_empty() {
if ctx.has_definition(re.source, symbol_name) { return Some(...); }
// Tries deeper even when the source file is NOT indexed
let deeper = resolve_barrel_export(ctx, re.source, symbol_name, visited);
if deeper.is_some() { return deeper; }
}When the source file in a wildcard reexport chain is not present in file_defs / file_symbols, the old code skipped that wildcard entirely (no deeper traversal). The new code always attempts deeper resolution. If the unindexed source file itself is in the reexport_map, the new code can now produce a result (Some(...)) where the old code returned None for that branch.
This is a real behavioral change in import edge generation that this PR describes as a pure refactoring. The same difference applies to named reexports (lines 50–61): when a source file isn't indexed, the old edge_builder.rs skipped deeper resolution and jumped to the fallback, while the new code always tries deeper first. That difference is less impactful (both paths eventually return the fallback for the named case), but the wildcard case can produce new edges. Given the hard requirement that "Native engine produces identical results to WASM," this should be verified against the TypeScript resolveBarrelExport() in resolve-imports.ts to confirm the new behaviour is intentional.
There was a problem hiding this comment.
Good catch. Verified against the TypeScript reference (resolveBarrelExport() in resolve-imports.ts lines 194-215):
-
TS behavior: Both named and wildcard branches guard deeper traversal with
const targetSymbols = ctx.fileSymbols.get(re.source); if (targetSymbols) { ... }— when the source file isn't indexed, named reexports fall through to the fallbackreturn re.source, and wildcard reexports are silently skipped. -
Rust (before fix):
has_definition()returnsfalsewhen the file isn't indexed, but the code still attemptedresolve_barrel_export()recursively. This could produce new edges for wildcard reexports through unindexed files.
Fixed in 4c101f7 by adding a has_file() method to BarrelContext that gates both branches, matching the TS reference exactly. All three implementors (TestContext, ImportEdgeContext in edge_builder.rs, ImportEdgeContext in import_edges.rs) now implement it.
There was a problem hiding this comment.
This concern is fully addressed. The merge with main (commit 64dad67) brought in PR #848 which resolved the barrel resolution approach differently -- removing the has_file guard entirely and simplifying the algorithm. The wildcard branch now always attempts deeper traversal, matching the TS reference behavior. The resolves_transitive_chain test also passes correctly with this approach.
Codegraph Impact Analysis69 functions changed → 74 callers affected across 27 files
|
…atch Fix FAST_PATH_MIN_EXISTING_FILES constant type from usize to i64 to match get_existing_file_count() return type, resolving Rust compile error. Add has_file() gate to BarrelContext trait so resolve_barrel_export() only recurses into indexed source files, matching the TypeScript reference implementation behavior where ctx.fileSymbols.get(re.source) guards deeper traversal.
|
The |
1 participant
Summary
match_cpp_node(worst MI=20.3, bugs=2.37) andmatch_scala_node(bugs=1.87) into per-node-kind dispatch functionsTitan Audit Context
Changes
crates/codegraph-core/src/extractors/csharp.rs-- extract handler functions from nested matchcrates/codegraph-core/src/extractors/elixir.rs-- extract handler functionscrates/codegraph-core/src/extractors/go.rs-- extract handler functionscrates/codegraph-core/src/extractors/java.rs-- extract handler functionscrates/codegraph-core/src/extractors/javascript.rs-- extract handler functionscrates/codegraph-core/src/extractors/php.rs-- extract handler functionscrates/codegraph-core/src/extractors/python.rs-- extract handler functionscrates/codegraph-core/src/extractors/ruby.rs-- extract handler functionscrates/codegraph-core/src/extractors/rust_lang.rs-- extract handler functionscrates/codegraph-core/src/extractors/zig.rs-- extract handler functionscrates/codegraph-core/src/extractors/cpp.rs-- decompose match_cpp_nodecrates/codegraph-core/src/extractors/scala.rs-- decompose match_scala_nodeMetrics Impact
Test plan