From 2ed6a54a39339ca37e9da4530b3f37a9d29b7c84 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Fri, 19 May 2023 22:35:33 -0400 Subject: [PATCH] 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 Run-TryBot: Bryan Mills Reviewed-by: Russ Cox TryBot-Result: Gopher Robot --- src/cmd/go/internal/modload/buildlist.go | 66 +++++++++++++++++-- .../testdata/script/mod_tidy_issue60313.txt | 7 +- 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/src/cmd/go/internal/modload/buildlist.go b/src/cmd/go/internal/modload/buildlist.go index def9c489e90c79..58ef80bfe5706a 100644 --- a/src/cmd/go/internal/modload/buildlist.go +++ b/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 } diff --git a/src/cmd/go/testdata/script/mod_tidy_issue60313.txt b/src/cmd/go/testdata/script/mod_tidy_issue60313.txt index 1ae2c13b74bf8b..cd704ce34c65d3 100644 --- a/src/cmd/go/testdata/script/mod_tidy_issue60313.txt +++ b/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