Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -953,15 +953,18 @@ 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
// `e.kind` (not the source node's kind) is the primary 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).
// a type-only import (Greptile, #1973). `caller.kind` is ALSO selected
// to further split that file-sourced `calls` case into its own
// `'topLevelCall'` kind (#2365), since its `name`/`line` are the file
// node's own values, not a real caller symbol/call-site.
let consumers_result = tx.prepare_cached(
"SELECT DISTINCT caller.name, caller.file, caller.line, e.kind \
"SELECT DISTINCT caller.name, caller.file, caller.line, caller.kind, 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",
);
Expand Down Expand Up @@ -992,16 +995,26 @@ pub fn record_deleted_export_advisories(conn: &Connection, removed_files: &[Stri
}
let _ = delete_stmt.execute([file]);
for (id, name, kind, line) in defs {
let consumers: Vec<(String, String, i64, String)> = match consumers_stmt
let consumers: Vec<(String, String, i64, String, String)> = match consumers_stmt
.query_map(rusqlite::params![id, file], |row| {
Ok((row.get(0)?, row.get(1)?, row.get(2)?, row.get(3)?))
Ok((
row.get(0)?,
row.get(1)?,
row.get(2)?,
row.get(3)?,
row.get(4)?,
))
}) {
Ok(rows) => rows.flatten().collect(),
Err(_) => continue,
};
for (consumer_name, consumer_file, consumer_line, edge_kind) in consumers {
for (consumer_name, consumer_file, consumer_line, caller_kind, edge_kind) in
consumers
{
let consumer_kind = if edge_kind == "imports-type" {
"file"
} else if caller_kind == "file" {
"topLevelCall"
} else {
"symbol"
};
Expand Down Expand Up @@ -2112,6 +2125,49 @@ mod tests {
);
}

/// Issue #2365: a genuine `calls` edge sourced from a FILE node
/// (findCaller's fallback for a bare top-level call with no enclosing
/// function/binding) must get its own `'topLevelCall'` consumer_kind,
/// distinct from `'symbol'` — `consumer_name`/`consumer_line` here are
/// the file node's own values, not a real caller symbol/call-site, so
/// lumping it in with a genuine named caller would let `codegraph check`
/// present a filename as if it were a calling function.
#[test]
fn record_deleted_export_advisories_discriminates_top_level_call_from_named_caller() {
let conn = test_conn_with_advisories();
let target = insert_exported_node(&conn, "target", "function", "src/gone.js", 1);
let caller_fn = insert_node(&conn, "callerA", "function", "src/a.js", 1);
let caller_file = insert_node(&conn, "src/c.js", "file", "src/c.js", 0);
conn.execute(
"INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?1, ?2, 'calls', 1.0, 0)",
rusqlite::params![caller_fn, target],
)
.unwrap();
conn.execute(
"INSERT INTO edges (source_id, target_id, kind, confidence, dynamic) VALUES (?1, ?2, 'calls', 1.0, 0)",
rusqlite::params![caller_file, target],
)
.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<String>)> = 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/c.js".to_string(), Some("topLevelCall".to_string())),
]
);
}

#[test]
fn record_deleted_export_advisories_skips_export_with_no_external_consumers() {
let conn = test_conn_with_advisories();
Expand Down
6 changes: 4 additions & 2 deletions src/db/repository/deleted-export-advisories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,10 @@ export function getDeletedExportAdvisories(
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'
// other pre-#1973 advisory row (#1973). 'topLevelCall' added by #2365.
...(row.consumer_kind === 'file' ||
row.consumer_kind === 'symbol' ||
row.consumer_kind === 'topLevelCall'
? { consumerKind: row.consumer_kind }
: {}),
});
Expand Down
35 changes: 20 additions & 15 deletions src/db/repository/edges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,25 +225,30 @@ export function findExternalConsumers(
const rows = cachedStmt(
_findExternalConsumersStmt,
db,
`SELECT DISTINCT caller.name, caller.file, caller.line, e.kind AS edgeKind
`SELECT DISTINCT caller.name, caller.file, caller.line, caller.kind AS callerKind, 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) as Array<ExternalConsumerRow & { edgeKind: string }>;
// `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 }) => ({
).all(nodeId, file) as Array<ExternalConsumerRow & { callerKind: string; edgeKind: string }>;
// `consumerKind` discriminates three cases. Keyed primarily 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). That file-sourced `calls` case gets its own `'topLevelCall'`
// kind (#2365) rather than being lumped in with `'symbol'`, since
// `name`/`line` there are the file node's own values, not a real caller
// symbol/call-site — renderers must not present either `'file'` or
// `'topLevelCall'` entries as if they were a named caller.
return rows.map(({ callerKind, edgeKind, ...row }) => ({
...row,
consumerKind: edgeKind === 'imports-type' ? ('file' as const) : ('symbol' as const),
consumerKind:
edgeKind === 'imports-type'
? ('file' as const)
: callerKind === 'file'
? ('topLevelCall' as const)
: ('symbol' as const),
Comment thread
carlos-alm marked this conversation as resolved.
}));
}

Expand Down
36 changes: 21 additions & 15 deletions src/domain/analysis/exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ function exportsFileImpl(
const consumersStmt = cachedStmt(
_consumersStmtCache,
db,
`SELECT n.name, n.file, n.line, e.kind AS edgeKind FROM edges e JOIN nodes n ON e.source_id = n.id
`SELECT n.name, n.file, n.line, n.kind AS callerKind, e.kind AS edgeKind FROM edges e JOIN nodes n ON e.source_id = n.id
WHERE e.target_id = ? AND e.kind IN ('calls', 'imports-type')`,
);
const reexportsFromStmt = cachedStmt(
Expand Down Expand Up @@ -232,6 +232,7 @@ function exportsFileImpl(
name: string;
file: string;
line: number;
callerKind: string;
edgeKind: string;
}>;
if (noTests) consumers = consumers.filter((c) => !isTestFile(c.file));
Expand All @@ -244,24 +245,29 @@ function exportsFileImpl(
role: s.role || null,
signature: fileLines ? extractSignature(fileLines, s.line, displayOpts) : null,
summary: fileLines ? extractSummary(fileLines, s.line, displayOpts) : null,
// `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/#2189). Renderers
// must not treat `name`/`line` on a `'file'` entry as a caller
// symbol/call-site (#1830).
// `consumerKind` discriminates three cases. Keyed primarily 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/#2189).
// That file-sourced `calls` case gets its own `'topLevelCall'` kind
// (#2365) rather than being lumped in with `'symbol'`, since
// `name`/`line` there are the file node's own values, not a real
// caller symbol/call-site. Renderers must not present either
// `'file'` or `'topLevelCall'` entries as if they were a named
// caller (#1830).
consumers: consumers.map((c) => ({
name: c.name,
file: c.file,
line: c.line,
consumerKind: c.edgeKind === 'imports-type' ? ('file' as const) : ('symbol' as const),
consumerKind:
c.edgeKind === 'imports-type'
? ('file' as const)
: c.callerKind === 'file'
? ('topLevelCall' as const)
: ('symbol' as const),
})),
consumerCount: consumers.length,
};
Expand Down
2 changes: 1 addition & 1 deletion src/features/check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,7 @@ interface ConsumerRef {
file: string;
line: number;
/** See `ExternalConsumerRow.consumerKind` — absent for advisory-derived rows (#1973). */
consumerKind?: 'file' | 'symbol';
consumerKind?: 'file' | 'symbol' | 'topLevelCall';
}

interface SignatureViolation {
Expand Down
8 changes: 7 additions & 1 deletion src/presentation/check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,12 @@ interface CheckViolation {
edgeKind?: string;
/** Set when this violation comes from `checkNoDeletedExportsInUse` (#1806). */
reason?: string;
consumers?: Array<{ name: string; file: string; line: number; consumerKind?: 'file' | 'symbol' }>;
consumers?: Array<{
name: string;
file: string;
line: number;
consumerKind?: 'file' | 'symbol' | 'topLevelCall';
}>;
}

interface CheckPredicate {
Expand Down Expand Up @@ -96,6 +101,7 @@ function formatPredicateViolations(pred: CheckPredicate): void {
.map((c) => {
if (c.consumerKind === 'file') return `${c.file} (type-only import)`;
if (c.consumerKind === 'symbol') return `${c.file}:${c.line}`;
if (c.consumerKind === 'topLevelCall') return `${c.file} (top-level call)`;
return `${c.file} (kind unknown — pre-existing advisory)`;
})
.join(', ');
Expand Down
8 changes: 7 additions & 1 deletion src/presentation/queries-cli/exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,21 @@ interface ExportConsumer {
* call-site). `'file'` — a whole-file reference such as
* `import type { X }`, where `name` equals `file` and `line` is always
* `0` because there is no specific call-site to report (#1830).
* `'topLevelCall'` — a genuine `calls` edge sourced from a bare top-level
* statement with no enclosing function/binding: `name`/`line` are the
* file node's own values, not a real caller symbol/call-site (#2365).
*/
consumerKind: 'file' | 'symbol';
consumerKind: 'file' | 'symbol' | 'topLevelCall';
}

/** Render one consumer entry, without a fabricated call-site line for file-level entries. */
function formatConsumer(c: ExportConsumer): string {
if (c.consumerKind === 'file') {
return `${c.file} (type-only import)`;
}
if (c.consumerKind === 'topLevelCall') {
return `${c.file} (top-level call)`;
}
return `${c.name} (${c.file}:${c.line})`;
}

Expand Down
30 changes: 20 additions & 10 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,19 +232,25 @@ 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).
* `consumerKind` discriminates three cases: `'symbol'` — a real
* caller/constructor (`name`/`line` are a genuine call-site); `'file'` — 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,
* #1830); `'topLevelCall'` — a genuine `calls` edge whose source is a bare
* top-level statement with no enclosing function/binding, so `findCaller`
* falls back to the FILE node itself as the edge's source (#2365) — `name`
* and `line` are the file node's own values (the file's basename, line `0`),
* not a real caller symbol/call-site, but this is still a genuine call
* (unlike `'file'`, which is never sourced from an actual `calls` edge).
* 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';
consumerKind?: 'file' | 'symbol' | 'topLevelCall';
}

/** Import target/source row. */
Expand Down Expand Up @@ -2435,19 +2441,23 @@ export interface FileExportEntry {
}

/**
* A single caller of an exported symbol. `consumerKind` discriminates two
* A single caller of an exported symbol. `consumerKind` discriminates three
* shapes that share this same struct:
* - `'symbol'` — a real caller/constructor: `name` is the calling
* function/method/class, `line` is the actual call-site line.
* - `'file'` — a whole-file reference such as `import type { X }`, where
* there is no specific calling symbol: `name` equals `file` and `line`
* is always `0` (no real call-site exists to report; see #1830).
* - `'topLevelCall'` — a genuine `calls` edge sourced from a bare
* top-level statement with no enclosing function/binding: `findCaller`
* falls back to the file node itself, so `name`/`line` are the file
* node's own values, not a real caller symbol/call-site (#2365).
*/
export interface FileExportConsumer {
name: string;
file: string;
line: number;
consumerKind: 'file' | 'symbol';
consumerKind: 'file' | 'symbol' | 'topLevelCall';
}

// ── Path ─────────────────────────────────────────────────────────────
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/check.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -979,7 +979,7 @@ describe('checkNoDeletedExportsInUse', () => {
const violation = result.violations.find((v) => v.name === 'topLevelTarget');
expect(violation).toBeDefined();
expect(violation.consumers).toEqual([
expect.objectContaining({ file: 'src/handler.js', consumerKind: 'symbol' }),
expect.objectContaining({ file: 'src/handler.js', consumerKind: 'topLevelCall' }),
]);
});
});
Expand Down
15 changes: 14 additions & 1 deletion tests/integration/exports.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,20 @@ describe('exportsData — import type consumer crediting (#1724)', () => {
const target = data.results.find((r) => r.name === 'topLevelTarget');
expect(target).toBeDefined();
expect(target.consumers.length).toBe(1);
expect(target.consumers[0].consumerKind).toBe('symbol');
expect(target.consumers[0].consumerKind).not.toBe('file');
});

// Regression coverage for #2365: that same top-level-call consumer must
// get its OWN discriminator distinct from a real named caller — `name`
// and `line` here are the file node's own values (the file's basename,
// line 0), not a genuine call-site, so lumping it in with 'symbol' would
// let renderers present a filename as if it were a calling function.
test('a top-level call sourced from a file node is discriminated distinctly from a real named caller (#2365)', () => {
const data = exportsData('types.ts', dbPath2);
const target = data.results.find((r) => r.name === 'topLevelTarget');
expect(target.consumers[0].consumerKind).toBe('topLevelCall');
expect(target.consumers[0].name).toBe('consumer.ts');
expect(target.consumers[0].line).toBe(0);
});

test('interface consumed only via `import type` is excluded from --unused', () => {
Expand Down
Loading
Loading