mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-12 23:02:45 +00:00
fix(ingestion): bound closure size, O(1) dequeue, track Strategy 4 (#816 review)
Address @xkonjin's review feedback on the import-resolution strategy refactor: 1. **DoS guard**: cap transitive closures at 5,000 files via `MAX_TRANSITIVE_CLOSURE_SIZE`. Pathological codebases (boost-style headers, monoheader kernels) could previously produce closures with tens of thousands of entries per translation unit. BFS now stops early and returns a partial closure rather than risking OOM. The closest-headers-first BFS ordering means the partial closure still contains the files overload resolution cares about. 2. **Perf**: replace `Array.prototype.shift()` (O(n)) with a head-index queue (O(1) dequeue). Deep chains previously had quadratic BFS behavior; now linear in closure size. 3. **Strategy 4 tracking**: change TODO in `dispatchSynthesis` to `TODO(#821)` referencing the filed issue for TS `export *` / Rust `pub use` DAG-walk implementation, and clarify that today's leaf fallthrough preserves correctness for direct imports — only the extra re-export traversal is missing. 4. **Test**: new unit test exercising the 5,000-file cap on a 10k-file synthetic chain, verifying partial-closure invariants (starts from importer side, bounded, deep nodes excluded). Not addressed in this commit (followups): - Review point 3 (graphImports-only deep-chain *integration* fixture): unit tests already exercise the `graphImports` traversal path directly in isolation and combined with `importMap`. A fixture that stresses graphImports-only transitive resolution is valuable but requires understanding when the pipeline populates graphImports distinctly from ctx.importMap — tracking as a followup rather than blocking this PR.
This commit is contained in:
parent
1357363395
commit
6da3998e69
2 changed files with 53 additions and 17 deletions
|
|
@ -43,6 +43,13 @@ const IMPORTABLE_SYMBOL_LABELS = new Set([
|
|||
* for C/C++ files that include many large headers. */
|
||||
const MAX_SYNTHETIC_BINDINGS_PER_FILE = 1000;
|
||||
|
||||
/** Max files allowed in a single transitive include closure. Guards against
|
||||
* OOM on pathological C/C++ codebases (boost, Linux kernel-style monoheaders)
|
||||
* where a single translation unit can transitively reach many thousands of
|
||||
* headers. When the cap is hit, BFS expansion stops early — the file still
|
||||
* synthesizes bindings from the partial closure rather than failing. */
|
||||
const MAX_TRANSITIVE_CLOSURE_SIZE = 5000;
|
||||
|
||||
/** Import semantics tags whose languages need synthesis of whole-module imports.
|
||||
* `wildcard-transitive` (C/C++) and `wildcard-leaf` (Go, Ruby, Swift, Dart) are
|
||||
* the file-based wildcard strategies. `explicit-reexport` is a scaffold tag —
|
||||
|
|
@ -105,6 +112,16 @@ export function needsSynthesis(lang: SupportedLanguages): boolean {
|
|||
* Cycle-safe: the `closure.has(file)` guard prevents infinite loops on circular
|
||||
* header includes, which are valid C/C++ when paired with `#pragma once` or
|
||||
* include guards.
|
||||
*
|
||||
* Size-bounded: the closure is capped at `MAX_TRANSITIVE_CLOSURE_SIZE` files to
|
||||
* prevent OOM on pathological codebases (e.g. boost, monoheader kernel code)
|
||||
* where one translation unit can transitively reach tens of thousands of
|
||||
* headers. Partial closures still yield useful bindings for the cluster of
|
||||
* headers closest to the importer, which is what overload resolution and
|
||||
* cross-file call resolution care about.
|
||||
*
|
||||
* Queue implementation: uses a head-index over a growing array (O(1) dequeue)
|
||||
* instead of `Array.prototype.shift()` (O(n)) so deep chains stay linear.
|
||||
*/
|
||||
export function expandTransitiveIncludeClosure(
|
||||
directImports: Iterable<string>,
|
||||
|
|
@ -113,33 +130,35 @@ export function expandTransitiveIncludeClosure(
|
|||
): Set<string> {
|
||||
const closure = new Set<string>();
|
||||
const queue: string[] = [];
|
||||
let head = 0; // O(1) dequeue: advance the head index instead of shift()-ing.
|
||||
|
||||
const tryEnqueue = (file: string): boolean => {
|
||||
if (closure.has(file)) return true;
|
||||
if (closure.size >= MAX_TRANSITIVE_CLOSURE_SIZE) return false;
|
||||
closure.add(file);
|
||||
queue.push(file);
|
||||
return true;
|
||||
};
|
||||
|
||||
// Seed direct imports in declaration order (see JSDoc on order-sensitivity).
|
||||
for (const f of directImports) {
|
||||
if (!closure.has(f)) {
|
||||
closure.add(f);
|
||||
queue.push(f);
|
||||
}
|
||||
if (!tryEnqueue(f)) break;
|
||||
}
|
||||
// True BFS for transitive reach: FIFO via shift() preserves the "closer
|
||||
// True BFS for transitive reach: head-index FIFO preserves the "closer
|
||||
// headers first" ordering that overload resolution depends on.
|
||||
while (queue.length > 0) {
|
||||
const file = queue.shift()!;
|
||||
while (head < queue.length) {
|
||||
if (closure.size >= MAX_TRANSITIVE_CLOSURE_SIZE) break;
|
||||
const file = queue[head++]!;
|
||||
const nested = importMap.get(file);
|
||||
if (nested) {
|
||||
for (const n of nested) {
|
||||
if (!closure.has(n)) {
|
||||
closure.add(n);
|
||||
queue.push(n);
|
||||
}
|
||||
if (!tryEnqueue(n)) break;
|
||||
}
|
||||
}
|
||||
const nestedGraph = graphImports.get(file);
|
||||
if (nestedGraph) {
|
||||
for (const n of nestedGraph) {
|
||||
if (!closure.has(n)) {
|
||||
closure.add(n);
|
||||
queue.push(n);
|
||||
}
|
||||
if (!tryEnqueue(n)) break;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -238,8 +257,10 @@ export function synthesizeWildcardImportBindings(
|
|||
* across header chains)
|
||||
* - `wildcard-leaf`: synthesize from direct imports only (Go, Ruby, Swift, Dart)
|
||||
* - `explicit-reexport`: scaffold tag; falls through to leaf behavior.
|
||||
* TODO: implement re-export DAG walk for TS `export *` / Rust `pub use` —
|
||||
* tracked as the larger TS barrel-file correctness gap (see plan 2026-04-14-001).
|
||||
* TODO(#821): implement re-export DAG walk for TS `export *` / Rust
|
||||
* `pub use`. The leaf fallthrough preserves today's TS/Rust behavior
|
||||
* (their direct imports still synthesize correctly); only the extra
|
||||
* re-export DAG walk for barrel-file correctness is missing.
|
||||
* - `namespace` / `named`: no-op here (namespace handled in Loop 3 below,
|
||||
* named needs no synthesis).
|
||||
*
|
||||
|
|
|
|||
|
|
@ -68,6 +68,21 @@ describe('expandTransitiveIncludeClosure', () => {
|
|||
expect(closure.size).toBe(0);
|
||||
});
|
||||
|
||||
it('caps closure size to prevent OOM on pathological codebases', () => {
|
||||
// Build a synthetic include graph of 10,000 files, each including the next.
|
||||
// The cap (5000) should halt BFS early with a partial but bounded closure.
|
||||
const importMap = new Map<string, ReadonlySet<string>>();
|
||||
for (let i = 0; i < 10_000; i++) {
|
||||
importMap.set(`h${i}.h`, new Set([`h${i + 1}.h`]));
|
||||
}
|
||||
const closure = expandTransitiveIncludeClosure(new Set(['h0.h']), importMap, EMPTY);
|
||||
expect(closure.size).toBe(5000);
|
||||
// Partial closure still starts from the importer's side (BFS ordering).
|
||||
expect(closure.has('h0.h')).toBe(true);
|
||||
expect(closure.has('h1.h')).toBe(true);
|
||||
expect(closure.has('h9999.h')).toBe(false);
|
||||
});
|
||||
|
||||
it('deduplicates when a file is reachable through multiple paths (diamond)', () => {
|
||||
// A
|
||||
// / \
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue