cmd/go/internal/modload: clean up error reporting

• Consolidate 'if ld.AllowErrors' conditions into an 'ld.errorf'
  method.

• Rename SilenceErrors to SilencePackageErrors and clarify its
  documentation. (There is currently no way to silence errors in the
  module graph. Perhaps we should add one, but for now let's at least
  clarify the existing behavior.)

• Move 'tidy -v' verbose logging into LoadPackages, where other
  logging happens.

• Make checkMultiplePaths a loader method (since it only matters
  during package loading anyway).

• Check package and module-graph errors in loadFromRoots instead of
  LoadPackages. These checks were previously omitted on the
  ImportFromFiles path, which seems likely to be a bug. (We now
  suppress package errors explicitly in ImportFromFiles, which at
  least makes the bug more explicit.)

This somewhat simplifies the code structure in preparation for
the lazy-mode tidy implementation.

For #36460

Change-Id: I3ce3586c6934989d5194f00f99e7cc4423cf767f
Reviewed-on: https://go-review.googlesource.com/c/go/+/313229
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>

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

@@ -2433,7 +2433,7 @@ func PackagesAndErrors(ctx context.Context, opts PackageOpts, patterns []string)
 		modOpts := modload.PackageOpts{
 			ResolveMissingImports: true,
 			LoadTests:             opts.ModResolveTests,
-			SilenceErrors:         true,
+			SilencePackageErrors:  true,
 		}
 		matches, _ = modload.LoadPackages(ctx, modOpts, patterns...)
 	} else {

src/cmd/go/internal/modcmd/why.go

@@ -71,7 +71,7 @@ func runWhy(ctx context.Context, cmd *base.Command, args []string) {
 		Tags:                     imports.AnyTags(),
 		VendorModulesInGOROOTSrc: true,
 		LoadTests:                !*whyVendor,
-		SilenceErrors:            true,
+		SilencePackageErrors:     true,
 		UseVendorAll:             *whyVendor,
 	}
 

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

@@ -1152,7 +1152,7 @@ func (r *resolver) loadPackages(ctx context.Context, patterns []string, findPack
 		Tags:                     imports.AnyTags(),
 		VendorModulesInGOROOTSrc: true,
 		LoadTests:                *getT,
-		SilenceErrors:            true, // May be fixed by subsequent upgrades or downgrades.
+		SilencePackageErrors:     true, // May be fixed by subsequent upgrades or downgrades.
 	}
 
 	opts.AllowPackage = func(ctx context.Context, path string, m module.Version) error {

src/cmd/go/internal/modload/buildlist.go

@@ -11,7 +11,7 @@ import (
 	"cmd/go/internal/par"
 	"context"
 	"fmt"
-	"os"
+	"reflect"
 	"runtime"
 	"strings"
 	"sync"
@@ -479,9 +479,9 @@ type Conflict struct {
 	Constraint module.Version
 }
 
-// tidyBuildList trims the build list to the minimal requirements needed to
+// tidyRoots trims the root requirements to the minimal roots needed to
 // retain the same versions of all packages loaded by ld.
-func tidyBuildList(ctx context.Context, ld *loader, initialRS *Requirements) *Requirements {
+func tidyRoots(ctx context.Context, rs *Requirements, pkgs []*loadPkg) (*Requirements, error) {
 	if go117LazyTODO {
 		// Tidy needs to maintain the lazy-loading invariants for lazy modules.
 		// The implementation for eager modules should be factored out into a function.
@@ -493,33 +493,10 @@ func tidyBuildList(ctx context.Context, ld *loader, initialRS *Requirements) *Re
 		// changed after loading packages.
 	}
 
-	var (
+	if depth == eager {
-		tidy *Requirements
+		return tidyEagerRoots(ctx, rs, pkgs)
-		err  error
-	)
-	if depth == lazy {
-		panic("internal error: 'go mod tidy' for lazy modules is not yet implemented")
-	} else {
-		tidy, err = tidyEagerRoots(ctx, ld.requirements, ld.pkgs)
 	}
-	if err != nil {
+	panic("internal error: 'go mod tidy' for lazy modules is not yet implemented")
-		base.Fatalf("go: %v", err)
-	}
-
-	if cfg.BuildV {
-		mg, _ := tidy.Graph(ctx)
-
-		for _, m := range initialRS.rootModules {
-			if mg.Selected(m.Path) == "none" {
-				fmt.Fprintf(os.Stderr, "unused %s\n", m.Path)
-			} else if go117LazyTODO {
-				// If the main module is lazy and we demote a root to a non-root
-				// (because it is not actually relevant), should we log that too?
-			}
-		}
-	}
-
-	return tidy
 }
 
 // tidyEagerRoots returns a minimal set of root requirements that maintains the
@@ -550,7 +527,11 @@ func tidyEagerRoots(ctx context.Context, rs *Requirements, pkgs []*loadPkg) (*Re
 
 	min, err := mvs.Req(Target, rootPaths, &mvsReqs{roots: keep})
 	if err != nil {
-		return nil, err
+		return rs, err
+	}
+	if reflect.DeepEqual(min, rs.rootModules) {
+		// rs is already tidy, so preserve its cached graph.
+		return rs, nil
 	}
 	return newRequirements(eager, min, rs.direct), nil
 }
@@ -660,27 +641,3 @@ func updateRoots(ctx context.Context, direct map[string]bool, rs *Requirements,
 
 	return newRequirements(rs.depth, min, direct), nil
 }
-
-// checkMultiplePaths verifies that a given module path is used as itself
-// or as a replacement for another module, but not both at the same time.
-//
-// (See https://golang.org/issue/26607 and https://golang.org/issue/34650.)
-func checkMultiplePaths(rs *Requirements) {
-	mods := rs.rootModules
-	if cached := rs.graph.Load(); cached != nil {
-		if mg := cached.(cachedGraph).mg; mg != nil {
-			mods = mg.BuildList()
-		}
-	}
-
-	firstPath := map[module.Version]string{}
-	for _, mod := range mods {
-		src := resolveReplacement(mod)
-		if prev, ok := firstPath[src]; !ok {
-			firstPath[src] = mod.Path
-		} else if prev != mod.Path {
-			base.Errorf("go: %s@%s used for two different module paths (%s and %s)", src.Path, src.Version, prev, mod.Path)
-		}
-	}
-	base.ExitIfErrors()
-}

src/cmd/go/internal/modload/load.go

@@ -180,9 +180,16 @@ type PackageOpts struct {
 	// an error occurs.
 	AllowErrors bool
 
-	// SilenceErrors indicates that LoadPackages should not print errors
+	// SilencePackageErrors indicates that LoadPackages should not print errors
-	// that occur while loading packages. SilenceErrors implies AllowErrors.
+	// that occur while matching or loading packages, and should not terminate the
-	SilenceErrors bool
+	// process if such an error occurs.
+	//
+	// Errors encountered in the module graph will still be reported.
+	//
+	// The caller may retrieve the silenced package errors using the Lookup
+	// function, and matching errors are still populated in the Errs field of the
+	// associated search.Match.)
+	SilencePackageErrors bool
 
 	// SilenceMissingStdImports indicates that LoadPackages should not print
 	// errors or terminate the process if an imported package is missing, and the
@@ -317,50 +324,12 @@ func LoadPackages(ctx context.Context, opts PackageOpts, patterns ...string) (ma
 	// One last pass to finalize wildcards.
 	updateMatches(ld.requirements, ld)
 
-	// Report errors, if any.
+	// List errors in matching patterns (such as directory permission
-	checkMultiplePaths(ld.requirements)
+	// errors for wildcard patterns).
-	for _, pkg := range ld.pkgs {
+	if !ld.SilencePackageErrors {
-		if !pkg.isTest() {
-			loadedPackages = append(loadedPackages, pkg.path)
-		}
-
-		if pkg.err != nil {
-			if sumErr := (*ImportMissingSumError)(nil); errors.As(pkg.err, &sumErr) {
-				if importer := pkg.stack; importer != nil {
-					sumErr.importer = importer.path
-					sumErr.importerVersion = importer.mod.Version
-					sumErr.importerIsTest = importer.testOf != nil
-				}
-			}
-
-			if opts.SilenceErrors {
-				continue
-			}
-			if stdErr := (*ImportMissingError)(nil); errors.As(pkg.err, &stdErr) &&
-				stdErr.isStd && opts.SilenceMissingStdImports {
-				continue
-			}
-			if opts.SilenceNoGoErrors && errors.Is(pkg.err, imports.ErrNoGo) {
-				continue
-			}
-
-			if opts.AllowErrors {
-				fmt.Fprintf(os.Stderr, "%s: %v\n", pkg.stackText(), pkg.err)
-			} else {
-				base.Errorf("%s: %v", pkg.stackText(), pkg.err)
-			}
-		}
-	}
-	if !opts.SilenceErrors {
-		// Also list errors in matching patterns (such as directory permission
-		// errors for wildcard patterns).
 		for _, match := range matches {
 			for _, err := range match.Errs {
-				if opts.AllowErrors {
+				ld.errorf("%v\n", err)
-					fmt.Fprintf(os.Stderr, "%v\n", err)
-				} else {
-					base.Errorf("%v", err)
-				}
 			}
 		}
 	}
@@ -370,14 +339,42 @@ func LoadPackages(ctx context.Context, opts PackageOpts, patterns ...string) (ma
 		search.WarnUnmatched(matches)
 	}
 
-	if ld.Tidy {
+	if opts.Tidy {
-		ld.requirements = tidyBuildList(ctx, ld, initialRS)
+		if cfg.BuildV {
+			mg, _ := ld.requirements.Graph(ctx)
+
+			for _, m := range initialRS.rootModules {
+				var unused bool
+				if ld.requirements.depth == eager {
+					// m is unused if it was dropped from the module graph entirely. If it
+					// was only demoted from direct to indirect, it may still be in use via
+					// a transitive import.
+					unused = mg.Selected(m.Path) == "none"
+				} else {
+					// m is unused if it was dropped from the roots. If it is still present
+					// as a transitive dependency, that transitive dependency is not needed
+					// by any package or test in the main module.
+					_, ok := ld.requirements.rootSelected(m.Path)
+					unused = !ok
+				}
+				if unused {
+					fmt.Fprintf(os.Stderr, "unused %s\n", m.Path)
+				}
+			}
+		}
+
 		modfetch.TrimGoSum(keepSums(ctx, ld, ld.requirements, loadedZipSumsOnly))
 	}
 
-	// Success! Update go.mod (if needed) and return the results.
+	// Success! Update go.mod and go.sum (if needed) and return the results.
 	loaded = ld
 	commitRequirements(ctx, loaded.requirements)
+
+	for _, pkg := range ld.pkgs {
+		if !pkg.isTest() {
+			loadedPackages = append(loadedPackages, pkg.path)
+		}
+	}
 	sort.Strings(loadedPackages)
 	return matches, loadedPackages
 }
@@ -582,6 +579,11 @@ func pathInModuleCache(ctx context.Context, dir string, rs *Requirements) string
 
 // ImportFromFiles adds modules to the build list as needed
 // to satisfy the imports in the named Go source files.
+//
+// Errors in missing dependencies are silenced.
+//
+// TODO(bcmills): Silencing errors seems off. Take a closer look at this and
+// figure out what the error-reporting actually ought to be.
 func ImportFromFiles(ctx context.Context, gofiles []string) {
 	rs := LoadModFile(ctx)
 
@@ -595,6 +597,7 @@ func ImportFromFiles(ctx context.Context, gofiles []string) {
 		PackageOpts: PackageOpts{
 			Tags:                  tags,
 			ResolveMissingImports: true,
+			SilencePackageErrors:  true,
 		},
 		requirements:       rs,
 		allClosesOverTests: index.allPatternClosesOverTests(),
@@ -772,6 +775,16 @@ func (ld *loader) reset() {
 	ld.pkgs = nil
 }
 
+// errorf reports an error via either os.Stderr or base.Errorf,
+// according to whether ld.AllowErrors is set.
+func (ld *loader) errorf(format string, args ...interface{}) {
+	if ld.AllowErrors {
+		fmt.Fprintf(os.Stderr, format, args...)
+	} else {
+		base.Errorf(format, args...)
+	}
+}
+
 // A loadPkg records information about a single loaded package.
 type loadPkg struct {
 	// Populated at construction time:
@@ -890,6 +903,14 @@ func loadFromRoots(ctx context.Context, params loaderParams) *loader {
 		work:         par.NewQueue(runtime.GOMAXPROCS(0)),
 	}
 
+	if ld.requirements.depth == eager {
+		var err error
+		ld.requirements, _, err = expandGraph(ctx, ld.requirements)
+		if err != nil {
+			ld.errorf("go: %v\n", err)
+		}
+	}
+
 	for {
 		ld.reset()
 
@@ -971,6 +992,46 @@ func loadFromRoots(ctx context.Context, params loaderParams) *loader {
 		// changing the build list. Iterate until the roots are stable.
 	}
 
+	// Tidy the build list, if applicable, before we report errors.
+	// (The process of tidying may remove errors from irrelevant dependencies.)
+	if ld.Tidy {
+		var err error
+		ld.requirements, err = tidyRoots(ctx, ld.requirements, ld.pkgs)
+		if err != nil {
+			ld.errorf("go: %v\n", err)
+		}
+	}
+
+	// Report errors, if any.
+	for _, pkg := range ld.pkgs {
+		if pkg.err == nil {
+			continue
+		}
+
+		// Add importer information to checksum errors.
+		if sumErr := (*ImportMissingSumError)(nil); errors.As(pkg.err, &sumErr) {
+			if importer := pkg.stack; importer != nil {
+				sumErr.importer = importer.path
+				sumErr.importerVersion = importer.mod.Version
+				sumErr.importerIsTest = importer.testOf != nil
+			}
+		}
+
+		if ld.SilencePackageErrors {
+			continue
+		}
+		if stdErr := (*ImportMissingError)(nil); errors.As(pkg.err, &stdErr) &&
+			stdErr.isStd && ld.SilenceMissingStdImports {
+			continue
+		}
+		if ld.SilenceNoGoErrors && errors.Is(pkg.err, imports.ErrNoGo) {
+			continue
+		}
+
+		ld.errorf("%s: %v\n", pkg.stackText(), pkg.err)
+	}
+
+	ld.checkMultiplePaths()
 	return ld
 }
 
@@ -1044,10 +1105,18 @@ func (ld *loader) updateRequirements(ctx context.Context, add []module.Version)
 	}
 
 	rs, err := updateRoots(ctx, direct, rs, add)
-	if err == nil {
+	if err != nil {
+		// We don't actually know what even the root requirements are supposed to be,
+		// so we can't proceed with loading. Return the error to the caller
+		return err
+	}
+	if rs != ld.requirements {
+		if _, err := rs.Graph(ctx); err != nil {
+			ld.errorf("go: %v\n", err)
+		}
 		ld.requirements = rs
 	}
-	return err
+	return nil
 }
 
 // resolveMissingImports returns a set of modules that could be added as
@@ -1387,6 +1456,29 @@ func (ld *loader) computePatternAll() (all []string) {
 	return all
 }
 
+// checkMultiplePaths verifies that a given module path is used as itself
+// or as a replacement for another module, but not both at the same time.
+//
+// (See https://golang.org/issue/26607 and https://golang.org/issue/34650.)
+func (ld *loader) checkMultiplePaths() {
+	mods := ld.requirements.rootModules
+	if cached := ld.requirements.graph.Load(); cached != nil {
+		if mg := cached.(cachedGraph).mg; mg != nil {
+			mods = mg.BuildList()
+		}
+	}
+
+	firstPath := map[module.Version]string{}
+	for _, mod := range mods {
+		src := resolveReplacement(mod)
+		if prev, ok := firstPath[src]; !ok {
+			firstPath[src] = mod.Path
+		} else if prev != mod.Path {
+			ld.errorf("go: %s@%s used for two different module paths (%s and %s)", src.Path, src.Version, prev, mod.Path)
+		}
+	}
+}
+
 // scanDir is like imports.ScanDir but elides known magic imports from the list,
 // so that we do not go looking for packages that don't really exist.
 //