From 768a39975d8851f1c309b163a8eb4b7a5388aa24 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Fri, 23 Apr 2021 01:13:50 -0400 Subject: [PATCH] cmd/go/internal/modload: remove the addedModuleFor map MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Run-TryBot: Bryan C. Mills TryBot-Result: Go Bot Reviewed-by: Michael Matloob --- src/cmd/go/internal/modload/load.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/cmd/go/internal/modload/load.go b/src/cmd/go/internal/modload/load.go index 0c9006e040..b13c41aaef 100644 --- 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 } -- 2.48.1