cmd/go/internal/mvs: factor out an incremental implementation

The new Graph type implements an incremental version of the MVS
algorithm, with requirements pushed in by the caller instead of pulled
by an internal MVS traversal.

To avoid redundancy going forward (and to ensure adequate test
coverage of the incremental implementation), the existing buildList
function is reimplemented in terms of Graph.

For #36460

Change-Id: Idd0b6ab8f17cc41d83a2a4c25a95f82e9ce1eab0
Reviewed-on: https://go-review.googlesource.com/c/go/+/244760
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/mvs/errors.go

@@ -31,13 +31,15 @@ type buildListErrorElem struct {
 // occurred at a module found along the given path of requirements and/or
 // upgrades, which must be non-empty.
 //
-// The isUpgrade function reports whether a path step is due to an upgrade.
-// A nil isUpgrade function indicates that none of the path steps are due to upgrades.
-func NewBuildListError(err error, path []module.Version, isUpgrade func(from, to module.Version) bool) *BuildListError {
+// The isVersionChange function reports whether a path step is due to an
+// explicit upgrade or downgrade (as opposed to an existing requirement in a
+// go.mod file). A nil isVersionChange function indicates that none of the path
+// steps are due to explicit version changes.
+func NewBuildListError(err error, path []module.Version, isVersionChange func(from, to module.Version) bool) *BuildListError {
 	stack := make([]buildListErrorElem, 0, len(path))
 	for len(path) > 1 {
 		reason := "requires"
-		if isUpgrade != nil && isUpgrade(path[0], path[1]) {
+		if isVersionChange != nil && isVersionChange(path[0], path[1]) {
 			reason = "updating to"
 		}
 		stack = append(stack, buildListErrorElem{

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

@@ -0,0 +1,223 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package mvs
+
+import (
+	"fmt"
+
+	"golang.org/x/mod/module"
+)
+
+// Graph implements an incremental version of the MVS algorithm, with the
+// requirements pushed by the caller instead of pulled by the MVS traversal.
+type Graph struct {
+	cmp   func(v1, v2 string) int
+	roots []module.Version
+
+	required map[module.Version][]module.Version
+
+	isRoot   map[module.Version]bool // contains true for roots and false for reachable non-roots
+	selected map[string]string       // path → version
+}
+
+// NewGraph returns an incremental MVS graph containing only a set of root
+// dependencies and using the given max function for version strings.
+//
+// The caller must ensure that the root slice is not modified while the Graph
+// may be in use.
+func NewGraph(cmp func(v1, v2 string) int, roots []module.Version) *Graph {
+	g := &Graph{
+		cmp:      cmp,
+		roots:    roots[:len(roots):len(roots)],
+		required: make(map[module.Version][]module.Version),
+		isRoot:   make(map[module.Version]bool),
+		selected: make(map[string]string),
+	}
+
+	for _, m := range roots {
+		g.isRoot[m] = true
+		if g.cmp(g.Selected(m.Path), m.Version) < 0 {
+			g.selected[m.Path] = m.Version
+		}
+	}
+
+	return g
+}
+
+// Require adds the information that module m requires all modules in reqs.
+// The reqs slice must not be modified after it is passed to Require.
+//
+// m must be reachable by some existing chain of requirements from g's target,
+// and Require must not have been called for it already.
+//
+// If any of the modules in reqs has the same path as g's target,
+// the target must have higher precedence than the version in req.
+func (g *Graph) Require(m module.Version, reqs []module.Version) {
+	// To help catch disconnected-graph bugs, enforce that all required versions
+	// are actually reachable from the roots (and therefore should affect the
+	// selected versions of the modules they name).
+	if _, reachable := g.isRoot[m]; !reachable {
+		panic(fmt.Sprintf("%v is not reachable from any root", m))
+	}
+
+	// Truncate reqs to its capacity to avoid aliasing bugs if it is later
+	// returned from RequiredBy and appended to.
+	reqs = reqs[:len(reqs):len(reqs)]
+
+	if _, dup := g.required[m]; dup {
+		panic(fmt.Sprintf("requirements of %v have already been set", m))
+	}
+	g.required[m] = reqs
+
+	for _, dep := range reqs {
+		// Mark dep reachable, regardless of whether it is selected.
+		if _, ok := g.isRoot[dep]; !ok {
+			g.isRoot[dep] = false
+		}
+
+		if g.cmp(g.Selected(dep.Path), dep.Version) < 0 {
+			g.selected[dep.Path] = dep.Version
+		}
+	}
+}
+
+// RequiredBy returns the slice of requirements passed to Require for m, if any,
+// with its capacity reduced to its length.
+// If Require has not been called for m, RequiredBy(m) returns ok=false.
+//
+// The caller must not modify the returned slice, but may safely append to it
+// and may rely on it not to be modified.
+func (g *Graph) RequiredBy(m module.Version) (reqs []module.Version, ok bool) {
+	reqs, ok = g.required[m]
+	return reqs, ok
+}
+
+// Selected returns the selected version of the given module path.
+//
+// If no version is selected, Selected returns version "none".
+func (g *Graph) Selected(path string) (version string) {
+	v, ok := g.selected[path]
+	if !ok {
+		return "none"
+	}
+	return v
+}
+
+// BuildList returns the selected versions of all modules present in the Graph,
+// beginning with the selected versions of each module path in the roots of g.
+//
+// The order of the remaining elements in the list is deterministic
+// but arbitrary.
+func (g *Graph) BuildList() []module.Version {
+	seenRoot := make(map[string]bool, len(g.roots))
+
+	var list []module.Version
+	for _, r := range g.roots {
+		if seenRoot[r.Path] {
+			// Multiple copies of the same root, with the same or different versions,
+			// are a bit of a degenerate case: we will take the transitive
+			// requirements of both roots into account, but only the higher one can
+			// possibly be selected. However — especially given that we need the
+			// seenRoot map for later anyway — it is simpler to support this
+			// degenerate case than to forbid it.
+			continue
+		}
+
+		if v := g.Selected(r.Path); v != "none" {
+			list = append(list, module.Version{Path: r.Path, Version: v})
+		}
+		seenRoot[r.Path] = true
+	}
+	uniqueRoots := list
+
+	for path, version := range g.selected {
+		if !seenRoot[path] {
+			list = append(list, module.Version{Path: path, Version: version})
+		}
+	}
+	module.Sort(list[len(uniqueRoots):])
+
+	return list
+}
+
+// WalkBreadthFirst invokes f once, in breadth-first order, for each module
+// version other than "none" that appears in the graph, regardless of whether
+// that version is selected.
+func (g *Graph) WalkBreadthFirst(f func(m module.Version)) {
+	var queue []module.Version
+	enqueued := make(map[module.Version]bool)
+	for _, m := range g.roots {
+		if m.Version != "none" {
+			queue = append(queue, m)
+			enqueued[m] = true
+		}
+	}
+
+	for len(queue) > 0 {
+		m := queue[0]
+		queue = queue[1:]
+
+		f(m)
+
+		reqs, _ := g.RequiredBy(m)
+		for _, r := range reqs {
+			if !enqueued[r] && r.Version != "none" {
+				queue = append(queue, r)
+				enqueued[r] = true
+			}
+		}
+	}
+}
+
+// FindPath reports a shortest requirement path starting at one of the roots of
+// the graph and ending at a module version m for which f(m) returns true, or
+// nil if no such path exists.
+func (g *Graph) FindPath(f func(module.Version) bool) []module.Version {
+	// firstRequires[a] = b means that in a breadth-first traversal of the
+	// requirement graph, the module version a was first required by b.
+	firstRequires := make(map[module.Version]module.Version)
+
+	queue := g.roots
+	for _, m := range g.roots {
+		firstRequires[m] = module.Version{}
+	}
+
+	for len(queue) > 0 {
+		m := queue[0]
+		queue = queue[1:]
+
+		if f(m) {
+			// Construct the path reversed (because we're starting from the far
+			// endpoint), then reverse it.
+			path := []module.Version{m}
+			for {
+				m = firstRequires[m]
+				if m.Path == "" {
+					break
+				}
+				path = append(path, m)
+			}
+
+			i, j := 0, len(path)-1
+			for i < j {
+				path[i], path[j] = path[j], path[i]
+				i++
+				j--
+			}
+
+			return path
+		}
+
+		reqs, _ := g.RequiredBy(m)
+		for _, r := range reqs {
+			if _, seen := firstRequires[r]; !seen {
+				queue = append(queue, r)
+				firstRequires[r] = m
+			}
+		}
+	}
+
+	return nil
+}

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

@@ -10,7 +10,6 @@ import (
 	"fmt"
 	"sort"
 	"sync"
-	"sync/atomic"
 
 	"cmd/go/internal/par"
 
@@ -91,151 +90,91 @@ func BuildList(target module.Version, reqs Reqs) ([]module.Version, error) {
 }
 
 func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) (module.Version, error)) ([]module.Version, error) {
-	// Explore work graph in parallel in case reqs.Required
-	// does high-latency network operations.
-	type modGraphNode struct {
-		m        module.Version
-		required []module.Version
-		upgrade  module.Version
-		err      error
-	}
-	var (
-		mu       sync.Mutex
-		modGraph = map[module.Version]*modGraphNode{}
-		min      = map[string]string{} // maps module path to minimum required version
-		haveErr  int32
-	)
-	setErr := func(n *modGraphNode, err error) {
-		n.err = err
-		atomic.StoreInt32(&haveErr, 1)
+	cmp := func(v1, v2 string) int {
+		if reqs.Max(v1, v2) != v1 {
+			return -1
+		}
+		if reqs.Max(v2, v1) != v2 {
+			return 1
+		}
+		return 0
 	}
 
+	var (
+		mu       sync.Mutex
+		g        = NewGraph(cmp, []module.Version{target})
+		upgrades = map[module.Version]module.Version{}
+		errs     = map[module.Version]error{} // (non-nil errors only)
+	)
+
+	// Explore work graph in parallel in case reqs.Required
+	// does high-latency network operations.
 	var work par.Work
 	work.Add(target)
 	work.Do(10, func(item interface{}) {
 		m := item.(module.Version)
 
-		node := &modGraphNode{m: m}
-		mu.Lock()
-		modGraph[m] = node
+		var required []module.Version
+		var err error
 		if m.Version != "none" {
-			if v, ok := min[m.Path]; !ok || reqs.Max(v, m.Version) != v {
-				min[m.Path] = m.Version
+			required, err = reqs.Required(m)
+		}
+
+		u := m
+		if upgrade != nil {
+			upgradeTo, upErr := upgrade(m)
+			if upErr == nil {
+				u = upgradeTo
+			} else if err == nil {
+				err = upErr
 			}
 		}
+
+		mu.Lock()
+		if err != nil {
+			errs[m] = err
+		}
+		if u != m {
+			upgrades[m] = u
+			required = append([]module.Version{u}, required...)
+		}
+		g.Require(m, required)
 		mu.Unlock()
 
-		if m.Version != "none" {
-			required, err := reqs.Required(m)
-			if err != nil {
-				setErr(node, err)
-				return
-			}
-			node.required = required
-			for _, r := range node.required {
-				work.Add(r)
-			}
-		}
-
-		if upgrade != nil {
-			u, err := upgrade(m)
-			if err != nil {
-				setErr(node, err)
-				return
-			}
-			if u != m {
-				node.upgrade = u
-				work.Add(u)
-			}
+		for _, r := range required {
+			work.Add(r)
 		}
 	})
 
 	// 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 {
-				pathUpgrade := map[module.Version]module.Version{}
-
-				// Construct the error path reversed (from the error to the main module),
-				// then reverse it to obtain the usual order (from the main module to
-				// the error).
-				errPath := []module.Version{node.m}
-				for n, prev := neededBy[node], node; n != nil; n, prev = neededBy[n], n {
-					if n.upgrade == prev.m {
-						pathUpgrade[n.m] = prev.m
-					}
-					errPath = append(errPath, n.m)
-				}
-				i, j := 0, len(errPath)-1
-				for i < j {
-					errPath[i], errPath[j] = errPath[j], errPath[i]
-					i++
-					j--
-				}
-
-				isUpgrade := func(from, to module.Version) bool {
-					return pathUpgrade[from] == to
-				}
-
-				return nil, NewBuildListError(node.err, errPath, isUpgrade)
-			}
-
-			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)
-			}
+	if len(errs) > 0 {
+		errPath := g.FindPath(func(m module.Version) bool {
+			return errs[m] != nil
+		})
+		if len(errPath) == 0 {
+			panic("internal error: could not reconstruct path to module with error")
 		}
+
+		err := errs[errPath[len(errPath)-1]]
+		isUpgrade := func(from, to module.Version) bool {
+			if u, ok := upgrades[from]; ok {
+				return u == to
+			}
+			return false
+		}
+		return nil, NewBuildListError(err.(error), errPath, isUpgrade)
 	}
 
 	// The final list is the minimum version of each module found in the graph.
-
-	if v := min[target.Path]; v != target.Version {
+	list := g.BuildList()
+	if v := list[0]; v != target {
 		// target.Version will be "" for modload, the main client of MVS.
 		// "" denotes the main module, which has no version. However, MVS treats
 		// version strings as opaque, so "" is not a special value here.
 		// See golang.org/issue/31491, golang.org/issue/29773.
-		panic(fmt.Sprintf("mistake: chose version %q instead of target %+v", v, target)) // TODO: Don't panic.
+		panic(fmt.Sprintf("mistake: chose version %q instead of target %+v", v, target))
 	}
-
-	list := []module.Version{target}
-	for path, vers := range min {
-		if path != target.Path {
-			list = append(list, module.Version{Path: path, Version: vers})
-		}
-
-		n := modGraph[module.Version{Path: path, Version: vers}]
-		required := n.required
-		for _, r := range required {
-			if r.Version == "none" {
-				continue
-			}
-			v := min[r.Path]
-			if r.Path != target.Path && reqs.Max(v, r.Version) != v {
-				panic(fmt.Sprintf("mistake: version %q does not satisfy requirement %+v", v, r)) // TODO: Don't panic.
-			}
-		}
-	}
-
-	tail := list[1:]
-	sort.Slice(tail, func(i, j int) bool {
-		return tail[i].Path < tail[j].Path
-	})
 	return list, nil
 }