Skip to content

fix(go): a dotted call is written through its own receiver, and a local named like an import is that variable - #2448

Merged
colbymchenry merged 9 commits into
mainfrom
claude/competent-vaughan-277302
Oct 7, 2026
Merged

colbymchenry merged 9 commits into
mainfrom
claude/competent-vaughan-277302

Conversation

@colbymchenry

@colbymchenry colbymchenry commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

Problem

goRefQualification (src/resolution/name-matcher.ts) decides which package a Go reference is written through, and isGoExternalQualified, isInGoQualifierPackage and resolveGoCrossPackageReference act on that answer (#2322, #2397). Two gaps were left after #2410.

1. A dotted call's qualifier was read from its line. A call ref like klog.Infof is recorded at its receiver's column, and the reader took the line's only X.Infof spelling. So:

  • A second spelling of the name left it with no qualifier, and the call kept a project namesake's method. Examples: harbor's &distribution.Descriptor{Digest: digest.Digest(dig)} (the Digest: key; 9 calls → Suite::Digest), kubernetes' klog.Infof("Log using Infof, …") and klog.Errorf(…) in component-base/logs/example/example.go → the etcd3 logger wrapper's methods, and cmp.Diff on a line that prints "Diff:" → kubectl's Differ::Diff.
  • Another selector earlier on the line was taken instead: in klog.Error(err.Error()), err.Error was read as a call through klog.

2. A parameter or local named like an import was taken for the package.

  • isGoExternalQualified rejected every candidate of a call through it, methods included. Examples: etcd's jwt, err := newTokenProviderJWT(…) then jwt.assign(…), and harbor's testify suites, whose func (suite *DaoTestSuite) … receivers sit in files that import testify's suite.
  • resolveGoCrossPackageReference followed the import for it: kubernetes' cache := &atomic.Bool{}; cache.Store(false) linked to client-go's cache.Store interface, and harbor's logger.Error(…) on a parameter logger logger.Interface to the package-level function logger.Error.

What changed

Resolver only (name-matcher.ts, one line in import-resolver.ts); extraction and the kernel are untouched.

  1. A dotted reference's qualifier is its name's first segment, the only segment that can be a package: klog in klog.Infof, s in s.cache.Get. Bare references are read from the line as before.
  2. A per-file Go scope reader finds what a name is bound to at a call:
    • What it reads: the receivers, parameters and results of every func with a body (function literals included, function types excluded), and the names each :=, var and const declares. Comments and string contents are blanked first.
    • Scopes follow Go's rules: a statement's local from the end of its statement to the end of its block (so jwt, err := jwt.Parse(…) still calls the package on its right-hand side), an if / for / switch header's names in that statement's blocks (else chains included), a select case's in its clause, and a parameter in its function's body.
    • A dotted call whose head is such a local is no call into the package: it is neither rejected as outside-qualified nor followed through the import. Only calls qualify. A type's qualifier is always a package, and a function-value ref can't tell a field read (quota.Name, args.PrefersProtobuf) from a method value; it is left as it was.
  3. The local's declared type decides what it reaches, when its declaration writes one:
    • A type from a package outside the project (clock clock.PassiveClock, cache := &atomic.Bool{}), or a value an outside package's function hands out (clock := clocktesting.NewFakePassiveClock(…), value := reflect.New(t)), has none of the project's methods. Such a call stays unlinked, as it was on main.
    • A project package's type is resolved as the receiver's type, through the typed path. That path follows embedding and aliases (harbor's robot := &robot.Robot{…} reaches the ToJSON that controller/robot.Robot gets from the embedded model.Robot). It also covers parameters declared through a package, like logger logger.Interface, which Go's receiver patterns don't read. A field read through the local keeps its own type: metadata.Type.String() is not MetricMetadata::String.
  4. clearNameMatcherMemos drops the scope reader's per-file cache with the other per-file memos, so a sync that edits the declaration is read afresh. There is a test for this.

Validation

Tests

  • New __tests__/go-ref-qualifier.test.ts, 6 tests:
    • the dotted-call cases (a repeated name in a string or a key, another selector before the call, a two-hop field chain whose middle segment is named like an import);
    • shadowing by a short declaration, a closure parameter, a function parameter, a range variable and a method receiver, plus the cases that must stay the package (another function, the declaration's own right-hand side, past a closure or a loop);
    • the declared-type rules (outside type, outside value, project interface parameter, embedding, a field read through the local);
    • a sync that removes the local.
  • 5 of the 6 fail on main (ed199e6). The sixth (the package stays the package elsewhere) passes there and pins that the scope reader doesn't overreach. On this branch, the sync test also fails if the new cache clear is taken out.
  • Same results with the kernel and with CODEGRAPH_KERNEL=0.
  • A 47-case harness over the scope reader, outside the repo, also passed. It covered headers with composite literals, else chains, select clauses, var (…) groups holding multi-line literals, map[string]func(jwt string){…} types, generics, multi-line parameter lists, comments, strings and labels.
  • tsc passes. 27 Go / resolution / extraction / framework suites (1,174 tests) pass with CODEGRAPH_KERNEL_EXPECT=1, plus the sync, sync-import-retry and dead-code suites (116 tests). The one exception is function-ref's Caller/impact graph misses methods passed as first-class references (callbacks), e.g. executor.submit(obj.method, …) #1820 Python case, which hit its own 60 s timeout at 100% CPU and passes with a longer limit.

Before / after (full index, kernel path, main@ed199e60 vs this branch, same kernel; natural-key edge diff, vendor/ excluded; each moved or re-labelled edge counts once in removed and once in added)

repo Go files edges main → branch removed added
gin 43fe48e 99 7,694 → 7,694 0 0
prometheus 39c878f 736 97,406 → 97,474 8 76
etcd 76d58e3 1,105 63,229 → 63,373 20 164
harbor b323c0c 1,605 98,696 → 99,504 32 840
kubernetes 4c05e667 13,501 1,232,433 → 1,232,533 135 235

Re-run on the final base (main@1fb8691d, after #2430, #2442, #2444, #2433, #2449 and #2434 landed during the merge): prometheus, etcd and harbor change exactly the same edges, edge for edge. Only the base totals moved: prometheus 97,315, etcd 63,177, harbor 98,475.

Every changed edge was triaged at its site. A site was matched across arms by caller, line and column, and the declaration of the receiver was read where the receiver mattered.

  • Removed outright: 65, all wrong on main.
    • etcd 17: zap.Error(err) → zapRaftLogger::Error, read through another selector on the line.
    • harbor 10: 9 digest.Digest → Suite::Digest, and config.ReadOnly(r), a func-typed field of the parameter config Config that was linked to lib/config's ReadOnly.
    • kubernetes 38:
      • 24 cmp.Diff → Differ::Diff;
      • 6 gomega.Expect → GomegaInstance::Expect;
      • klog.Infof / klog.Errorf → klogWrapper;
      • 2 fwk.EnqueueExtensions() on fwk, err := newFramework(…) and 2 cache.Store(false) on cache := &atomic.Bool{}, which had gone to the imports' interfaces;
      • client.Do on client := &http.Client{…} → HTTPClient::Do;
      • server.Start() on server := httptest.NewUnstartedServer(…) → a kubelet server::Start.
  • Moved to another target at the same site: 80. 77 now reach the declared type's method (typed, 0.9); 3 went from one wrong target to another.
    • kubernetes 63: parameters like rest rest.Interface, meta metav1.Object, clientset clientset.Interface, watch watch.Interface, version *version.Version, apps appsv1.AppsV1Interface, cadvisor cadvisor.Interface. On main these were receiver-word guesses (0.65 to 0.85) into unrelated packages, like RESTClient::Patch or kubelet's Version::String.
    • harbor 8: logger.Error / Infof / Debugf on logger logger.Interface now reach Interface::* instead of the package's function.
    • prometheus 4:
      • storage.Appender on storage *teststorage.TestStorage → DB::Appender, through the embedded *tsdb.DB;
      • 3 labels.String → model/labels' Labels::String instead of prompb's.
    • etcd 2: client.Compact / client.Txn on client *client.RecordingClient.
    • Wrong → wrong (3): harbor's 2 logger.* in copy.go (logger := log.GetLogger(ctx), whose type nothing reads), and prometheus's storage := promqltest.LoadedStorage(…). They went from the import's function or interface to a guessed Logger or Storage method.
  • Same target, now typed (guess → 0.9): 50 (kubernetes 34, harbor 12, prometheus 3, etcd 1).
  • Added through a parameter or local named like an import: 856, of which 843 are typed.
    • harbor 818: 815 testify-suite receiver calls, plus reg reg.Client, repository *model.RepoRecord and an allowlist local.
    • kubernetes 24 and prometheus 1 (config.UnmarshalYAML on var config SDConfig).
    • Guesses, 13, as for any other local of unknown type:
      • etcd's 3 jwt.assign / jwt.info (correct);
      • kubernetes' 6 hcn.* on hcn := (proxier.hcn).(*fakehcn.HcnMock) and status.FindContainerStatusByName (correct);
      • kubernetes' status.GetInfo and clock.Now on clock := testKubelet.fakeClock (wrong);
      • prometheus' api.QueryRange → the project's queryRangeAPI interface, on api, err := newAPI(…), which returns client_golang's v1.API (a guess).
  • Added through a receiver that isn't an import: 329. On main the line reader had taken another selector on the line as these calls' qualifier, so they were rejected as calls into it (http.Error(w, err.Error(), …), zap.String("id", c.cid.String())).
    • Correct, 113: etcd's 102 two-hop field chains (0.85, e.g. c.cid.String() on a types.ID field) and 10 typed calls, plus prometheus' q.stmt.String chain.

    • Plausible, 20: etcd's 18 id.String() on types.ID parameters → ID::String, and 2 reqStringer.String.

    • Wrong, 196: guesses for a standard method name.

      • Kubernetes 105: 103 X.Error() → notRegisteredErr::Error by a shared word, incl. maxinflight.go:53 from the report, and 2 by a capitalized receiver.
      • Prometheus 65: err.Error() and the like → ParseErr::Error.
      • etcd 26: 17 err.Error() → ErrKeepAliveHalted::Error, and 9 String() calls → a protobuf request's String by a shared word, like leaderID → MoveLeaderRequest.

      This is an existing guess class, not a new one. On main, kubernetes already has 2,374 guesses for a standard method name (1,995 for Error), prometheus 193 and etcd 323. Main skipped these sites only because of the misreading this PR fixes. Follow-up below.

Performance

  • The scope reader builds once per Go file per resolver thread; the counts match distinct files (instrumented etcd index: 880 builds across 6 threads for 880 files).
  • One pass over every Go file: about 22 ms per MB of source (etcd 125 ms, kubernetes 3.1 s for 140 MB) on a machine at 100% CPU.
  • CPU profiles of a full etcd index attribute 235 ms to it, about 0.5% of 46 s. goRefQualification itself got cheaper, since a dotted ref no longer builds two regexes from its line.
  • Full-index CPU, 7 interleaved etcd rounds: median 46.2 s vs 47.0 s, ranges overlapping (other sessions kept the box at 100%).
  • The blanking was rewritten for speed. It gives identical text on all 21,367 Go files of the five trees (vendored ones included), and arms before and after it produce identical graphs on prometheus, etcd and harbor.

Limits and follow-ups (not in this PR)

  • Receiver-word guesses for Error() / String(). Strategy 3 accepts a project method of a standard name when the receiver shares a word with its owner, so err matches notRegisteredErr or ParseErr. It also guesses through a receiver declared with a predeclared type (err error at maxinflight.go:53), because the typed path stops only at an undeclared capitalized type. Fixing it changes thousands of edges on main, so it deserves its own A/B.
  • Locals named like a standard-library package. isBuiltInOrExternal skips any dotted Go call whose head is in GO_STDLIB_PACKAGES (log, user, token, parser, scanner, context, …) before resolution, whether or not it is a local. Examples: gin's context.AbortWithError on a context *Context parameter, and harbor's user.Username. The scope reader isn't consulted there. fix(go): a project package named like a standard-library package is not the standard library #2368 (@danusha2345, open) lets project packages named like the standard library through. That composes with this change but doesn't cover locals. Un-skipping locals also exposes calls on locals of standard-library types (scanner.Text(), url.String()) to the guess above, so it needs the guess fix first.
  • Go receiver inference doesn't read a package-qualified parameter type (c clock.PassiveClock, pod *v1.Pod) unless the parameter is named like an import, which this PR covers. Doing it for every receiver is a much wider change.
  • := locals whose value comes from a project function or a field (newAPI(…), testKubelet.fakeClock) keep the usual guesses.

Overlap

No open PR touches these functions. #2368 edits isBuiltInOrExternal in src/resolution/index.ts, with no textual overlap; see the follow-ups.

🤖 Generated with Claude Code

colbymchenry and others added 9 commits October 7, 2026 06:33
…al named like an import is that variable

goRefQualification read a dotted Go call's package qualifier from its line,
as the line's only `X.Name` spelling. A second spelling of the name left it
with none (`Digest:` beside `digest.Digest(dig)`, `"Log using Infof"`), and
another selector before it was taken instead (`klog` for `err.Error` in
`klog.Error(err.Error())`). It now takes the reference name's first segment,
the only one that can be a package.

A call through a parameter or local that takes an import's name (etcd's
`jwt, err := newTokenProviderJWT(…)`, testify suites' `suite` receivers) is
a call on that variable. A per-file scope reader finds the parameters,
receivers, results, `:=`, `var` and `const` names in scope at the call, so
such a call is no longer one into the package. The variable's declared type,
when its declaration writes one, decides what it reaches: an outside
package's type has none of the project's methods, a project package's type
is resolved as the receiver's type.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s it

The Go scope reader keeps each file's declarations per resolution context.
clearNameMatcherMemos now drops them with the other per-file memos, so a
sync that removes a local named like an import stops treating the call as
one on that local.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…own type

`metadata.Type.String()` on a parameter `metadata prompb.MetricMetadata`
calls String on the field, so the parameter's declared type only stands
for a call made on the parameter itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d like an import

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…function, has no project method

`clock := clocktesting.NewFakePassiveClock(…)` holds a value of a type that
package declares, so `clock.Now()` is no call to a project `Clock`'s Now.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ts braces by regex

The Go scope reader built its blanked text by appending each piece and
replacing every character of each comment and string. It now collects the
pieces and joins them once, and skips from one comment or quote to the next.
Same text on every Go file of gin, prometheus, etcd, harbor and kubernetes;
about twice as fast.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant