internal/imports: use module cache index if it exists goimports can be very slow as the existing code scans the module cache. The new version uses the index to the module cache if it exists. The index usually exists if the user uses gopls. There is a new test for the goimports command that uses just a module cache index, with no files in the module cache. None of the tests in internal/imports needed to be changed, as they do not create an index. On my laptop the new code executes in .66 seconds instead of 3.4 seconds. golang/go#80087 discusses an unresolved edge case Fixes: golang/go#78671 Fixes: golang/go#76310 Change-Id: Ice9a6ef8075fbea9fa4220db2576da0a394e7b10 Reviewed-on: https://go-review.googlesource.com/c/tools/+/767881 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Hongxiang Jiang <hxjiang@golang.org>
diff --git a/cmd/goimports/goimports.go b/cmd/goimports/goimports.go index f7dec9b..e796905 100644 --- a/cmd/goimports/goimports.go +++ b/cmd/goimports/goimports.go
@@ -19,6 +19,7 @@ "runtime" "runtime/pprof" "strings" + "testing" "golang.org/x/telemetry/counter" "golang.org/x/tools/internal/gocommand" @@ -199,7 +200,8 @@ } func main() { - // is anyone using this command? + // Measure how many people still use goimports. + // (See https://go.dev/issue/78671 for one.) counter.Open() counter.Inc("tools/cmd:goimports") runtime.GOMAXPROCS(runtime.NumCPU()) @@ -208,7 +210,9 @@ // so that it can use defer and have them // run before the exit. gofmtMain() - os.Exit(exitCode) + if !testing.Testing() { + os.Exit(exitCode) + } } // parseFlags parses command line flags and returns the paths to process.
diff --git a/cmd/goimports/main_test.go b/cmd/goimports/main_test.go new file mode 100644 index 0000000..192eb28 --- /dev/null +++ b/cmd/goimports/main_test.go
@@ -0,0 +1,107 @@ +// Copyright 2026 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "log" + "os" + "path/filepath" + "testing" + + "golang.org/x/tools/internal/modindex" + "golang.org/x/tools/txtar" +) + +const cache = ` +-- github.com/nbutton23/zxcvbn-go@v0.0.0-20210217022336-fa2cb2858354/zxcvnb.go -- +package zxcvbn +func PasswordStrength(password string, userInputs []string, filters ...func(match.Matcher) bool) {} +-- github.com/klauspost/compress/zstd@v1.16.7/zstd.go -- +package zstd +func EncoderLevelFromString(s string) (EncoderLevel, error) {} +-- github.com/containerd/stargz-snapshotter@v0.11.3/estargz/zstdchunked/zstdchunked.go -- +package zstdchunked +const FooterSize = 4 +-- github.com/nbutton23/zxcvbn-go@v0.0.0-20210217022336-fa2cb2858354/utils/math/mathutils.go -- +package zxcvbnmath +func NChoseK(n, k float64) float64 {return 0} +` +const src = `package p + +var _ = zxcvbnmath.NChoseK +var _ = zxcvbn.PasswordStrength +var _ = zstdchunked.FooterSize +var _ = zstd.EncoderLevelFromString +` +const want = `package p + +import ( + "github.com/containerd/stargz-snapshotter/estargz/zstdchunked" + "github.com/klauspost/compress/zstd" + "github.com/nbutton23/zxcvbn-go" + zxcvbnmath "github.com/nbutton23/zxcvbn-go/utils/math" +) + +var _ = zxcvbnmath.NChoseK +var _ = zxcvbn.PasswordStrength +var _ = zstdchunked.FooterSize +var _ = zstd.EncoderLevelFromString +` + +// TestCmd runs the command on a module cache with +// an index but no cached files. The test sets up +// the environment and calls main(). The alternative +// approach, to run the binary as a subprocess, does +// not work, as building the repository may require +// getting modules from the network, and not all +// the builders have network access. +func TestCmd(t *testing.T) { + log.SetFlags(log.Lshortfile) + dir := t.TempDir() + modindex.IndexDir = filepath.Join(modindex.IndexDir, "goimports") + if err := os.MkdirAll(modindex.IndexDir, 0777); err != nil { + t.Fatalf("failed to create index dir: %v", err) + } + // write the cache + modcache := filepath.Join(dir, "/mod") + if err := os.MkdirAll(modcache, 0755); err != nil { + t.Fatalf("failed to create modcache: %v", err) + } + archive := txtar.Parse([]byte(cache)) + fsys, err := txtar.FS(archive) + if err != nil { + t.Fatalf("failed to create fsys: %v", err) + } + if err := os.CopyFS(modcache, fsys); err != nil { + t.Fatalf("failed to copy fsys: %v", err) + } + // create the index + _, err = modindex.Update(modcache) + if err != nil { + t.Fatalf("failed to create index: %v", err) + } + // now remove all the files, so go imports uses only the index + if err := os.RemoveAll(filepath.Join(modcache, "github.com")); err != nil { + t.Fatalf("failed to remove github.com: %v", err) + } + // write the test file + fname := filepath.Join(dir, "main.go") + if err := os.WriteFile(fname, []byte(src), 0644); err != nil { + t.Fatalf("failed to write file: %v", err) + } + + os.Args = append(os.Args, "-w", fname) + // need to change the current environment + os.Setenv("GOMODCACHE", modcache) + gofmtMain() + + got, err := os.ReadFile(fname) + if err != nil { + t.Fatalf("failed to read file: %v", err) + } + if string(got) != want { + t.Errorf("goimports -w %s failed: got %s, want %s", fname, got, want) + } +}
diff --git a/internal/imports/fix.go b/internal/imports/fix.go index b99ea6c..6aa1e27 100644 --- a/internal/imports/fix.go +++ b/internal/imports/fix.go
@@ -32,6 +32,7 @@ "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/gopathwalk" + "golang.org/x/tools/internal/modindex" "golang.org/x/tools/internal/stdlib" ) @@ -320,6 +321,7 @@ // load reads in everything necessary to run a pass, and reports whether the // file already has all the imports it needs. It fills in p.missingRefs with the // file's missing symbols, if any, or removes unused imports if not. +// This is called 3(!) times: self, otherFiles, loadRealPackageNames func (p *pass) load(ctx context.Context) ([]*ImportFix, bool) { p.knownPackages = map[string]*PackageInfo{} p.missingRefs = References{} @@ -577,6 +579,17 @@ } func getFixesWithSource(ctx context.Context, fset *token.FileSet, f *ast.File, filename string, goroot string, logf func(string, ...any), source Source) ([]*ImportFix, error) { + // If there is an Index for the GOMODCACHE, remember that, and later make it so that the + // directory walk doesn't go into the module cache, since we already have all the information + var ix *modindex.Index + src, ok := source.(*ProcessEnvSource) + if ok { + var err error + if ix, err = modindex.Read(src.env.Env["GOMODCACHE"]); err != nil { + ix = nil // don't use it if there was an error + } + } + // This logic is defensively duplicated from getFixes. abs, err := filepath.Abs(filename) if err != nil { @@ -635,6 +648,20 @@ } p.loadRealPackageNames = true p.otherFiles = otherFiles + if ix != nil { + src, ok := p.source.(*ProcessEnvSource) + if ok { + // For safety, clone the env so that we don't modify the caller's env. + env := *src.env + env.Env = maps.Clone(src.env.Env) + src.env = &env + // avoid looking in the module cache, as we have the index instead: + // This makes a later call to newModuleresolver (from + // LoadPackageNames) produce a resolver that will not look + // in the module cache + src.env.Env["GOMODCACHE"] = "" + } + } if fixes, done := p.load(ctx); done { return fixes, nil } @@ -649,7 +676,7 @@ // Go look for candidates in $GOPATH, etc. We don't necessarily load // the real exports of sibling imports, so keep assuming their contents. - if err := addExternalCandidates(ctx, p, p.missingRefs, filename); err != nil { + if err := addExternalCandidates(ctx, p, p.missingRefs, filename, ix); err != nil { return nil, err } @@ -1184,7 +1211,7 @@ exportsLoaded func(pkg *pkg, exports []stdlib.Symbol) } -func addExternalCandidates(ctx context.Context, pass *pass, refs References, filename string) error { +func addExternalCandidates(ctx context.Context, pass *pass, refs References, filename string, ix *modindex.Index) error { ctx, done := event.Start(ctx, "imports.addExternalCandidates") defer done() @@ -1193,6 +1220,24 @@ return err } + // Add candidates from the module cache. + if ix != nil { + for k, v := range refs { + for n := range v { + cands := ix.Lookup(k, n, false) + for _, cand := range cands { + x := &Result{ + &ImportInfo{ImportPath: cand.ImportPath}, + &PackageInfo{Name: cand.PkgName, + Exports: map[string]bool{cand.Name: true}, + }, + } + results = append(results, x) + } + } + } + } + for _, result := range results { if result == nil { continue
diff --git a/internal/imports/mod.go b/internal/imports/mod.go index 76fae6a..a05faf9 100644 --- a/internal/imports/mod.go +++ b/internal/imports/mod.go
@@ -166,10 +166,7 @@ } } - r.moduleCacheDir = gomodcacheForEnv(goenv) - if r.moduleCacheDir == "" { - return nil, fmt.Errorf("cannot resolve GOMODCACHE") - } + r.moduleCacheDir = goenv["GOMODCACHE"] sort.Slice(r.modsByModPath, func(i, j int) bool { count := func(x int) int { @@ -238,26 +235,6 @@ return r, nil } -// gomodcacheForEnv returns the GOMODCACHE value to use based on the given env -// map, which must have GOMODCACHE and GOPATH populated. -// -// TODO(rfindley): this is defensive refactoring. -// 1. Is this even relevant anymore? Can't we just read GOMODCACHE. -// 2. Use this to separate module cache scanning from other scanning. -func gomodcacheForEnv(goenv map[string]string) string { - if gmc := goenv["GOMODCACHE"]; gmc != "" { - // golang/go#67156: ensure that the module cache is clean, since it is - // assumed as a prefix to directories scanned by gopathwalk, which are - // themselves clean. - return filepath.Clean(gmc) - } - gopaths := filepath.SplitList(goenv["GOPATH"]) - if len(gopaths) == 0 { - return "" - } - return filepath.Join(gopaths[0], "/pkg/mod") -} - func (r *ModuleResolver) initAllMods() error { stdout, err := r.env.invokeGo(context.TODO(), "list", "-m", "-e", "-json", "...") if err != nil {
diff --git a/internal/imports/source_env.go b/internal/imports/source_env.go index ec996c3..272ae80 100644 --- a/internal/imports/source_env.go +++ b/internal/imports/source_env.go
@@ -53,7 +53,7 @@ found := make(map[string][]pkgDistance) callback := &scanCallback{ rootFound: func(gopathwalk.Root) bool { - return true // We want everything. + return true }, dirFound: func(pkg *pkg) bool { return pkgIsCandidate(filename, refs, pkg)