diff --git a/gitnexus/src/core/ingestion/import-resolvers/csharp.ts b/gitnexus/src/core/ingestion/import-resolvers/csharp.ts index 6886118cd..e8d6fdf7a 100644 --- a/gitnexus/src/core/ingestion/import-resolvers/csharp.ts +++ b/gitnexus/src/core/ingestion/import-resolvers/csharp.ts @@ -39,7 +39,17 @@ import { csharpSuffixFallbackAllowed } from '../csharp-namespace-gate.js'; * have to move together — see the note at step 3. * - the needle ends with '/', so every occurrence of it lies wholly inside * `D + '/'` and never reaches into the file name — which is what lets the - * whole test be evaluated on `D` alone. + * whole test be evaluated on `D` alone; + * - and then the '/' cancels. `(D + '/').endsWith(P + '/')` IS `D.endsWith(P)`: + * the appended character only ever matches itself, so it decides nothing and + * the comparison of everything before it is unchanged. The predicate the code + * actually runs is therefore + * + * match ⟺ D.endsWith(dirPrefix) + * + * with no concatenation on either side. Verified rather than argued: over + * every ordered pair of strings up to length 5 over `{a, b, '/'}` including + * the empty string — 132 496 pairs — the two forms disagreed 0 times. * * NOT the same query as `package-dir-index.ts`, and the difference is exactly * one character on each side: that module tests `'/'+D+'/'` against @@ -52,6 +62,11 @@ import { csharpSuffixFallbackAllowed } from '../csharp-namespace-gate.js'; * "cleaned up" into a reuse of `filesDirectlyInPkgDir` — see * `test/unit/import-resolvers/csharp-csproj-parity.test.ts`. * + * That one character is also why the cancellation above empties this predicate + * out but not that one: the decoration is one term per side here (`D + '/'`) and + * two there (`'/' + D + '/'`), and only the TRAILING '/' cancels. Here nothing + * is left to concatenate; there the leading segment anchor has to stay. + * * Candidates are narrowed by the directory's LAST segment, the same * O(directories) bucket `package-dir-index.ts` uses instead of an * O(files × depth) suffix map (#2649). @@ -178,15 +193,32 @@ function* matchingDirPositions( index: CsharpNamespaceDirIndex, dirPrefix: string, ): Generator { - const needle = dirPrefix + '/'; for (const dir of candidateDirs(index, dirPrefix)) { - const haystack = dir + '/'; + // `(dir + '/').endsWith(dirPrefix + '/')` IS `dir.endsWith(dirPrefix)` — the + // appended '/' only ever matches itself, so it decides nothing and BOTH + // concatenations go. Exhaustively verified, not assumed: 0 disagreements + // over every ordered pair of strings up to length 5 over `{a, b, '/'}` + // including '' (132 496 pairs). Measured 64.9 ns -> 18.4 ns per candidate + // (Node 22.18); the `dir + '/'` was paid once per candidate, on every sweep + // of the last-segment keys. + // + // Still deliberately UNANCHORED (no leading '/'), so `src/SubModels` keeps + // answering `Models` — see the derivation above. That is also exactly why + // the reduction empties this predicate out while `package-dir-index.ts` + // keeps its concatenations: one decorating term per side here, two there, + // and only the trailing one cancels. + // // `endsWith` subsumes the length guard the `indexOf` form needed: a shorter - // haystack is simply false, where `indexOf` returned -1 and + // `dir` is simply false, where `indexOf` returned -1 and // `haystack.length - needle.length` could also be -1 and report a bogus - // match. Still deliberately UNANCHORED (no leading '/'), so `src/SubModels` - // keeps answering `Models` — see the derivation above. - if (!haystack.endsWith(needle)) continue; + // match. + // + // Do NOT "finish the job" with the two-argument overload. `endsWith(search, + // endPosition)` measured 8.8-11.8 ns against 9.5-14.9 ns for the + // one-argument form across seven call-site shapes (Node 22.18) — a wash — + // and `dir.endsWith(dirPrefix, dir.length)` is character-for-character this + // same test anyway. There is nothing left here to win. + if (!dir.endsWith(dirPrefix)) continue; const positions = index.positionsByDir.get(dir); if (positions !== undefined) yield positions; } @@ -295,12 +327,33 @@ export function resolveCSharpImportInternal( // pairs. It rejected before #2881 only because it asked `indexOf` for the // FIRST `//`, which is the rule that issue removed. // + // That widening does not stay inside step 2's own bucket. This step + // 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: over `SubModels/Models/F1.cs` + `SubModels/F3.cs`, + // `using App.Models` answered both through step 3 and now answers only the + // first through step 2. The new answer is the more precise one — a + // directory literally named `Models` beating a character-suffix hit on + // `SubModels` — and it is what this module's step-2-before-step-3 layering + // asks for, so it is kept rather than worked around. Pinned absolutely by + // the parity test, which is differentially blind to it (its frozen legacy + // copy moved in lockstep with this line). + // // The empty prefix is the exception and keeps a real filter. `getDirMap` - // emits an empty directory suffix for any path whose first slash is its - // last — including a doubled slash, `a//X.cs` — so the bucket is not - // "one directory deep" on its own, and step 3 answers that same query - // from `singleSegmentDirs`, which is. Filtering on `D` holding no slash - // is what keeps steps 2 and 3 in agreement. + // keys a file under every suffix of its DIRECTORY, so it emits the EMPTY + // one exactly when that directory's last component is empty: a leading '/' + // on a root-level file, or a doubled slash immediately before the file + // name. Probed against `getDirMap`'s own key emission: + // + // src/X.cs -> ['src:.cs'] no empty key + // /X.cs -> [':.cs'] empty key + // a//X.cs -> [':.cs', 'a/:.cs'] empty key + // /a/b/X.cs -> ['b:.cs', 'a/b:.cs', '/a/b:.cs'] no empty key + // + // So the `''` bucket is not "one directory deep" on its own — `a//X.cs` + // sits in it two components down — while step 3 answers that same query + // from `singleSegmentDirs`, which is. Filtering on `D` holding no slash is + // what rejects `a//X.cs` and keeps steps 2 and 3 in agreement. for (const f of dirFiles) { if (dirPrefix === '') { const normalized = f.replace(/\\/g, '/'); diff --git a/gitnexus/src/core/ingestion/import-resolvers/go.ts b/gitnexus/src/core/ingestion/import-resolvers/go.ts index d4b36e145..eb87b0997 100644 --- a/gitnexus/src/core/ingestion/import-resolvers/go.ts +++ b/gitnexus/src/core/ingestion/import-resolvers/go.ts @@ -3,10 +3,23 @@ * * Strategy lives in configs/go.ts. * This file contains the shared helpers used by the strategy. + * + * **Reachability, as of #2929:** nothing in production calls either export + * today. The only path in is `configs/go.ts` → `createImportResolver` → + * the `importResolver` field on Go's `LanguageProvider`, and that field is + * read at exactly two lines — `import-target-adapter.ts:74-75` — whose two + * exports (`buildImportTargetWorkspace`, + * `resolveImportTargetAcrossLanguages`) have no importer anywhere but their + * own unit test. So this is a live-looking but currently unwired leg; the + * tests in `test/unit/import-resolvers/go-package-resolve.test.ts` are the + * only thing watching it. */ import type { GoModuleConfig } from '../language-config.js'; +/** `'/'`, for the parent-directory boundary check in `resolveGoPackage`. */ +const SLASH_CODE = 47; + /** * Extract the package directory suffix from a Go import path. * Returns the suffix string (e.g., "/internal/auth/") or null if invalid. @@ -28,27 +41,38 @@ export function resolveGoPackage( normalizedFileList: readonly string[], allFileList: readonly string[], ): string[] { - if (!importPath.startsWith(goModule.modulePath)) return []; + // Identical to the six lines this used to re-derive; `resolveGoPackageDir` + // returns the '/'-wrapped form and the scan wants the bare path, so unwrap. + const pkgDir = resolveGoPackageDir(importPath, goModule); + if (pkgDir === null) return []; + const relativePkg = pkgDir.slice(1, -1); // "/internal/auth/" → "internal/auth" - // Strip module path to get relative package path - const relativePkg = importPath.slice(goModule.modulePath.length + 1); // e.g., "internal/auth" - if (!relativePkg) return []; - - const pkgSuffix = '/' + relativePkg + '/'; + const pkgLen = relativePkg.length; // >= 1: `resolveGoPackageDir` rejects empty const matches: string[] = []; for (let i = 0; i < normalizedFileList.length; i++) { - // Prepend '/' so paths like "internal/auth/service.go" match suffix "/internal/auth/" - const normalized = '/' + normalizedFileList[i]; + const normalized = normalizedFileList[i]; if (!normalized.endsWith('.go') || normalized.endsWith('_test.go')) continue; // The file's PARENT directory ends with the package path — the same - // predicate `package-dir-index.ts` states, in the same `endsWith` form. - // This used to ask `indexOf` for the FIRST `//` and then check that - // nothing after it held a slash, which made `a/pkg/b/pkg/x.go` not a member - // of `pkg` (#2881). Writing it as ends-with rather than swapping in - // `lastIndexOf` also drops the second scan the `includes` guard needed. - const lastSlash = normalized.lastIndexOf('/'); // >= 0: `normalized` starts with '/' - if (!normalized.slice(0, lastSlash + 1).endsWith(pkgSuffix)) continue; + // predicate `package-dir-index.ts` states. This used to ask `indexOf` for + // the FIRST `//` and then check that nothing after it held a slash, + // which made `a/pkg/b/pkg/x.go` not a member of `pkg` (#2881). + // + // Expressed as "`relativePkg` sits immediately before the last slash, on a + // segment boundary". The boundary is either the start of the path (an + // import matching from index 0, `internal/auth/x.go`) or a `/` — which is + // what the old `'/' + path` cons bought, at the price of a per-file + // concatenation the first `endsWith` forced V8 to flatten (#2929). + // + // Rewriting this as `endsWith(relativePkg, lastSlash)` buys nothing: the + // two-argument overload measured a wash against `startsWith(needle, pos)` + // here (10.28 ns vs 9.82 ns), so it trades the clarity of an explicit start + // index for no gain. A "the 2-arg overload leaves V8's fast path, 20x" + // claim from review did not reproduce on Node 22.18 — its baseline was a + // one-argument call that early-exited on the length precheck. + const start = normalized.lastIndexOf('/') - pkgLen; // < 0 when there is no parent dir + if (start < 0 || !normalized.startsWith(relativePkg, start)) continue; + if (start > 0 && normalized.charCodeAt(start - 1) !== SLASH_CODE) continue; matches.push(allFileList[i]); } diff --git a/gitnexus/src/core/ingestion/import-resolvers/package-dir-index.ts b/gitnexus/src/core/ingestion/import-resolvers/package-dir-index.ts index 51c389021..cb7e77ed0 100644 --- a/gitnexus/src/core/ingestion/import-resolvers/package-dir-index.ts +++ b/gitnexus/src/core/ingestion/import-resolvers/package-dir-index.ts @@ -33,10 +33,27 @@ * The strongest evidence that the rule was accidental is that a sixth * implementation of the same question never had it. `import-resolvers/jvm.ts` * answers "files directly inside a directory ending with " for - * Java and Kotlin wildcard imports, and has used `lastIndexOf` since #488 — it - * is live, wired as `importResolver` by `languages/{java,kotlin}.ts`. So before - * #2881 the Java and Kotlin LanguageProvider hook and their ScopeResolver hook - * disagreed about which files a package holds. Now all six agree. + * Java and Kotlin wildcard imports, and has used `lastIndexOf` since #488. + * + * That is evidence about how the predicate was WRITTEN, not about live + * behaviour, and the distinction matters enough to spell out. `jvm.ts` is + * reached only through `provider.importResolver`, which `languages/java.ts` and + * `languages/kotlin.ts` do wire — but that field currently has no production + * READER. Its only reader anywhere is `import-target-adapter.ts`, whose own + * docblock says it is "threaded through `finalizeScopeModel`"; nothing threads + * it, and neither that module nor its two exports + * (`buildImportTargetWorkspace`, `resolveImportTargetAcrossLanguages`) is + * referenced outside its own unit test. So `jvm.ts`'s `resolveJvmWildcard` and + * `import-resolvers/go.ts`'s `resolveGoPackage` are dormant, while THIS index, + * `csharp.ts`'s `resolveCSharpImportInternal` and Kotlin's `dirChildren` are + * the ones that run. Whether those two dormant resolvers should be deleted or + * actually wired up is an open question and wants its own issue; it is not + * settled here. + * + * The argument survives that correction intact, because it never needed the + * resolvers to be live: an independent implementation of the same question, + * written without reference to the pre-index scan, reached for `lastIndexOf`. + * The extra clause was never a rule anyone chose. All six spellings now agree. * * The length guard the `indexOf` form needed is gone with it: `endsWith` is * false for a shorter `D` instead of comparing -1 to -1. @@ -139,14 +156,40 @@ function* matchingDirs(index: PackageDirIndex, pkgPath: string): Generator