From a53dc4c1ce0e21da328bd5984900448bab354ba1 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Fri, 23 Apr 2021 01:49:00 -0400 Subject: [PATCH] cmd/go/internal/modload: use (*loadPkg).mod only to indicate the module from which the package was loaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The (*loadPkg).mod field normally indicates the module from which the package was loaded. However, if the package was missing, we previously used the mod field to instead store the module from which we intend to load the package next time around. That sort of dual use makes the semantics (and synchronization) of the mod field much more complex to reason about. For example, it would be nice to have the invariant that the mod field is always one of the modules in the overall build list, or one of the modules selected in the overall module graph. Similarly, it would be nice to have the invariant that the version indicated by the mod field can coexist with (without upgrading) all of the other versions indicated in the mod fields of other packages. This repurposing of the mod field appears to be solely in the service of storing the module when resolving missing imports. To keep conceptually-separate fields separate, I have changed resolveMissingImports to store a slice of package–module pairs, instead of just packages that need to be revisited. This may increase allocation pressure slightly if we have many unresolved packages, but most packages are not unresolved, and it seems worth the cost to use a little extra memory if it means we can reason more clearly about the (quite complex) behaviors of the module loader. For #36460 Change-Id: Ic434df0f38185c6e9e892c5e9ba9ff53b3efe01f Reviewed-on: https://go-review.googlesource.com/c/go/+/312930 Trust: Bryan C. Mills Run-TryBot: Bryan C. Mills Reviewed-by: Michael Matloob --- src/cmd/go/internal/modload/load.go | 45 +++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/src/cmd/go/internal/modload/load.go b/src/cmd/go/internal/modload/load.go index b13c41aaefc..8cbb768341c 100644 --- a/src/cmd/go/internal/modload/load.go +++ b/src/cmd/go/internal/modload/load.go @@ -1057,7 +1057,11 @@ func (ld *loader) updateRequirements(ctx context.Context, add []module.Version) // 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) (modAddedBy map[module.Version]*loadPkg) { - var needPkgs []*loadPkg + type pkgMod struct { + pkg *loadPkg + mod *module.Version + } + var pkgMods []pkgMod for _, pkg := range ld.pkgs { if pkg.err == nil { continue @@ -1072,24 +1076,47 @@ func (ld *loader) resolveMissingImports(ctx context.Context) (modAddedBy map[mod continue } - needPkgs = append(needPkgs, pkg) - pkg := pkg + var mod module.Version ld.work.Add(func() { - pkg.mod, pkg.err = queryImport(ctx, pkg.path, ld.requirements) + var err error + mod, err = queryImport(ctx, pkg.path, ld.requirements) + if err != nil { + // pkg.err was already non-nil, so we can reasonably attribute the error + // for pkg to either the original error or the one returned by + // queryImport. The existing error indicates only that we couldn't find + // the package, whereas the query error also explains why we didn't fix + // the problem — so we prefer the latter. + pkg.err = err + } + + // err is nil, but we intentionally leave pkg.err non-nil and pkg.mod + // unset: we still haven't satisfied other invariants of a + // successfully-loaded package, such as scanning and loading the imports + // of that package. If we succeed in resolving the new dependency graph, + // the caller can reload pkg and update the error at that point. + // + // Even then, the package might not be loaded from the version we've + // identified here. The module may be upgraded by some other dependency, + // or by a transitive dependency of mod itself, or — less likely — the + // package may be rejected by an AllowPackage hook or rendered ambiguous + // by some other newly-added or newly-upgraded dependency. }) + + pkgMods = append(pkgMods, pkgMod{pkg: pkg, mod: &mod}) } <-ld.work.Idle() modAddedBy = map[module.Version]*loadPkg{} - for _, pkg := range needPkgs { - if pkg.err != nil { + for _, pm := range pkgMods { + pkg, mod := pm.pkg, *pm.mod + if mod.Path == "" { continue } - fmt.Fprintf(os.Stderr, "go: found %s in %s %s\n", pkg.path, pkg.mod.Path, pkg.mod.Version) - if modAddedBy[pkg.mod] == nil { - modAddedBy[pkg.mod] = pkg + fmt.Fprintf(os.Stderr, "go: found %s in %s %s\n", pkg.path, mod.Path, mod.Version) + if modAddedBy[mod] == nil { + modAddedBy[mod] = pkg } }