fix: resolve type-only imports for dead code analysis#862
fix: resolve type-only imports for dead code analysis#862carlos-alm wants to merge 4 commits intomainfrom
Conversation
Type-only imports (import type { Foo }) created file-level edges but no
symbol-level edges, so interfaces consumed only via type imports had
fanIn=0 and were falsely classified as dead-unresolved.
- Create symbol-level imports-type edges in buildImportEdges targeting
the actual imported symbol nodes
- Include imports-type edges in fan-in, isExported, and prodFanIn
queries in both full and incremental role classification
|
Claude finished @carlos-alm's task —— View job PR Review CompleteReviewing PR #862: "fix: resolve type-only imports for dead code analysis" Todo List:
Root Cause AnalysisProblem Being Solved: Type-only imports ( Why This Approach: The fix correctly addresses the root cause by creating symbol-level Risk Assessment: Low risk. The change is surgical and well-contained, only affecting dead code detection for type-imported symbols. Backlog Compliance
Code Quality AssessmentStrengths
Implementation DetailsThe fix adds logic in
Fan-in Query UpdatesUpdated 5 SQL queries to include
Test Analysis
Minor Observations
Critical ConcernsNone identified. This is a solid, well-targeted fix. Final Recommendation
Additional Notes: Consider verifying that the native engine path ( |
Greptile SummaryThis PR fixes a false-positive dead-code classification for TypeScript interfaces and types consumed only via The fix creates symbol-level Key details:
Confidence Score: 5/5Safe to merge — the fix is correctly applied across all three engine paths with solid test coverage, and no P0/P1 issues remain. The main concern from the prior review (native engine parity gap) is fully addressed in this revision. Both the napi path (edge_builder.rs, symbol_node_map) and the DB-pipeline path (import_edges.rs, get_symbol_node_id) now emit symbol-level imports-type edges. The incremental watch-mode path is also covered. All fan-in, isExported, and production fan-in queries in both the full and incremental classification paths are correctly updated. The two remaining findings are both P2: a minor fanInDist median skew (pre-existing architectural difference, small practical impact) and a namespace-import prefix inconsistency in import_edges.rs (extremely rare scenario). Neither affects correctness for real-world codebases. import_edges.rs: minor prefix-stripping inconsistency for tab-separated namespace imports. structure.ts: incremental fanInDist includes file-node targets from file-level imports-type edges. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["import type { Foo } from './a'"] --> B{Engine path?}
B -->|"JS path\n(≤3 files or WASM)"| C["buildImportEdges\nbuild-edges.ts"]
B -->|"Native napi path\n(>3 files, native)"| D["build_import_edges\nedge_builder.rs"]
B -->|"DB-pipeline path\n(Rust standalone)"| E["build_import_edges\nimport_edges.rs"]
B -->|"Watch mode"| W["buildImportEdges\nincremental.ts"]
C --> F1["① file-level edge\nfileNode → targetFileNode\nkind=imports-type"]
C --> F2["② symbol-level edge\nfileNode → Foo node\n(via nodesByNameAndFile)"]
D --> D1["① file-level edge\n② symbol-level edge\n(via symbol_node_map)"]
E --> E1["① file-level edge\n② symbol-level edge\n(via get_symbol_node_id DB query)"]
W --> W1["① file-level edge\n② symbol-level edge\n(via findNodeInFile stmt)"]
F1 & F2 & D1 & E1 & W1 --> DB[("SQLite DB\nedges table")]
DB --> K["classifyNodeRoles\nstructure.ts"]
K --> L{Full or Incremental?}
L -->|Full| M["fan_in = COUNT\nkind IN calls,imports-type\nMedian from classified nodes"]
L -->|Incremental| N["fan_in per node same\nGlobal median from ALL targets\n(incl. file nodes — P2 concern)"]
M & N --> O["Foo.fanIn > 0\n→ NOT dead-unresolved ✓"]
Reviews (2): Last reviewed commit: "fix: add symbol-level type-import edges ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (imp.typeOnly && ctx.nodesByNameAndFile) { | ||
| for (const name of imp.names) { | ||
| const cleanName = name.replace(/^\*\s+as\s+/, ''); | ||
| let targetFile = resolvedPath; | ||
| if (isBarrelFile(ctx, resolvedPath)) { | ||
| const actual = resolveBarrelExport(ctx, resolvedPath, cleanName); | ||
| if (actual) targetFile = actual; | ||
| } | ||
| const candidates = ctx.nodesByNameAndFile.get(`${cleanName}|${targetFile}`); | ||
| if (candidates && candidates.length > 0) { | ||
| allEdgeRows.push([fileNodeId, candidates[0]!.id, 'imports-type', 1.0, 0]); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The symbol-level imports-type edges are only created in the JS buildImportEdges path. The native Rust counterpart (crates/codegraph-core/src/import_edges.rs, build_import_edges) was not updated and only creates file-level edges:
// Rust only looks up file nodes — never symbol nodes
let target_id = match get_file_node_id(conn, &resolved_path) {
Some(id) => id,
None => continue,
};The fallback at line 790–793 only retriggers JS if native produced zero edges total. Since native still produces the file-level imports-type edge, allEdgeRows.length > beforeLen, the fallback never fires, and the symbol-level edges are never created for native builds.
Result: on any full build or incremental build with fileSymbols.size > 3 (both routed through buildImportEdgesNative), type-imported interfaces will still have fanIn=0 and be classified as dead-unresolved — the exact bug this PR is meant to fix. CLAUDE.md explicitly states that engine divergence is a bug that must be fixed.
There was a problem hiding this comment.
Fixed in df2fd4a. Added symbol-level imports-type edge emission to both native paths:
-
edge_builder.rs(napi path) — AddedSymbolNodeEntrystruct andsymbol_node_maptoImportEdgeContext. The JS side now passes all symbol nodes via a newsymbolNodesparameter. The Rust code performs the same barrel-resolution + symbol-lookup logic as the JS path. -
import_edges.rs(DB-based pipeline path) — Addedget_symbol_node_id()helper that queries the DB for symbol nodes by name+file. Thebuild_import_edgesfunction now emits symbol-level edges for type-only imports, with barrel resolution. -
incremental.ts(watch mode path) — Also lacked symbol-level edges. Now usesfindNodeInFileto resolve symbol nodes for type-only imports.
All 569 tests pass.
Codegraph Impact Analysis11 functions changed → 20 callers affected across 7 files
|
computeImportEdgeMaps queries all imports-type edges for file-level fan-in/fan-out metrics. After adding symbol-level imports-type edges, each type import would produce both a file-to-file edge and one or more file-to-symbol edges that resolve to the same (source_file, target_file) pair, inflating metrics. Adding AND n2.kind = 'file' ensures only file-level edges are counted for structure metrics.
The symbol-level imports-type edges added in the JS buildImportEdges path were missing from three other code paths: 1. Native napi build_import_edges (edge_builder.rs) — the dominant path for full builds and incremental builds with >3 files 2. Native DB-based build_import_edges (import_edges.rs) — used by the Rust build pipeline 3. Incremental single-file rebuild (incremental.ts) — used by watch mode Without these edges, type-imported symbols still had fanIn=0 on native builds and were falsely classified as dead-unresolved. Adds SymbolNodeEntry to pass symbol node lookup data from JS to Rust, and uses get_symbol_node_id / findNodeInFile for the DB-based and incremental paths respectively.
|
Addressed both P1 issues raised by Greptile: 1. Native engine parity gap (fixed in df2fd4a)
2. File-level metrics double-counting (fixed in a6aedc2)
All 569 tests pass. |
1 participant
Summary
Fixes #840
import type { Foo }) created file-levelimports-typeedges but no symbol-level edges, so interfaces/types consumed only via type imports hadfanIn=0and were falsely classified asdead-unresolvedimports-typeedges inbuildImportEdgestargeting actual imported symbol nodes (with barrel file resolution)imports-typeedges in fan-in,isExported, and production fan-in queries in both full and incremental role classification pathsTest plan