From 8ca64c5666f8ed469d7890210b4181b7fd8b7399 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Mon, 10 Aug 2020 17:56:49 -0400 Subject: [PATCH] internal/imports: fix crash when adding stdlib imports We failed to initialize the ProcessEnv when adding stdlib imports. Somehow this went unnnoticed for a month or something. Initialize on demand there too, and add a stronger check so there's a better error message. Fixes golang/go#40670. Change-Id: I391c115f417390be4a0e18f92fbfbcbfbdcc052c Reviewed-on: https://go-review.googlesource.com/c/tools/+/247797 Run-TryBot: Heschi Kreinick TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/gopathwalk/walk.go | 11 ---- internal/imports/fix.go | 104 ++++++++++++++++++++++++----------- internal/imports/fix_test.go | 8 ++- internal/imports/mod.go | 10 +++- internal/imports/mod_test.go | 4 +- 5 files changed, 87 insertions(+), 50 deletions(-) diff --git a/internal/gopathwalk/walk.go b/internal/gopathwalk/walk.go index 390cb9db79..925ff53560 100644 --- a/internal/gopathwalk/walk.go +++ b/internal/gopathwalk/walk.go @@ -10,7 +10,6 @@ import ( "bufio" "bytes" "fmt" - "go/build" "io/ioutil" "log" "os" @@ -47,16 +46,6 @@ type Root struct { Type RootType } -// SrcDirsRoots returns the roots from build.Default.SrcDirs(). Not modules-compatible. -func SrcDirsRoots(ctx *build.Context) []Root { - var roots []Root - roots = append(roots, Root{filepath.Join(ctx.GOROOT, "src"), RootGOROOT}) - for _, p := range filepath.SplitList(ctx.GOPATH) { - roots = append(roots, Root{filepath.Join(p, "src"), RootGOPATH}) - } - return roots -} - // Walk walks Go source directories ($GOROOT, $GOPATH, etc) to find packages. // For each package found, add will be called (concurrently) with the absolute // paths of the containing source directory and the package directory. diff --git a/internal/imports/fix.go b/internal/imports/fix.go index ecd13e87ad..0cb09e26a9 100644 --- a/internal/imports/fix.go +++ b/internal/imports/fix.go @@ -573,7 +573,9 @@ func getFixes(fset *token.FileSet, f *ast.File, filename string, env *ProcessEnv return fixes, nil } - addStdlibCandidates(p, p.missingRefs) + if err := addStdlibCandidates(p, p.missingRefs); err != nil { + return nil, err + } p.assumeSiblingImportsValid() if fixes, done := p.fix(); done { return fixes, nil @@ -601,10 +603,14 @@ func getCandidatePkgs(ctx context.Context, wrappedCallback *scanCallback, filena notSelf := func(p *pkg) bool { return p.packageName != filePkg || p.dir != filepath.Dir(filename) } + goenv, err := env.goEnv() + if err != nil { + return err + } // Start off with the standard library. for importPath, exports := range stdlib { p := &pkg{ - dir: filepath.Join(env.goroot(), "src", importPath), + dir: filepath.Join(goenv["GOROOT"], "src", importPath), importPathShort: importPath, packageName: path.Base(importPath), relevance: MaxRelevance, @@ -779,33 +785,44 @@ type ProcessEnv struct { // If Logf is non-nil, debug logging is enabled through this function. Logf func(format string, args ...interface{}) + initialized bool + resolver Resolver } -func (e *ProcessEnv) goroot() string { - return e.mustGetEnv("GOROOT") -} - -func (e *ProcessEnv) gopath() string { - return e.mustGetEnv("GOPATH") -} - -func (e *ProcessEnv) mustGetEnv(k string) string { - v, ok := e.Env[k] - if !ok { - panic(fmt.Sprintf("%v not set in evaluated environment", k)) +func (e *ProcessEnv) goEnv() (map[string]string, error) { + if err := e.init(); err != nil { + return nil, err } - return v + return e.Env, nil +} + +func (e *ProcessEnv) matchFile(dir, name string) (bool, error) { + return build.Default.MatchFile(dir, name) } // CopyConfig copies the env's configuration into a new env. func (e *ProcessEnv) CopyConfig() *ProcessEnv { - copy := *e - copy.resolver = nil - return © + copy := &ProcessEnv{ + GocmdRunner: e.GocmdRunner, + initialized: e.initialized, + BuildFlags: e.BuildFlags, + Logf: e.Logf, + WorkingDir: e.WorkingDir, + resolver: nil, + Env: map[string]string{}, + } + for k, v := range e.Env { + copy.Env[k] = v + } + return copy } func (e *ProcessEnv) init() error { + if e.initialized { + return nil + } + foundAllRequired := true for _, k := range RequiredGoEnvVars { if _, ok := e.Env[k]; !ok { @@ -814,6 +831,7 @@ func (e *ProcessEnv) init() error { } } if foundAllRequired { + e.initialized = true return nil } @@ -832,6 +850,7 @@ func (e *ProcessEnv) init() error { for k, v := range goEnv { e.Env[k] = v } + e.initialized = true return nil } @@ -858,10 +877,14 @@ func (e *ProcessEnv) GetResolver() (Resolver, error) { return e.resolver, nil } -func (e *ProcessEnv) buildContext() *build.Context { +func (e *ProcessEnv) buildContext() (*build.Context, error) { ctx := build.Default - ctx.GOROOT = e.goroot() - ctx.GOPATH = e.gopath() + goenv, err := e.goEnv() + if err != nil { + return nil, err + } + ctx.GOROOT = goenv["GOROOT"] + ctx.GOPATH = goenv["GOPATH"] // As of Go 1.14, build.Context has a Dir field // (see golang.org/issue/34860). @@ -877,7 +900,7 @@ func (e *ProcessEnv) buildContext() *build.Context { dir.SetString(e.WorkingDir) } - return &ctx + return &ctx, nil } func (e *ProcessEnv) invokeGo(ctx context.Context, verb string, args ...string) (*bytes.Buffer, error) { @@ -892,10 +915,14 @@ func (e *ProcessEnv) invokeGo(ctx context.Context, verb string, args ...string) return e.GocmdRunner.Run(ctx, inv) } -func addStdlibCandidates(pass *pass, refs references) { +func addStdlibCandidates(pass *pass, refs references) error { + goenv, err := pass.env.goEnv() + if err != nil { + return err + } add := func(pkg string) { // Prevent self-imports. - if path.Base(pkg) == pass.f.Name.Name && filepath.Join(pass.env.goroot(), "src", pkg) == pass.srcDir { + if path.Base(pkg) == pass.f.Name.Name && filepath.Join(goenv["GOROOT"], "src", pkg) == pass.srcDir { return } exports := copyExports(stdlib[pkg]) @@ -916,6 +943,7 @@ func addStdlibCandidates(pass *pass, refs references) { } } } + return nil } // A Resolver does the build-system-specific parts of goimports. @@ -1112,21 +1140,24 @@ func (r *gopathResolver) ClearForNewScan() { func (r *gopathResolver) loadPackageNames(importPaths []string, srcDir string) (map[string]string, error) { names := map[string]string{} + bctx, err := r.env.buildContext() + if err != nil { + return nil, err + } for _, path := range importPaths { - names[path] = importPathToName(r.env, path, srcDir) + names[path] = importPathToName(bctx, path, srcDir) } return names, nil } // importPathToName finds out the actual package name, as declared in its .go files. -// If there's a problem, it returns "". -func importPathToName(env *ProcessEnv, importPath, srcDir string) (packageName string) { +func importPathToName(bctx *build.Context, importPath, srcDir string) string { // Fast path for standard library without going to disk. if _, ok := stdlib[importPath]; ok { return path.Base(importPath) // stdlib packages always match their paths. } - buildPkg, err := env.buildContext().Import(importPath, srcDir, build.FindOnly) + buildPkg, err := bctx.Import(importPath, srcDir, build.FindOnly) if err != nil { return "" } @@ -1287,8 +1318,18 @@ func (r *gopathResolver) scan(ctx context.Context, callback *scanCallback) error } stop := r.cache.ScanAndListen(ctx, processDir) defer stop() + + goenv, err := r.env.goEnv() + if err != nil { + return err + } + var roots []gopathwalk.Root + roots = append(roots, gopathwalk.Root{filepath.Join(goenv["GOROOT"], "src"), gopathwalk.RootGOROOT}) + for _, p := range filepath.SplitList(goenv["GOPATH"]) { + roots = append(roots, gopathwalk.Root{filepath.Join(p, "src"), gopathwalk.RootGOPATH}) + } // The callback is not necessarily safe to use in the goroutine below. Process roots eagerly. - roots := filterRoots(gopathwalk.SrcDirsRoots(r.env.buildContext()), callback.rootFound) + roots = filterRoots(roots, callback.rootFound) // We can't cancel walks, because we need them to finish to have a usable // cache. Instead, run them in a separate goroutine and detach. scanDone := make(chan struct{}) @@ -1348,8 +1389,6 @@ func VendorlessPath(ipath string) string { } func loadExportsFromFiles(ctx context.Context, env *ProcessEnv, dir string, includeTest bool) (string, []string, error) { - var exports []string - // Look for non-test, buildable .go files which could provide exports. all, err := ioutil.ReadDir(dir) if err != nil { @@ -1361,7 +1400,7 @@ func loadExportsFromFiles(ctx context.Context, env *ProcessEnv, dir string, incl if !strings.HasSuffix(name, ".go") || (!includeTest && strings.HasSuffix(name, "_test.go")) { continue } - match, err := env.buildContext().MatchFile(dir, fi.Name()) + match, err := env.matchFile(dir, fi.Name()) if err != nil || !match { continue } @@ -1373,6 +1412,7 @@ func loadExportsFromFiles(ctx context.Context, env *ProcessEnv, dir string, incl } var pkgName string + var exports []string fset := token.NewFileSet() for _, fi := range files { select { diff --git a/internal/imports/fix_test.go b/internal/imports/fix_test.go index 170b6d8d62..e14f1d309b 100644 --- a/internal/imports/fix_test.go +++ b/internal/imports/fix_test.go @@ -1568,8 +1568,8 @@ var _ = bytes.Buffer }, }.test(t, func(t *goimportTest) { // Run in GOROOT/src so that the std module shows up in go list -m all. - t.env.WorkingDir = filepath.Join(t.env.goroot(), "src") - got, err := t.processNonModule(filepath.Join(t.env.goroot(), "src/x.go"), []byte(input), nil) + t.env.WorkingDir = filepath.Join(t.goroot, "src") + got, err := t.processNonModule(filepath.Join(t.goroot, "src/x.go"), []byte(input), nil) if err != nil { t.Fatalf("Process() = %v", err) } @@ -1591,7 +1591,7 @@ var _ = ecdsa.GenerateKey Files: fm{"x.go": "package x"}, }, }.test(t, func(t *goimportTest) { - got, err := t.processNonModule(filepath.Join(t.env.goroot(), "src/crypto/ecdsa/foo.go"), []byte(input), nil) + got, err := t.processNonModule(filepath.Join(t.goroot, "src/crypto/ecdsa/foo.go"), []byte(input), nil) if err != nil { t.Fatalf("Process() = %v", err) } @@ -1646,6 +1646,7 @@ func (c testConfig) test(t *testing.T, fn func(*goimportTest)) { // packagestest clears out GOROOT to work around golang/go#32849, // which isn't relevant here. Fill it back in so we can find the standard library. it.env.Env["GOROOT"] = build.Default.GOROOT + it.goroot = build.Default.GOROOT fn(it) }) @@ -1662,6 +1663,7 @@ func (c testConfig) processTest(t *testing.T, module, file string, contents []by type goimportTest struct { *testing.T + goroot string env *ProcessEnv exported *packagestest.Exported } diff --git a/internal/imports/mod.go b/internal/imports/mod.go index 664fbbf5ba..94880d6160 100644 --- a/internal/imports/mod.go +++ b/internal/imports/mod.go @@ -53,6 +53,10 @@ func (r *ModuleResolver) init() error { return nil } + goenv, err := r.env.goEnv() + if err != nil { + return err + } inv := gocommand.Invocation{ BuildFlags: r.env.BuildFlags, Env: r.env.env(), @@ -82,7 +86,7 @@ func (r *ModuleResolver) init() error { if gmc := r.env.Env["GOMODCACHE"]; gmc != "" { r.moduleCacheDir = gmc } else { - r.moduleCacheDir = filepath.Join(filepath.SplitList(r.env.gopath())[0], "/pkg/mod") + r.moduleCacheDir = filepath.Join(filepath.SplitList(goenv["GOPATH"])[0], "/pkg/mod") } sort.Slice(r.modsByModPath, func(i, j int) bool { @@ -99,7 +103,7 @@ func (r *ModuleResolver) init() error { }) r.roots = []gopathwalk.Root{ - {filepath.Join(r.env.goroot(), "/src"), gopathwalk.RootGOROOT}, + {filepath.Join(goenv["GOROOT"], "/src"), gopathwalk.RootGOROOT}, } if r.main != nil { r.roots = append(r.roots, gopathwalk.Root{r.main.Dir, gopathwalk.RootCurrentModule}) @@ -240,7 +244,7 @@ func (r *ModuleResolver) findPackage(importPath string) (*gocommand.ModuleJSON, // files in that directory. If not, it could be provided by an // outer module. See #29736. for _, fi := range pkgFiles { - if ok, _ := r.env.buildContext().MatchFile(pkgDir, fi.Name()); ok { + if ok, _ := r.env.matchFile(pkgDir, fi.Name()); ok { return m, pkgDir } } diff --git a/internal/imports/mod_test.go b/internal/imports/mod_test.go index 9666fb8c2c..254bc28db0 100644 --- a/internal/imports/mod_test.go +++ b/internal/imports/mod_test.go @@ -652,6 +652,7 @@ var proxyDir string type modTest struct { *testing.T env *ProcessEnv + gopath string resolver *ModuleResolver cleanup func() } @@ -714,6 +715,7 @@ func setup(t *testing.T, main, wd string) *modTest { } return &modTest{ T: t, + gopath: env.Env["GOPATH"], env: env, resolver: resolver.(*ModuleResolver), cleanup: func() { removeDir(dir) }, @@ -841,7 +843,7 @@ package x import _ "rsc.io/quote" `, "") defer mt.cleanup() - want := filepath.Join(mt.resolver.env.gopath(), "pkg/mod", "rsc.io/quote@v1.5.2") + want := filepath.Join(mt.gopath, "pkg/mod", "rsc.io/quote@v1.5.2") found := mt.assertScanFinds("rsc.io/quote", "quote") modDir, _ := mt.resolver.modInfo(found.dir)