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 <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

diff --git a/src/cmd/go/internal/modload/load.go b/src/cmd/go/internal/modload/load.go
index ea0ac6771fba43b9ebe15259665c60d5b0e5364f..78681b165a33dc2d2b7f3f2a33de0b6bdda6a1bf 100644 (file)
--- a/src/cmd/go/internal/modload/load.go
+++ b/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

diff --git a/src/cmd/go/internal/mvs/mvs.go b/src/cmd/go/internal/mvs/mvs.go
index aa109693f307e5b30e6deae93c81f1e238bc818e..160e6089db78e7099e3f38a9150585d0938edd14 100644 (file)
--- a/src/cmd/go/internal/mvs/mvs.go
+++ b/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,13 +149,49 @@ 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.
        }
@@ -128,11 +199,8 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) mo
        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 {

diff --git a/src/cmd/go/internal/mvs/mvs_test.go b/src/cmd/go/internal/mvs/mvs_test.go
index 2a27dfb28890d99ea24bd95313cbddcd89d324ae..cab4bb241bc701fcee70af4712e4285db2eef1e8 100644 (file)
--- a/src/cmd/go/internal/mvs/mvs_test.go
+++ b/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
 }

diff --git a/src/cmd/go/testdata/mod/example.com_badchain_a_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_a_v1.0.0.txt
new file mode 100644 (file)
index 0000000..d7bf647
--- /dev/null
+++ b/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"

diff --git a/src/cmd/go/testdata/mod/example.com_badchain_a_v1.1.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_a_v1.1.0.txt
new file mode 100644 (file)
index 0000000..92190d8
--- /dev/null
+++ 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"

diff --git a/src/cmd/go/testdata/mod/example.com_badchain_b_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_b_v1.0.0.txt
new file mode 100644 (file)
index 0000000..d42b8aa
--- /dev/null
+++ 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"

diff --git a/src/cmd/go/testdata/mod/example.com_badchain_b_v1.1.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_b_v1.1.0.txt
new file mode 100644 (file)
index 0000000..6648184
--- /dev/null
+++ b/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"

diff --git a/src/cmd/go/testdata/mod/example.com_badchain_c_v1.0.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_c_v1.0.0.txt
new file mode 100644 (file)
index 0000000..9c717cb
--- /dev/null
+++ b/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

diff --git a/src/cmd/go/testdata/mod/example.com_badchain_c_v1.1.0.txt b/src/cmd/go/testdata/mod/example.com_badchain_c_v1.1.0.txt
new file mode 100644 (file)
index 0000000..da19ebd
--- /dev/null
+++ b/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

diff --git a/src/cmd/go/testdata/script/mod_load_badchain.txt b/src/cmd/go/testdata/script/mod_load_badchain.txt
new file mode 100644 (file)
index 0000000..ded6e16
--- /dev/null
+++ b/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/a@v1.1.0
+go mod download example.com/badchain/b@v1.1.0
+go mod download example.com/badchain/c@v1.1.0
+
+# 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/a@v1.1.0
+! 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/c@v1.0.0 ->
+       example.com/badchain/c@v1.1.0: parsing go.mod: unexpected module path "example.com/badchain/wrong"
+-- upgrade-a-expected --
+go get: example.com/badchain/a@v1.1.0 ->
+       example.com/badchain/b@v1.1.0 ->
+       example.com/badchain/c@v1.1.0: parsing go.mod: unexpected module path "example.com/badchain/wrong"
+-- list-expected --
+go: example.com/badchain/a@v1.1.0 ->
+       example.com/badchain/b@v1.1.0 ->
+       example.com/badchain/c@v1.1.0: parsing go.mod: unexpected module path "example.com/badchain/wrong"