cmd/go: eliminate redundancy in import error messages

This change introduces a new interface, load.ImportPathError. An error
may satisfy this by providing an ImportPath method and including the
import path in its error text. modload.ImportMissingError satisfies
this interface. load.ImportErrorf also provides a convenient way to
create an error satisfying this interface with an arbitrary message.

When load.PackageError formats its error text, it may omit the last
path on the import stack if the wrapped error satisfies
ImportPathError and has a matching path.

To make this work, PackageError.Err is now an error instead of a
string. PackageError.MarshalJSON will write Err as a string for
'go list -json' output.

When go/build.Import invokes 'go list' in module mode, it now runs
with '-e' and includes '.Error' in the output format instead of
expecting the error to be in the raw stderr text. If a package error
is printed and a directory was not found, the error will be returned
without extra decoration.

Fixes #34752

Change-Id: I2d81dab7dec19e0ae9f51f6412bc9f30433a8596
Reviewed-on: https://go-review.googlesource.com/c/go/+/199840
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

src/cmd/go/internal/fmtcmd/fmt.go

@@ -72,7 +72,7 @@ func runFmt(cmd *base.Command, args []string) {
 			continue
 		}
 		if pkg.Error != nil {
-			if strings.HasPrefix(pkg.Error.Err, "build constraints exclude all Go files") {
+			if strings.HasPrefix(pkg.Error.Err.Error(), "build constraints exclude all Go files") {
 				// Skip this error, as we will format
 				// all files regardless.
 			} else {

src/cmd/go/internal/get/get.go

@@ -274,7 +274,7 @@ func download(arg string, parent *load.Package, stk *load.ImportStack, mode int)
 		stk.Push(arg)
 		err := downloadPackage(p)
 		if err != nil {
-			base.Errorf("%s", &load.PackageError{ImportStack: stk.Copy(), Err: err.Error()})
+			base.Errorf("%s", &load.PackageError{ImportStack: stk.Copy(), Err: err})
 			stk.Pop()
 			return
 		}
@@ -355,7 +355,7 @@ func download(arg string, parent *load.Package, stk *load.ImportStack, mode int)
 				stk.Push(path)
 				err := &load.PackageError{
 					ImportStack: stk.Copy(),
-					Err:         "must be imported as " + path[j+len("vendor/"):],
+					Err:         load.ImportErrorf(path, "%s must be imported as %s", path, path[j+len("vendor/"):]),
 				}
 				stk.Pop()
 				base.Errorf("%s", err)

src/cmd/go/internal/load/test.go

@@ -110,7 +110,7 @@ func TestPackagesAndErrors(p *Package, cover *TestCover) (pmain, ptest, pxtest *
 			// non-test copy of a package.
 			ptestErr = &PackageError{
 				ImportStack:   testImportStack(stk[0], p1, p.ImportPath),
-				Err:           "import cycle not allowed in test",
+				Err:           errors.New("import cycle not allowed in test"),
 				IsImportCycle: true,
 			}
 		}
@@ -271,7 +271,7 @@ func TestPackagesAndErrors(p *Package, cover *TestCover) (pmain, ptest, pxtest *
 	// afterward that gathers t.Cover information.
 	t, err := loadTestFuncs(ptest)
 	if err != nil && pmain.Error == nil {
-		pmain.Error = &PackageError{Err: err.Error()}
+		pmain.Error = &PackageError{Err: err}
 	}
 	t.Cover = cover
 	if len(ptest.GoFiles)+len(ptest.CgoFiles) > 0 {
@@ -322,7 +322,7 @@ func TestPackagesAndErrors(p *Package, cover *TestCover) (pmain, ptest, pxtest *
 
 	data, err := formatTestmain(t)
 	if err != nil && pmain.Error == nil {
-		pmain.Error = &PackageError{Err: err.Error()}
+		pmain.Error = &PackageError{Err: err}
 	}
 	if data != nil {
 		pmain.Internal.TestmainGo = &data

src/cmd/go/internal/modload/import.go

@@ -16,6 +16,7 @@ import (
 	"time"
 
 	"cmd/go/internal/cfg"
+	"cmd/go/internal/load"
 	"cmd/go/internal/modfetch"
 	"cmd/go/internal/module"
 	"cmd/go/internal/par"
@@ -25,32 +26,38 @@ import (
 )
 
 type ImportMissingError struct {
-	ImportPath string
-	Module     module.Version
-	QueryErr   error
+	Path     string
+	Module   module.Version
+	QueryErr error
 
 	// newMissingVersion is set to a newer version of Module if one is present
 	// in the build list. When set, we can't automatically upgrade.
 	newMissingVersion string
 }
 
+var _ load.ImportPathError = (*ImportMissingError)(nil)
+
 func (e *ImportMissingError) Error() string {
 	if e.Module.Path == "" {
-		if str.HasPathPrefix(e.ImportPath, "cmd") {
-			return fmt.Sprintf("package %s is not in GOROOT (%s)", e.ImportPath, filepath.Join(cfg.GOROOT, "src", e.ImportPath))
+		if str.HasPathPrefix(e.Path, "cmd") {
+			return fmt.Sprintf("package %s is not in GOROOT (%s)", e.Path, filepath.Join(cfg.GOROOT, "src", e.Path))
 		}
 		if e.QueryErr != nil {
-			return fmt.Sprintf("cannot find module providing package %s: %v", e.ImportPath, e.QueryErr)
+			return fmt.Sprintf("cannot find module providing package %s: %v", e.Path, e.QueryErr)
 		}
-		return "cannot find module providing package " + e.ImportPath
+		return "cannot find module providing package " + e.Path
 	}
-	return fmt.Sprintf("missing module for import: %s@%s provides %s", e.Module.Path, e.Module.Version, e.ImportPath)
+	return fmt.Sprintf("missing module for import: %s@%s provides %s", e.Module.Path, e.Module.Version, e.Path)
 }
 
 func (e *ImportMissingError) Unwrap() error {
 	return e.QueryErr
 }
 
+func (e *ImportMissingError) ImportPath() string {
+	return e.Path
+}
+
 // An AmbiguousImportError indicates an import of a package found in multiple
 // modules in the build list, or found in both the main module and its vendor
 // directory.
@@ -121,7 +128,7 @@ func Import(path string) (m module.Version, dir string, err error) {
 		return module.Version{}, dir, nil
 	}
 	if str.HasPathPrefix(path, "cmd") {
-		return module.Version{}, "", &ImportMissingError{ImportPath: path}
+		return module.Version{}, "", &ImportMissingError{Path: path}
 	}
 
 	// -mod=vendor is special.
@@ -220,7 +227,7 @@ func Import(path string) (m module.Version, dir string, err error) {
 			}
 			_, ok := dirInModule(path, m.Path, root, isLocal)
 			if ok {
-				return m, "", &ImportMissingError{ImportPath: path, Module: m}
+				return m, "", &ImportMissingError{Path: path, Module: m}
 			}
 		}
 	}
@@ -230,7 +237,7 @@ func Import(path string) (m module.Version, dir string, err error) {
 		if errors.Is(err, os.ErrNotExist) {
 			// Return "cannot find module providing package […]" instead of whatever
 			// low-level error QueryPackage produced.
-			return module.Version{}, "", &ImportMissingError{ImportPath: path, QueryErr: err}
+			return module.Version{}, "", &ImportMissingError{Path: path, QueryErr: err}
 		} else {
 			return module.Version{}, "", err
 		}
@@ -255,7 +262,7 @@ func Import(path string) (m module.Version, dir string, err error) {
 			}
 		}
 	}
-	return m, "", &ImportMissingError{ImportPath: path, Module: m, newMissingVersion: newMissingVersion}
+	return m, "", &ImportMissingError{Path: path, Module: m, newMissingVersion: newMissingVersion}
 }
 
 // maybeInModule reports whether, syntactically,

src/cmd/go/internal/work/exec.go

@@ -433,7 +433,7 @@ func (b *Builder) build(a *Action) (err error) {
 			err = fmt.Errorf("go build %s: %v", a.Package.ImportPath, err)
 		}
 		if err != nil && b.IsCmdList && b.NeedError && p.Error == nil {
-			p.Error = &load.PackageError{Err: err.Error()}
+			p.Error = &load.PackageError{Err: err}
 		}
 	}()
 	if cfg.BuildN {

src/cmd/go/testdata/script/cmd_import_error.txt

@@ -5,7 +5,7 @@ env GO111MODULE=on
 # a clear error in module mode.
 
 ! go list cmd/unknown
-stderr '^can''t load package: package cmd/unknown: package cmd/unknown is not in GOROOT \('$GOROOT'[/\\]src[/\\]cmd[/\\]unknown\)$'
+stderr '^can''t load package: package cmd/unknown is not in GOROOT \('$GOROOT'[/\\]src[/\\]cmd[/\\]unknown\)$'
 
 go list -f '{{range .DepsErrors}}{{.Err}}{{end}}' x.go
 stdout '^package cmd/unknown is not in GOROOT \('$GOROOT'[/\\]src[/\\]cmd[/\\]unknown\)$'

src/go/build/build.go

@@ -1046,7 +1046,7 @@ func (ctxt *Context) importGo(p *Package, path, srcDir string, mode ImportMode,
 		parent = d
 	}
 
-	cmd := exec.Command("go", "list", "-compiler="+ctxt.Compiler, "-tags="+strings.Join(ctxt.BuildTags, ","), "-installsuffix="+ctxt.InstallSuffix, "-f={{.Dir}}\n{{.ImportPath}}\n{{.Root}}\n{{.Goroot}}\n", path)
+	cmd := exec.Command("go", "list", "-e", "-compiler="+ctxt.Compiler, "-tags="+strings.Join(ctxt.BuildTags, ","), "-installsuffix="+ctxt.InstallSuffix, "-f={{.Dir}}\n{{.ImportPath}}\n{{.Root}}\n{{.Goroot}}\n{{if .Error}}{{.Error}}{{end}}\n", "--", path)
 
 	// TODO(bcmills): This is wrong if srcDir is in a vendor directory, or if
 	// srcDir is in some module dependency of the main module. The main module
@@ -1073,12 +1073,22 @@ func (ctxt *Context) importGo(p *Package, path, srcDir string, mode ImportMode,
 		return fmt.Errorf("go/build: importGo %s: %v\n%s\n", path, err, stderr.String())
 	}
 
-	f := strings.Split(stdout.String(), "\n")
-	if len(f) != 5 || f[4] != "" {
+	f := strings.SplitN(stdout.String(), "\n", 5)
+	if len(f) != 5 {
 		return fmt.Errorf("go/build: importGo %s: unexpected output:\n%s\n", path, stdout.String())
 	}
+	dir := f[0]
+	errStr := strings.TrimSpace(f[4])
+	if errStr != "" && p.Dir == "" {
+		// If 'go list' could not locate the package, return the same error that
+		// 'go list' reported.
+		// If 'go list' did locate the package (p.Dir is not empty), ignore the
+		// error. It was probably related to loading source files, and we'll
+		// encounter it ourselves shortly.
+		return errors.New(errStr)
+	}
 
-	p.Dir = f[0]
+	p.Dir = dir
 	p.ImportPath = f[1]
 	p.Root = f[2]
 	p.Goroot = f[3] == "true"

src/go/build/build_test.go

@@ -7,6 +7,7 @@ package build
 import (
 	"internal/testenv"
 	"io"
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -447,3 +448,29 @@ func TestIssue23594(t *testing.T) {
 		t.Fatalf("incorrectly set .Doc to %q", p.Doc)
 	}
 }
+
+// TestMissingImportErrorRepetition checks that when an unknown package is
+// imported, the package path is only shown once in the error.
+// Verifies golang.org/issue/34752.
+func TestMissingImportErrorRepetition(t *testing.T) {
+	testenv.MustHaveGoBuild(t) // need 'go list' internally
+	tmp, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmp)
+	if err := ioutil.WriteFile(filepath.Join(tmp, "go.mod"), []byte("module m"), 0666); err != nil {
+		t.Fatal(err)
+	}
+	defer os.Setenv("GO111MODULE", os.Getenv("GO111MODULE"))
+	os.Setenv("GO111MODULE", "on")
+	defer os.Setenv("GOPROXY", os.Getenv("GOPROXY"))
+	os.Setenv("GOPROXY", "off")
+
+	pkgPath := "example.com/hello"
+	if _, err = Import(pkgPath, tmp, FindOnly); err == nil {
+		t.Fatal("unexpected success")
+	} else if n := strings.Count(err.Error(), pkgPath); n != 1 {
+		t.Fatalf("package path %q appears in error %d times; should appear once\nerror: %v", pkgPath, n, err)
+	}
+}