diff --git a/crates/codegraph-core/src/db/connection.rs b/crates/codegraph-core/src/db/connection.rs index fe9353925..dd090d461 100644 --- a/crates/codegraph-core/src/db/connection.rs +++ b/crates/codegraph-core/src/db/connection.rs @@ -356,6 +356,12 @@ const MIGRATIONS: &[Migration] = &[ CREATE INDEX IF NOT EXISTS idx_deleted_export_advisories_file ON deleted_export_advisories(file); "#, }, + Migration { + version: 22, + up: r#" + ALTER TABLE deleted_export_advisories ADD COLUMN consumer_kind TEXT; + "#, + }, ]; // ── napi types ────────────────────────────────────────────────────────── diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs b/crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs index 29e59bbd0..be68979fe 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/detect_changes.rs @@ -874,15 +874,22 @@ pub fn record_deleted_export_advisories(conn: &Connection, removed_files: &[Stri WHERE file = ?1 AND kind IN ('function', 'method', 'class') AND exported = 1 \ ORDER BY line", ); + // `e.kind` (not the source node's kind) is the discriminator: an + // `imports-type` edge is always sourced from the importing file's own + // node by construction, while a `calls` edge is always a genuine call + // even when `findCaller`'s TS/Rust mirror falls back to the file node as + // source for a bare top-level call with no enclosing function/binding — + // keying on source-node kind instead would misclassify that real call as + // a type-only import (Greptile, #1973). let consumers_result = tx.prepare( - "SELECT DISTINCT caller.name, caller.file, caller.line \ + "SELECT DISTINCT caller.name, caller.file, caller.line, e.kind \ FROM edges e JOIN nodes caller ON e.source_id = caller.id \ WHERE e.target_id = ?1 AND e.kind IN ('calls', 'imports-type') AND caller.file != ?2", ); let insert_result = tx.prepare( "INSERT INTO deleted_export_advisories \ - (file, name, kind, line, consumer_name, consumer_file, consumer_line, deleted_at) \ - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)", + (file, name, kind, line, consumer_name, consumer_file, consumer_line, consumer_kind, deleted_at) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)", ); if let (Ok(mut defs_stmt), Ok(mut consumers_stmt), Ok(mut insert_stmt)) = @@ -905,14 +912,15 @@ pub fn record_deleted_export_advisories(conn: &Connection, removed_files: &[Stri } let _ = tx.execute("DELETE FROM deleted_export_advisories WHERE file = ?1", [file]); for (id, name, kind, line) in defs { - let consumers: Vec<(String, String, i64)> = match consumers_stmt + let consumers: Vec<(String, String, i64, String)> = match consumers_stmt .query_map(rusqlite::params![id, file], |row| { - Ok((row.get(0)?, row.get(1)?, row.get(2)?)) + Ok((row.get(0)?, row.get(1)?, row.get(2)?, row.get(3)?)) }) { Ok(rows) => rows.flatten().collect(), Err(_) => continue, }; - for (consumer_name, consumer_file, consumer_line) in consumers { + for (consumer_name, consumer_file, consumer_line, edge_kind) in consumers { + let consumer_kind = if edge_kind == "imports-type" { "file" } else { "symbol" }; let _ = insert_stmt.execute(rusqlite::params![ file, name, @@ -921,6 +929,7 @@ pub fn record_deleted_export_advisories(conn: &Connection, removed_files: &[Stri consumer_name, consumer_file, consumer_line, + consumer_kind, now ]); } @@ -1593,6 +1602,7 @@ mod tests { consumer_name TEXT NOT NULL, consumer_file TEXT NOT NULL, consumer_line INTEGER NOT NULL, + consumer_kind TEXT, deleted_at INTEGER NOT NULL );", ) @@ -1647,6 +1657,47 @@ mod tests { ); } + /// Issue #1973: `consumer_kind` must be derived from the *edge* kind + /// ('calls' -> 'symbol', 'imports-type' -> 'file'), not the source node's + /// own kind — a genuine top-level call can legitimately be sourced from a + /// file node too (findCaller's fallback for a call with no enclosing + /// function/binding), so keying on source-node kind would misclassify it. + #[test] + fn record_deleted_export_advisories_derives_consumer_kind_from_edge_kind() { + let conn = test_conn_with_advisories(); + let helper = insert_exported_node(&conn, "helper", "function", "src/gone.js", 1); + let caller_a = insert_node(&conn, "callerA", "function", "src/a.js", 1); + let caller_b = insert_node(&conn, "callerB", "function", "src/b.js", 1); + conn.execute( + "INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?1, ?2, 'calls', 1.0, 0)", + rusqlite::params![caller_a, helper], + ) + .unwrap(); + conn.execute( + "INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?1, ?2, 'imports-type', 1.0, 0)", + rusqlite::params![caller_b, helper], + ) + .unwrap(); + + record_deleted_export_advisories(&conn, &["src/gone.js".to_string()]); + + let mut stmt = conn + .prepare("SELECT consumer_file, consumer_kind FROM deleted_export_advisories WHERE file = ?1 ORDER BY consumer_file") + .unwrap(); + let rows: Vec<(String, Option)> = stmt + .query_map(["src/gone.js"], |row| Ok((row.get(0)?, row.get(1)?))) + .unwrap() + .flatten() + .collect(); + assert_eq!( + rows, + vec![ + ("src/a.js".to_string(), Some("symbol".to_string())), + ("src/b.js".to_string(), Some("file".to_string())), + ] + ); + } + #[test] fn record_deleted_export_advisories_skips_export_with_no_external_consumers() { let conn = test_conn_with_advisories(); diff --git a/src/db/migrations.ts b/src/db/migrations.ts index 47bd3b9b4..ab0e6b7dd 100644 --- a/src/db/migrations.ts +++ b/src/db/migrations.ts @@ -348,6 +348,12 @@ export const MIGRATIONS: Migration[] = [ CREATE INDEX IF NOT EXISTS idx_deleted_export_advisories_file ON deleted_export_advisories(file); `, }, + { + version: 22, + up: ` + ALTER TABLE deleted_export_advisories ADD COLUMN consumer_kind TEXT; + `, + }, ]; interface PragmaColumnInfo { diff --git a/src/db/repository/deleted-export-advisories.ts b/src/db/repository/deleted-export-advisories.ts index cddf65109..5de7fd0b4 100644 --- a/src/db/repository/deleted-export-advisories.ts +++ b/src/db/repository/deleted-export-advisories.ts @@ -35,6 +35,7 @@ interface DeletedExportAdvisoryRow { consumer_name: string; consumer_file: string; consumer_line: number; + consumer_kind: string | null; } /** @@ -53,6 +54,22 @@ function hasAdvisoryTable(db: BetterSqlite3Database): boolean { } } +/** + * `consumer_kind` was only added in migration v22 (#1973) — a read-only + * `check` invocation can still hit a DB whose `deleted_export_advisories` + * table exists (v21) but hasn't run v22 yet, if the last write-mode + * `codegraph build` predates this column. Same try/catch probe pattern as + * `hasAdvisoryTable` above, for the same reason. + */ +function hasConsumerKindColumn(db: BetterSqlite3Database): boolean { + try { + db.prepare('SELECT consumer_kind FROM deleted_export_advisories LIMIT 1').get(); + return true; + } catch { + return false; + } +} + /** * Snapshots, for each deleted export that still has an external consumer, * one row per consumer — captured by `detectChanges` BEFORE the build @@ -79,8 +96,8 @@ export function recordDeletedExportAdvisories( const deleteStmt = db.prepare('DELETE FROM deleted_export_advisories WHERE file = ?'); const insertStmt = db.prepare( `INSERT INTO deleted_export_advisories - (file, name, kind, line, consumer_name, consumer_file, consumer_line, deleted_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, + (file, name, kind, line, consumer_name, consumer_file, consumer_line, consumer_kind, deleted_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, ); const tx = db.transaction(() => { @@ -103,6 +120,7 @@ export function recordDeletedExportAdvisories( consumer.name, consumer.file, consumer.line, + consumer.consumerKind ?? null, now, ); } @@ -151,9 +169,12 @@ export function getDeletedExportAdvisories( if (files.length === 0 || !hasAdvisoryTable(db)) return []; const placeholders = files.map(() => '?').join(','); + const consumerKindSelect = hasConsumerKindColumn(db) + ? ', consumer_kind' + : ', NULL AS consumer_kind'; const rows = db .prepare( - `SELECT file, name, kind, line, consumer_name, consumer_file, consumer_line + `SELECT file, name, kind, line, consumer_name, consumer_file, consumer_line${consumerKindSelect} FROM deleted_export_advisories WHERE file IN (${placeholders}) ORDER BY file, line`, @@ -173,6 +194,12 @@ export function getDeletedExportAdvisories( name: row.consumer_name, file: row.consumer_file, line: row.consumer_line, + // Rows persisted before migration v22 have consumer_kind = NULL — leave + // consumerKind undefined for those rather than guessing, same as any + // other pre-#1973 advisory row (#1973). + ...(row.consumer_kind === 'file' || row.consumer_kind === 'symbol' + ? { consumerKind: row.consumer_kind } + : {}), }); } return [...grouped.values()].filter((e) => e.consumers.length > 0); diff --git a/src/db/repository/edges.ts b/src/db/repository/edges.ts index a17390913..a9d75f65c 100644 --- a/src/db/repository/edges.ts +++ b/src/db/repository/edges.ts @@ -222,14 +222,29 @@ export function findExternalConsumers( nodeId: number, file: string, ): ExternalConsumerRow[] { - return cachedStmt( + const rows = cachedStmt( _findExternalConsumersStmt, db, - `SELECT DISTINCT caller.name, caller.file, caller.line + `SELECT DISTINCT caller.name, caller.file, caller.line, e.kind AS edgeKind FROM edges e JOIN nodes caller ON e.source_id = caller.id WHERE e.target_id = ? AND e.kind IN ('calls', 'imports-type') AND caller.file != ?`, - ).all(nodeId, file); + ).all(nodeId, file) as Array; + // `consumerKind` discriminates a real caller/constructor symbol (a genuine + // `calls` edge, with a real call-site line) from a whole-file reference + // such as `import type { X }` (an `imports-type` edge, always sourced from + // the importing file node itself — see emitNamedSymbolEdges). Keyed off the + // *edge* kind, not the source node's kind: findCaller falls back to the + // file node as a call's source for a genuine top-level call with no + // enclosing function/binding (e.g. a bare statement at module scope), so a + // `calls` edge can legitimately have a file-kind source too — using source + // kind alone would misclassify that real call as a type-only import + // (Greptile, #1973). Renderers must not treat `name`/`line` on a `'file'` + // entry as a caller symbol/call-site. + return rows.map(({ edgeKind, ...row }) => ({ + ...row, + consumerKind: edgeKind === 'imports-type' ? ('file' as const) : ('symbol' as const), + })); } /** diff --git a/src/features/check.ts b/src/features/check.ts index d53f9b70d..e544be867 100644 --- a/src/features/check.ts +++ b/src/features/check.ts @@ -564,6 +564,8 @@ interface ConsumerRef { name: string; file: string; line: number; + /** See `ExternalConsumerRow.consumerKind` — absent for advisory-derived rows (#1973). */ + consumerKind?: 'file' | 'symbol'; } interface SignatureViolation { diff --git a/src/presentation/check.ts b/src/presentation/check.ts index dba2601b6..5cc8e2f43 100644 --- a/src/presentation/check.ts +++ b/src/presentation/check.ts @@ -31,7 +31,7 @@ interface CheckViolation { edgeKind?: string; /** Set when this violation comes from `checkNoDeletedExportsInUse` (#1806). */ reason?: string; - consumers?: Array<{ name: string; file: string; line: number }>; + consumers?: Array<{ name: string; file: string; line: number; consumerKind?: 'file' | 'symbol' }>; } interface CheckPredicate { @@ -81,9 +81,25 @@ function formatPredicateViolations(pred: CheckPredicate): void { return `${v.from} -> ${v.to} (${v.edgeKind})`; } if (v.reason === 'file-deleted' && v.consumers) { + // `consumerKind === 'file'` means this is a type-only-import reference, + // not a real call-site — `c.line` is a fabricated `0` in that case, so + // rendering it as `file:0` would misleadingly look like a real line + // number (#1973). `consumerKind === undefined` means an advisory row + // persisted before this discriminator existed (or before migration + // v22 added the column) — its underlying nodes/edges are already + // purged by definition (that's why it fell back to the advisory + // snapshot at all), so there is no way to retroactively re-derive + // which case it was. Render that as explicitly unknown rather than + // defaulting to file:line, which would silently re-introduce the same + // "confidently wrong" fabricated-line risk for exactly the legacy rows + // that can't be verified (Greptile, #1973). const sample = v.consumers .slice(0, 3) - .map((c) => `${c.file}:${c.line}`) + .map((c) => { + if (c.consumerKind === 'file') return `${c.file} (type-only import)`; + if (c.consumerKind === 'symbol') return `${c.file}:${c.line}`; + return `${c.file} (kind unknown — pre-existing advisory)`; + }) .join(', '); const more = v.consumers.length > 3 ? `, ... and ${v.consumers.length - 3} more` : ''; return `${v.name} (${v.kind}) — file ${v.file} deleted but still used by ${v.consumers.length} external consumer(s): ${sample}${more}`; diff --git a/src/types.ts b/src/types.ts index 0c2d5fe46..3d011627e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -225,11 +225,20 @@ export interface ExportedDefRow { /** * A cross-file consumer of an exported symbol (from findExternalConsumers), * or a persisted deleted-export advisory's consumer row (#1938). + * + * `consumerKind` discriminates a real caller/constructor symbol (`name`/`line` + * are a genuine call-site) from a whole-file reference such as + * `import type { X}` (`name` equals `file`, `line` is always `0` because there + * is no specific call-site to report) — mirrors the same discriminator on + * exports' consumer rows (#1830). Optional because the persisted + * deleted-export-advisories snapshot (#1938) doesn't store this discriminator; + * only `findExternalConsumers`'s live-DB query populates it (#1973). */ export interface ExternalConsumerRow { name: string; file: string; line: number; + consumerKind?: 'file' | 'symbol'; } /** Import target/source row. */ diff --git a/tests/integration/check.test.ts b/tests/integration/check.test.ts index cda203041..c5467a44b 100644 --- a/tests/integration/check.test.ts +++ b/tests/integration/check.test.ts @@ -56,6 +56,17 @@ beforeAll(() => { const add = insertNode(db, 'add', 'function', 'src/math.js', 1, 5, 1); const multiply = insertNode(db, 'multiply', 'function', 'src/math.js', 7, 12, 1); insertNode(db, 'roundHalfEven', 'function', 'src/math.js', 14, 16, 0); + // MathOpts (exported class, src/math.js): only ever referenced via + // `import type { MathOpts }` from utils.js — a file-level consumer, not a + // real call-site (issue #1973). + const mathOpts = insertNode(db, 'MathOpts', 'class', 'src/math.js', 18, 20, 1); + // topLevelTarget (exported function, src/math.js): called from a bare + // top-level statement in handler.js with no enclosing function/binding — + // findCaller falls back to the *file* node as the call's source in that + // case, so this is a genuine 'calls' edge sourced from a file-kind node + // (issue #1973, Greptile finding: must not be misclassified as a + // type-only import just because its source happens to be a file node). + const topLevelTarget = insertNode(db, 'topLevelTarget', 'function', 'src/math.js', 22, 24, 1); // src/utils.js: formatResult (line 1-10), parseInput (line 12-20) const formatResult = insertNode(db, 'formatResult', 'function', 'src/utils.js', 1, 10); @@ -103,6 +114,14 @@ beforeAll(() => { // No cycle for handler.js insertEdge(db, fileHandler, fileMath, 'imports'); + + // fileUtils.js does `import type { MathOpts } from './math.js'` — a + // file-level (not symbol-level) consumer of MathOpts (issue #1973). + insertEdge(db, fileUtils, mathOpts, 'imports-type'); + + // handler.js calls topLevelTarget() from a bare top-level statement — a + // real 'calls' edge sourced from the file node itself (issue #1973). + insertEdge(db, fileHandler, topLevelTarget, 'calls'); }); afterAll(() => { @@ -932,6 +951,37 @@ describe('checkNoDeletedExportsInUse', () => { const noTests = checkNoDeletedExportsInUse(db, new Set(['src/only-test-consumer.js']), true); expect(noTests.violations.map((v) => v.name)).not.toContain('onlyTestConsumer'); }); + + test('discriminates a file-level (imports-type) consumer from a real call-site consumer (#1973)', () => { + // MathOpts is only ever referenced via `import type { MathOpts }` from + // utils.js — a whole-file reference (source is the importing file node), + // not a genuine call/construct site with a real line number. + const result = checkNoDeletedExportsInUse(db, new Set(['src/math.js']), false); + const violation = result.violations.find((v) => v.name === 'MathOpts'); + expect(violation).toBeDefined(); + // `line` here is whatever the shared `fileUtils` file node in this fixture + // happens to carry (1) — not a fabricated call-site line, since a file + // node has no real one. The discriminator (`consumerKind: 'file'`) is + // what distinguishes this from a genuine call/construct site; renderers + // must key off that field, not assume any particular line value (#1973). + expect(violation.consumers).toEqual([ + expect.objectContaining({ file: 'src/utils.js', consumerKind: 'file' }), + ]); + }); + + test('does not misclassify a genuine top-level call sourced from a file node as a type-only import (#1973)', () => { + // topLevelTarget is called via a real 'calls' edge whose source happens + // to be the file node (handler.js has no enclosing function/binding for + // this call) — the discriminator must key off the *edge* kind, not the + // source node's kind, or this would be wrongly reported as a type-only + // import instead of a genuine dangling caller. + const result = checkNoDeletedExportsInUse(db, new Set(['src/math.js']), false); + const violation = result.violations.find((v) => v.name === 'topLevelTarget'); + expect(violation).toBeDefined(); + expect(violation.consumers).toEqual([ + expect.objectContaining({ file: 'src/handler.js', consumerKind: 'symbol' }), + ]); + }); }); // ─── checkNoDeletedExportsInUse: advisory fallback (issue #1938) ───────