cmd/go/internal/mvs: recompute build list in Reqs before minimizing

modload.MinReqs was passing modload.buildList to mvs.Reqs explicitly,
apparently as an optimization. However, we do not always have the
invariant that modload.buildList is complete: in particular, 'go mod
tidy' begins by reducing modload.buildList to only the set of modules
that provide packages to the build, which may be substantially smaller
than the final build list.

Other operations, such as 'go mod graph', do not load the entire
import graph, and therefore call Reqs with the unreduced build list.

Since Reqs retains modules according to a post-order traversal of the
list, an incomplete list may produce a different traversal order — and
therefore a different minimal solution, when multiple minimal
solutions exist. That caused 'go mod tidy' to produce different output
from other 'go' subcommands when certain patterns of dependencies are
present.

Since passing in the build list is only an optimization anyway, remove
the parameter and recompute the actual (complete) list at the
beginning of mvs.Reqs itself. That way, it is guaranteed to be
complete and in canonical order.

Fixes #34086

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

Author: Bryan C. Mills

src/cmd/go/internal/modload/init.go

@@ -634,7 +634,7 @@ func MinReqs() mvs.Reqs {
 			direct = append(direct, m.Path)
 		}
 	}
-	min, err := mvs.Req(Target, buildList, direct, Reqs())
+	min, err := mvs.Req(Target, direct, Reqs())
 	if err != nil {
 		base.Fatalf("go: %v", err)
 	}

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

@@ -250,10 +250,15 @@ func buildList(target module.Version, reqs Reqs, upgrade func(module.Version) (m
 	return list, nil
 }
 
-// Req returns the minimal requirement list for the target module
-// that results in the given build list, with the constraint that all
-// module paths listed in base must appear in the returned list.
-func Req(target module.Version, list []module.Version, base []string, reqs Reqs) ([]module.Version, error) {
+// Req returns the minimal requirement list for the target module,
+// with the constraint that all module paths listed in base must
+// appear in the returned list.
+func Req(target module.Version, base []string, reqs Reqs) ([]module.Version, error) {
+	list, err := BuildList(target, reqs)
+	if err != nil {
+		return nil, err
+	}
+
 	// Note: Not running in parallel because we assume
 	// that list came from a previous operation that paged
 	// in all the requirements, so there's no I/O to overlap now.

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

@@ -280,6 +280,20 @@ D2:
 build A: A B1 C1 D1
 upgrade* A: A B2 C2 D2
 
+# Cycles with multiple possible solutions.
+# (golang.org/issue/34086)
+name: cycle3
+M: A1 C2
+A1: B1
+B1: C1
+B2: C2
+C1:
+C2: B2
+build M: M A1 B2 C2
+req M: A1 B2
+req M A: A1 B2
+req M C: A1 C2
+
 # Requirement minimization.
 
 name: req1
@@ -390,7 +404,15 @@ func Test(t *testing.T) {
 			fns = append(fns, func(t *testing.T) {
 				list, err := Upgrade(m(kf[1]), reqs, ms(kf[2:])...)
 				if err == nil {
-					list, err = Req(m(kf[1]), list, nil, reqs)
+					// Copy the reqs map, but substitute the upgraded requirements in
+					// place of the target's original requirements.
+					upReqs := make(reqsMap, len(reqs))
+					for m, r := range reqs {
+						upReqs[m] = r
+					}
+					upReqs[m(kf[1])] = list
+
+					list, err = Req(m(kf[1]), nil, upReqs)
 				}
 				checkList(t, key, list, err, val)
 			})
@@ -418,11 +440,7 @@ func Test(t *testing.T) {
 				t.Fatalf("req takes at least one argument: %q", line)
 			}
 			fns = append(fns, func(t *testing.T) {
-				list, err := BuildList(m(kf[1]), reqs)
-				if err != nil {
-					t.Fatal(err)
-				}
-				list, err = Req(m(kf[1]), list, kf[2:], reqs)
+				list, err := Req(m(kf[1]), kf[2:], reqs)
 				checkList(t, key, list, err, val)
 			})
 			continue

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

@@ -0,0 +1,75 @@
+# Regression test for https://golang.org/issue/34086:
+# 'go mod tidy' produced different go.mod file from other
+# subcommands when certain kinds of cycles were present
+# in the build graph.
+
+env GO111MODULE=on
+
+cp go.mod go.mod.orig
+go mod tidy
+cmp go.mod go.mod.orig
+
+# If the go.mod file is already tidy, 'go mod graph' should not modify it.
+go mod graph
+cmp go.mod go.mod.orig
+
+-- go.mod --
+module root
+
+go 1.13
+
+replace (
+	a v0.1.0 => ./a1
+	b v0.1.0 => ./b1
+	b v0.2.0 => ./b2
+	c v0.1.0 => ./c1
+	c v0.2.0 => ./c2
+)
+
+require (
+	a v0.1.0
+	b v0.2.0 // indirect
+)
+-- main.go --
+package main
+
+import _ "a"
+
+func main() {}
+
+-- a1/go.mod --
+module a
+
+go 1.13
+
+require b v0.1.0
+-- a1/a.go --
+package a
+
+import _ "c"
+-- b1/go.mod --
+module b
+
+go 1.13
+
+require c v0.1.0
+-- b2/go.mod --
+module b
+
+go 1.13
+
+require c v0.2.0
+-- c1/go.mod --
+module c
+
+go 1.13
+-- c2/c.go --
+package c
+-- c2/go.mod --
+module c
+
+go 1.13
+
+require b v0.2.0
+-- c2/c.go --
+package c