fix(go): resolve cross-module calls in multi-module layouts - #1521
Open
ericquan8 wants to merge 1 commit into
Open
fix(go): resolve cross-module calls in multi-module layouts#1521ericquan8 wants to merge 1 commit into
ericquan8 wants to merge 1 commit into
Conversation
A Go monorepo with several independent modules under one root (and no root go.mod) resolved cross-module calls at ~4% recall / ~7% precision: the single-module reader only checked projectRoot/go.mod, so every in-repo import was classified third-party and resolution fell through to global name-matching, wiring most cross-module calls to the wrong target. A wrong edge is worse than a missing one for impact analysis. This is the follow-up colbymchenry#388's own code comment asked for. Multi-module import resolution: - loadGoModules(): index EVERY go.mod under projectRoot (skip-list, depth/count caps, entries sorted by modulePath length DESCENDING so the longest prefix wins, deterministic traversal, shortest-relDir wins on a duplicated module path). packageDir() reproduces the old single-module algorithm byte-for-byte when relDir=''; it can legally return '', so callers test === null. loadGoModule()/GoModule untouched. - ResolutionContext.getGoModules?() added (optional); wired as a lazy, memoised provider alongside getGoModule(), matching the existing path-aliases / workspace-packages convention. Like those, it is resolver metadata only — it produces no nodes and no edges. - isExternalImport (Go branch), resolveGoCrossPackageReference and the name-matcher Go field-type guard prefer the index and fall back to the single-module path verbatim. The exact-parent match (fileDir === pkgDir) and the colbymchenry#1276 anti-fabrication guard are preserved. Two extraction-layer defects with the same symptom (both reproduce in a single-module repo too): - Go const/var/method isExported was never set, so the resolver's `if (!node.isExported) continue` dropped them. extractMethod never called the hook; the Go const/var branch omitted it AND would have fed the hook the declaration node, which has no `name` field — the identifier is on the const_spec/var_spec child. Computed per-spec inside the Go branch; the go.ts hook and the shared declaration-level isExported are untouched. - Grouped `var (...)` produced zero nodes. tree-sitter-go wraps a grouped var's specs in a `var_spec_list` node while a grouped const's specs are direct children, so the direct-child filter found nothing. Flattening the wrapper also restores calls made inside package-level grouped-var initializers, which previously had no source node at all. Verification (reproducible) — 14 tests build real multi-module layouts in temp dirs and index them for real, no mocks: npx vitest run __tests__/resolution.test.ts -t "Go multi-module" npx vitest run __tests__/extraction.test.ts -t "Go const/var extraction" covering cross-module resolution, same-name symbol in a non-imported module must not be picked, longest-prefix precedence, single-root-module no-regress, no-go.mod no-op, sub-package exclusion, scan-depth cap, grouped var/const extraction, isExported across all four const/var forms, and TS/Python/Rust no-regress. Full suite green. Scale check (private repo, NOT reproducible here; reported for magnitude). 11.5k Go files, 61 side-by-side modules. Ground truth is derived from the source with no manual labelling — an import alias binds to one module path, which maps to one local directory; a target defined exactly once in that directory is unambiguous. Anything ambiguous is discarded, so the numbers are conservative. cross-module call recall 4.03% -> 99.67% cross-module target precision 6.83% -> 100% file coverage 99.94% -> 99.94% call-site line precision 99.20% -> 99.73% node count 302,900 -> 311,347 (+2.8%, grouped vars) Known gap, out of scope: cross-module references to package-level const/var still do not resolve. Value-position `alias.Symbol` references are never EXTRACTED as references (flushValueRefs is same-file only by design), so they never reach the resolver regardless of isExported. The isExported fix above is still correct, but it removes a guard nothing currently reaches. The real fix is an extraction-layer change, tracked separately. Design notes: docs/design/go-multi-module-resolution.md Refs colbymchenry#388.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A Go monorepo with several independent modules under one root (and no root
go.mod) resolved cross-module calls at ~4% recall / ~7% precision: the
single-module reader only checked projectRoot/go.mod, so every in-repo
import was classified third-party and resolution fell through to global
name-matching, wiring most cross-module calls to the wrong target. A wrong
edge is worse than a missing one for impact analysis.
This is the follow-up #388's own code comment asked for.
Multi-module import resolution:
depth/count caps, entries sorted by modulePath length DESCENDING so the
longest prefix wins, deterministic traversal, shortest-relDir wins on a
duplicated module path). packageDir() reproduces the old single-module
algorithm byte-for-byte when relDir=''; it can legally return '', so
callers test === null. loadGoModule()/GoModule untouched.
memoised provider alongside getGoModule(), matching the existing
path-aliases / workspace-packages convention. Like those, it is resolver
metadata only — it produces no nodes and no edges.
name-matcher Go field-type guard prefer the index and fall back to the
single-module path verbatim. The exact-parent match (fileDir === pkgDir)
and the Go: external receiver calls resolve to unrelated local interface methods #1276 anti-fabrication guard are preserved.
Two extraction-layer defects with the same symptom (both reproduce in a
single-module repo too):
if (!node.isExported) continuedropped them. extractMethod never calledthe hook; the Go const/var branch omitted it AND would have fed the hook
the declaration node, which has no
namefield — the identifier is on theconst_spec/var_spec child. Computed per-spec inside the Go branch; the
go.ts hook and the shared declaration-level isExported are untouched.
var (...)produced zero nodes. tree-sitter-go wraps a groupedvar's specs in a
var_spec_listnode while a grouped const's specs aredirect children, so the direct-child filter found nothing. Flattening the
wrapper also restores calls made inside package-level grouped-var
initializers, which previously had no source node at all.
Verification (reproducible) — 14 tests build real multi-module layouts in
temp dirs and index them for real, no mocks:
covering cross-module resolution, same-name symbol in a non-imported module
must not be picked, longest-prefix precedence, single-root-module
no-regress, no-go.mod no-op, sub-package exclusion, scan-depth cap, grouped
var/const extraction, isExported across all four const/var forms, and
TS/Python/Rust no-regress. Full suite green.
Scale check (private repo, NOT reproducible here; reported for magnitude).
11.5k Go files, 61 side-by-side modules. Ground truth is derived from the
source with no manual labelling — an import alias binds to one module path,
which maps to one local directory; a target defined exactly once in that
directory is unambiguous. Anything ambiguous is discarded, so the numbers
are conservative.
Known gap, out of scope: cross-module references to package-level const/var
still do not resolve. Value-position
alias.Symbolreferences are neverEXTRACTED as references (flushValueRefs is same-file only by design), so
they never reach the resolver regardless of isExported. The isExported fix
above is still correct, but it removes a guard nothing currently reaches.
The real fix is an extraction-layer change, tracked separately.
Design notes: docs/design/go-multi-module-resolution.md
Refs #388.