cmd/go/internal/modload: use (*loadPkg).mod only to indicate the module from which...

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 <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
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 b13c41aaefc5380e72767a5418e43979f2f906fb..8cbb768341c2728a8dea9cd5849f6f3dee7dd6f6 100644 (file)
--- 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
                }
        }