diff --git a/gitnexus/test/unit/import-resolvers/csharp-csproj-parity.test.ts b/gitnexus/test/unit/import-resolvers/csharp-csproj-parity.test.ts index 7b99565b7..d3ed9b42d 100644 --- a/gitnexus/test/unit/import-resolvers/csharp-csproj-parity.test.ts +++ b/gitnexus/test/unit/import-resolvers/csharp-csproj-parity.test.ts @@ -14,8 +14,8 @@ * SEGMENT-aligned directory suffixes; step 3 runs an UNANCHORED * `normalized.lastIndexOf(dirPrefix + '/')` (`indexOf` before #2881, which is * the first-occurrence rule that issue removed; the empty-prefix case still - * takes `indexOf` — see the guard in both copies). Step 3 therefore answers - * strictly more: + * takes `indexOf` — see `directChildIdx`, which both copies call). Step 3 + * therefore answers strictly more: * * - `dirPrefix = 'ubModels'` matches `src/SubModels/` (character suffix of a * segment, not a segment); @@ -61,6 +61,26 @@ import type { // `SuffixIndex`) are imported from production because this PR does not touch // them; only the function below changed. +/** + * The direct-child probe the frozen copies below run — four call sites, two in + * each copy, that have to move together. + * + * Direct child of a directory ENDING with `dirPrefix` — since #2881, minus + * "…and that occurrence is the FIRST". Empty `dirPrefix` keeps `indexOf`: its + * needle is a bare '/', and step 3 answers that query from the + * one-directory-deep set, which only the first occurrence expresses. + * + * Local to this file on purpose. A parity harness has to stay independent of + * PRODUCTION — that independence is the whole instrument, and importing this + * expression from `csharp.ts` would make the differential compare production + * against itself. But all four copies live inside the harness, so one local + * helper keeps the independence while removing three sites that could silently + * drift apart from each other. + */ +function directChildIdx(normalized: string, dirPrefix: string, dirTrail: string): number { + return dirPrefix === '' ? normalized.indexOf(dirTrail) : normalized.lastIndexOf(dirTrail); +} + function legacyResolveCSharpImportInternal( importPath: string, csharpConfigs: CSharpProjectConfig[], @@ -112,13 +132,7 @@ function legacyResolveCSharpImportInternal( const dirFiles = index.getFilesInDir(dirPrefix, '.cs'); for (const f of dirFiles) { const normalized = f.replace(/\\/g, '/'); - // Direct child of a directory ENDING with `dirPrefix` — since #2881, - // minus "…and that occurrence is the FIRST". Empty `dirPrefix` keeps - // `indexOf`: its needle is a bare '/', and step 3 answers that query - // from the one-directory-deep set, which only the first occurrence - // expresses. - const prefixIdx = - dirPrefix === '' ? normalized.indexOf(dirTrail) : normalized.lastIndexOf(dirTrail); + const prefixIdx = directChildIdx(normalized, dirPrefix, dirTrail); if (prefixIdx < 0) continue; const afterDir = normalized.substring(prefixIdx + dirPrefix.length + 1); if (!afterDir.includes('/')) { @@ -133,8 +147,7 @@ function legacyResolveCSharpImportInternal( for (let i = 0; i < normalizedFileList.length; i++) { const normalized = normalizedFileList[i]; if (!normalized.endsWith('.cs')) continue; - const prefixIdx = - dirPrefix === '' ? normalized.indexOf(dirTrail) : normalized.lastIndexOf(dirTrail); + const prefixIdx = directChildIdx(normalized, dirPrefix, dirTrail); if (prefixIdx < 0) continue; const afterDir = normalized.substring(prefixIdx + dirTrail.length); if (!afterDir.includes('/')) { @@ -204,8 +217,7 @@ function skipStep3WhenIndexed( const dirFiles = index.getFilesInDir(dirPrefix, '.cs'); for (const f of dirFiles) { const normalized = f.replace(/\\/g, '/'); - const prefixIdx = - dirPrefix === '' ? normalized.indexOf(dirTrail) : normalized.lastIndexOf(dirTrail); + const prefixIdx = directChildIdx(normalized, dirPrefix, dirTrail); if (prefixIdx < 0) continue; const afterDir = normalized.substring(prefixIdx + dirPrefix.length + 1); if (!afterDir.includes('/')) { @@ -219,8 +231,7 @@ function skipStep3WhenIndexed( for (let i = 0; i < normalizedFileList.length; i++) { const normalized = normalizedFileList[i]; if (!normalized.endsWith('.cs')) continue; - const prefixIdx = - dirPrefix === '' ? normalized.indexOf(dirTrail) : normalized.lastIndexOf(dirTrail); + const prefixIdx = directChildIdx(normalized, dirPrefix, dirTrail); if (prefixIdx < 0) continue; const afterDir = normalized.substring(prefixIdx + dirTrail.length); if (!afterDir.includes('/')) { @@ -646,3 +657,101 @@ describe('C# csproj leg — the directory index is built once per file set (#290 ); }); }); + +/** + * ABSOLUTE arms, deliberately not differential. + * + * The harness above is blind to everything in this block. Its frozen legacy copy + * carries the same `dirPrefix === '' ? indexOf : lastIndexOf` rule production + * does (see `directChildIdx`, and the header's note that #2881's edit landed in + * BOTH), so where #2881 moved step 2 the two sides moved together and the + * differential stays green by construction. Only stated expectations can see + * these, so each arm below names the exact line it gates. + */ +describe('C# csproj leg — where step 2 stops and step 3 begins (absolute)', () => { + function corpus(raw: readonly string[]): { paths: ReadonlySet; index: SuffixIndex } { + const all = [...raw]; + const normalized = all.map((f) => f.replace(/\\/g, '/')); + return { paths: new Set(all), index: buildSuffixIndex(normalized, all) }; + } + + const ROOT_NS_ONLY: CSharpProjectConfig[] = [{ rootNamespace: 'App', projectDir: '' }]; + + it("step 2's empty-`dirPrefix` filter rejects a doubled slash, which is NOT one directory deep", () => { + // Gates the five-line `if (dirPrefix === '')` guard in step 2 of + // `resolveCSharpImportInternal`. Delete it and this arm is the only thing in + // the suite that fails. + // + // `getDirMap` keys a file under every suffix of its DIRECTORY, so the empty + // key holds every path whose directory's last component is empty. That is a + // leading '/' on a root-level file (`/Root.cs`, directory ''), but ALSO a + // doubled slash immediately before the file name (`a//Doubled.cs`, directory + // 'a/'). Only the first is one directory deep — the query an empty + // `dirPrefix` is asking, and the one step 3 answers from `singleSegmentDirs` + // — so without the guard step 2 and step 3 disagree. + const { paths, index } = corpus([ + '/Root.cs', + 'a//Doubled.cs', + 'Top.cs', + 'one/Deep.cs', + 'a/b/Deeper.cs', + ]); + // Not vacuous: `a//Doubled.cs` really is in the bucket step 2 filters, so + // this arm fails by ADDING it rather than by finding nothing to reject. + expect(index.getFilesInDir('', '.cs')).toEqual(['/Root.cs', 'a//Doubled.cs']); + expect(resolveCSharpImportInternal('App', ROOT_NS_ONLY, paths, index)).toEqual(['/Root.cs']); + }); + + it('step 2 answering a query it used to miss also PREEMPTS step 3', () => { + // #2881 widened step 2 from "the FIRST `//`" to "the directory + // ENDS with `dirPrefix`". The justification reasons about step 2's own + // bucket and is right there — but step 2 returns as soon as it pushes + // anything, so a query it used to answer with nothing now also suppresses + // step 3, whose unanchored match set is a strict SUPERSET of step 2's. + // + // Pinned in both directions: step 2's answer with an index, and step 3's own + // answer with none. The gap between them is the suppression. + const { paths, index } = corpus([ + 'nest/src/SubModels/F0.cs', + 'SubModels/Models/F1.cs', + 'F2.cs', + 'SubModels/F3.cs', + ]); + // Step 2 alone: `SubModels/Models` is the only SEGMENT-aligned `Models`. + expect(resolveCSharpImportInternal('App.Models', ROOT_NS_ONLY, paths, index)).toEqual([ + 'SubModels/Models/F1.cs', + ]); + // Step 3 alone: every directory whose path merely ENDS with `Models`, which + // is the segment-aligned hit plus both `SubModels` character-suffix ones. + // Before #2881 step 2 rejected here and this was the answer; the widened + // step 2 now returns first, and the narrower, more precise answer above is + // the one that reaches the graph. + expect(resolveCSharpImportInternal('App.Models', ROOT_NS_ONLY, paths, undefined)).toEqual([ + 'nest/src/SubModels/F0.cs', + 'SubModels/Models/F1.cs', + 'SubModels/F3.cs', + ]); + }); + + it('a name that is not a suffix of the PARENT directory stays out of the bucket', () => { + // The negative control for the widened rule, matching the one Kotlin's + // parity test carries. The rule is "the file's parent directory ENDS with + // `dirPrefix`", not "`dirPrefix` appears anywhere in the path" — dropping + // the first-occurrence half must not widen it that far. Both positions the + // old `indexOf` distinguished are covered: leading, and mid-path. + // + // Asserted through both legs, because they run different predicates on + // different indexes and either one alone could widen without the other. + const { paths, index } = corpus([ + 'Models/sub/Leading.cs', + 'top/Models/mid/Middle.cs', + 'Models/Direct.cs', + ]); + expect(resolveCSharpImportInternal('App.Models', ROOT_NS_ONLY, paths, index)).toEqual([ + 'Models/Direct.cs', + ]); + expect(resolveCSharpImportInternal('App.Models', ROOT_NS_ONLY, paths, undefined)).toEqual([ + 'Models/Direct.cs', + ]); + }); +}); diff --git a/gitnexus/test/unit/import-resolvers/go-package-resolve.test.ts b/gitnexus/test/unit/import-resolvers/go-package-resolve.test.ts index 7651b85b9..2394a44d9 100644 --- a/gitnexus/test/unit/import-resolvers/go-package-resolve.test.ts +++ b/gitnexus/test/unit/import-resolvers/go-package-resolve.test.ts @@ -13,25 +13,37 @@ * here rather than silently moving Go IMPORTS edges in every repository that * nests a package name inside itself (`internal/…/internal` is the shape Go * actually produces). + * + * One caveat on "the LanguageProvider leg", recorded in #2929 review: nothing + * in production reads that field today. `configs/go.ts` reaches this function + * through `createImportResolver` → `LanguageProvider.importResolver`, and the + * only readers of `importResolver` are `import-target-adapter.ts:74-75`, whose + * two exports have no importer outside their own unit test. So the leg is wired + * but unreached, and this file is the only thing exercising it. */ import { describe, expect, it } from 'vitest'; import { resolveGoPackage, resolveGoPackageDir, } from '../../../src/core/ingestion/import-resolvers/go.js'; +import type { GoModuleConfig } from '../../../src/core/ingestion/language-config.js'; import { resolveGoImportTarget } from '../../../src/core/ingestion/languages/go/import-target.js'; -const MOD = { modulePath: 'example.com/mod' } as const; +const MOD: GoModuleConfig = { modulePath: 'example.com/mod' }; function resolve(files: readonly string[], importPath: string): string[] { const normalized = files.map((f) => f.replace(/\\/g, '/')); - return resolveGoPackage(importPath, MOD as never, normalized, files); + return resolveGoPackage(importPath, MOD, normalized, files); } /** The indexed leg, for the agreement arm. */ function indexed(files: readonly string[], importPath: string): readonly string[] { const got = resolveGoImportTarget(importPath, 'main.go', new Set(files), MOD); - return got === null ? [] : Array.isArray(got) ? got : [got]; + // `typeof got === 'string'`, not `Array.isArray(got)`: `Array.isArray` narrows + // to `any[]`, which does not subsume `readonly string[]`, so the false branch + // kept the array member and `[got]` did not typecheck (TS2322 under + // `tsconfig.test.json`). Runtime behaviour is identical. + return got === null ? [] : typeof got === 'string' ? [got] : got; } describe('resolveGoPackage', () => { @@ -92,10 +104,40 @@ describe('resolveGoPackage', () => { expect(resolve(['internal/auth/x.go'], 'github.com/other/repo/internal/auth')).toEqual([]); // The root package is the caller's `findRootPackageFiles` leg, not this one. expect(resolve(['main.go'], 'example.com/mod')).toEqual([]); - expect(resolveGoPackageDir('example.com/mod', MOD as never)).toBeNull(); - expect(resolveGoPackageDir('example.com/mod/internal/auth', MOD as never)).toBe( - '/internal/auth/', - ); + expect(resolveGoPackageDir('example.com/mod', MOD)).toBeNull(); + expect(resolveGoPackageDir('example.com/mod/internal/auth', MOD)).toBe('/internal/auth/'); + }); + + it('vendor/, testdata/ and nested-module directories all merge in (unmodelled)', () => { + // Go excludes all three from a package: `vendor/` is a dependency tree + // resolved against the vendoring module, the go tool ignores `testdata/` + // entirely, and a directory carrying its own `go.mod` is a separate module + // whose packages this module's import paths never name. + // + // This resolver models NONE of that — it matches on the parent directory's + // path suffix alone. That is unchanged by #2881: the pre-#2881 rule + // (first `//`, nothing but a filename after it) accepted all three + // shapes too. These assertions record what the function actually does so + // the gap is visible and a change to it is deliberate; they document the + // behaviour rather than endorse it. + const files = [ + 'go.mod', + 'internal/auth/service.go', + 'vendor/example.com/dep/internal/auth/vendored.go', + 'testdata/internal/auth/fixture.go', + 'sub/go.mod', // `sub/` is its own module; its packages are not ours + 'sub/internal/auth/other_module.go', + ]; + expect(resolve(files, 'example.com/mod/internal/auth')).toEqual([ + 'internal/auth/service.go', + 'vendor/example.com/dep/internal/auth/vendored.go', + 'testdata/internal/auth/fixture.go', + 'sub/internal/auth/other_module.go', + ]); + // A `go.mod` beside the files changes nothing — it is not read here. + expect(resolve(['sub/go.mod', 'sub/pkg/x.go'], 'example.com/mod/pkg')).toEqual([ + 'sub/pkg/x.go', + ]); }); it('agrees with the indexed leg on the repeated-name shapes', () => { diff --git a/gitnexus/test/unit/scope-resolution/java-import-target-parity.test.ts b/gitnexus/test/unit/scope-resolution/java-import-target-parity.test.ts index 5fb7b2529..868993f07 100644 --- a/gitnexus/test/unit/scope-resolution/java-import-target-parity.test.ts +++ b/gitnexus/test/unit/scope-resolution/java-import-target-parity.test.ts @@ -352,6 +352,30 @@ const HAND_CASES: readonly Case[] = [ files: ['com/example/model/User.java'], target: 'java.util.List', }, + // ── negative control for the widening (#2881), matching Kotlin's ────────── + // Everything above pins the widened leg from the POSITIVE side: a file whose + // package directory repeats higher in its own path now answers. Nothing here + // pinned the other half — that dropping the first-occurrence rule did not + // widen "the parent directory ends with `pathLike`" into "`pathLike` appears + // somewhere in the path". These three are the same trio + // `kotlin-import-target-parity.test.ts` carries: the two positions the old + // `atRoot`/`indexOf` pair distinguished, plus the positive control that keeps + // the pair from passing by refusing everything. + { + label: 'not-the-parent-directory-stays-out-leading', + files: ['com/example/sub/Repo.java'], + target: 'com.example', + }, + { + label: 'not-the-parent-directory-stays-out-mid-path', + files: ['top/com/example/mid/Repo.java'], + target: 'com.example', + }, + { + label: 'the-parent-directory-itself-does-answer', + files: ['top/com/example/Repo.java'], + target: 'com.example', + }, ]; /** Absolute pre-change behaviour, so the differential cannot pass vacuously. */ @@ -389,6 +413,12 @@ const HAND_EXPECTED: readonly string[] = [ 'directory-named-like-a-java-file => null', 'jdk-import-strips-into-a-local-lookalike => src/main/java/util/List.java', 'jdk-import-with-no-lookalike-resolves-to-nothing => null', + // `com/example/sub` ends with `example/sub`, not with `com/example`, and the + // stripping loop's `example` level does not reach it either — so the file is + // in no bucket the query can name. + 'not-the-parent-directory-stays-out-leading => null', + 'not-the-parent-directory-stays-out-mid-path => null', + 'the-parent-directory-itself-does-answer => top/com/example/Repo.java', ]; // ─── generated corpus ──────────────────────────────────────────────────────── @@ -617,6 +647,55 @@ describe('Java import target — index hoist parity (#2908)', () => { ]); }); + it('pins WHICH of two competing package directories the first-child leg takes', () => { + // `firstFileDirectlyInPkgDir` commits to ONE file with no downstream + // filter, and #2881 widened the set it chooses from. Every widened-shape + // case in HAND_CASES uses a ONE-FILE corpus, so the widened bucket has a + // single member and the choice is not exercised anywhere — yet a different + // first child is the largest class of movement the change produced. + // + // Both files below are legitimate members of the widened `com/example` + // set: `com/example/legacy/com/example` ends with `com/example` (it is the + // self-nested shape #2881 admitted) and `src/main/java/com/example` ends + // with it too. Nothing in the resolver prefers one over the other. The + // tie-break is FILE-SET ITERATION ORDER — the insertion order of the Set + // the caller passes — so reversing the corpus reverses the answer. That is + // the property these assertions pin, and the one nothing else watches: + // membership is already pinned above, the WINNER was not. + const set = (files: readonly string[]): WorkspaceIndex => + ({ fromFile: FROM_FILE, allFilePaths: new Set(files) }) as WorkspaceIndex; + const nestedFirst = [ + 'com/example/legacy/com/example/Old.java', + 'src/main/java/com/example/App.java', + ]; + const conventionalFirst = [...nestedFirst].reverse(); + + expect(resolveJavaImportTarget(javaImport('com.example'), set(nestedFirst))).toBe( + 'com/example/legacy/com/example/Old.java', + ); + expect(resolveJavaImportTarget(javaImport('com.example'), set(conventionalFirst))).toBe( + 'src/main/java/com/example/App.java', + ); + // A wildcard drops its `.*` before any of that, so it lands on the same leg + // and moves with it. Spelled out because `com.example.*` is how real Java + // source reaches this tier. + expect(resolveJavaImportTarget(javaImport('com.example.*'), set(nestedFirst))).toBe( + 'com/example/legacy/com/example/Old.java', + ); + expect(resolveJavaImportTarget(javaImport('com.example.*'), set(conventionalFirst))).toBe( + 'src/main/java/com/example/App.java', + ); + // The pre-change copy agrees on both orders, which places the movement in + // #2881's removal of the first-occurrence rule rather than in #2908's + // index hoist: the hoist did not touch the tie-break, it inherited it. + expect(legacyResolveJavaImportTarget(javaImport('com.example'), set(nestedFirst))).toBe( + 'com/example/legacy/com/example/Old.java', + ); + expect(legacyResolveJavaImportTarget(javaImport('com.example'), set(conventionalFirst))).toBe( + 'src/main/java/com/example/App.java', + ); + }); + it('builds each index once per file set rather than once per import', () => { const files = new CountingSet(generatedFiles()); const ws = { fromFile: FROM_FILE, allFilePaths: files }; diff --git a/gitnexus/test/unit/scope-resolution/kotlin/kotlin-import-target-parity.test.ts b/gitnexus/test/unit/scope-resolution/kotlin/kotlin-import-target-parity.test.ts index c5516ff21..d4104c9d2 100644 --- a/gitnexus/test/unit/scope-resolution/kotlin/kotlin-import-target-parity.test.ts +++ b/gitnexus/test/unit/scope-resolution/kotlin/kotlin-import-target-parity.test.ts @@ -13,6 +13,16 @@ * first-occurrence restriction on `dirChildren`, so a file whose package * directory name repeats higher in its path is now a child of that package. * The cases carrying it say so and name the issue. + * + * That removal has two consequences a widened-shape case built on a ONE-FILE + * corpus cannot express, and both are pinned at the bottom of this file: + * + * - a bucket with TWO members makes `children[0]` a CHOICE. The wildcard tier + * commits to it with no downstream filter, so widening the bucket moves + * which file an already-resolving `import data.*` binds to. + * - tier 3 sits in front of tier 4, so a bucket the removed guards used to + * leave empty no longer lets tier 4 run — which can turn a bound answer + * into a candidate list that does not carry the symbol. */ import { describe, it, expect } from 'vitest'; import { resolveKotlinImportTarget } from '../../../../src/core/ingestion/languages/kotlin/import-target.js'; @@ -184,4 +194,70 @@ describe('resolveKotlinImportTarget — index parity', () => { it('an unknown target resolves to null', () => { expect(resolve(['pkg/A.kt'], 'nowhere.Thing')).toBeNull(); }); + + it('two competing `data` directories: WHICH one the first-child tier picks (#2881)', () => { + // The gap every other widened-shape case above leaves open. Each of them + // uses a ONE-FILE corpus, so the widened bucket has exactly one member and + // `findKotlinDirectoryChild`'s `children[0]` has nothing to choose between. + // #2881 moved 149 of the census's 235 records to a DIFFERENT first child, + // and not one of those 149 is a shape any single-file case can express. + // + // Both files below are legitimate members of the widened `data` bucket: + // each is a direct child of a directory named `data`. Neither is "the right + // answer" — the resolver has no rule that prefers one, and the ONLY thing + // deciding it is FILE-SET ITERATION ORDER, i.e. the insertion order of the + // Set the caller hands in. That is what these assertions pin: not that the + // bucket contains both (the cases above already pin membership) but WHICH + // member wins, which is the property nothing else in the repo watches. + const files = ['top/data/mid/data/Wrong.kt', 'src/data/Correct.kt']; + const reversed = ['src/data/Correct.kt', 'top/data/mid/data/Wrong.kt']; + + // Tier 3, the member path: `data.helper` strips to `data`, misses the file + // tiers and fans the WHOLE bucket out. Order is preserved but nothing is + // dropped, so this path commits to nothing on its own — the finalize pass + // still gets to pick by `localDefs` (#1759). + expect(resolve(files, 'data.helper')).toEqual([ + 'top/data/mid/data/Wrong.kt', + 'src/data/Correct.kt', + ]); + expect(resolve(reversed, 'data.helper')).toEqual([ + 'src/data/Correct.kt', + 'top/data/mid/data/Wrong.kt', + ]); + + // Tier 1, the wildcard path: `data.*` strips to `data`, which IS the whole + // `pathLike`, so `findKotlinFile` answers with `children[0]` — one file, + // unfiltered, no later tier and no downstream narrowing. This is the leg + // where the widening changes a bound answer rather than adding a candidate, + // and it flips with insertion order alone. + expect(resolve(files, 'data.*')).toBe('top/data/mid/data/Wrong.kt'); + expect(resolve(reversed, 'data.*')).toBe('src/data/Correct.kt'); + }); + + it('tier 3 preempts tier 4: a widened bucket replaces a BOUND answer (#2881)', () => { + // The class the published `54 / 149 / 32` census has no bucket for, because + // that taxonomy is shape-preserving (null→resolved, wider array, different + // first child) and this one is not. `findKotlinPackageFiles` runs BEFORE + // `findByProgressivePrefixStrip`, so a bucket the removed guards used to + // leave empty returned null and let tier 4 run; a now-populated bucket + // stops tier 4 from running at all. + // + // Read the two assertions together. The answer here is no longer a bound + // file — it is a CANDIDATE LIST, and the only file in it does not carry + // `helper`. `common/helper.kt`, the file tier 4 used to bind, is not in the + // list at all: it is not a child of any `data` directory, so no widening of + // the bucket can ever reach it. A resolved answer became an unresolved one. + // The bench corpus contains ZERO instances of this class, which is why it + // ships ungated — `bench/kotlin-import-target`'s own generator at 4000 + // repositories hits it only 4-12 times per seed. This case is the gate. + expect( + resolve(['data/src/main/kotlin/com/example/data/Repo.kt', 'common/helper.kt'], 'data.helper'), + ).toEqual(['data/src/main/kotlin/com/example/data/Repo.kt']); + + // The control that makes the arm above a transition rather than a fact: + // drop the `data` directory and tier 3 has nothing, so tier 4 runs and + // binds the same import to the file that actually holds `helper`. This is + // what the first assertion returned before #2881. + expect(resolve(['common/helper.kt'], 'data.helper')).toBe('common/helper.kt'); + }); }); diff --git a/gitnexus/test/unit/scope-resolution/kotlin/kotlin-index-internals.test.ts b/gitnexus/test/unit/scope-resolution/kotlin/kotlin-index-internals.test.ts index 6de56d22f..c4f985677 100644 --- a/gitnexus/test/unit/scope-resolution/kotlin/kotlin-index-internals.test.ts +++ b/gitnexus/test/unit/scope-resolution/kotlin/kotlin-index-internals.test.ts @@ -2,25 +2,52 @@ * Structural guard for the two `getKotlinFileIndex` optimizations added in * #2881 — the per-directory key memo and the bucket compaction. * - * Both are output-identical by construction, which is why they exist and also - * why nothing else in the repo watches them. `bench/kotlin-import-target`'s - * correctness fingerprint observes the index only through the four resolver - * tiers, so it cannot see a Map key-ORDER move, and reverting either - * optimization leaves every arm of both benches green (the heap ceiling is a - * 1.5x bound, not an equality). Without this file the memo's safety argument — - * "the key list is a pure function of `dir`, so interning it per directory - * cannot change the key set, the key insertion order or any bucket's order" — - * is asserted nowhere. + * ## What this file can and cannot see + * + * Read this before adding a case here, and before citing this file as coverage + * for either optimization. Both claims below were checked by re-running every + * arm in this file against a mutated COPY of the resolver: + * + * - **the key memo (`dirKeys`) is not observable.** Deleting it outright — + * cutting the component-suffix key list once per FILE, the way the code did + * before — leaves every arm here green. That is not a hole to be plugged: it + * is the optimization's safety argument restated, since the key list is a + * pure function of `dir` and interning it cannot change the key set, the key + * insertion order or any bucket's order. In particular + * `expect(first).toBe(second)` proves nothing about it — bucket identity + * across calls comes from the OUTER `perFileSet` memo on the Set's identity, + * a different cache, guarded in + * `test/integration/kotlin-import-index-reuse.test.ts`. What the arms below + * pin is the OUTPUT INVARIANT the memo has to preserve, and the one + * plausible way to get the memo wrong — keying it on the last directory + * segment instead of the whole `dir`, which hands `x/pkg`'s key list to + * `y/pkg` and merges them — fails three of them. So: a MIS-KEYED memo is + * caught here, a DELETED one is not. + * + * - **the compaction (`bucket.slice()`) is not observable either.** Deleting + * the slice and freezing the grown bucket in place leaves every arm green, + * `Object.isFrozen` included. A JS array's backing-store capacity has no + * reflective surface, so no assertion through any API can see the reclaimed + * slack. The only instrument that can is `heap_ceiling_bytes.kotlin` in + * `bench/import-target/baselines.json` — a CEILING, not a floor: compaction + * reclaims, so deleting it makes the retained reading GROW (measured + * +12.57%, 42 805 256 -> 48 184 784 B), which no floor could see. + * `Object.isFrozen` is still worth + * asserting for what it DOES catch: no freeze at all, and "compact, freeze + * the copy, forget to `set` it back" — which hands out the original, + * unfrozen bucket, and which fails the arm (verified). + * + * ## What the arms are for * * The index is module-private, so these assertions run through the resolver's * observable surface and reconstruct what they need: * * - bucket CONTENTS and ORDER come from the fan-out tier, which hands out the * bucket array itself; - * - bucket IDENTITY across calls proves the memoization, and `Object.isFrozen` - * proves the compaction pass ran over the array actually handed out; - * - the memo HIT path is reached only by a second file in the same directory, - * which is the case a single-file corpus never exercises. + * - the FIRST-CHILD tier reads `[0]` of that same array, so the two tiers + * agreeing is what makes the freeze load-bearing rather than decorative; + * - a second file in the same directory is what reaches the memo's hit path + * at all, which a single-file corpus never exercises. * * A key-order assertion is deliberately absent: `dirChildren` is only ever read * by `.get(key)`, so Map key order has no consumer and pinning it would assert @@ -30,9 +57,17 @@ import { describe, expect, it } from 'vitest'; import type { ParsedImport } from 'gitnexus-shared'; import { resolveKotlinImportTarget } from '../../../../src/core/ingestion/languages/kotlin/import-target.js'; -function bucket(files: readonly string[], targetRaw: string, set = new Set(files)) { +/** + * Takes the file Set directly. It used to take `(files, targetRaw, set = new + * Set(files))`, which three call sites drove as `bucket([], 'pkg.fn', set)` — + * an empty first argument that reads as "no files" in the one place the corpus + * matters most. Callers now spell `new Set(...)`, which also makes it visible + * where a Set is REUSED across calls (the `perFileSet` cache hit) and where a + * fresh one is built. + */ +function bucket(files: ReadonlySet, targetRaw: string) { const parsed = { kind: 'named', localName: 'X', importedName: 'X', targetRaw } as ParsedImport; - return resolveKotlinImportTarget(parsed, { fromFile: 'App.kt', allFilePaths: set } as never); + return resolveKotlinImportTarget(parsed, { fromFile: 'App.kt', allFilePaths: files } as never); } describe('getKotlinFileIndex internals (#2881)', () => { @@ -41,9 +76,10 @@ describe('getKotlinFileIndex internals (#2881)', () => { // between the two paths shows up as a missing or reordered member. The // per-file form and the memoized form must agree element for element. const files = ['a/b/pkg/One.kt', 'a/b/pkg/Two.kt', 'a/b/pkg/Three.kt', 'a/b/pkg/Four.kts']; - expect(bucket(files, 'pkg.someTopLevelFun')).toEqual(files); - expect(bucket(files, 'b.pkg.someTopLevelFun')).toEqual(files); - expect(bucket(files, 'a.b.pkg.someTopLevelFun')).toEqual(files); + const set = new Set(files); + expect(bucket(set, 'pkg.someTopLevelFun')).toEqual(files); + expect(bucket(set, 'b.pkg.someTopLevelFun')).toEqual(files); + expect(bucket(set, 'a.b.pkg.someTopLevelFun')).toEqual(files); }); it('two directories sharing a component-suffix keep separate buckets', () => { @@ -51,40 +87,112 @@ describe('getKotlinFileIndex internals (#2881)', () => { // one way this optimization can move an answer — would hand `x/pkg`'s key // list to `y/pkg` and merge them. const files = ['x/pkg/One.kt', 'y/pkg/Two.kt']; - expect(bucket(files, 'x.pkg.fn')).toEqual(['x/pkg/One.kt']); - expect(bucket(files, 'y.pkg.fn')).toEqual(['y/pkg/Two.kt']); + const set = new Set(files); + expect(bucket(set, 'x.pkg.fn')).toEqual(['x/pkg/One.kt']); + expect(bucket(set, 'y.pkg.fn')).toEqual(['y/pkg/Two.kt']); // `pkg` alone is a component-suffix of both, so it legitimately holds both, // in file-set iteration order. - expect(bucket(files, 'pkg.fn')).toEqual(files); + expect(bucket(set, 'pkg.fn')).toEqual(files); }); - it('the bucket handed out is frozen, compacted and the same object every call', () => { - // `slice()` in the freeze loop must not break the caching contract: the - // fan-out tier hands out the cached array, and the first-child tier reads - // `[0]` from the same one, so a downstream `sort()` would reorder the index - // itself. Freezing makes that a loud TypeError. + it('a shorter key list cached first does not truncate a longer one', () => { + // The case above has two directories of EQUAL depth, so a mis-keyed memo + // merges two lists of the same length and only the bucket contents move. + // Here the first directory seen (`pkg`) contributes one key and the second + // (`a/pkg`) contributes two, so reusing the first's list by last segment + // loses the `a/pkg` key entirely — a lookup that resolves today returning + // null. Different failure, same mis-keying. + const set = new Set(['pkg/One.kt', 'a/pkg/Two.kt']); + expect(bucket(set, 'pkg.fn')).toEqual(['pkg/One.kt', 'a/pkg/Two.kt']); + expect(bucket(set, 'a.pkg.fn')).toEqual(['a/pkg/Two.kt']); + }); + + it('directories sharing a MULTI-segment suffix keep separate buckets', () => { + // `q/pkg` is a shared suffix of both directories and `pkg` is a shared + // suffix of that, so the two files collide on two keys and stay apart on a + // third. A memo keyed on anything shorter than the whole `dir` merges the + // third as well. + const set = new Set(['p/q/pkg/One.kt', 'r/q/pkg/Two.kt']); + expect(bucket(set, 'p.q.pkg.fn')).toEqual(['p/q/pkg/One.kt']); + expect(bucket(set, 'r.q.pkg.fn')).toEqual(['r/q/pkg/Two.kt']); + expect(bucket(set, 'q.pkg.fn')).toEqual(['p/q/pkg/One.kt', 'r/q/pkg/Two.kt']); + expect(bucket(set, 'pkg.fn')).toEqual(['p/q/pkg/One.kt', 'r/q/pkg/Two.kt']); + }); + + it('the bucket handed out is frozen and the same object every call', () => { + // What this pins is the FREEZE, not the compaction (see the header): the + // array the fan-out tier hands out must be the one stored in the index and + // must be immutable. The finalize pass normalizes with `Array.isArray(t) ? + // t : [t]`, whose `arg is any[]` predicate widens the true branch, so + // `tsc --strict` accepts a `.sort()` or `.push()` there — and a sort would + // permanently reorder the cached bucket and flip the first-child tier's + // answer for every later import in the run. Freezing makes that a loud + // TypeError. It also fails if the compacted copy is frozen but never + // written back, since the array handed out is then the original. const set = new Set(['pkg/One.kt', 'pkg/Two.kt']); - const first = bucket([], 'pkg.fn', set) as readonly string[]; - const second = bucket([], 'pkg.fn', set) as readonly string[]; + const first = bucket(set, 'pkg.fn') as readonly string[]; + const second = bucket(set, 'pkg.fn') as readonly string[]; expect(first).toBe(second); expect(Object.isFrozen(first)).toBe(true); expect(() => (first as string[]).push('pkg/Three.kt')).toThrow(TypeError); }); + it('the first-child tier reads position 0 of the SAME bucket the fan-out returns', () => { + // The reason the freeze above matters, made observable. `import pkg.*` + // strips to `pkg`, which is the whole `pathLike`, so it answers from + // `findKotlinDirectoryChild`'s `children[0]`; `import pkg.fn` strips to + // `pkg` and fans the bucket out. One array, two tiers — so any reordering + // of the fan-out array moves the wildcard's single answer with it. + const set = new Set(['pkg/One.kt', 'pkg/Two.kt']); + const fanOut = bucket(set, 'pkg.fn') as readonly string[]; + expect(fanOut).toEqual(['pkg/One.kt', 'pkg/Two.kt']); + expect(bucket(set, 'pkg.*')).toBe(fanOut[0]); + }); + + it('every key of one directory hands out its own frozen array', () => { + // The compaction loop walks EVERY key, and a file's directory contributes + // one key per component-suffix. Checking a single key would leave a loop + // that freezes only the first entry — or that interns one array across the + // keys, which would make a future in-place edit of one bucket visible + // through all of them — passing. + const set = new Set(['a/b/pkg/One.kt', 'a/b/pkg/Two.kt']); + const full = bucket(set, 'a.b.pkg.fn') as readonly string[]; + const mid = bucket(set, 'b.pkg.fn') as readonly string[]; + const leaf = bucket(set, 'pkg.fn') as readonly string[]; + expect(Object.isFrozen(full)).toBe(true); + expect(Object.isFrozen(mid)).toBe(true); + expect(Object.isFrozen(leaf)).toBe(true); + expect(full).not.toBe(mid); + expect(mid).not.toBe(leaf); + expect(full).toEqual(leaf); + }); + it('a single-child bucket is frozen too, on the length === 1 skip path', () => { // Compaction skips `slice()` for a bucket that never grew. That branch must // still freeze, or exactly the packages with one file stay mutable. - const set = new Set(['solo/One.kt']); - const only = bucket([], 'solo.fn', set) as readonly string[]; + const only = bucket(new Set(['solo/One.kt']), 'solo.fn') as readonly string[]; expect(only).toEqual(['solo/One.kt']); expect(Object.isFrozen(only)).toBe(true); }); - it('a package larger than V8 s first growth step keeps every member in order', () => { - // 40 files takes the backing store through 1 -> 17 -> 41, so this is the - // shape compaction actually reclaims. Nothing may be lost or reordered. + it('a package larger than V8 s first growth steps keeps every member in order', () => { + // Measured on this repo's Node (v22.18.0, x64, 8 bytes per element slot), + // by allocating 40 000 push-grown arrays per length and reading retained + // heap against the same arrays rebuilt at exact length: a bucket minted as + // `[raw]` and pushed into takes its backing store through + // + // capacity 1 -> 19 -> 46 -> 86 -> 146 + // growing at lengths 2, 20, 47, 87 + // + // so 40 files sits inside the 46-slot store with 6 slots — 48 bytes — of + // retained slack, which is what compaction reclaims. (The older `1 -> 17 -> + // 41` note in this comment described a capacity that never appears here and + // under-counted that slack by 6x. The arm is unaffected either way: 40 is + // past a growth step under both models. The exact steps are a V8 detail and + // may move with the Node floor — the assertion deliberately depends only on + // there BEING slack, not on how much.) const files = Array.from({ length: 40 }, (_, i) => `big/Item${i}.kt`); - expect(bucket(files, 'big.fn')).toEqual(files); + expect(bucket(new Set(files), 'big.fn')).toEqual(files); }); it('the memo keys on the NORMALIZED directory while storing raw paths', () => { @@ -93,15 +201,10 @@ describe('getKotlinFileIndex internals (#2881)', () => { // separator is the same character at the same index — but only the KEY is // normalized; the memo must not leak that into the stored value, which // stays the raw path the file set holds. - expect(bucket(['win\\pkg\\A.kt', 'win\\pkg\\B.kt'], 'win.pkg.fn')).toEqual([ - 'win\\pkg\\A.kt', - 'win\\pkg\\B.kt', - ]); + const set = new Set(['win\\pkg\\A.kt', 'win\\pkg\\B.kt']); + expect(bucket(set, 'win.pkg.fn')).toEqual(['win\\pkg\\A.kt', 'win\\pkg\\B.kt']); // Same directory reached by its component-suffix, i.e. through the memo's // second and later keys rather than the full-dir key. - expect(bucket(['win\\pkg\\A.kt', 'win\\pkg\\B.kt'], 'pkg.fn')).toEqual([ - 'win\\pkg\\A.kt', - 'win\\pkg\\B.kt', - ]); + expect(bucket(set, 'pkg.fn')).toEqual(['win\\pkg\\A.kt', 'win\\pkg\\B.kt']); }); });