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

[Philip McGrath]

Hi,

On 12/18/21 12:52, Liliana Marie Prikler wrote:
> Hi,
>
> Am Samstag, dem 18.12.2021 um 12:03 -0500 schrieb Philip McGrath:
> > [...]
> >
> > > 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.
> I think you misread me here.  One thing that's bugging me is that you
> (just like whoever wrote this before) strip the @ only to reintroduce
> it.  I think it'd be better if (resolve-dependencies) simply took a
> list and the let-block deconstructed the json.
>
> As for the package-meta -> package-meta conversion, imo that could
> perfectly be done with match or SXML transformation.  WDYT?

I definitely am not understanding what you have in mind here. When you 
write "strip the @", I'm not sure what you're referring to, because 
there are multiple "@" tags here, one beginning each JSON object. (Maybe 
this is obvious, but it hadn't been obvious to me.) So, the (guix build 
json) representation of a "package.json" file might look like this:

```
`(@ ("name" . "apple")
    ("version" . "1.0")
    ("dependencies". (@ ("banana" . "*")
                        ("pear" . "*")))
    ("devDependencies" . (@ ("peach" . "*")
                            ("orange" . "*")))
    ("peerDependencies" . (@ ("node-gyp" . "7.x")))
    ("peerDependenciesMeta" . (@ ("node-gyp" . (@ ("optional" . #t)))))
    ("optionalDependencies" . (@ ("node-gyp" . "7.x"))))
```

An unfortunate consequence of this representation is that JSON objects 
are not usable directly as association lists: some procedures expecting 
association lists seem to silently ignore the non-pair at the head of 
the list, but I don't think that's guaranteed, and other procedures just 
don't work. In particular, `append` applied to two JSON objects does not 
produce a JSON object, even ignoring the problem of duplicate keys.

Given that the current code adds "peerDependencies" as additional 
"dependencies", the choice (as I see it) is between the current 
approach, in which `resolve-dependencies` returns genuine association 
lists and the `let*` block turns them JSON objects, or changing 
`resolve-dependencies` to return JSON objects and implementing 
`json-object-append`, which doesn't seem obviously better, unless it 
were part of broader changes. (As an aside, I am not convinced that the 
current handling of "peerDependencies" is right, but I think 
reevaluating that behavior is out of scope for this patch series, and 
particularly for this patch, in which, as Tim said in 
<https://issues.guix.gnu.org/51838#234>, my goal was merely to make the 
use of `assoc-set!` safe.)

I definitely think the broader situation should be improved!

But I think those improvements are out of scope for this patch series. 
It seems like much more discussion would be needed on what the 
improvements should be, and potentially coordination with other users of 
(guix build json). Personally, I'd want to represent JSON objects with a 
real immutable dictionary type that gave us more guarantees about 
correctness by construction. If we continue with tagged association 
lists, we should write a little library of purely functional operations 
on JSON objects. But that all seems very far afield.

-Philip