cmd/go: retain extra roots to disambiguate imports in 'go mod tidy'

We don't normally keep explicit requirements for test dependencies of
packages loaded from other modules when the required version is
already the selected version in the module graph. However, in some
cases we may need to keep an explicit requirement in order to make use
of lazy module loading to disambiguate an otherwise-ambiguous import.

Note that there is no Go version guard for this change: in the cases
where the behavior of 'go mod tidy' has changed, previous versions of
Go would produce go.mod files that break successive calls to
'go mod tidy'. Given that, I suspect that any existing user in the
wild affected by this bug either already has a workaround in place
using redundant import statements (in which case the change does not
affect them) or is running 'go mod tidy -e' to force past the error
(in which case a change in behavior to a non-error should not be
surprising).

Fixes #60313.

Change-Id: Idf294f72cbe3904b871290d79e4493595a0c7bfc
Reviewed-on: https://go-review.googlesource.com/c/go/+/496635
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>

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

@@ -11,6 +11,7 @@ import (
 	"cmd/go/internal/par"
 	"cmd/go/internal/slices"
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"reflect"
@@ -758,8 +759,8 @@ func updateWorkspaceRoots(ctx context.Context, rs *Requirements, add []module.Ve
 // roots) until the set of roots has converged.
 func tidyPrunedRoots(ctx context.Context, mainModule module.Version, direct map[string]bool, pkgs []*loadPkg) (*Requirements, error) {
 	var (
-		roots        []module.Version
-		pathIncluded = map[string]bool{mainModule.Path: true}
+		roots      []module.Version
+		pathIsRoot = map[string]bool{mainModule.Path: true}
 	)
 	// We start by adding roots for every package in "all".
 	//
@@ -779,9 +780,9 @@ func tidyPrunedRoots(ctx context.Context, mainModule module.Version, direct map[
 		if !pkg.flags.has(pkgInAll) {
 			continue
 		}
-		if pkg.fromExternalModule() && !pathIncluded[pkg.mod.Path] {
+		if pkg.fromExternalModule() && !pathIsRoot[pkg.mod.Path] {
 			roots = append(roots, pkg.mod)
-			pathIncluded[pkg.mod.Path] = true
+			pathIsRoot[pkg.mod.Path] = true
 		}
 		queue = append(queue, pkg)
 		queued[pkg] = true
@@ -813,11 +814,12 @@ func tidyPrunedRoots(ctx context.Context, mainModule module.Version, direct map[
 				queue = append(queue, pkg.test)
 				queued[pkg.test] = true
 			}
-			if !pathIncluded[m.Path] {
+
+			if !pathIsRoot[m.Path] {
 				if s := mg.Selected(m.Path); cmpVersion(s, m.Version) < 0 {
 					roots = append(roots, m)
+					pathIsRoot[m.Path] = true
 				}
-				pathIncluded[m.Path] = true
 			}
 		}
 
@@ -827,10 +829,62 @@ func tidyPrunedRoots(ctx context.Context, mainModule module.Version, direct map[
 		}
 	}
 
+	roots = tidy.rootModules
 	_, err := tidy.Graph(ctx)
 	if err != nil {
 		return nil, err
 	}
+
+	// We try to avoid adding explicit requirements for test-only dependencies of
+	// packages in external modules. However, if we drop the explicit
+	// requirements, that may change an import from unambiguous (due to lazy
+	// module loading) to ambiguous (because lazy module loading no longer
+	// disambiguates it). For any package that has become ambiguous, we try
+	// to fix it by promoting its module to an explicit root.
+	// (See https://go.dev/issue/60313.)
+	q := par.NewQueue(runtime.GOMAXPROCS(0))
+	for {
+		var disambiguateRoot sync.Map
+		for _, pkg := range pkgs {
+			if pkg.mod.Path == "" || pathIsRoot[pkg.mod.Path] {
+				// Lazy module loading will cause m to be checked before any other modules
+				// that are only indirectly required. It is as unambiguous as possible.
+				continue
+			}
+			pkg := pkg
+			q.Add(func() {
+				skipModFile := true
+				_, _, _, _, err := importFromModules(ctx, pkg.path, tidy, nil, skipModFile)
+				if aie := (*AmbiguousImportError)(nil); errors.As(err, &aie) {
+					disambiguateRoot.Store(pkg.mod, true)
+				}
+			})
+		}
+		<-q.Idle()
+
+		disambiguateRoot.Range(func(k, _ any) bool {
+			m := k.(module.Version)
+			roots = append(roots, m)
+			pathIsRoot[m.Path] = true
+			return true
+		})
+
+		if len(roots) > len(tidy.rootModules) {
+			module.Sort(roots)
+			tidy = newRequirements(pruned, roots, tidy.direct)
+			_, err = tidy.Graph(ctx)
+			if err != nil {
+				return nil, err
+			}
+			// Adding these roots may have pulled additional modules into the module
+			// graph, causing additional packages to become ambiguous. Keep iterating
+			// until we reach a fixed point.
+			continue
+		}
+
+		break
+	}
+
 	return tidy, nil
 }
 

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

@@ -2,12 +2,9 @@
 # dependencies needed to prevent 'ambiguous import' errors in external test
 # dependencies.
 
+cp go.mod go.mod.orig
 go mod tidy
-cp go.mod tidy1.mod
-
-! go mod tidy  # BUG: This should succeed and leave go.mod unchanged.
-	# cmp go.mod tidy1.mod
-stderr 'ambiguous import'
+cmp go.mod go.mod.orig
 
 -- go.mod --
 module example