[bug#51838] [PATCH v5 06/45] guix: node-build-system: Refactor patch-dependencies phase.

[Philip McGrath]

Hi,

On 12/16/21 23:29, Liliana Marie Prikler wrote:
> Hi,
>
> Am Donnerstag, dem 16.12.2021 um 21:02 -0500 schrieb Philip McGrath:
> > * guix/build/node-build-system.scm (patch-dependencies): Strictly
> > follow the linearity rules for `assoc-set!` and friends.
> > Clarify the types of the arguments to and return value from the
> > internal helper function `resolve-dependencies`.
> > [...]
> > -  (define (resolve-dependencies package-meta meta-key)
> > -    (fold (lambda (key+value acc)
> > -            (match key+value
> > -              ('@ acc)
> > -              ((key . value) (acons key (hash-ref index key value)
> > acc))))
> > -          '()
> > -          (or (assoc-ref package-meta meta-key) '())))
> > +  (define (resolve-dependencies meta-alist meta-key)
> > +    ;; Given:
> > +    ;;  - The alist from "package.json", with the '@ unwrapped
> > +    ;;  - A string key, like "dependencies"
> > +    ;; Returns: an alist (without a wrapping '@) like the entry in
> > +    ;; meta-alist for meta-key, but with dependencies supplied
> > +    ;; by Guix packages mapped to the absolute store paths to use.
> > +    (match (assoc-ref meta-alist meta-key)
> > +      (#f
> > +       '())
> > +      (('@ . orig-deps)
> > +       (fold (match-lambda*
> > +               (((key . value) acc)
> > +                (acons key (hash-ref index key value) acc)))
> > +             '()
> > +             orig-deps))))
> >   
> >     (with-atomic-file-replacement "package.json"
> >       (lambda (in out)
> > -      (let ((package-meta (read-json in)))
> > -        (assoc-set! package-meta "dependencies"
> > -                    (append
> > -                     '(@)
> > -                     (resolve-dependencies package-meta
> > "dependencies")
> > -                     (resolve-dependencies package-meta
> > "peerDependencies")))
> > -        (assoc-set! package-meta "devDependencies"
> > -                    (append
> > -                     '(@)
> > -                     (resolve-dependencies package-meta
> > "devDependencies")))
> > +      ;; It is unsafe to rely on 'assoc-set!' to update an
> > +      ;; existing assosciation list variable:
> > +      ;; see 'info "(guile)Adding or Setting Alist Entries"'.
> > +      (let* ((package-meta (read-json in))
> > +             (alist (match package-meta
> > +                      ((@ . alist) alist)))
> > +             (alist
> > +              (assoc-set!
> > +               alist "dependencies"
> > +               (append
> > +                '(@)
> > +                (resolve-dependencies alist "dependencies")
> > +                (resolve-dependencies alist "peerDependencies"))))
> > +             (alist
> > +              (assoc-set!
> > +               alist "devDependencies"
> > +               (append
> > +                '(@)
> > +                (resolve-dependencies alist "devDependencies"))))
> > +             (package-meta (cons '@ alist)))
> >           (write-json package-meta out))))
> >     #t)
> The Guix codebase is generally not the place to play around with
> destructive semantics.  If you can avoid assoc-set!, I think you ought
> to, especially if it helps making a two-step process into a single-step
> one.  Anything I'm missing here?

I agree that assoc-set! is best avoided. (I am a Racketeer: we don't 
mutate pairs.) However, this code was already using assoc-set!: the 
change in this patch is merely to use it correctly.

AFAIK neither `info "(guile)Association Lists"` nor SRFI-1 (`info 
"(guile)SRFI-1 Association Lists"`) provide a non-destructive assoc-set 
operation. Note in particular that acons and alist-cons do not work, 
since they don't remove existing entries for the same key: they would 
result in duplicate keys being written to the JSON object. (In theory, 
this has undefined semantics; in practice, I believe Node.js would use 
the wrong entry.)

Of course, I know how to write a little library of purely functional 
association list operations---but that seems vastly out of scope for 
this patch series (which has already grown quite large). Furthermore, if 
we were going to make such changes, I think it might be better to change 
the representation of JSON objects to use a real immutable dictionary 
type, probably VHash (though it looks like those would still need 
missing functions, at which point a wrapper type that validated keys and 
maintained a consistent hash-proc might be even better). Alternatively 
we could use guile-json, which at least avoids the need for improper 
alists to disambiguate objects from arrays, but we would have to address 
the issues in Guix commit a4bb18921099b2ec8c1699e08a73ca0fa78d0486.

All of that reinforces my sense that we should not try to change this here.

-Philip