[bug#49602] [PATCH] import: go: Upgrade go.mod parser.

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2042 bytes --]

Hello Sarah,

Sarah Morgensen <iskarian@mgsn.dev> writes:

[...]

>>> +(define (go.mod-requirements go.mod)
>>> +  "Compute and return the list of requirements specified by GO.MOD."
>>> +  (define (replace directive requirements)
>>> +    (define (maybe-replace module-path new-requirement)
>>> +      ;; Do not allow version updates for indirect dependencies
>>> +      ;; TODO: Is this correct behavior? It's in the go.mod for a
>>> reason...
>>
>> According to [1], it seems that yes: "replace directives only apply in
>> the main module's go.mod file and are ignored in other modules.", IIUC.
>>
>> [1]  https://golang.org/ref/mod#go-mod-file-replace
>>
>
> My read of it is that if module A requires module B, replace directives
> in B's go.mod are ignored, but A's go.mod can replace any module, direct
> or indirect. For example, if module B hasn't been updated, but the
> version of module C it depends on has a bug that affects it, module A
> can use a replace in it's go.mod without requiring an upstream update of
> module B. To be fair, this is usually handled by specifying the indirect
> dependency with a specific version as a requirement in module A's
> go.mod, but the replace should be valid as well. 

Thank you for explaining.  It makes sense.  So it seems that we should
honor the replacement for any dependency.

> The reason it was skipped before, I think (if it was intentional), is
> that since we only have the one version of a module in Guix at a time,
> it's not necessary to make the indirect dependency explicitly an input,
> so don't include it.

Sounds plausible!

> On the other hand, if it *was* used to avoid a bug
> in a version used by an indirect dependency, wouldn't we want to make
> sure the Guix package was the fixed version? This is why I was
> questioning whether leaving it out was correct. 

Now that I have a better understanding (I think!), I'd like to propose
the attached patch; it should make the replacement logic more in line
with the upstream specification.

Thanks for the discussion!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-import-go-Fix-replacement-directive-behavior.patch --]
[-- Type: text/x-patch, Size: 7213 bytes --]

From ab53cfccc4479b2fa069748c3c816376462168ae Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Tue, 3 Aug 2021 00:46:02 -0400
Subject: [PATCH] import: go: Fix replacement directive behavior.

Per upstream specifications, the 'replace' directive should be able to replace
any dependency found in its requirements (transitively).  It should also
discriminate based on the source module version, if present.

* guix/import/go.scm (go.mod-requirements): Handle version from the
replacement specification and no longer skip replacements to self.
* tests/go.scm (fixture-go-mod-simple): Add two new requirements and a replace
directive.
("parse-go.mod-simple"): Adjust expected result.
("parse-go.mod-complete"): Correct expected result.  Since nothing requires
the "github.com/go-check/check" module, the replacement directive should not
add it to the requirements.
("parse-go.mod: simple"): Adjust expected result.
---
 guix/import/go.scm | 61 +++++++++++++++++++++++++++++++---------------
 tests/go.scm       | 15 +++++++++---
 2 files changed, 52 insertions(+), 24 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 617a0d0e23..77eba56f57 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -46,6 +46,7 @@
   #:use-module (ice-9 receive)
   #:use-module (ice-9 regex)
   #:use-module (ice-9 textual-ports)
+  #:use-module ((rnrs base) #:select (assert))
   #:use-module ((rnrs io ports) #:select (call-with-port))
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-2)
@@ -348,31 +349,51 @@ DIRECTIVE."
 (define (go.mod-requirements go.mod)
   "Compute and return the list of requirements specified by GO.MOD."
   (define (replace directive requirements)
-    (define (maybe-replace module-path new-requirement)
-      ;; Do not allow version updates for indirect dependencies (see:
-      ;; https://golang.org/ref/mod#go-mod-file-replace).
-      (if (and (equal? module-path (first new-requirement))
-               (not (assoc-ref requirements module-path)))
-          requirements
-          (cons new-requirement (alist-delete module-path requirements))))
+    ;; Refer to https://golang.org/ref/mod#go-mod-file-replace for the
+    ;; specifications.
+    (define* (replace* module-path version
+                       to-module-path to-version
+                       requirements)
+      "Do the replacement, if a matching MODULE-PATH and VERSION is found in
+the requirements.  When TO-MODULE-PATH is #f, delete the MODULE-PATH from
+REQUIREMENTS.  Return the updated requirements."
+      (when to-module-path
+        (assert to-version))
+
+      (let* ((requirement-version (and=> (assoc-ref requirements module-path)
+                                         first))
+             (replacement-match? (if version
+                                     (and=> requirement-version
+                                            (cut string=? version <>))
+                                     requirement-version)))
+        (if replacement-match?
+            (begin
+              (if to-module-path
+                  (cons (list to-module-path to-version)
+                        (alist-delete module-path requirements))
+                  (alist-delete module-path requirements))) ;file-path case
+            requirements)))
 
     (match directive
-      ((('original ('module-path module-path) . _) with . _)
-       (match with
-         (('with ('file-path _) . _)
-          (alist-delete module-path requirements))
-         (('with ('module-path new-module-path) ('version new-version) . _)
-          (maybe-replace module-path
-                         (list new-module-path new-version)))))))
-
-  (define (require directive requirements)
-    (match directive
-      ((('module-path module-path) ('version version) . _)
-       (cons (list module-path version) requirements))))
+      ((('original ('module-path module-path))
+        ('with ('file-path _)) . _)
+       (replace* module-path #f #f #f requirements))
+      ((('original ('module-path module-path) ('version version))
+        ('with ('file-path _)) . _)
+       (replace* module-path version #f #f requirements))
+      ((('original ('module-path module-path))
+        ('with ('module-path new-module-path) ('version new-version)) . _)
+       (replace* module-path #f new-module-path new-version requirements))
+      ((('original ('module-path module-path) ('version version))
+        ('with ('module-path new-module-path) ('version new-version)) . _)
+       (replace* module-path version new-module-path new-version requirements))))
 
   (let* ((requires (go.mod-directives go.mod 'require))
          (replaces (go.mod-directives go.mod 'replace))
-         (requirements (fold require '() requires)))
+         (requirements (map (match-lambda
+                              ((('module-path module-path) ('version version))
+                               (list module-path version)))
+                            requires)))
     (fold replace requirements replaces)))
 
 ;; Prevent inlining of this procedure, which is accessed by unit tests.
diff --git a/tests/go.scm b/tests/go.scm
index 6749f4585f..8c30f20c1e 100644
--- a/tests/go.scm
+++ b/tests/go.scm
@@ -43,8 +43,11 @@
 go 1.12
 require other/thing v1.0.2
 require new/thing/v2 v2.3.4
+require bad/thing v1.4.5
+require extra/thing v2
 exclude old/thing v1.2.3
 replace bad/thing v1.4.5 => good/thing v1.4.5
+replace extra/thing v1 => replaced/extra v1
 ")
 
 (define fixture-go-mod-with-block
@@ -220,7 +223,8 @@ require github.com/kr/pretty v0.2.1
     (sort (go.mod-requirements (parse-go.mod input)) inf?)))
 
 (testing-parse-mod "parse-go.mod-simple"
-                   '(("good/thing" "v1.4.5")
+                   '(("extra/thing" "v2")
+                     ("good/thing" "v1.4.5")
                      ("new/thing/v2" "v2.3.4")
                      ("other/thing" "v1.0.2"))
                    fixture-go-mod-simple)
@@ -249,8 +253,7 @@ require github.com/kr/pretty v0.2.1
    ("bitbucket.org/user/project" "v1.11.20")
    ("k8s.io/kubernetes/subproject" "v1.1.101")
    ("github.com/user/project/sub/directory" "v1.1.12")
-   ("github.com/user/project" "v1.1.11")
-   ("github.com/go-check/check" "v0.0.0-20140225173054-eb6ee6f84d0a"))
+   ("github.com/user/project" "v1.1.11"))
  fixture-go-mod-complete)
 
 (test-equal "parse-go.mod: simple"
@@ -258,9 +261,13 @@ require github.com/kr/pretty v0.2.1
     (go (version "1.12"))
     (require (module-path "other/thing") (version "v1.0.2"))
     (require (module-path "new/thing/v2") (version "v2.3.4"))
+    (require (module-path "bad/thing") (version "v1.4.5"))
+    (require (module-path "extra/thing") (version "v2"))
     (exclude (module-path "old/thing") (version "v1.2.3"))
     (replace (original (module-path "bad/thing") (version "v1.4.5"))
-      (with (module-path "good/thing") (version "v1.4.5"))))
+      (with (module-path "good/thing") (version "v1.4.5")))
+    (replace (original (module-path "extra/thing") (version "v1"))
+      (with (module-path "replaced/extra") (version "v1"))))
   (parse-go.mod fixture-go-mod-simple))
 
 (test-equal "parse-go.mod: comments and unparseable lines"
-- 
2.32.0


[-- Attachment #3: Type: text/plain, Size: 7 bytes --]


Maxim