diff --git a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs index ba9c732eb..288f907c1 100644 --- a/crates/codegraph-core/src/domain/graph/builder/pipeline.rs +++ b/crates/codegraph-core/src/domain/graph/builder/pipeline.rs @@ -744,8 +744,8 @@ pub fn run_pipeline( // `barrel_candidates_added` (empty on full builds) rather than every key // in `file_symbols` — a file that's genuinely part of this build's // changed set must always get its own non-reexport imports emitted, - // regardless of whether it happens to satisfy the reexports>=ownDefs - // heuristic (#1848). + // regardless of whether it happens to satisfy the reexports-outnumber- + // ownDefs heuristic (#1848, #2339). import_ctx.reexport_map = import_edges::build_reexport_map(&import_ctx); import_ctx.barrel_only_files = import_edges::detect_barrel_only_files(&import_ctx, &barrel_candidates_added); diff --git a/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs b/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs index ae7eaca0b..77bf147a3 100644 --- a/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs +++ b/crates/codegraph-core/src/domain/graph/builder/stages/import_edges.rs @@ -29,7 +29,7 @@ pub struct ImportEdgeContext { pub batch_resolved: HashMap, /// Map of relPath -> reexport entries. pub reexport_map: HashMap>, - /// Set of files that are barrel-only (reexport count >= definition count). + /// Set of files that are barrel-only (reexport count > definition count). pub barrel_only_files: HashSet, /// Parsed symbols per relative path. pub file_symbols: BTreeMap, @@ -72,7 +72,16 @@ impl ImportEdgeContext { ) } - /// Check if a file is a barrel file (reexport count >= definition count). + /// Check if a file is a barrel file (reexport count strictly exceeds + /// definition count). Strict `>`, not `>=` (issue #2339): a file with + /// exactly one reexport and one own definition is a genuine hybrid (real + /// logic plus a reexport), not a pure barrel — `>=` misclassified it as + /// barrel-only, silently dropping its own outgoing call/receiver edges + /// whenever it got pulled into Stage 6b's barrel-candidate reparse. This + /// is orthogonal to #1848's fix, which scopes *which files* are even + /// eligible for this check (only transient barrel-candidate reparses, + /// never a file genuinely part of this build's changed set) — not the + /// comparison itself. pub fn is_barrel_file(&self, rel_path: &str) -> bool { let symbols = match self.file_symbols.get(rel_path) { Some(s) => s, @@ -86,7 +95,7 @@ impl ImportEdgeContext { if reexport_count == 0 { return false; } - reexport_count >= symbols.definitions.len() + reexport_count > symbols.definitions.len() } /// Recursively resolve a barrel export to its actual source file. @@ -1066,8 +1075,8 @@ mod tests { /// Regression test for #1848: `detect_barrel_only_files` must only /// classify files present in the supplied candidate list, even when a - /// file outside that list also satisfies the reexports>=ownDefs - /// heuristic. `run_pipeline` relies on this scoping to keep a + /// file outside that list also satisfies the reexports-outnumber- + /// ownDefs heuristic. `run_pipeline` relies on this scoping to keep a /// genuinely-changed (or, on a full build, every) file's own /// non-reexport imports from ever being dropped — only files /// transiently side-loaded by `reparse_barrel_candidates` for barrel diff --git a/src/domain/graph/builder/stages/resolve-imports.ts b/src/domain/graph/builder/stages/resolve-imports.ts index 6d6e78466..9c32d97c8 100644 --- a/src/domain/graph/builder/stages/resolve-imports.ts +++ b/src/domain/graph/builder/stages/resolve-imports.ts @@ -120,11 +120,11 @@ function findBarrelCandidates( * them for the next level of barrel candidates. * * A re-parsed file is marked `barrel-only` only when it really is one (the - * `isBarrelFile` check — reexports >= ownDefs). The previous unconditional - * `.add(relPath)` caused hybrid barrels with many local defs (e.g. a file - * with one `export type ... from` and dozens of internal functions) to drop - * all their non-reexport imports in build-edges, since the barrel-only branch - * skips them (#1174). + * `isBarrelFile` check — reexports strictly outnumber ownDefs, #2339). The + * previous unconditional `.add(relPath)` caused hybrid barrels with many + * local defs (e.g. a file with one `export type ... from` and dozens of + * internal functions) to drop all their non-reexport imports in + * build-edges, since the barrel-only branch skips them (#1174). */ async function reparseBarrelFiles( ctx: PipelineContext, @@ -146,6 +146,22 @@ async function reparseBarrelFiles( // candidates are merged here *after* insertNodes, so wiping those kinds // would permanently drop them (mirrors the Rust orchestrator's Stage 6b // delete in domain/graph/builder/pipeline.rs). + // + // Clear dataflow rows that reference these outgoing edges via call_edge_id + // BEFORE deleting the edges — avoids a FOREIGN KEY constraint failure when + // `PRAGMA foreign_keys` is on (`dataflow.call_edge_id REFERENCES edges.id`). + // Discovered while writing #2339's regression test: this delete previously + // threw on every barrel candidate whose own call edges were also tracked by + // interprocedural dataflow, and the exception was silently swallowed by the + // catch below (only surfaced via `debug()`, which is a no-op unless + // verbose), so the barrel simply never got reparsed — the Rust engine + // already had this exact fix (#979); the JS engine never got the mirror. + const deleteReferencingDataflow = db.prepare( + `DELETE FROM dataflow WHERE call_edge_id IN ( + SELECT id FROM edges WHERE source_id IN (SELECT id FROM nodes WHERE file = ?) + AND kind NOT IN ('contains', 'parameter_of') + )`, + ); const deleteOutgoingEdges = db.prepare( `DELETE FROM edges WHERE source_id IN (SELECT id FROM nodes WHERE file = ?) AND kind NOT IN ('contains', 'parameter_of')`, @@ -155,6 +171,7 @@ async function reparseBarrelFiles( try { const barrelSymbols = await parseFilesAuto(barrelPaths, rootDir, engineOpts); for (const [relPath, fileSym] of barrelSymbols) { + deleteReferencingDataflow.run(relPath); deleteOutgoingEdges.run(relPath); fileSymbols.set(relPath, fileSym); if (isBarrelFile(ctx, relPath)) { @@ -330,13 +347,24 @@ export function getResolved(ctx: PipelineContext, absFile: string, importSource: return resolveImportPath(absFile, importSource, ctx.rootDir, ctx.aliases, ctx.allFiles); } +/** + * A file is a barrel file when its reexport count strictly exceeds its + * definition count. Strict `>`, not `>=` (issue #2339): a file with exactly + * one reexport and one own definition is a genuine hybrid (real logic plus + * a reexport), not a pure barrel — `>=` misclassified it as barrel-only, + * silently dropping its own outgoing call/receiver edges whenever it got + * pulled into `reparseBarrelFiles`' transient reparse. Orthogonal to + * #1848's fix, which scopes *which files* are even eligible for this check + * (only transient barrel-candidate reparses, never a file genuinely part of + * this build's changed set) — not the comparison itself. + */ export function isBarrelFile(ctx: PipelineContext, relPath: string): boolean { const symbols = ctx.fileSymbols.get(relPath); if (!symbols) return false; const reexports = symbols.imports.filter((imp) => imp.reexport); if (reexports.length === 0) return false; const ownDefs = symbols.definitions.length; - return reexports.length >= ownDefs; + return reexports.length > ownDefs; } /** Check if a re-export source directly defines the symbol. */ diff --git a/tests/fixtures/issue-2339-barrel-tiebreak/app.js b/tests/fixtures/issue-2339-barrel-tiebreak/app.js new file mode 100644 index 000000000..d99e243b6 --- /dev/null +++ b/tests/fixtures/issue-2339-barrel-tiebreak/app.js @@ -0,0 +1,5 @@ +import { doWork } from './hybrid.js'; + +export function main(input) { + return doWork(input); +} diff --git a/tests/fixtures/issue-2339-barrel-tiebreak/helper.js b/tests/fixtures/issue-2339-barrel-tiebreak/helper.js new file mode 100644 index 000000000..da257aaa6 --- /dev/null +++ b/tests/fixtures/issue-2339-barrel-tiebreak/helper.js @@ -0,0 +1,3 @@ +export function helperFn(input) { + return input; +} diff --git a/tests/fixtures/issue-2339-barrel-tiebreak/hybrid.js b/tests/fixtures/issue-2339-barrel-tiebreak/hybrid.js new file mode 100644 index 000000000..a5a7bd57d --- /dev/null +++ b/tests/fixtures/issue-2339-barrel-tiebreak/hybrid.js @@ -0,0 +1,12 @@ +// Hybrid file at the exact tie boundary (issue #2339): exactly one reexport +// and exactly one own definition. Under the old `reexports >= ownDefs` +// heuristic this was misclassified as barrel-only, so its own outgoing call +// edge to helper.js was silently dropped whenever this file got reparsed as +// a Stage 6b barrel candidate on an incremental rebuild. +export { Named } from './other.js'; + +import { helperFn } from './helper.js'; + +export function doWork(input) { + return helperFn(input); +} diff --git a/tests/fixtures/issue-2339-barrel-tiebreak/other.js b/tests/fixtures/issue-2339-barrel-tiebreak/other.js new file mode 100644 index 000000000..a0685f496 --- /dev/null +++ b/tests/fixtures/issue-2339-barrel-tiebreak/other.js @@ -0,0 +1,3 @@ +export function Named() { + return 'named'; +} diff --git a/tests/integration/issue-2339-barrel-tiebreak-incremental.test.ts b/tests/integration/issue-2339-barrel-tiebreak-incremental.test.ts new file mode 100644 index 000000000..18e3ed781 --- /dev/null +++ b/tests/integration/issue-2339-barrel-tiebreak-incremental.test.ts @@ -0,0 +1,147 @@ +/** + * Regression for #2339: `detect_barrel_only_files` / `isBarrelFile` used + * `reexports >= ownDefs` to decide whether a file is a pure barrel. A file + * with exactly one reexport and exactly one own definition hit that `>=` + * and got misclassified as barrel-only — even though it's a genuine hybrid + * (real logic plus a reexport), not a pure barrel. Once misclassified, + * `build_and_insert_call_edges` (and its JS equivalent) skip emitting ANY + * of that file's own outgoing call edges, so on any incremental build where + * the file gets pulled into Stage 6b's barrel-candidate reparse (because + * something imports the reexported symbol from it), its own call edges are + * silently dropped and never re-emitted — even on an otherwise-correct + * build. + * + * Fixture shape: + * + * app.js + * └─ imports `doWork` from hybrid.js + * + * hybrid.js (exact tie: 1 reexport + 1 own def) + * ├─ `export { Named } from './other.js'` + * └─ `doWork()` calls `helperFn` from helper.js + * + * other.js (defines the reexported symbol) + * helper.js (defines the function hybrid.js's own def calls) + * + * Before the fix, editing app.js triggered a reparse of hybrid.js (it has + * one reexport edge in the DB, so the orchestrator flags it as a barrel + * candidate every incremental build), and the `>=` tie caused it to be + * (re)classified barrel-only, dropping the doWork -> helperFn call edge. + * Mirrors tests/integration/issue-1174-chained-barrel-incremental.test.ts's + * structure (that fixture's hybrid file has 1 reexport vs. 4 defs, so it + * never exercised this exact tie). + */ + +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import Database from 'better-sqlite3'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { buildGraph } from '../../src/domain/graph/builder.js'; +import type { EngineMode } from '../../src/types.js'; + +const FIXTURE_DIR = path.join(import.meta.dirname, '..', 'fixtures', 'issue-2339-barrel-tiebreak'); + +function copyDirSync(src: string, dest: string) { + fs.mkdirSync(dest, { recursive: true }); + for (const entry of fs.readdirSync(src, { withFileTypes: true })) { + const s = path.join(src, entry.name); + const d = path.join(dest, entry.name); + if (entry.isDirectory()) copyDirSync(s, d); + else fs.copyFileSync(s, d); + } +} + +interface EdgeRow { + source_file: string; + source_name: string; + target_file: string; + target_name: string; + kind: string; +} + +function readEdges(dbPath: string): EdgeRow[] { + const db = new Database(dbPath, { readonly: true }); + try { + return db + .prepare( + `SELECT n1.file AS source_file, n1.name AS source_name, + n2.file AS target_file, n2.name AS target_name, e.kind + FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + ORDER BY n1.file, n1.name, n2.file, n2.name, e.kind`, + ) + .all() as EdgeRow[]; + } finally { + db.close(); + } +} + +const ENGINES: EngineMode[] = ['wasm', 'native']; + +describe.each(ENGINES)('Issue #2339 barrel reexports==ownDefs tie-break parity (%s)', (engine) => { + let fullEdges: EdgeRow[]; + let incrEdges: EdgeRow[]; + let tmpBase: string; + + beforeAll(async () => { + tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), `codegraph-2339-${engine}-`)); + const fullDir = path.join(tmpBase, 'full'); + const incrDir = path.join(tmpBase, 'incr'); + copyDirSync(FIXTURE_DIR, fullDir); + copyDirSync(FIXTURE_DIR, incrDir); + + // Establish baseline on the incremental copy + await buildGraph(incrDir, { incremental: false, skipRegistry: true, engine }); + + // Mutate app.js (the only "changed" file) on both copies + const mutate = (dir: string) => { + fs.appendFileSync(path.join(dir, 'app.js'), '\n// touch\n'); + }; + mutate(fullDir); + mutate(incrDir); + + // Full build on the full copy + await buildGraph(fullDir, { incremental: false, skipRegistry: true, engine }); + // Incremental rebuild on the incr copy + await buildGraph(incrDir, { incremental: true, skipRegistry: true, engine }); + + fullEdges = readEdges(path.join(fullDir, '.codegraph', 'graph.db')); + incrEdges = readEdges(path.join(incrDir, '.codegraph', 'graph.db')); + }, 90_000); + + afterAll(() => { + if (tmpBase) fs.rmSync(tmpBase, { recursive: true, force: true }); + }); + + it('emits the doWork -> helperFn call edge on full build (hybrid.js is not barrel-only)', () => { + const callEdge = fullEdges.filter( + (e) => + e.source_file === 'hybrid.js' && + e.source_name === 'doWork' && + e.target_file === 'helper.js' && + e.target_name === 'helperFn' && + e.kind === 'calls', + ); + expect(callEdge.length).toBeGreaterThan(0); + }); + + it('the doWork -> helperFn call edge survives the incremental rebuild', () => { + const callEdge = incrEdges.filter( + (e) => + e.source_file === 'hybrid.js' && + e.source_name === 'doWork' && + e.target_file === 'helper.js' && + e.target_name === 'helperFn' && + e.kind === 'calls', + ); + expect(callEdge.length).toBeGreaterThan(0); + }); + + it('call edge count matches full rebuild', () => { + const fullCalls = fullEdges.filter((e) => e.kind === 'calls'); + const incrCalls = incrEdges.filter((e) => e.kind === 'calls'); + expect(incrCalls.length).toBe(fullCalls.length); + }); +});