From 1a5665533be3641511b72dac5b91f1c7500e40b5 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 25 Mar 2021 23:26:56 -0400 Subject: [PATCH] cmd/go/internal/modload: migrate editBuildList to use a structured requirement graph For #36460 Change-Id: Ic87d7e25402bb938d2872d33d26c4bf397776d1b Reviewed-on: https://go-review.googlesource.com/c/go/+/308517 Trust: Bryan C. Mills Run-TryBot: Bryan C. Mills TryBot-Result: Go Bot Reviewed-by: Michael Matloob Reviewed-by: Jay Conrod --- src/cmd/go/internal/modload/buildlist.go | 66 --- src/cmd/go/internal/modload/edit.go | 450 ++++++++++++++---- .../go/testdata/script/mod_load_badchain.txt | 3 +- 3 files changed, 358 insertions(+), 161 deletions(-) diff --git a/src/cmd/go/internal/modload/buildlist.go b/src/cmd/go/internal/modload/buildlist.go index 2e79e85127..a833dbee62 100644 --- a/src/cmd/go/internal/modload/buildlist.go +++ b/src/cmd/go/internal/modload/buildlist.go @@ -12,7 +12,6 @@ import ( "context" "fmt" "os" - "reflect" "runtime" "strings" "sync" @@ -455,71 +454,6 @@ func EditBuildList(ctx context.Context, add, mustSelect []module.Version) (chang return changed, err } -func editRequirements(ctx context.Context, rs *Requirements, add, mustSelect []module.Version) (edited *Requirements, changed bool, err error) { - mg, err := rs.Graph(ctx) - if err != nil { - return nil, false, err - } - buildList := mg.BuildList() - - final, err := editBuildList(ctx, buildList, add, mustSelect) - if err != nil { - return nil, false, err - } - - if !reflect.DeepEqual(final, buildList) { - changed = true - } else if len(mustSelect) == 0 { - // No change to the build list and no explicit roots to promote, so we're done. - return rs, false, nil - } - - var rootPaths []string - for _, m := range mustSelect { - if m.Version != "none" && m.Path != Target.Path { - rootPaths = append(rootPaths, m.Path) - } - } - for _, m := range final[1:] { - if v, ok := rs.rootSelected(m.Path); ok && (v == m.Version || rs.direct[m.Path]) { - // m.Path was formerly a root, and either its version hasn't changed or - // we believe that it provides a package directly imported by a package - // or test in the main module. For now we'll assume that it is still - // relevant. If we actually load all of the packages and tests in the - // main module (which we are not doing here), we can revise the explicit - // roots at that point. - rootPaths = append(rootPaths, m.Path) - } - } - - if go117LazyTODO { - // mvs.Req is not lazy, and in a lazily-loaded module we don't want - // to minimize the roots anyway. (Instead, we want to retain explicit - // root paths so that they remain explicit: only 'go mod tidy' should - // remove roots.) - } - - min, err := mvs.Req(Target, rootPaths, &mvsReqs{roots: final[1:]}) - if err != nil { - return nil, false, err - } - - // A module that is not even in the build list necessarily cannot provide - // any imported packages. Mark as direct only the direct modules that are - // still in the build list. - // - // TODO(bcmills): Would it make more sense to leave the direct map as-is - // but allow it to refer to modules that are no longer in the build list? - // That might complicate updateRoots, but it may be cleaner in other ways. - direct := make(map[string]bool, len(rs.direct)) - for _, m := range final { - if rs.direct[m.Path] { - direct[m.Path] = true - } - } - return newRequirements(rs.depth, min, direct), changed, nil -} - // A ConstraintError describes inconsistent constraints in EditBuildList type ConstraintError struct { // Conflict lists the source of the conflict for each version in mustSelect diff --git a/src/cmd/go/internal/modload/edit.go b/src/cmd/go/internal/modload/edit.go index 858fec5dd5..2921b38157 100644 --- a/src/cmd/go/internal/modload/edit.go +++ b/src/cmd/go/internal/modload/edit.go @@ -5,100 +5,321 @@ package modload import ( - "context" - "sort" - "cmd/go/internal/mvs" + "context" + "reflect" + "sort" "golang.org/x/mod/module" "golang.org/x/mod/semver" ) -// editBuildList returns an edited version of initial such that: +// editRequirements returns an edited version of rs such that: // // 1. Each module version in mustSelect is selected. // // 2. Each module version in tryUpgrade is upgraded toward the indicated // version as far as can be done without violating (1). // -// 3. Each module version in initial is downgraded from its original version -// only to the extent needed to satisfy (1), or upgraded only to the extent -// needed to satisfy (1) and (2). +// 3. Each module version in rs.rootModules (or rs.graph, if rs.depth is eager) +// is downgraded from its original version only to the extent needed to +// satisfy (1), or upgraded only to the extent needed to satisfy (1) and +// (2). // // 4. No module is upgraded above the maximum version of its path found in the -// combined dependency graph of list, tryUpgrade, and mustSelect. -func editBuildList(ctx context.Context, initial, tryUpgrade, mustSelect []module.Version) ([]module.Version, error) { - // Per https://research.swtch.com/vgo-mvs#algorithm_4: - // “To avoid an unnecessary downgrade to E 1.1, we must also add a new - // requirement on E 1.2. We can apply Algorithm R to find the minimal set of - // new requirements to write to go.mod.” - // - // In order to generate those new requirements, we need consider versions for - // every module in the existing build list, plus every module being directly - // added by the edit. However, modules added only as dependencies of tentative - // versions should not be retained if they end up being upgraded or downgraded - // away due to versions in mustSelect. - - // When we downgrade modules in order to reach mustSelect, we don't want to - // upgrade any existing module above the version that would be selected if we - // just added all of the new requirements and *didn't* downgrade. - // - // So we'll do exactly that: just add all of the new requirements and not - // downgrade, and return the resulting versions as an upper bound. This - // intentionally limits our solution space so that edits that the user - // percieves as “downgrades” will not also result in upgrades. - max := make(map[string]string) - maxes, err := mvs.Upgrade(Target, &mvsReqs{ - roots: append(capVersionSlice(initial[1:]), mustSelect...), - }, tryUpgrade...) +// dependency graph of rs, the combined dependency graph of the versions in +// mustSelect, or the dependencies of each individual module version in +// tryUpgrade. +// +// Generally, the module versions in mustSelect are due to the module or a +// package within the module matching an explicit command line argument to 'go +// get', and the versions in tryUpgrade are transitive dependencies that are +// either being upgraded by 'go get -u' or being added to satisfy some +// otherwise-missing package import. +func editRequirements(ctx context.Context, rs *Requirements, tryUpgrade, mustSelect []module.Version) (edited *Requirements, changed bool, err error) { + limiter, err := limiterForEdit(ctx, rs, tryUpgrade, mustSelect) if err != nil { - return nil, err + return rs, false, err } - for _, m := range maxes { - max[m.Path] = m.Version - } - // The versions in mustSelect override whatever we would naively select — - // we will downgrade other modules as needed in order to meet them. - for _, m := range mustSelect { - max[m.Path] = m.Version - } - - limiter := newVersionLimiter(max) var conflicts []Conflict for _, m := range mustSelect { - dq := limiter.check(m) - switch { - case dq.err != nil: - return nil, err - case dq.conflict != module.Version{}: + conflict, err := limiter.Select(m) + if err != nil { + return rs, false, err + } + if conflict.Path != "" { conflicts = append(conflicts, Conflict{ Source: m, - Dep: dq.conflict, + Dep: conflict, Constraint: module.Version{ - Path: dq.conflict.Path, - Version: limiter.max[dq.conflict.Path], + Path: conflict.Path, + Version: limiter.max[conflict.Path], }, }) } - limiter.selected[m.Path] = m.Version } if len(conflicts) > 0 { - return nil, &ConstraintError{Conflicts: conflicts} + return rs, false, &ConstraintError{Conflicts: conflicts} } - // For each module, we want to get as close as we can to either the upgrade - // version or the previously-selected version in the build list, whichever is - // higher. We can compute those in either order, but the upgrades will tend to - // be higher than the build list, so we arbitrarily start with those. - for _, m := range tryUpgrade { - if err := limiter.upgradeToward(ctx, m); err != nil { - return nil, err + mods, changed, err := selectPotentiallyImportedModules(ctx, limiter, rs, tryUpgrade) + if err != nil { + return rs, false, err + } + + var roots []module.Version + if rs.depth == eager { + // In an eager module, modules that provide packages imported by the main + // module may either be explicit roots or implicit transitive dependencies. + // We promote the modules in mustSelect to be explicit requirements. + var rootPaths []string + for _, m := range mustSelect { + if m.Version != "none" && m.Path != Target.Path { + rootPaths = append(rootPaths, m.Path) + } + } + if !changed && len(rootPaths) == 0 { + // The build list hasn't changed and we have no new roots to add. + // We don't need to recompute the minimal roots for the module. + return rs, false, nil + } + + for _, m := range mods { + if v, ok := rs.rootSelected(m.Path); ok && (v == m.Version || rs.direct[m.Path]) { + // m.Path was formerly a root, and either its version hasn't changed or + // we believe that it provides a package directly imported by a package + // or test in the main module. For now we'll assume that it is still + // relevant enough to remain a root. If we actually load all of the + // packages and tests in the main module (which we are not doing here), + // we can revise the explicit roots at that point. + rootPaths = append(rootPaths, m.Path) + } + } + + roots, err = mvs.Req(Target, rootPaths, &mvsReqs{roots: mods}) + if err != nil { + return nil, false, err + } + } else { + // In a lazy module, every module that provides a package imported by the + // main module must be retained as a root. + roots = mods + if !changed { + // Because the roots we just computed are unchanged, the entire graph must + // be the same as it was before. Save the original rs, since we have + // probably already loaded its requirement graph. + return rs, false, nil } } + + // A module that is not even in the build list necessarily cannot provide + // any imported packages. Mark as direct only the direct modules that are + // still in the build list. + // + // TODO(bcmills): Would it make more sense to leave the direct map as-is + // but allow it to refer to modules that are no longer in the build list? + // That might complicate updateRoots, but it may be cleaner in other ways. + direct := make(map[string]bool, len(rs.direct)) + for _, m := range roots { + if rs.direct[m.Path] { + direct[m.Path] = true + } + } + return newRequirements(rs.depth, roots, direct), changed, nil +} + +// limiterForEdit returns a versionLimiter with its max versions set such that +// the max version for every module path in mustSelect is the version listed +// there, and the max version for every other module path is the maximum version +// of its path found in the dependency graph of rs, the combined dependency +// graph of the versions in mustSelect, or the dependencies of each individual +// module version in tryUpgrade. +func limiterForEdit(ctx context.Context, rs *Requirements, tryUpgrade, mustSelect []module.Version) (*versionLimiter, error) { + mg, err := rs.Graph(ctx) + if err != nil { + return nil, err + } + + maxVersion := map[string]string{} // module path → version + restrictTo := func(m module.Version) { + v, ok := maxVersion[m.Path] + if !ok || cmpVersion(v, m.Version) > 0 { + maxVersion[m.Path] = m.Version + } + } + + if rs.depth == eager { + // Eager go.mod files don't indicate which transitive dependencies are + // actually relevant to the main module, so we have to assume that any module + // that could have provided any package — that is, any module whose selected + // version was not "none" — may be relevant. + for _, m := range mg.BuildList() { + restrictTo(m) + } + } else { + // The go.mod file explicitly records every module that provides a package + // imported by the main module. + // + // If we need to downgrade an existing root or a new root found in + // tryUpgrade, we don't want to allow that downgrade to incidentally upgrade + // a module imported by the main module to some arbitrary version. + // However, we don't particularly care about arbitrary upgrades to modules + // that are (at best) only providing packages imported by tests of + // dependencies outside the main module. + for _, m := range rs.rootModules { + restrictTo(module.Version{ + Path: m.Path, + Version: mg.Selected(m.Path), + }) + } + } + + if err := raiseLimitsForUpgrades(ctx, maxVersion, rs.depth, tryUpgrade, mustSelect); err != nil { + return nil, err + } + + // The versions in mustSelect override whatever we would naively select — + // we will downgrade other modules as needed in order to meet them. + for _, m := range mustSelect { + restrictTo(m) + } + + return newVersionLimiter(rs.depth, maxVersion), nil +} + +// raiseLimitsForUpgrades increases the module versions in maxVersions to the +// versions that would be needed to allow each of the modules in tryUpgrade +// (individually) and all of the modules in mustSelect (simultaneously) to be +// added as roots. +// +// Versions not present in maxVersion are unrestricted, and it is assumed that +// they will not be promoted to root requirements (and thus will not contribute +// their own dependencies if the main module is lazy). +// +// These limits provide an upper bound on how far a module may be upgraded as +// part of an incidental downgrade, if downgrades are needed in order to select +// the versions in mustSelect. +func raiseLimitsForUpgrades(ctx context.Context, maxVersion map[string]string, depth modDepth, tryUpgrade []module.Version, mustSelect []module.Version) error { + // allow raises the limit for m.Path to at least m.Version. + // If m.Path was already unrestricted, it remains unrestricted. + allow := func(m module.Version) { + v, ok := maxVersion[m.Path] + if !ok { + return // m.Path is unrestricted. + } + if cmpVersion(v, m.Version) < 0 { + maxVersion[m.Path] = m.Version + } + } + + var eagerUpgrades []module.Version + if depth == eager { + eagerUpgrades = tryUpgrade + } else { + for _, m := range tryUpgrade { + if m.Path == Target.Path { + // Target is already considered to be higher than any possible m, so we + // won't be upgrading to it anyway and there is no point scanning its + // dependencies. + continue + } + + summary, err := goModSummary(m) + if err != nil { + return err + } + if summary.depth() == eager { + // For efficiency, we'll load all of the eager upgrades as one big + // graph, rather than loading the (potentially-overlapping) subgraph for + // each upgrade individually. + eagerUpgrades = append(eagerUpgrades, m) + continue + } + + for _, r := range summary.require { + allow(r) + } + } + } + + if len(eagerUpgrades) > 0 { + // Compute the max versions for eager upgrades all together. + // Since these modules are eager, we'll end up scanning all of their + // transitive dependencies no matter which versions end up selected, + // and since we have a large dependency graph to scan we might get + // a significant benefit from not revisiting dependencies that are at + // common versions among multiple upgrades. + upgradeGraph, err := readModGraph(ctx, eager, eagerUpgrades) + if err != nil { + if go117LazyTODO { + // Compute the requirement path from a module path in tryUpgrade to the + // error, and the requirement path (if any) from rs.rootModules to the + // tryUpgrade module path. Return a *mvs.BuildListError showing the + // concatenation of the paths (with an upgrade in the middle). + } + return err + } + + for _, r := range upgradeGraph.BuildList() { + // Upgrading to m would upgrade to r, and the caller requested that we + // try to upgrade to m, so it's ok to upgrade to r. + allow(r) + } + } + + if len(mustSelect) > 0 { + mustGraph, err := readModGraph(ctx, depth, mustSelect) + if err != nil { + return err + } + + for _, r := range mustGraph.BuildList() { + // Some module in mustSelect requires r, so we must allow at least r.Version + // unless it conflicts with an entry in mustSelect. + allow(r) + } + } + + return nil +} + +// selectPotentiallyImportedModules increases the limiter-selected version of +// every module in rs that potentially provides a package imported (directly or +// indirectly) by the main module, and every module in tryUpgrade, toward the +// highest version seen in rs or tryUpgrade, but not above the maximums enforced +// by the limiter. +// +// It returns the list of module versions selected by the limiter, sorted by +// path, along with a boolean indicating whether that list is different from the +// list of modules read from rs. +func selectPotentiallyImportedModules(ctx context.Context, limiter *versionLimiter, rs *Requirements, tryUpgrade []module.Version) (mods []module.Version, changed bool, err error) { + for _, m := range tryUpgrade { + if err := limiter.UpgradeToward(ctx, m); err != nil { + return nil, false, err + } + } + + var initial []module.Version + if rs.depth == eager { + mg, err := rs.Graph(ctx) + if err != nil { + return nil, false, err + } + initial = mg.BuildList()[1:] + } else { + initial = rs.rootModules + } for _, m := range initial { - if err := limiter.upgradeToward(ctx, m); err != nil { - return nil, err + if err := limiter.UpgradeToward(ctx, m); err != nil { + return nil, false, err + } + } + + mods = make([]module.Version, 0, len(limiter.selected)) + for path, v := range limiter.selected { + if v != "none" && path != Target.Path { + mods = append(mods, module.Version{Path: path, Version: v}) } } @@ -107,46 +328,46 @@ func editBuildList(ctx context.Context, initial, tryUpgrade, mustSelect []module // downgraded module may require a higher (but still allowed) version of // another. The lower version may require extraneous dependencies that aren't // actually relevant, so we need to compute the actual selected versions. - adjusted := make([]module.Version, 0, len(maxes)) - for _, m := range maxes[1:] { - if v, ok := limiter.selected[m.Path]; ok { - adjusted = append(adjusted, module.Version{Path: m.Path, Version: v}) - } - } - consistent, err := mvs.BuildList(Target, &mvsReqs{roots: adjusted}) + mg, err := readModGraph(ctx, rs.depth, mods) if err != nil { - return nil, err + return nil, false, err } - - // We have the correct selected versions. Now we need to re-run MVS with only - // the actually-selected versions in order to eliminate extraneous - // dependencies from lower-than-selected ones. - compacted := consistent[:0] - for _, m := range consistent[1:] { - if _, ok := limiter.selected[m.Path]; ok { - // The fact that the limiter has a version for m.Path indicates that we - // care about retaining that path, even if the version was upgraded for - // consistency. - compacted = append(compacted, m) + mods = make([]module.Version, 0, len(limiter.selected)) + for path, _ := range limiter.selected { + if path != Target.Path { + if v := mg.Selected(path); v != "none" { + mods = append(mods, module.Version{Path: path, Version: v}) + } } } + module.Sort(mods) - return mvs.BuildList(Target, &mvsReqs{roots: compacted}) + changed = !reflect.DeepEqual(mods, initial) + + return mods, changed, err } // A versionLimiter tracks the versions that may be selected for each module // subject to constraints on the maximum versions of transitive dependencies. type versionLimiter struct { + // depth is the depth at which the dependencies of the modules passed to + // Select and UpgradeToward are loaded. + depth modDepth + // max maps each module path to the maximum version that may be selected for - // that path. Paths with no entry are unrestricted. + // that path. + // + // Paths with no entry are unrestricted, and we assume that they will not be + // promoted to root dependencies (so will not contribute dependencies if the + // main module is lazy). max map[string]string // selected maps each module path to a version of that path (if known) whose // transitive dependencies do not violate any max version. The version kept - // is the highest one found during any call to upgradeToward for the given + // is the highest one found during any call to UpgradeToward for the given // module path. // - // If a higher acceptable version is found during a call to upgradeToward for + // If a higher acceptable version is found during a call to UpgradeToward for // some *other* module path, that does not update the selected version. // Ignoring those versions keeps the downgrades computed for two modules // together close to the individual downgrades that would be computed for each @@ -183,18 +404,32 @@ func (dq dqState) isDisqualified() bool { return dq != dqState{} } -func newVersionLimiter(max map[string]string) *versionLimiter { +// newVersionLimiter returns a versionLimiter that restricts the module paths +// that appear as keys in max. +// +// max maps each module path to its maximum version; paths that are not present +// in the map are unrestricted. The limiter assumes that unrestricted paths will +// not be promoted to root dependencies. +// +// If depth is lazy, then if a module passed to UpgradeToward or Select is +// itself lazy, its unrestricted dependencies are skipped when scanning +// requirements. +func newVersionLimiter(depth modDepth, max map[string]string) *versionLimiter { return &versionLimiter{ - selected: map[string]string{Target.Path: Target.Version}, + depth: depth, max: max, + selected: map[string]string{Target.Path: Target.Version}, dqReason: map[module.Version]dqState{}, requiring: map[module.Version][]module.Version{}, } } -// upgradeToward attempts to upgrade the selected version of m.Path as close as +// UpgradeToward attempts to upgrade the selected version of m.Path as close as // possible to m.Version without violating l's maximum version limits. -func (l *versionLimiter) upgradeToward(ctx context.Context, m module.Version) error { +// +// If depth is lazy and m itself is lazy, the the dependencies of unrestricted +// dependencies of m will not be followed. +func (l *versionLimiter) UpgradeToward(ctx context.Context, m module.Version) error { selected, ok := l.selected[m.Path] if ok { if cmpVersion(selected, m.Version) >= 0 { @@ -205,7 +440,7 @@ func (l *versionLimiter) upgradeToward(ctx context.Context, m module.Version) er selected = "none" } - if l.check(m).isDisqualified() { + if l.check(m, l.depth).isDisqualified() { candidates, err := versions(ctx, m.Path, CheckAllowed) if err != nil { // This is likely a transient error reaching the repository, @@ -222,7 +457,7 @@ func (l *versionLimiter) upgradeToward(ctx context.Context, m module.Version) er }) candidates = candidates[:i] - for l.check(m).isDisqualified() { + for l.check(m, l.depth).isDisqualified() { n := len(candidates) if n == 0 || cmpVersion(selected, candidates[n-1]) >= 0 { // We couldn't find a suitable candidate above the already-selected version. @@ -237,9 +472,26 @@ func (l *versionLimiter) upgradeToward(ctx context.Context, m module.Version) er return nil } +// Select attempts to set the selected version of m.Path to exactly m.Version. +func (l *versionLimiter) Select(m module.Version) (conflict module.Version, err error) { + dq := l.check(m, l.depth) + if !dq.isDisqualified() { + l.selected[m.Path] = m.Version + } + return dq.conflict, dq.err +} + // check determines whether m (or its transitive dependencies) would violate l's // maximum version limits if added to the module requirement graph. -func (l *versionLimiter) check(m module.Version) dqState { +// +// If depth is lazy and m itself is lazy, then the dependencies of unrestricted +// dependencies of m will not be followed. If the lazy loading invariants hold +// for the main module up to this point, the packages in those modules are at +// best only imported by tests of dependencies that are themselves loaded from +// outside modules. Although we would like to keep 'go test all' as reproducible +// as is feasible, we don't want to retain test dependencies that are only +// marginally relevant at best. +func (l *versionLimiter) check(m module.Version, depth modDepth) dqState { if m.Version == "none" || m == Target { // version "none" has no requirements, and the dependencies of Target are // tautological. @@ -270,8 +522,20 @@ func (l *versionLimiter) check(m module.Version) dqState { return l.disqualify(m, dqState{err: err}) } + if summary.depth() == eager { + depth = eager + } for _, r := range summary.require { - if dq := l.check(r); dq.isDisqualified() { + if depth == lazy { + if _, restricted := l.max[r.Path]; !restricted { + // r.Path is unrestricted, so we don't care at what version it is + // selected. We assume that r.Path will not become a root dependency, so + // since m is lazy, r's dependencies won't be followed. + continue + } + } + + if dq := l.check(r, depth); dq.isDisqualified() { return l.disqualify(m, dq) } diff --git a/src/cmd/go/testdata/script/mod_load_badchain.txt b/src/cmd/go/testdata/script/mod_load_badchain.txt index 9e28b2d9a4..8cb7eec0aa 100644 --- a/src/cmd/go/testdata/script/mod_load_badchain.txt +++ b/src/cmd/go/testdata/script/mod_load_badchain.txt @@ -69,8 +69,7 @@ import ( func Test(t *testing.T) {} -- update-main-expected -- -go get: example.com/badchain/c@v1.0.0 updating to - example.com/badchain/c@v1.1.0: parsing go.mod: +go get: example.com/badchain/c@v1.1.0: parsing go.mod: module declares its path as: badchain.example.com/c but was required as: example.com/badchain/c -- update-a-expected --