cmd/go/internal/modload: split updateRoots into separate functions for updating and tidying

In CL 293689, I fused the mvs.Reqs calls that were formerly in MinReqs
and TidyBuildList into a single function, updateRoots, in the hope
that it expressed a fundamental operation. As I have been working on
the lazy equivalents, I have come to realize that these functions are
deeply related but fundamentally different.

In order to help me reason about the two different roles, I am making
the two functions separate once more, but leaving them colocated in
the code.

For #36460

Change-Id: I851d6d81fbfd84f39411e0d076ee72a9909c60ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/310629
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>
Reviewed-by: Jay Conrod <jayconrod@google.com>

1 files changed, 92 insertions, 78 deletions

diff --git a/src/cmd/go/internal/modload/buildlist.go b/src/cmd/go/internal/modload/buildlist.go
index 2eb47d2c9f..07d9fdfc54 100644
--- a/src/cmd/go/internal/modload/buildlist.go
+++ b/src/cmd/go/internal/modload/buildlist.go
@@ -409,7 +409,7 @@ func expandGraph(ctx context.Context, rs *Requirements) (*Requirements, *ModuleG
 		// roots — but in a lazy module it may pull in previously-irrelevant
 		// transitive dependencies.
 
-		newRS, rsErr := updateRoots(ctx, rs.depth, rs.direct, nil, rs)
+		newRS, rsErr := updateRoots(ctx, rs.direct, rs)
 		if rsErr != nil {
 			// Failed to update roots, perhaps because of an error in a transitive
 			// dependency needed for the update. Return the original Requirements
@@ -552,7 +552,15 @@ func tidyBuildList(ctx context.Context, ld *loader, initialRS *Requirements) *Re
 		// changed after loading packages.
 	}
 
-	tidy, err := updateRoots(ctx, depth, ld.requirements.direct, ld.pkgs, nil)
+	var (
+		tidy *Requirements
+		err  error
+	)
+	if depth == lazy {
+		panic("internal error: 'go mod tidy' for lazy modules is not yet implemented")
+	} else {
+		tidy, err = tidyEagerRoots(ctx, ld.requirements, ld.pkgs)
+	}
 	if err != nil {
 		base.Fatalf("go: %v", err)
 	}
@@ -573,75 +581,57 @@ func tidyBuildList(ctx context.Context, ld *loader, initialRS *Requirements) *Re
 	return tidy
 }
 
-// updateRoots returns a set of root requirements that includes the selected
-// version of every module path in direct as a root, and maintains the selected
-// versions of every module selected in the graph of rs (if rs is non-nil), or
-// every module that provides any package in pkgs (otherwise).
-//
-// If pkgs is non-empty and rs is non-nil, the packages are assumed to be loaded
-// from the modules selected in the graph of rs.
-//
-// The roots are updated such that:
-//
-// 	1. The selected version of every module path in direct is included as a root
-// 	   (if it is not "none").
-// 	2. Each root is the selected version of its path. (We say that such a root
-// 	   set is “consistent”.)
-// 	3. The selected version of the module providing each package in pkgs remains
-// 	   selected.
-// 	4. If rs is non-nil, every version selected in the graph of rs remains selected.
-func updateRoots(ctx context.Context, depth modDepth, direct map[string]bool, pkgs []*loadPkg, rs *Requirements) (*Requirements, error) {
+// 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 rs.direct as a root.
+func tidyEagerRoots(ctx context.Context, rs *Requirements, pkgs []*loadPkg) (*Requirements, error) {
+	var (
+		keep     []module.Version
+		keptPath = map[string]bool{}
+	)
 	var (
 		rootPaths   []string // module paths that should be included as roots
 		inRootPaths = map[string]bool{}
 	)
-
-	var keep []module.Version
-	if rs != nil {
-		mg, err := rs.Graph(ctx)
-		if err != nil {
-			// We can't ignore errors in the module graph even if the user passed the -e
-			// flag to try to push past them. If we can't load the complete module
-			// dependencies, then we can't reliably compute a minimal subset of them.
-			return rs, err
-		}
-		keep = mg.BuildList()[1:]
-
-		for _, root := range rs.rootModules {
-			// If the selected version of the root is the same as what was already
-			// listed in the go.mod file, retain it as a root (even if redundant) to
-			// avoid unnecessary churn. (See https://golang.org/issue/34822.)
-			//
-			// We do this even for indirect requirements, since we don't know why they
-			// were added and they could become direct at any time.
-			if !inRootPaths[root.Path] && mg.Selected(root.Path) == root.Version {
-				rootPaths = append(rootPaths, root.Path)
-				inRootPaths[root.Path] = true
-			}
+	for _, pkg := range pkgs {
+		if !pkg.fromExternalModule() {
+			continue
 		}
-	} else {
-		kept := map[module.Version]bool{Target: true}
-		for _, pkg := range pkgs {
-			if pkg.mod.Path != "" && !kept[pkg.mod] {
-				keep = append(keep, pkg.mod)
-				kept[pkg.mod] = true
+		if m := pkg.mod; !keptPath[m.Path] {
+			keep = append(keep, m)
+			keptPath[m.Path] = true
+			if rs.direct[m.Path] && !inRootPaths[m.Path] {
+				rootPaths = append(rootPaths, m.Path)
+				inRootPaths[m.Path] = true
 			}
 		}
 	}
 
-	// “The selected version of every module path in direct is included as a root.”
-	//
-	// This is only for convenience and clarity for end users: the choice of
-	// explicit vs. implicit dependency has no impact on MVS selection (for itself
-	// or any other module).
-	if go117LazyTODO {
-		// Update the above comment to reflect lazy loading once implemented.
+	min, err := mvs.Req(Target, rootPaths, &mvsReqs{roots: keep})
+	if err != nil {
+		return nil, err
 	}
-	for _, m := range keep {
-		if direct[m.Path] && !inRootPaths[m.Path] {
-			rootPaths = append(rootPaths, m.Path)
-			inRootPaths[m.Path] = true
-		}
+	return newRequirements(eager, min, rs.direct), nil
+}
+
+// updateRoots returns a set of root requirements that includes the selected
+// version of every module path in direct as a root, and maintains the selected
+// version of every module selected in the graph of rs.
+//
+// The roots are updated such that:
+//
+// 	1. The selected version of every module path in direct is included as a root
+// 	   (if it is not "none").
+// 	2. Each root is the selected version of its path. (We say that such a root
+// 	   set is “consistent”.)
+// 	3. Every version selected in the graph of rs remains selected.
+func updateRoots(ctx context.Context, direct map[string]bool, rs *Requirements) (*Requirements, error) {
+	mg, err := rs.Graph(ctx)
+	if err != nil {
+		// We can't ignore errors in the module graph even if the user passed the -e
+		// flag to try to push past them. If we can't load the complete module
+		// dependencies, then we can't reliably compute a minimal subset of them.
+		return rs, err
 	}
 
 	if cfg.BuildMod != "mod" {
@@ -652,7 +642,13 @@ func updateRoots(ctx context.Context, depth modDepth, direct map[string]bool, pk
 			// but we aren't even allowed to modify them.
 			return rs, errGoModDirty
 		}
-		for _, mPath := range rootPaths {
+		for _, m := range rs.rootModules {
+			if m.Version != mg.Selected(m.Path) {
+				// The root version v is misleading: the actual selected version is higher.
+				return rs, errGoModDirty
+			}
+		}
+		for mPath := range direct {
 			if _, ok := rs.rootSelected(mPath); !ok {
 				// Module m is supposed to be listed explicitly, but isn't.
 				//
@@ -662,21 +658,6 @@ func updateRoots(ctx context.Context, depth modDepth, direct map[string]bool, pk
 				return rs, errGoModDirty
 			}
 		}
-		for _, m := range keep {
-			if v, ok := rs.rootSelected(m.Path); ok && v != m.Version {
-				// The root version v is misleading: the actual selected version is
-				// m.Version.
-				return rs, errGoModDirty
-			}
-		}
-		for _, m := range rs.rootModules {
-			if v, ok := rs.rootSelected(m.Path); ok && v != m.Version {
-				// The roots list both m.Version and some higher version of m.Path.
-				// The root for m.Version is misleading: the actual selected version is
-				// *at least* v.
-				return rs, errGoModDirty
-			}
-		}
 
 		// No explicit roots are missing and all roots are already at the versions
 		// we want to keep. Any other changes we would make are purely cosmetic,
@@ -685,6 +666,39 @@ func updateRoots(ctx context.Context, depth modDepth, direct map[string]bool, pk
 		return rs, nil
 	}
 
+	var (
+		rootPaths   []string // module paths that should be included as roots
+		inRootPaths = map[string]bool{}
+	)
+	for _, root := range rs.rootModules {
+		// If the selected version of the root is the same as what was already
+		// listed in the go.mod file, retain it as a root (even if redundant) to
+		// avoid unnecessary churn. (See https://golang.org/issue/34822.)
+		//
+		// We do this even for indirect requirements, since we don't know why they
+		// were added and they could become direct at any time.
+		if !inRootPaths[root.Path] && mg.Selected(root.Path) == root.Version {
+			rootPaths = append(rootPaths, root.Path)
+			inRootPaths[root.Path] = true
+		}
+	}
+
+	// “The selected version of every module path in direct is included as a root.”
+	//
+	// This is only for convenience and clarity for end users: the choice of
+	// explicit vs. implicit dependency has no impact on MVS selection (for itself
+	// or any other module).
+	if go117LazyTODO {
+		// Update the above comment to reflect lazy loading once implemented.
+	}
+	keep := mg.BuildList()[1:]
+	for _, m := range keep {
+		if direct[m.Path] && !inRootPaths[m.Path] {
+			rootPaths = append(rootPaths, m.Path)
+			inRootPaths[m.Path] = true
+		}
+	}
+
 	min, err := mvs.Req(Target, rootPaths, &mvsReqs{roots: keep})
 	if err != nil {
 		return rs, err
@@ -695,7 +709,7 @@ func updateRoots(ctx context.Context, depth modDepth, direct map[string]bool, pk
 	// the root set is the same as the original root set in rs and recycle its
 	// module graph and build list, if they have already been loaded.
 
-	return newRequirements(depth, min, direct), nil
+	return newRequirements(rs.depth, min, direct), nil
 }
 
 // checkMultiplePaths verifies that a given module path is used as itself