cmd/go: spot-check the explicit requirements of root module dependencies when loading packages from them

For #36460

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

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

@@ -665,6 +665,8 @@ func updateLazyRoots(ctx context.Context, direct map[string]bool, rs *Requiremen
 	roots := rs.rootModules
 	rootsUpgraded := false
 
+	spotCheckRoot := map[module.Version]bool{}
+
 	// “The selected version of the module providing each package marked with
 	// either pkgInAll or pkgIsRoot is included as a root.”
 	needSort := false
@@ -710,26 +712,36 @@ func updateLazyRoots(ctx context.Context, direct map[string]bool, rs *Requiremen
 			// relevant to consumers of the main module either), and its dependencies
 			// should already be in the module graph — included in the dependencies of
 			// the package that imported it.
-
-			if go117LazyTODO {
-				// It is possible that one of the packages we just imported came from a
-				// module with an incomplete or erroneous go.mod file — for example,
-				// perhaps the author forgot to 'git add' their updated go.mod file
-				// after adding a new package import. If that happens, ideally we want
-				// to detect the missing requirements and fix them up here.
-				//
-				// However, we should ignore transitive dependencies of external tests:
-				// the go.mod file for the module containing the test itself is expected
-				// to provide all of the relevant dependencies, and we explicitly don't
-				// want to pull in requirements on *irrelevant* requirements that happen
-				// to occur in the go.mod files for these transitive-test-only
-				// dependencies.
-			}
-
 			continue
 		}
 
-		if _, ok := rs.rootSelected(pkg.mod.Path); !ok {
+		if _, ok := rs.rootSelected(pkg.mod.Path); ok {
+			// It is possible that the main module's go.mod file is incomplete or
+			// otherwise erroneous — for example, perhaps the author forgot to 'git
+			// add' their updated go.mod file after adding a new package import, or
+			// perhaps they made an edit to the go.mod file using a third-party tool
+			// ('git merge'?) that doesn't maintain consistency for module
+			// dependencies. If that happens, ideally we want to detect the missing
+			// requirements and fix them up here.
+			//
+			// However, we also need to be careful not to be too aggressive. For
+			// transitive dependencies of external tests, the go.mod file for the
+			// module containing the test itself is expected to provide all of the
+			// relevant dependencies, and we explicitly don't want to pull in
+			// requirements on *irrelevant* requirements that happen to occur in the
+			// go.mod files for these transitive-test-only dependencies. (See the test
+			// in mod_lazy_test_horizon.txt for a concrete example.
+			//
+			// The “goldilocks zone” seems to be to spot-check exactly the same
+			// modules that we promote to explicit roots: namely, those that provide
+			// packages transitively imported by the main module, and those that
+			// provide roots of the package-import graph. That will catch erroneous
+			// edits to the main module's go.mod file and inconsistent requirements in
+			// dependencies that provide imported packages, but will ignore erroneous
+			// or misleading requirements in dependencies that aren't obviously
+			// relevant to the packages in the main module.
+			spotCheckRoot[pkg.mod] = true
+		} else {
 			roots = append(roots, pkg.mod)
 			rootsUpgraded = true
 			// The roots slice was initially sorted because rs.rootModules was sorted,
@@ -774,8 +786,31 @@ func updateLazyRoots(ctx context.Context, direct map[string]bool, rs *Requiremen
 		} else {
 			// Since none of the roots have been upgraded, we have no reason to
 			// suspect that they are inconsistent with the requirements of any other
-			// roots. Only look at the full module graph if we've already loaded it.
+			// roots. Only look at the full module graph if we've already loaded it;
-			mg, _ = rs.graph.Load().(*ModuleGraph) // May be nil.
+			// otherwise, just spot-check the explicit requirements of the roots from
+			// which we loaded packages.
+			if rs.graph.Load() != nil {
+				// We've already loaded the full module graph, which includes the
+				// requirements of all of the root modules — even the transitive
+				// requirements, if they are eager!
+				mg, _ = rs.Graph(ctx)
+			} else if cfg.BuildMod == "vendor" {
+				// We can't spot-check the requirements of other modules because we
+				// don't in general have their go.mod files available in the vendor
+				// directory. (Fortunately this case is impossible, because mg.graph is
+				// always non-nil in vendor mode!)
+				panic("internal error: rs.graph is unexpectedly nil with -mod=vendor")
+			} else if !spotCheckRoots(ctx, rs, spotCheckRoot) {
+				// We spot-checked the explicit requirements of the roots that are
+				// relevant to the packages we've loaded. Unfortunately, they're
+				// inconsistent in some way; we need to load the full module graph
+				// so that we can fix the roots properly.
+				var err error
+				mg, err = rs.Graph(ctx)
+				if err != nil {
+					return rs, err
+				}
+			}
 		}
 
 		roots = make([]module.Version, 0, len(rs.rootModules))
@@ -835,6 +870,45 @@ func updateLazyRoots(ctx context.Context, direct map[string]bool, rs *Requiremen
 	return newRequirements(lazy, roots, direct), nil
 }
 
+// spotCheckRoots reports whether the versions of the roots in rs satisfy the
+// explicit requirements of the modules in mods.
+func spotCheckRoots(ctx context.Context, rs *Requirements, mods map[module.Version]bool) bool {
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
+
+	work := par.NewQueue(runtime.GOMAXPROCS(0))
+	for m := range mods {
+		m := m
+		work.Add(func() {
+			if ctx.Err() != nil {
+				return
+			}
+
+			summary, err := goModSummary(m)
+			if err != nil {
+				cancel()
+				return
+			}
+
+			for _, r := range summary.require {
+				if v, ok := rs.rootSelected(r.Path); ok && cmpVersion(v, r.Version) < 0 {
+					cancel()
+					return
+				}
+			}
+		})
+	}
+	<-work.Idle()
+
+	if ctx.Err() != nil {
+		// Either we failed a spot-check, or the caller no longer cares about our
+		// answer anyway.
+		return false
+	}
+
+	return true
+}
+
 // tidyEagerRoots returns a minimal set of root requirements that maintains the
 // selected version of every module that provided a package in pkgs, and
 // includes the selected version of every such module in direct as a root.

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

@@ -0,0 +1,95 @@
+# If the root requirements in a lazy module are inconsistent
+# (for example, due to a bad hand-edit or git merge),
+# they can go unnoticed as long as the module with the violated
+# requirement is not used.
+# When we load a package from that module, we should spot-check its
+# requirements and either emit an error or update the go.mod file.
+
+cp go.mod go.mod.orig
+
+
+# If we load package x from x.1, we only check the requirements of x,
+# which are fine: loading succeeds.
+
+go list -deps ./usex
+stdout '^example.net/x$'
+cmp go.mod go.mod.orig
+
+
+# However, if we load needx2, we should load the requirements of needx2.
+# Those requirements indicate x.2, not x.1, so the module graph is
+# inconsistent and needs to be fixed.
+
+! go list -deps ./useneedx2
+stderr '^go: updates to go.mod needed; to update it:\n\tgo mod tidy$'
+
+! go list -deps example.net/needx2
+stderr '^go: updates to go.mod needed; to update it:\n\tgo mod tidy$'
+
+
+# The command printed in the error message should fix the problem.
+
+go mod tidy
+go list -deps ./useneedx2
+stdout '^example.net/m/useneedx2$'
+stdout '^example.net/needx2$'
+stdout '^example.net/x$'
+
+go list -m all
+stdout '^example.net/needx2 v0\.1\.0 '
+stdout '^example.net/x v0\.2\.0 '
+
+
+-- go.mod --
+module example.net/m
+
+go 1.17
+
+require (
+	example.net/needx2 v0.1.0
+	example.net/x v0.1.0
+)
+
+replace (
+	example.net/needx2 v0.1.0 => ./needx2.1
+	example.net/x v0.1.0 => ./x.1
+	example.net/x v0.2.0 => ./x.2
+)
+-- useneedx2/useneedx2.go --
+package useneedx2
+
+import _ "example.net/needx2"
+-- usex/usex.go --
+package usex
+
+import _ "example.net/x"
+
+-- x.1/go.mod --
+module example.com/x
+
+go 1.17
+-- x.1/x.go --
+package x
+
+-- x.2/go.mod --
+module example.com/x
+
+go 1.17
+-- x.2/x.go --
+package x
+
+const AddedInV2 = true
+
+-- needx2.1/go.mod --
+module example.com/x
+
+go 1.17
+
+require example.net/x v0.2.0
+-- needx2.1/needx2.go --
+// Package needx2 needs x v0.2.0 or higher.
+package needx2
+
+import "example.net/x"
+
+var _ = x.AddedInV2