cmd/go/internal/mvs: fix Downgrade to match Algorithm 4

mvs.Downgrade is pretty clearly intended to match Algorithm 4 from the
MVS blog post (https://research.swtch.com/vgo-mvs#algorithm_4).

Per the blog post:
“Downgrading one module may require downgrading other modules, but we
want to downgrade as few other modules as possible. … 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.”

mvs.Downgrade does not match that behavior today: it fails to retain
the selected versions of transitive dependencies that are not implied
by downgraded direct dependencies of the target (module E in the
post). This bug is currently masked by the fact that we only call
Downgrade today with a *modload.mvsReqs, for which the Required method
happens to return the complete build list — rather than only the
direct dependencies as documented for the mvs.Reqs interface.

For #36460

Change-Id: If9c8f413b156b5f67c02787d9359394e169951b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/287633
Trust: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

diff --git a/src/cmd/go/internal/mvs/mvs.go b/src/cmd/go/internal/mvs/mvs.go
index f016d8ff15d96ddcddb91b235105b2645ddb4d29..bed4d5c1ba53ada25a7cac1197a046b4a94b448c 100644 (file)
--- a/src/cmd/go/internal/mvs/mvs.go
+++ b/src/cmd/go/internal/mvs/mvs.go
@@ -375,10 +375,19 @@ func Upgrade(target module.Version, reqs Reqs, upgrade ...module.Version) ([]mod
 // reqs.Previous, but the methods of reqs must otherwise handle such versions
 // correctly.
 func Downgrade(target module.Version, reqs Reqs, downgrade ...module.Version) ([]module.Version, error) {
-       list, err := reqs.Required(target)
+       // 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 to identify versions
+       // for every module in the build list — not just reqs.Required(target).
+       list, err := BuildList(target, reqs)
        if err != nil {
                return nil, err
        }
+       list = list[1:] // remove target
+
        max := make(map[string]string)
        for _, r := range list {
                max[r.Path] = r.Version
@@ -411,6 +420,9 @@ func Downgrade(target module.Version, reqs Reqs, downgrade ...module.Version) ([
                }
                added[m] = true
                if v, ok := max[m.Path]; ok && reqs.Max(m.Version, v) != v {
+                       // m would upgrade an existing dependency — it is not a strict downgrade,
+                       // and because it was already present as a dependency, it could affect the
+                       // behavior of other relevant packages.
                        exclude(m)
                        return
                }
@@ -427,6 +439,7 @@ func Downgrade(target module.Version, reqs Reqs, downgrade ...module.Version) ([
                        // is transient (we couldn't download go.mod), return the error from
                        // Downgrade. Currently, we can't tell what kind of error it is.
                        exclude(m)
+                       return
                }
                for _, r := range list {
                        add(r)
@@ -438,8 +451,8 @@ func Downgrade(target module.Version, reqs Reqs, downgrade ...module.Version) ([
                }
        }
 
-       var out []module.Version
-       out = append(out, target)
+       downgraded := make([]module.Version, 0, len(list)+1)
+       downgraded = append(downgraded, target)
 List:
        for _, r := range list {
                add(r)
@@ -466,10 +479,14 @@ List:
                        add(p)
                        r = p
                }
-               out = append(out, r)
+               downgraded = append(downgraded, r)
        }
 
-       return out, nil
+       return BuildList(target, &override{
+               target: target,
+               list:   downgraded,
+               Reqs:   reqs,
+       })
 }
 
 type override struct {

diff --git a/src/cmd/go/internal/mvs/mvs_test.go b/src/cmd/go/internal/mvs/mvs_test.go
index b8ff3bd8c292814adb06bbea86b64829b580b883..742e396e0dc95e86e97e230e8b13d79c9f75029a 100644 (file)
--- a/src/cmd/go/internal/mvs/mvs_test.go
+++ b/src/cmd/go/internal/mvs/mvs_test.go
@@ -32,8 +32,7 @@ build A:       A B1 C2 D4 E2 F1
 upgrade* A:    A B1 C4 D5 E2 F1 G1
 upgrade A C4:  A B1 C4 D4 E2 F1 G1
 build A2:     A2 B1 C4 D4 E2 F1 G1
-# BUG: selected versions E2 and F1 are not preserved.
-downgrade A2 D2: A2 C4 D2
+downgrade A2 D2: A2 C4 D2 E2 F1 G1
 
 name: trim
 A: B1 C2
@@ -216,8 +215,7 @@ A: B2
 B1: C1
 B2: C2
 build A:        A B2 C2
-# BUG: build list from downgrade omits selected version C1.
-downgrade A C1: A B1
+downgrade A C1: A B1 C1
 
 name: down2
 A: B2 E2
@@ -231,9 +229,7 @@ E2: D2
 E1:
 F1:
 build A:        A B2 C2 D2 E2 F2
-# BUG: selected versions C1 and D1 are not preserved, and
-# requested version F1 is not selected.
-downgrade A F1: A B1 E1
+downgrade A F1: A B1 C1 D1 E1 F1
 
 # https://research.swtch.com/vgo-mvs#algorithm_4:
 # “[D]owngrades are constrained to only downgrade packages, not also upgrade
@@ -252,8 +248,7 @@ C2:
 D1:
 D2:
 build A:        A B2 C1 D2
-# BUG: requested version D1 is not selected.
-downgrade A D1: A
+downgrade A D1: A       D1
 
 # https://research.swtch.com/vgo-mvs#algorithm_4:
 # “Unlike upgrades, downgrades must work by removing requirements, not adding
@@ -270,10 +265,8 @@ B2: D2
 C1:
 D1:
 D2:
-build A:        A B2 D2
-# BUG: requested version D1 is not selected,
-# and selected version C1 is omitted from the returned build list.
-downgrade A D1: A B1
+build A:        A B2    D2
+downgrade A D1: A B1 C1 D1
 
 name: downcycle
 A: A B2

diff --git a/src/cmd/go/testdata/script/mod_load_badchain.txt b/src/cmd/go/testdata/script/mod_load_badchain.txt
index c0c382bfa638b0fcb71473c560704179e7726795..32d9fb24d112c206f6be607080615c5c3a17ec84 100644 (file)
--- a/src/cmd/go/testdata/script/mod_load_badchain.txt
+++ b/src/cmd/go/testdata/script/mod_load_badchain.txt
@@ -74,8 +74,7 @@ go get: example.com/badchain/c@v1.0.0 updating to
        module declares its path as: badchain.example.com/c
                but was required as: example.com/badchain/c
 -- update-a-expected --
-go get: example.com/badchain/a@v1.0.0 updating to
-       example.com/badchain/a@v1.1.0 requires
+go get: example.com/badchain/a@v1.1.0 requires
        example.com/badchain/b@v1.1.0 requires
        example.com/badchain/c@v1.1.0: parsing go.mod:
        module declares its path as: badchain.example.com/c