cmd/go/internal/modload: remove the addedModuleFor map

At one point this map checked for infinite loops during package iteration.
The last write to the map was mistakenly removed in CL 251445.

However, looking at the code before that change, the map-based
termination strategy was never quite right to begin with: it checked
whether we had ever added any module for the given package, not
whether we had already added the module being proposed right now. (For
packages within nested modules, we could try adding multiple different
modules for a given package without looping.)

Moreover, the "looping trying to add package" failure message was only
marginally helpful. Users are capable of noticing that an invocation
of the 'go' command is taking too long, and will report a bug for an
infinite loop just as readily as a "looping trying to add package"
error.

We could try to add this tracking back in, but it's no substitute for
a proper proof of convergence, and the code is simpler without it.
Instead I'm going to add a proper proof of convergence — or, barring
that, a more accurate and useful check for failure to converge. In the
meantime, this invariantly-empty map isn't doing anybody any good.

For #36460

Change-Id: I2c111d4b4bf59159af0d7e62d1c0ef4ce0a43a71
Reviewed-on: https://go-review.googlesource.com/c/go/+/312929
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>

diff --git a/src/cmd/go/internal/modload/load.go b/src/cmd/go/internal/modload/load.go
index 0c9006e04045ec4bdbfc8e886445b6860dfa33f1..b13c41aaefc5380e72767a5418e43979f2f906fb 100644 (file)
--- a/src/cmd/go/internal/modload/load.go
+++ b/src/cmd/go/internal/modload/load.go
@@ -890,7 +890,6 @@ func loadFromRoots(ctx context.Context, params loaderParams) *loader {
                work:         par.NewQueue(runtime.GOMAXPROCS(0)),
        }
 
-       addedModuleFor := make(map[string]bool)
        for {
                ld.reset()
 
@@ -931,7 +930,7 @@ func loadFromRoots(ctx context.Context, params loaderParams) *loader {
                        // We've loaded as much as we can without resolving missing imports.
                        break
                }
-               modAddedBy := ld.resolveMissingImports(ctx, addedModuleFor)
+               modAddedBy := ld.resolveMissingImports(ctx)
                if len(modAddedBy) == 0 {
                        break
                }
@@ -1057,7 +1056,7 @@ func (ld *loader) updateRequirements(ctx context.Context, add []module.Version)
 // The newly-resolved packages are added to the addedModuleFor map, and
 // resolveMissingImports returns a map from each new module version to
 // the first missing package that module would resolve.
-func (ld *loader) resolveMissingImports(ctx context.Context, addedModuleFor map[string]bool) (modAddedBy map[module.Version]*loadPkg) {
+func (ld *loader) resolveMissingImports(ctx context.Context) (modAddedBy map[module.Version]*loadPkg) {
        var needPkgs []*loadPkg
        for _, pkg := range ld.pkgs {
                if pkg.err == nil {
@@ -1089,11 +1088,6 @@ func (ld *loader) resolveMissingImports(ctx context.Context, addedModuleFor map[
                }
 
                fmt.Fprintf(os.Stderr, "go: found %s in %s %s\n", pkg.path, pkg.mod.Path, pkg.mod.Version)
-               if addedModuleFor[pkg.path] {
-                       // TODO(bcmills): This should only be an error if pkg.mod is the same
-                       // version we already tried to add previously.
-                       base.Fatalf("go: %s: looping trying to add package", pkg.stackText())
-               }
                if modAddedBy[pkg.mod] == nil {
                        modAddedBy[pkg.mod] = pkg
                }