internal/imports: require valid options, move LocalPrefix up

CL 239754 eagerly initialized the environment. This turns out to be a
problem for gopls, which calls ApplyFixes with no ProcessEnv.
Reinitializing it every time seriously harmed the performance of
unimported completions. Move back to lazy initialization.

Working with invalid options has caused a lot of confusion; this is only
the most recent. We have to maintain backwards compatibility in the
externally visible API, but everywhere else we can require fully
populated options. That includes the source byte slice and the options.

LocalPrefix is really more of an Option than an attribute of the
ProcessEnv, and it is needed in ApplyFixes where we really don't want to
have to pass a ProcessEnv. Move it up to Options.

Change-Id: Ib9466c375a640a521721da4587091bf93bbdaa3c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/241159
Run-TryBot: Heschi Kreinick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>

Author: heschi

cmd/goimports/goimports.go

@@ -21,6 +21,7 @@ import (
 	"runtime/pprof"
 	"strings"
 
+	"golang.org/x/tools/internal/gocommand"
 	"golang.org/x/tools/internal/imports"
 )
 
@@ -42,14 +43,16 @@ var (
 		TabIndent: true,
 		Comments:  true,
 		Fragment:  true,
-		Env: &imports.ProcessEnv{},
+		Env: &imports.ProcessEnv{
+			GocmdRunner: &gocommand.Runner{},
+		},
 	}
 	exitCode = 0
 )
 
 func init() {
 	flag.BoolVar(&options.AllErrors, "e", false, "report all errors (not just the first 10 on different lines)")
-	flag.StringVar(&options.Env.LocalPrefix, "local", "", "put imports beginning with this string after 3rd-party packages; comma-separated list")
+	flag.StringVar(&options.LocalPrefix, "local", "", "put imports beginning with this string after 3rd-party packages; comma-separated list")
 	flag.BoolVar(&options.FormatOnly, "format-only", false, "if true, don't fix imports and only format. In this mode, goimports is effectively gofmt, with the addition that imports are grouped into sections.")
 }
 

imports/forward.go

@@ -3,8 +3,10 @@
 package imports // import "golang.org/x/tools/imports"
 
 import (
+	"io/ioutil"
 	"log"
 
+	"golang.org/x/tools/internal/gocommand"
 	intimp "golang.org/x/tools/internal/imports"
 )
 
@@ -29,25 +31,34 @@ var Debug = false
 var LocalPrefix string
 
 // Process formats and adjusts imports for the provided file.
-// If opt is nil the defaults are used.
+// If opt is nil the defaults are used, and if src is nil the source
+// is read from the filesystem.
 //
 // Note that filename's directory influences which imports can be chosen,
 // so it is important that filename be accurate.
 // To process data ``as if'' it were in filename, pass the data as a non-nil src.
 func Process(filename string, src []byte, opt *Options) ([]byte, error) {
+	var err error
+	if src == nil {
+		src, err = ioutil.ReadFile(filename)
+		if err != nil {
+			return nil, err
+		}
+	}
 	if opt == nil {
 		opt = &Options{Comments: true, TabIndent: true, TabWidth: 8}
 	}
 	intopt := &intimp.Options{
 		Env: &intimp.ProcessEnv{
-			LocalPrefix: LocalPrefix,
+			GocmdRunner: &gocommand.Runner{},
 		},
-		AllErrors:  opt.AllErrors,
-		Comments:   opt.Comments,
-		FormatOnly: opt.FormatOnly,
-		Fragment:   opt.Fragment,
-		TabIndent:  opt.TabIndent,
-		TabWidth:   opt.TabWidth,
+		LocalPrefix: LocalPrefix,
+		AllErrors:   opt.AllErrors,
+		Comments:    opt.Comments,
+		FormatOnly:  opt.FormatOnly,
+		Fragment:    opt.Fragment,
+		TabIndent:   opt.TabIndent,
+		TabWidth:    opt.TabWidth,
 	}
 	if Debug {
 		intopt.Env.Logf = log.Printf

internal/imports/fix.go

@@ -32,25 +32,25 @@ import (
 
 // importToGroup is a list of functions which map from an import path to
 // a group number.
-var importToGroup = []func(env *ProcessEnv, importPath string) (num int, ok bool){
-	func(env *ProcessEnv, importPath string) (num int, ok bool) {
-		if env.LocalPrefix == "" {
+var importToGroup = []func(localPrefix, importPath string) (num int, ok bool){
+	func(localPrefix, importPath string) (num int, ok bool) {
+		if localPrefix == "" {
 			return
 		}
-		for _, p := range strings.Split(env.LocalPrefix, ",") {
+		for _, p := range strings.Split(localPrefix, ",") {
 			if strings.HasPrefix(importPath, p) || strings.TrimSuffix(p, "/") == importPath {
 				return 3, true
 			}
 		}
 		return
 	},
-	func(_ *ProcessEnv, importPath string) (num int, ok bool) {
+	func(_, importPath string) (num int, ok bool) {
 		if strings.HasPrefix(importPath, "appengine") {
 			return 2, true
 		}
 		return
 	},
-	func(_ *ProcessEnv, importPath string) (num int, ok bool) {
+	func(_, importPath string) (num int, ok bool) {
 		firstComponent := strings.Split(importPath, "/")[0]
 		if strings.Contains(firstComponent, ".") {
 			return 1, true
@@ -59,9 +59,9 @@ var importToGroup = []func(env *ProcessEnv, importPath string) (num int, ok bool
 	},
 }
 
-func importGroup(env *ProcessEnv, importPath string) int {
+func importGroup(localPrefix, importPath string) int {
 	for _, fn := range importToGroup {
-		if n, ok := fn(env, importPath); ok {
+		if n, ok := fn(localPrefix, importPath); ok {
 			return n
 		}
 	}
@@ -278,7 +278,12 @@ func (p *pass) loadPackageNames(imports []*ImportInfo) error {
 		unknown = append(unknown, imp.ImportPath)
 	}
 
-	names, err := p.env.GetResolver().loadPackageNames(unknown, p.srcDir)
+	resolver, err := p.env.GetResolver()
+	if err != nil {
+		return err
+	}
+
+	names, err := resolver.loadPackageNames(unknown, p.srcDir)
 	if err != nil {
 		return err
 	}
@@ -640,15 +645,23 @@ func getCandidatePkgs(ctx context.Context, wrappedCallback *scanCallback, filena
 			wrappedCallback.exportsLoaded(pkg, exports)
 		},
 	}
-	return env.GetResolver().scan(ctx, scanFilter)
+	resolver, err := env.GetResolver()
+	if err != nil {
+		return err
+	}
+	return resolver.scan(ctx, scanFilter)
 }
 
-func ScoreImportPaths(ctx context.Context, env *ProcessEnv, paths []string) map[string]int {
+func ScoreImportPaths(ctx context.Context, env *ProcessEnv, paths []string) (map[string]int, error) {
 	result := make(map[string]int)
+	resolver, err := env.GetResolver()
+	if err != nil {
+		return nil, err
+	}
 	for _, path := range paths {
-		result[path] = env.GetResolver().scoreImportPath(ctx, path)
+		result[path] = resolver.scoreImportPath(ctx, path)
 	}
-	return result
+	return result, nil
 }
 
 func PrimeCache(ctx context.Context, env *ProcessEnv) error {
@@ -674,8 +687,9 @@ func candidateImportName(pkg *pkg) string {
 	return ""
 }
 
-// getAllCandidates gets all of the candidates to be imported, regardless of if they are needed.
-func getAllCandidates(ctx context.Context, wrapped func(ImportFix), searchPrefix, filename, filePkg string, env *ProcessEnv) error {
+// GetAllCandidates gets all of the packages starting with prefix that can be
+// imported by filename, sorted by import path.
+func GetAllCandidates(ctx context.Context, wrapped func(ImportFix), searchPrefix, filename, filePkg string, env *ProcessEnv) error {
 	callback := &scanCallback{
 		rootFound: func(gopathwalk.Root) bool {
 			return true
@@ -714,7 +728,8 @@ type PackageExport struct {
 	Exports []string
 }
 
-func getPackageExports(ctx context.Context, wrapped func(PackageExport), searchPkg, filename, filePkg string, env *ProcessEnv) error {
+// GetPackageExports returns all known packages with name pkg and their exports.
+func GetPackageExports(ctx context.Context, wrapped func(PackageExport), searchPkg, filename, filePkg string, env *ProcessEnv) error {
 	callback := &scanCallback{
 		rootFound: func(gopathwalk.Root) bool {
 			return true
@@ -749,8 +764,6 @@ var RequiredGoEnvVars = []string{"GO111MODULE", "GOFLAGS", "GOINSECURE", "GOMOD"
 // ProcessEnv contains environment variables and settings that affect the use of
 // the go command, the go/build package, etc.
 type ProcessEnv struct {
-	LocalPrefix string
-
 	GocmdRunner *gocommand.Runner
 
 	BuildFlags []string
@@ -830,16 +843,19 @@ func (e *ProcessEnv) env() []string {
 	return env
 }
 
-func (e *ProcessEnv) GetResolver() Resolver {
+func (e *ProcessEnv) GetResolver() (Resolver, error) {
 	if e.resolver != nil {
-		return e.resolver
+		return e.resolver, nil
+	}
+	if err := e.init(); err != nil {
+		return nil, err
 	}
 	if len(e.Env["GOMOD"]) == 0 {
 		e.resolver = newGopathResolver(e)
-		return e.resolver
+		return e.resolver, nil
 	}
 	e.resolver = newModuleResolver(e)
-	return e.resolver
+	return e.resolver, nil
 }
 
 func (e *ProcessEnv) buildContext() *build.Context {
@@ -964,10 +980,13 @@ func addExternalCandidates(pass *pass, refs references, filename string) error {
 			return false // We'll do our own loading after we sort.
 		},
 	}
-	err := pass.env.GetResolver().scan(context.Background(), callback)
+	resolver, err := pass.env.GetResolver()
 	if err != nil {
 		return err
 	}
+	if err = resolver.scan(context.Background(), callback); err != nil {
+		return err
+	}
 
 	// Search for imports matching potential package references.
 	type result struct {
@@ -1408,6 +1427,10 @@ func findImport(ctx context.Context, pass *pass, candidates []pkgDistance, pkgNa
 			pass.env.Logf("%s candidate %d/%d: %v in %v", pkgName, i+1, len(candidates), c.pkg.importPathShort, c.pkg.dir)
 		}
 	}
+	resolver, err := pass.env.GetResolver()
+	if err != nil {
+		return nil, err
+	}
 
 	// Collect exports for packages with matching names.
 	rescv := make([]chan *pkg, len(candidates))
@@ -1446,7 +1469,7 @@ func findImport(ctx context.Context, pass *pass, candidates []pkgDistance, pkgNa
 				}
 				// If we're an x_test, load the package under test's test variant.
 				includeTest := strings.HasSuffix(pass.f.Name.Name, "_test") && c.pkg.dir == pass.srcDir
-				_, exports, err := pass.env.GetResolver().loadExports(ctx, c.pkg, includeTest)
+				_, exports, err := resolver.loadExports(ctx, c.pkg, includeTest)
 				if err != nil {
 					if pass.env.Logf != nil {
 						pass.env.Logf("loading exports in dir %s (seeking package %s): %v", c.pkg.dir, pkgName, err)

internal/imports/fix_test.go

@@ -8,6 +8,8 @@ import (
 	"context"
 	"flag"
 	"fmt"
+	"go/build"
+	"io/ioutil"
 	"log"
 	"path/filepath"
 	"reflect"
@@ -1157,13 +1159,6 @@ func TestSimpleCases(t *testing.T) {
 	const localPrefix = "local.com,github.com/local"
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			options := &Options{
-				TabWidth:   8,
-				TabIndent:  true,
-				Comments:   true,
-				Fragment:   true,
-				FormatOnly: tt.formatOnly,
-			}
 			testConfig{
 				modules: []packagestest.Module{
 					{
@@ -1201,7 +1196,14 @@ func TestSimpleCases(t *testing.T) {
 					},
 				},
 			}.test(t, func(t *goimportTest) {
-				t.env.LocalPrefix = localPrefix
+				options := &Options{
+					LocalPrefix: localPrefix,
+					TabWidth:    8,
+					TabIndent:   true,
+					Comments:    true,
+					Fragment:    true,
+					FormatOnly:  tt.formatOnly,
+				}
 				t.assertProcessEquals("golang.org/fake", "x.go", nil, options, tt.out)
 			})
 
@@ -1638,12 +1640,13 @@ func (c testConfig) test(t *testing.T, fn func(*goimportTest)) {
 				},
 				exported: exported,
 			}
-			if err := it.env.init(); err != nil {
-				t.Fatal(err)
-			}
 			if *testDebug {
 				it.env.Logf = log.Printf
 			}
+			// 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
+
 			fn(it)
 		})
 	}
@@ -1673,6 +1676,13 @@ func (t *goimportTest) process(module, file string, contents []byte, opts *Optio
 }
 
 func (t *goimportTest) processNonModule(file string, contents []byte, opts *Options) ([]byte, error) {
+	if contents == nil {
+		var err error
+		contents, err = ioutil.ReadFile(file)
+		if err != nil {
+			return nil, err
+		}
+	}
 	if opts == nil {
 		opts = &Options{Comments: true, TabIndent: true, TabWidth: 8}
 	}
@@ -1869,8 +1879,14 @@ const _ = runtime.GOOS
 					Files: fm{"t.go": tt.src},
 				}}, tt.modules...),
 			}.test(t, func(t *goimportTest) {
-				t.env.LocalPrefix = tt.localPrefix
-				t.assertProcessEquals("test.com", "t.go", nil, nil, tt.want)
+				options := &Options{
+					LocalPrefix: tt.localPrefix,
+					TabWidth:    8,
+					TabIndent:   true,
+					Comments:    true,
+					Fragment:    true,
+				}
+				t.assertProcessEquals("test.com", "t.go", nil, options, tt.want)
 			})
 		})
 	}
@@ -1920,7 +1936,10 @@ func TestImportPathToNameGoPathParse(t *testing.T) {
 		if strings.Contains(t.Name(), "GoPackages") {
 			t.Skip("go/packages does not ignore package main")
 		}
-		r := t.env.GetResolver()
+		r, err := t.env.GetResolver()
+		if err != nil {
+			t.Fatal(err)
+		}
 		srcDir := filepath.Dir(t.exported.File("example.net/pkg", "z.go"))
 		names, err := r.loadPackageNames([]string{"example.net/pkg"}, srcDir)
 		if err != nil {
@@ -2577,7 +2596,7 @@ func TestGetCandidates(t *testing.T) {
 				}
 			}
 		}
-		if err := getAllCandidates(context.Background(), add, "", "x.go", "x", t.env); err != nil {
+		if err := GetAllCandidates(context.Background(), add, "", "x.go", "x", t.env); err != nil {
 			t.Fatalf("GetAllCandidates() = %v", err)
 		}
 		// Sort, then clear out relevance so it doesn't mess up the DeepEqual.
@@ -2628,7 +2647,7 @@ func TestGetPackageCompletions(t *testing.T) {
 				}
 			}
 		}
-		if err := getPackageExports(context.Background(), add, "rand", "x.go", "x", t.env); err != nil {
+		if err := GetPackageExports(context.Background(), add, "rand", "x.go", "x", t.env); err != nil {
 			t.Fatalf("getPackageCompletions() = %v", err)
 		}
 		// Sort, then clear out relevance so it doesn't mess up the DeepEqual.