Skip to content

fix(go): a comment in an import declaration is not part of an import - #2374

Closed
danusha2345 wants to merge 1 commit into
colbymchenry:mainfrom
danusha2345:fix/go-import-comment-not-an-import
Closed

danusha2345 wants to merge 1 commit into
colbymchenry:mainfrom
danusha2345:fix/go-import-comment-not-an-import

Conversation

@danusha2345

Copy link
Copy Markdown
Contributor

Problem

The Go import reader runs its regexes over the raw file text, comments included. In

import (
	_ "example.com/app/lib/cache/memory" // memory cache
	"example.com/app/lib/config"
)

func main() {
	config.Load()
}

the alias pattern (\w+)\s+" matches across the line break, so the last word of the trailing comment becomes the alias of the import on the next line: the file binds cache to lib/config and has no config at all. Every config.X() in that file then loses its target, or lands on a same-named method of another package.

Two more shapes come from the same cause:

  • a quoted word in a comment is read as an import of its own: a commented-out // "example.com/app/pkg/legacy/store" shadows the real store import below it, and an import "x" usage example in a doc comment is taken for an import;
  • a ) in a comment ("example.com/app/lib/a" // (legacy)) ends the import block early, so the rest of the block is not read.

Fix

Comments are blanked (with the existing Go comment stripper, which keeps string contents and offsets) before the import regexes run. A // inside a quoted path is therefore still part of the path. Import mappings are computed at resolution time in TypeScript only, so the native and WASM extraction paths are unaffected and behave the same.

Out of scope

  • /vN import paths. example.com/errors/v2 still gets the local name v2. I tried the usual "a trailing major-version element is not the package name" rule and dropped it: it is not safe without knowing whether the path is a module path. harbor has five project directories named v1 / v2 whose package clause really is package v1 / package v2 (pkg/scan/rest/v1, pkg/token/claims/v2, …), and one file imports such a package without an alias; with the rule, its v1.Artifact{…} lost a correct edge. Telling the two apart needs the module table (go.mod module and require lines) at the point where the name is derived, which is a separate change. gopkg.in/yaml.v3 is likewise unchanged.
  • Raw-string import paths (import `x/y`) were not read before and are not read now.
  • A single-line dot import (import . "x/y") yields no mapping, as before; inside a block it yields one under the package name, as before.

Tests

New __tests__/go-import-comments.test.ts:

  • unit tests on the import reader: trailing // and /* */ comments, a comment between an alias and its path, comment lines with quoted words, a commented-out import, a ) in a comment, a // inside the quoted path, Windows line endings, the cgo preamble above import "C", and the other import forms (single line, alias, blank, dot, several declarations in one file);
  • two end-to-end cases on a real temporary project: the harbor shape (config.Load() after two commented blank imports, with a same-named method elsewhere), and a call through an import that sits under a commented-out import of a same-named package.

8 of the 10 cases fail on main and all pass with the change, with the native kernel and with CODEGRAPH_KERNEL=0. The other Go suites and __tests__/resolution.test.ts pass in both modes.

Measured on goharbor/harbor

src/ at f25e9da, 1,595 Go files, 38,243 nodes before and after.

  • Import mappings change in 9 files: 8 imports that had taken the last word of a comment as their name (cache, driver, only, function, staticcheck) get their own name back, and 1 commented-out import is no longer listed.
  • Edges: 98,545 → 98,560. Go calls 25,103 → 25,117, instantiates 6,185 → 6,186, imports and references unchanged.
  • 16 edges gained, all read and correct: 14 config.X() calls in core/main.go, metadata.Instance() in a test, and a models.PostGreSQL{…} literal.
  • 1 edge lost, a retarget: config.Load(ctx) in core/main.go used to land on the method ConfigStore.Load and now reaches the function Load of lib/config.

The file from the original finding, controller/scanner/base_controller.go, imports harbor's own lib/errors after a commented blank import. Its errors.X() calls additionally need #2368 (a project package named like a standard-library package); with both changes together the same run gains 42 edges, 26 of them from that file into lib/errors, and loses only the retargeted one above.

Four other Go projects (10 to 99 Go files) show no change in import mappings or edges.

🤖 Generated with Claude Code

The Go import reader ran its regexes over the raw file text, so inside

    import (
        _ "example.com/app/lib/cache/memory" // memory cache
        "example.com/app/lib/config"
    )

the last word of the trailing comment was read as the alias of the import
on the next line: the file bound `cache` to `lib/config`, and every
`config.X()` in it lost its target or landed on a same-named method of
another package. A quoted word in a comment (a commented-out import, an
`import "x"` usage example in a doc comment) became an import of its own,
and a `)` in a comment ended the import block early.

Comments are now blanked before the import regexes run; string contents are
kept, so a `//` inside a quoted path is still part of the path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@danusha2345

Copy link
Copy Markdown
Contributor Author

Closing in favour of #2410, which landed the same fix and goes further.

#2410 blanks comments before the Go import regexes run, reads only the import section (so an import "…" inside a later string literal is no import either), and adds the name goimports assumes for an unaliased import (…/errors/v2 → errors), which this PR left for later.

Checked on main at ed199e6: this PR's __tests__/go-import-comments.test.ts, copied over unchanged, passes 10/10 with the native kernel and with CODEGRAPH_KERNEL=0. Nothing here is left to land.

Thanks for carrying it through, and for the credit in the changelog entry.

@danusha2345 danusha2345 closed this Oct 7, 2026
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