Skip to content

Commit a53dc4c

Browse files
author
Bryan C. Mills
committed
cmd/go/internal/modload: use (*loadPkg).mod only to indicate the module from which the package was loaded
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 golang#36460 Change-Id: Ic434df0f38185c6e9e892c5e9ba9ff53b3efe01f Reviewed-on: https://go-review.googlesource.com/c/go/+/312930 Trust: Bryan C. Mills <[email protected]> Run-TryBot: Bryan C. Mills <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
1 parent 0d1280c commit a53dc4c

1 file changed

Lines changed: 36 additions & 9 deletions

File tree

‎src/cmd/go/internal/modload/load.go‎

Lines changed: 36 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1057,7 +1057,11 @@ func (ld *loader) updateRequirements(ctx context.Context, add []module.Version)
10571057
// resolveMissingImports returns a map from each new module version to
10581058
// the first missing package that module would resolve.
10591059
func (ld *loader) resolveMissingImports(ctx context.Context) (modAddedBy map[module.Version]*loadPkg) {
1060-
var needPkgs []*loadPkg
1060+
type pkgMod struct {
1061+
pkg *loadPkg
1062+
mod *module.Version
1063+
}
1064+
var pkgMods []pkgMod
10611065
for _, pkg := range ld.pkgs {
10621066
if pkg.err == nil {
10631067
continue
@@ -1072,24 +1076,47 @@ func (ld *loader) resolveMissingImports(ctx context.Context) (modAddedBy map[mod
10721076
continue
10731077
}
10741078

1075-
needPkgs = append(needPkgs, pkg)
1076-
10771079
pkg := pkg
1080+
var mod module.Version
10781081
ld.work.Add(func() {
1079-
pkg.mod, pkg.err = queryImport(ctx, pkg.path, ld.requirements)
1082+
var err error
1083+
mod, err = queryImport(ctx, pkg.path, ld.requirements)
1084+
if err != nil {
1085+
// pkg.err was already non-nil, so we can reasonably attribute the error
1086+
// for pkg to either the original error or the one returned by
1087+
// queryImport. The existing error indicates only that we couldn't find
1088+
// the package, whereas the query error also explains why we didn't fix
1089+
// the problem — so we prefer the latter.
1090+
pkg.err = err
1091+
}
1092+
1093+
// err is nil, but we intentionally leave pkg.err non-nil and pkg.mod
1094+
// unset: we still haven't satisfied other invariants of a
1095+
// successfully-loaded package, such as scanning and loading the imports
1096+
// of that package. If we succeed in resolving the new dependency graph,
1097+
// the caller can reload pkg and update the error at that point.
1098+
//
1099+
// Even then, the package might not be loaded from the version we've
1100+
// identified here. The module may be upgraded by some other dependency,
1101+
// or by a transitive dependency of mod itself, or — less likely — the
1102+
// package may be rejected by an AllowPackage hook or rendered ambiguous
1103+
// by some other newly-added or newly-upgraded dependency.
10801104
})
1105+
1106+
pkgMods = append(pkgMods, pkgMod{pkg: pkg, mod: &mod})
10811107
}
10821108
<-ld.work.Idle()
10831109

10841110
modAddedBy = map[module.Version]*loadPkg{}
1085-
for _, pkg := range needPkgs {
1086-
if pkg.err != nil {
1111+
for _, pm := range pkgMods {
1112+
pkg, mod := pm.pkg, *pm.mod
1113+
if mod.Path == "" {
10871114
continue
10881115
}
10891116

1090-
fmt.Fprintf(os.Stderr, "go: found %s in %s %s\n", pkg.path, pkg.mod.Path, pkg.mod.Version)
1091-
if modAddedBy[pkg.mod] == nil {
1092-
modAddedBy[pkg.mod] = pkg
1117+
fmt.Fprintf(os.Stderr, "go: found %s in %s %s\n", pkg.path, mod.Path, mod.Version)
1118+
if modAddedBy[mod] == nil {
1119+
modAddedBy[mod] = pkg
10931120
}
10941121
}
10951122

0 commit comments

Comments
 (0)