cmd/go: rationalize errors in internal/load and internal/modload

This change is a non-minimal fix for #32917, but incidentally fixes
several other bugs and makes the error messages much more ergonomic.

Updates #32917
Updates #27122
Updates #28459
Updates #29280
Updates #30590
Updates #37214
Updates #36173
Updates #36587
Fixes #36008
Fixes #30992

Change-Id: Iedb26d2e0963697c130df5d0f72e7f83ec2dcf06
Reviewed-on: https://go-review.googlesource.com/c/go/+/185345
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

Author: Bryan C. Mills

src/cmd/go/internal/clean/clean.go

@@ -232,7 +232,7 @@ func clean(p *load.Package) {
 	cleaned[p] = true
 
 	if p.Dir == "" {
-		base.Errorf("can't load package: %v", p.Error)
+		base.Errorf("%v", p.Error)
 		return
 	}
 	dirs, err := ioutil.ReadDir(p.Dir)

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

@@ -6,11 +6,11 @@
 package fmtcmd
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 	"runtime"
-	"strings"
 	"sync"
 
 	"cmd/go/internal/base"
@@ -72,11 +72,12 @@ func runFmt(cmd *base.Command, args []string) {
 			continue
 		}
 		if pkg.Error != nil {
-			if strings.HasPrefix(pkg.Error.Err.Error(), "build constraints exclude all Go files") {
+			var nogo *load.NoGoError
+			if errors.As(pkg.Error, &nogo) && len(pkg.InternalAllGoFiles()) > 0 {
 				// Skip this error, as we will format
 				// all files regardless.
 			} else {
-				base.Errorf("can't load package: %s", pkg.Error)
+				base.Errorf("%v", pkg.Error)
 				continue
 			}
 		}

src/cmd/go/internal/list/list.go

@@ -451,6 +451,7 @@ func runList(cmd *base.Command, args []string) {
 		pkgs = load.PackagesAndErrors(args)
 	} else {
 		pkgs = load.Packages(args)
+		base.ExitIfErrors()
 	}
 
 	if cache.Default() == nil {

src/cmd/go/internal/load/pkg.go

@@ -187,20 +187,17 @@ type PackageInternal struct {
 	Gccgoflags []string // -gccgoflags for this package
 }
 
+// A NoGoError indicates that no Go files for the package were applicable to the
+// build for that package.
+//
+// That may be because there were no files whatsoever, or because all files were
+// excluded, or because all non-excluded files were test sources.
 type NoGoError struct {
 	Package *Package
 }
 
 func (e *NoGoError) Error() string {
-	// Count files beginning with _ and ., which we will pretend don't exist at all.
-	dummy := 0
-	for _, name := range e.Package.IgnoredGoFiles {
-		if strings.HasPrefix(name, "_") || strings.HasPrefix(name, ".") {
-			dummy++
-		}
-	}
-
-	if len(e.Package.IgnoredGoFiles) > dummy {
+	if len(e.Package.constraintIgnoredGoFiles()) > 0 {
 		// Go files exist, but they were ignored due to build constraints.
 		return "build constraints exclude all Go files in " + e.Package.Dir
 	}
@@ -213,6 +210,23 @@ func (e *NoGoError) Error() string {
 	return "no Go files in " + e.Package.Dir
 }
 
+// rewordError returns a version of err with trivial layers removed and
+// (possibly-wrapped) instances of build.NoGoError replaced with load.NoGoError,
+// which more clearly distinguishes sub-cases.
+func (p *Package) rewordError(err error) error {
+	if mErr, ok := err.(*search.MatchError); ok && mErr.Match.IsLiteral() {
+		err = mErr.Err
+	}
+	var noGo *build.NoGoError
+	if errors.As(err, &noGo) {
+		if p.Dir == "" && noGo.Dir != "" {
+			p.Dir = noGo.Dir
+		}
+		err = &NoGoError{Package: p}
+	}
+	return err
+}
+
 // Resolve returns the resolved version of imports,
 // which should be p.TestImports or p.XTestImports, NOT p.Imports.
 // The imports in p.TestImports and p.XTestImports are not recursively
@@ -313,10 +327,7 @@ type PackageError struct {
 
 func (p *PackageError) Error() string {
 	// Import cycles deserve special treatment.
-	if p.IsImportCycle {
-		return fmt.Sprintf("%s\npackage %s\n", p.Err, strings.Join(p.ImportStack, "\n\timports "))
-	}
-	if p.Pos != "" {
+	if p.Pos != "" && !p.IsImportCycle {
 		// Omit import stack. The full path to the file where the error
 		// is the most important thing.
 		return p.Pos + ": " + p.Err.Error()
@@ -339,6 +350,8 @@ func (p *PackageError) Error() string {
 	return "package " + strings.Join(stack, "\n\timports ") + ": " + p.Err.Error()
 }
 
+func (p *PackageError) Unwrap() error { return p.Err }
+
 // PackageError implements MarshalJSON so that Err is marshaled as a string
 // and non-essential fields are omitted.
 func (p *PackageError) MarshalJSON() ([]byte, error) {
@@ -583,9 +596,10 @@ func loadImport(pre *preload, path, srcDir string, parent *Package, stk *ImportS
 		if !cfg.ModulesEnabled && path != cleanImport(path) {
 			p.Error = &PackageError{
 				ImportStack: stk.Copy(),
-				Err:         fmt.Errorf("non-canonical import path: %q should be %q", path, pathpkg.Clean(path)),
+				Err:         ImportErrorf(path, "non-canonical import path %q: should be %q", path, pathpkg.Clean(path)),
 			}
 			p.Incomplete = true
+			setErrorPos(p, importPos)
 		}
 	}
 
@@ -1533,12 +1547,8 @@ func (p *Package) load(stk *ImportStack, bp *build.Package, err error) {
 	}
 
 	if err != nil {
-		if _, ok := err.(*build.NoGoError); ok {
-			err = &NoGoError{Package: p}
-		}
 		p.Incomplete = true
-
-		setError(base.ExpandScanner(err))
+		setError(base.ExpandScanner(p.rewordError(err)))
 		if _, isScanErr := err.(scanner.ErrorList); !isScanErr {
 			return
 		}
@@ -1934,13 +1944,22 @@ func (p *Package) InternalXGoFiles() []string {
 // using absolute paths. "Possibly relevant" means that files are not excluded
 // due to build tags, but files with names beginning with . or _ are still excluded.
 func (p *Package) InternalAllGoFiles() []string {
-	var extra []string
+	return p.mkAbs(str.StringList(p.constraintIgnoredGoFiles(), p.GoFiles, p.CgoFiles, p.TestGoFiles, p.XTestGoFiles))
+}
+
+// constraintIgnoredGoFiles returns the list of Go files ignored for reasons
+// other than having a name beginning with '.' or '_'.
+func (p *Package) constraintIgnoredGoFiles() []string {
+	if len(p.IgnoredGoFiles) == 0 {
+		return nil
+	}
+	files := make([]string, 0, len(p.IgnoredGoFiles))
 	for _, f := range p.IgnoredGoFiles {
-		if f != "" && f[0] != '.' || f[0] != '_' {
-			extra = append(extra, f)
+		if f != "" && f[0] != '.' && f[0] != '_' {
+			files = append(files, f)
 		}
 	}
-	return p.mkAbs(str.StringList(extra, p.GoFiles, p.CgoFiles, p.TestGoFiles, p.XTestGoFiles))
+	return files
 }
 
 // usesSwig reports whether the package needs to run SWIG.
@@ -2034,7 +2053,7 @@ func Packages(args []string) []*Package {
 	var pkgs []*Package
 	for _, pkg := range PackagesAndErrors(args) {
 		if pkg.Error != nil {
-			base.Errorf("can't load package: %s", pkg.Error)
+			base.Errorf("%v", pkg.Error)
 			continue
 		}
 		pkgs = append(pkgs, pkg)
@@ -2092,8 +2111,24 @@ func PackagesAndErrors(patterns []string) []*Package {
 			pkgs = append(pkgs, p)
 		}
 
-		// TODO: if len(m.Pkgs) == 0 && len(m.Errs) > 0, should we add a *Package
-		// with a non-nil Error field?
+		if len(m.Errs) > 0 {
+			// In addition to any packages that were actually resolved from the
+			// pattern, there was some error in resolving the pattern itself.
+			// Report it as a synthetic package.
+			p := new(Package)
+			p.ImportPath = m.Pattern()
+			p.Error = &PackageError{
+				ImportStack: nil, // The error arose from a pattern, not an import.
+				Err:         p.rewordError(m.Errs[0]),
+			}
+			p.Incomplete = true
+			p.Match = append(p.Match, m.Pattern())
+			p.Internal.CmdlinePkg = true
+			if m.IsLiteral() {
+				p.Internal.CmdlinePkgLiteral = true
+			}
+			pkgs = append(pkgs, p)
+		}
 	}
 
 	// Now that CmdlinePkg is set correctly,
@@ -2129,7 +2164,7 @@ func PackagesForBuild(args []string) []*Package {
 	printed := map[*PackageError]bool{}
 	for _, pkg := range pkgs {
 		if pkg.Error != nil {
-			base.Errorf("can't load package: %s", pkg.Error)
+			base.Errorf("%v", pkg.Error)
 			printed[pkg.Error] = true
 		}
 		for _, err := range pkg.DepsErrors {
@@ -2139,7 +2174,7 @@ func PackagesForBuild(args []string) []*Package {
 			// Only print each once.
 			if !printed[err] {
 				printed[err] = true
-				base.Errorf("%s", err)
+				base.Errorf("%v", err)
 			}
 		}
 	}

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

@@ -132,8 +132,6 @@ func Import(path string) (m module.Version, dir string, err error) {
 		}
 		dir := filepath.Join(cfg.GOROOT, "src", path)
 		return module.Version{}, dir, nil
-	} else if pathIsStd && path == cfg.GOROOTsrc {
-		return module.Version{}, dir, errors.New("directory should not directly contain source files")
 	}
 
 	// -mod=vendor is special.