Re: Layout of ‘define-configuration’ records

[Maxim Cournoyer]

Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> This is so that the first field of the generated record matches the first one
>> declared, which makes 'define-configuration' record API compatible with
>> define-record-type* ones.
>>
>> * gnu/services/configuration.scm (define-configuration-helper): Move the
>> %location field below the ones declared by the user.
>> * gnu/services/monitoring.scm (zabbix-front-end-config): Adjust match pattern
>> accordingly.
>
> [...]
>
>> +++ b/gnu/services/configuration.scm
>> @@ -242,17 +242,17 @@ (define-record-type* #,(id #'stem #'< #'stem #'>)
>>                 stem
>>                 #,(id #'stem #'make- #'stem)
>>                 #,(id #'stem #'stem #'?)
>> -               (%location #,(id #'stem #'stem #'-location)
>> -                          (default (and=> (current-source-location)
>> -                                          source-properties->location))
>> -                          (innate))
>>                 #,@(map (lambda (name getter def)
>>                           #`(#,name #,getter (default #,def)
>>                                     (sanitize
>>                                      #,(id #'stem #'validate- #'stem #'- 
>> name))))
>>                         #'(field ...)
>>                         #'(field-getter ...)
>> -                       #'(field-default ...)))
>> +                       #'(field-default ...))
>> +               (%location #,(id #'stem #'stem #'-location)
>> +                          (default (and=> (current-source-location)
>> +                                          source-properties->location))
>> +                          (innate)))
>
> Moving the field last is problematic as we’ve seen for any user that
> uses ‘match’ on records—something that’s not recommended but still used
> a lot.

Yep.  I had that on mind when I made the change, though I still missed a
few occurrences.

>>  (define (zabbix-front-end-config config)
>>    (match-record config <zabbix-front-end-configuration>
>> -    (%location db-host db-port db-name db-user db-password db-secret-file
>> -               zabbix-host zabbix-port)
>> +    (db-host db-port db-name db-user db-password db-secret-file
>> +             zabbix-host zabbix-port %location)
>
> This change has no effect because ‘match-record’ matches fields by name,
> precisely to avoid problems when changing the layout of record types.

Good catch; I got confused there, although my confusion luckily had no
side effect :-).

> I’m not sure what was meant by “compatible” in the commit log; how
> fields are laid out is something user code should not be aware of.

I wanted match on define-configuration'd fields to be
backward-compatible with fields migrated from define-record-type*, so
that they such change can be made without worrying breakages.  It seems
good for consistency that both our define-record-type* and
define-configuration fields share a similar internal layout, at least
until all the old-fashion (ice-9 match) record matching usages are
rewritten to use something like 'match-record'.

I initially got tricked by that discrepancy and it took me quite some
time hunting down a cryptic backtrace when authoring the new mcron
configuration records.

> The only thing to keep in mind is that records must not be matched with
> ‘match’ because then the code silently breaks when the record type
> layout is changed.  This is why ‘match-record’ was introduced:
>
>   https://issues.guix.gnu.org/28960#4
>
> One last thing: placing ‘%location’ first can let us implement:
>
>   (define (configuration-location config)
>     (struct-ref config 0))

Would this have worked?

--8<---------------cut here---------------start------------->8---
scheme@(gnu services mcron)> (define config (mcron-configuration))
scheme@(gnu services mcron)> (struct-ref 0 config)
ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure struct-ref: Wrong type argument in position 1 (expecting struct): 0

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
scheme@(gnu services mcron) [1]> ,q
scheme@(gnu services mcron)> ,use (oop goops)
scheme@(gnu services mcron)> (class-of config)
$5 = #<<class> <<mcron-configuration>> 7fe20fd83e00>
--8<---------------cut here---------------end--------------->8---


All in all, I think that's a rather small change that got our internal
implementation of both type of records in sync between
define-configuration and define-record-type*, that should pave the way
for migrating more of the later to the former without risking breaking
something, going forward.

Alternatively, if we have a good reason to not to go with this, I think
we should abstract all the internal-implementation dependent code from
our code base (e.g., the match patterns directly matching on field
slots).

What do you think?

-- 
Thanks,
Maxim