]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: retain extra roots to disambiguate imports in 'go mod tidy'
authorBryan C. Mills <bcmills@google.com>
Sat, 20 May 2023 02:35:33 +0000 (22:35 -0400)
committerGopher Robot <gobot@golang.org>
Mon, 22 May 2023 21:07:10 +0000 (21:07 +0000)
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
src/cmd/go/testdata/script/mod_tidy_issue60313.txt

index def9c489e90c797c1b002f23c5d42b8747723b76..58ef80bfe5706a7027d501f2b86a4210320eaf09 100644 (file)
@@ -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
 }
 
index 1ae2c13b74bf8ba0acd5f5001be10ad01b9f53d3..cd704ce34c65d3ca36003a13e3c52a6481bd1f99 100644 (file)
@@ -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