cmd/go: print require chains in build list errors

mvs.BuildList and functions that invoke it directly (UpgradeAll) now
return an *mvs.BuildListError when there is an error retrieving the
requirements for a module. This new error prints the chain of
requirements from the main module to the module where the error
occurred.

These errors come up most commonly when a go.mod file has an
unexpected module path or can't be parsed for some other reason. It's
currently difficult to debug these errors because it's not clear where
the "bad" module is required from. Tools like "go list -m" and
"go mod why" don't work without the build graph.

Fixes #30661

Change-Id: I3c9d4683dcd9a5d7c259e5e4cc7e1ee209700b10
Reviewed-on: https://go-review.googlesource.com/c/go/+/166984
Run-TryBot: Jay Conrod <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Bryan C. Mills <[email protected]>

Author: Jay Conrod

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

@@ -1023,13 +1023,11 @@ func (r *mvsReqs) required(mod module.Version) ([]module.Version, error) {
 			gomod := filepath.Join(dir, "go.mod")
 			data, err := ioutil.ReadFile(gomod)
 			if err != nil {
-				base.Errorf("go: parsing %s: %v", base.ShortPath(gomod), err)
-				return nil, ErrRequire
+				return nil, fmt.Errorf("parsing %s: %v", base.ShortPath(gomod), err)
 			}
 			f, err := modfile.ParseLax(gomod, data, nil)
 			if err != nil {
-				base.Errorf("go: parsing %s: %v", base.ShortPath(gomod), err)
-				return nil, ErrRequire
+				return nil, fmt.Errorf("parsing %s: %v", base.ShortPath(gomod), err)
 			}
 			if f.Go != nil {
 				r.versions.LoadOrStore(mod, f.Go.Version)
@@ -1050,22 +1048,18 @@ func (r *mvsReqs) required(mod module.Version) ([]module.Version, error) {
 
 	data, err := modfetch.GoMod(mod.Path, mod.Version)
 	if err != nil {
-		base.Errorf("go: %s@%s: %v\n", mod.Path, mod.Version, err)
-		return nil, ErrRequire
+		return nil, fmt.Errorf("%s@%s: %v", mod.Path, mod.Version, err)
 	}
 	f, err := modfile.ParseLax("go.mod", data, nil)
 	if err != nil {
-		base.Errorf("go: %s@%s: parsing go.mod: %v", mod.Path, mod.Version, err)
-		return nil, ErrRequire
+		return nil, fmt.Errorf("%s@%s: parsing go.mod: %v", mod.Path, mod.Version, err)
 	}
 
 	if f.Module == nil {
-		base.Errorf("go: %s@%s: parsing go.mod: missing module line", mod.Path, mod.Version)
-		return nil, ErrRequire
+		return nil, fmt.Errorf("%s@%s: parsing go.mod: missing module line", mod.Path, mod.Version)
 	}
 	if mpath := f.Module.Mod.Path; mpath != origPath && mpath != mod.Path {
-		base.Errorf("go: %s@%s: parsing go.mod: unexpected module path %q", mod.Path, mod.Version, mpath)
-		return nil, ErrRequire
+		return nil, fmt.Errorf("%s@%s: parsing go.mod: unexpected module path %q", mod.Path, mod.Version, mpath)
 	}
 	if f.Go != nil {
 		r.versions.LoadOrStore(mod, f.Go.Version)
@@ -1074,11 +1068,6 @@ func (r *mvsReqs) required(mod module.Version) ([]module.Version, error) {
 	return r.modFileToList(f), nil
 }
 
-// ErrRequire is the sentinel error returned when Require encounters problems.
-// It prints the problems directly to standard error, so that multiple errors
-// can be displayed easily.
-var ErrRequire = errors.New("error loading module requirements")
-
 func (*mvsReqs) Max(v1, v2 string) string {
 	if v1 != "" && semver.Compare(v1, v2) == -1 {
 		return v2

src/cmd/go/internal/mvs/mvs.go

@@ -9,7 +9,9 @@ package mvs
 import (
 	"fmt"
 	"sort"
+	"strings"
 	"sync"
+	"sync/atomic"
 
 	"cmd/go/internal/base"
 	"cmd/go/internal/module"
@@ -59,12 +61,38 @@ type Reqs interface {
 	Previous(m module.Version) (module.Version, error)
 }
 
-type MissingModuleError struct {
-	Module module.Version
+// BuildListError decorates an error that occurred gathering requirements
+// while constructing a build list. BuildListError prints the chain
+// of requirements to the module where the error occurred.
+type BuildListError struct {
+	Err   error
+	Stack []module.Version
 }
 
-func (e *MissingModuleError) Error() string {
-	return fmt.Sprintf("missing module: %v", e.Module)
+func (e *BuildListError) Error() string {
+	b := &strings.Builder{}
+	errMsg := e.Err.Error()
+	stack := e.Stack
+
+	// Don't print modules at the beginning of the chain without a
+	// version. These always seem to be the main module or a
+	// synthetic module ("target@").
+	for len(stack) > 0 && stack[len(stack)-1].Version == "" {
+		stack = stack[:len(stack)-1]
+	}
+
+	// Don't print the last module if the error message already
+	// starts with module path and version.
+	if len(stack) > 0 && strings.HasPrefix(errMsg, fmt.Sprintf("%s@%s: ", stack[0].Path, stack[0].Version)) {
+		// error already mentions module
+		stack = stack[1:]
+	}
+
+	for i := len(stack) - 1; i >= 0; i-- {
+		fmt.Fprintf(b, "%s@%s ->\n\t", stack[i].Path, stack[i].Version)
+	}
+	b.WriteString(errMsg)
+	return b.String()
 }
 
 // BuildList returns the build list for the target module.
@@ -78,33 +106,40 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) mo
 	// does high-latency network operations.
 	var work par.Work
 	work.Add(target)
+
+	type modGraphNode struct {
+		m        module.Version
+		required []module.Version
+		upgrade  module.Version
+		err      error
+	}
 	var (
 		mu       sync.Mutex
-		min      = map[string]string{target.Path: target.Version}
-		firstErr error
+		modGraph = map[module.Version]*modGraphNode{}
+		min      = map[string]string{} // maps module path to minimum required version
+		haveErr  int32
 	)
+
+	work.Add(target)
 	work.Do(10, func(item interface{}) {
 		m := item.(module.Version)
-		required, err := reqs.Required(m)
 
+		node := &modGraphNode{m: m}
 		mu.Lock()
-		if err != nil && firstErr == nil {
-			firstErr = err
-		}
-		if firstErr != nil {
-			mu.Unlock()
-			return
-		}
+		modGraph[m] = node
 		if v, ok := min[m.Path]; !ok || reqs.Max(v, m.Version) != v {
 			min[m.Path] = m.Version
 		}
 		mu.Unlock()
 
-		for _, r := range required {
-			if r.Path == "" {
-				base.Errorf("Required(%v) returned zero module in list", m)
-				continue
-			}
+		required, err := reqs.Required(m)
+		if err != nil {
+			node.err = err
+			atomic.StoreInt32(&haveErr, 1)
+			return
+		}
+		node.required = required
+		for _, r := range node.required {
 			work.Add(r)
 		}
 
@@ -114,25 +149,58 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) mo
 				base.Errorf("Upgrade(%v) returned zero module", m)
 				return
 			}
-			work.Add(u)
+			if u != m {
+				node.upgrade = u
+				work.Add(u)
+			}
 		}
 	})
 
-	if firstErr != nil {
-		return nil, firstErr
+	// If there was an error, find the shortest path from the target to the
+	// node where the error occurred so we can report a useful error message.
+	if haveErr != 0 {
+		// neededBy[a] = b means a was added to the module graph by b.
+		neededBy := make(map[*modGraphNode]*modGraphNode)
+		q := make([]*modGraphNode, 0, len(modGraph))
+		q = append(q, modGraph[target])
+		for len(q) > 0 {
+			node := q[0]
+			q = q[1:]
+
+			if node.err != nil {
+				err := &BuildListError{Err: node.err}
+				for n := node; n != nil; n = neededBy[n] {
+					err.Stack = append(err.Stack, n.m)
+				}
+				return nil, err
+			}
+
+			neighbors := node.required
+			if node.upgrade.Path != "" {
+				neighbors = append(neighbors, node.upgrade)
+			}
+			for _, neighbor := range neighbors {
+				nn := modGraph[neighbor]
+				if neededBy[nn] != nil {
+					continue
+				}
+				neededBy[nn] = node
+				q = append(q, nn)
+			}
+		}
 	}
+
+	// Construct the list by traversing the graph again, replacing older
+	// modules with required minimum versions.
 	if v := min[target.Path]; v != target.Version {
 		panic(fmt.Sprintf("mistake: chose version %q instead of target %+v", v, target)) // TODO: Don't panic.
 	}
 
 	list := []module.Version{target}
 	listed := map[string]bool{target.Path: true}
 	for i := 0; i < len(list); i++ {
-		m := list[i]
-		required, err := reqs.Required(m)
-		if err != nil {
-			return nil, err
-		}
+		n := modGraph[list[i]]
+		required := n.required
 		for _, r := range required {
 			v := min[r.Path]
 			if r.Path != target.Path && reqs.Max(v, r.Version) != v {

src/cmd/go/internal/mvs/mvs_test.go

@@ -5,6 +5,7 @@
 package mvs
 
 import (
+	"fmt"
 	"reflect"
 	"strings"
 	"testing"
@@ -446,7 +447,7 @@ func (r reqsMap) Upgrade(m module.Version) (module.Version, error) {
 		}
 	}
 	if u.Path == "" {
-		return module.Version{}, &MissingModuleError{module.Version{Path: m.Path, Version: ""}}
+		return module.Version{}, fmt.Errorf("missing module: %v", module.Version{Path: m.Path})
 	}
 	return u, nil
 }
@@ -467,7 +468,7 @@ func (r reqsMap) Previous(m module.Version) (module.Version, error) {
 func (r reqsMap) Required(m module.Version) ([]module.Version, error) {
 	rr, ok := r[m]
 	if !ok {
-		return nil, &MissingModuleError{m}
+		return nil, fmt.Errorf("missing module: %v", m)
 	}
 	return rr, nil
 }

src/cmd/go/testdata/mod/example.com_badchain_a_v1.0.0.txt

@@ -0,0 +1,12 @@
+example.com/badchain/a v1.0.0
+
+-- .mod --
+module example.com/badchain/a
+
+require example.com/badchain/b v1.0.0
+-- .info --
+{"Version":"v1.0.0"}
+-- a.go --
+package a
+
+import _ "example.com/badchain/b"

src/cmd/go/testdata/mod/example.com_badchain_a_v1.1.0.txt

@@ -0,0 +1,12 @@
+example.com/badchain/a v1.1.0
+
+-- .mod --
+module example.com/badchain/a
+
+require example.com/badchain/b v1.1.0
+-- .info --
+{"Version":"v1.1.0"}
+-- a.go --
+package a
+
+import _ "example.com/badchain/b"

src/cmd/go/testdata/mod/example.com_badchain_b_v1.0.0.txt

@@ -0,0 +1,12 @@
+example.com/badchain/b v1.0.0
+
+-- .mod --
+module example.com/badchain/b
+
+require example.com/badchain/c v1.0.0
+-- .info --
+{"Version":"v1.0.0"}
+-- b.go --
+package b
+
+import _ "example.com/badchain/c"

src/cmd/go/testdata/mod/example.com_badchain_b_v1.1.0.txt

@@ -0,0 +1,12 @@
+example.com/badchain/b v1.1.0
+
+-- .mod --
+module example.com/badchain/b
+
+require example.com/badchain/c v1.1.0
+-- .info --
+{"Version":"v1.1.0"}
+-- b.go --
+package b
+
+import _ "example.com/badchain/c"

src/cmd/go/testdata/mod/example.com_badchain_c_v1.0.0.txt

@@ -0,0 +1,8 @@
+example.com/badchain/c v1.0.0
+
+-- .mod --
+module example.com/badchain/c
+-- .info --
+{"Version":"v1.0.0"}
+-- c.go --
+package c

src/cmd/go/testdata/mod/example.com_badchain_c_v1.1.0.txt

@@ -0,0 +1,8 @@
+example.com/badchain/c v1.1.0
+
+-- .mod --
+module example.com/badchain/wrong
+-- .info --
+{"Version":"v1.1.0"}
+-- c.go --
+package c

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

@@ -0,0 +1,41 @@
+[short] skip
+env GO111MODULE=on
+
+# Download everything to avoid "finding" messages in stderr later.
+cp go.mod.orig go.mod
+go mod download
+go mod download example.com/badchain/[email protected]
+go mod download example.com/badchain/[email protected]
+go mod download example.com/badchain/[email protected]
+
+# Try to upgrade example.com/badchain/a (and its dependencies).
+! go get -u example.com/badchain/a
+cmp stderr upgrade-a-expected
+cmp go.mod go.mod.orig
+
+# Try to upgrade the main module. This upgrades everything, including
+# modules that aren't direct requirements, so the error stack is shorter.
+! go get -u
+cmp stderr upgrade-main-expected
+cmp go.mod go.mod.orig
+
+# Upgrade manually. Listing modules should produce an error.
+go mod edit -require=example.com/badchain/[email protected]
+! go list -m
+cmp stderr list-expected
+
+-- go.mod.orig --
+module m
+
+require example.com/badchain/a v1.0.0
+-- upgrade-main-expected --
+go get: example.com/badchain/[email protected] ->
+	example.com/badchain/[email protected]: parsing go.mod: unexpected module path "example.com/badchain/wrong"
+-- upgrade-a-expected --
+go get: example.com/badchain/[email protected] ->
+	example.com/badchain/[email protected] ->
+	example.com/badchain/[email protected]: parsing go.mod: unexpected module path "example.com/badchain/wrong"
+-- list-expected --
+go: example.com/badchain/[email protected] ->
+	example.com/badchain/[email protected] ->
+	example.com/badchain/[email protected]: parsing go.mod: unexpected module path "example.com/badchain/wrong"