Re: [PATCH v4 4/4] load: Display modules depth in output when using %load-verbosely.

[Maxim Cournoyer]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> writes:

[...]

> I now see:
>
>> +  /* For compatibility with older load hooks procedures, fall-back to
>> +     calling it with a single argument if calling it with two fails. */
>> +  scm_internal_catch (scm_from_latin1_symbol ("wrong-number-of-args"),
>> +                      call_hook_2_body, &args_data,
>> +                      call_hook_1_handler, &args_data);
>
> But that doesn't work properly, as it catches too much -- the
> 'wrong-numer-of-args' might be caused by something inside the handler
> instead of an incorrect-arity invocation of the handler itself.
>
> Something like 'program-arity' would be more accurate, albeit not 100%
> guaranteed.  But for backwards compatibility it might be good enough.
> (Caveat: I don't know if uncompiled procedures have arity information,
> though perhaps you could go ‘no arity information -> assume new
> interface'.)
>
> (On the C level, there is scm_i_procedure_arity.)

I see what you mean about potential nested wrong-number-of-args being
raised by the hook procedure itself, but I'm not sure how that can be
improved.  I had tried introspecting the arity of a procedure and it
didn't work on the C side, at least using 'scm_procedure_minimum_arity'
(which is implemented in terms of scm_i_procedure_arity).  From my
#guile IRC logs:

--8<---------------cut here---------------start------------->8---
2023-09-08 21:13:50     apteryx interesting, arity = 
scm_procedure_minimum_arity (hook); doesn't work in load.c
2023-09-08 21:14:03     apteryx it produces arity=(0 0 #t)
--8<---------------cut here---------------end--------------->8---

Also, what do you mean by 'not 100% guaranteed' ?  I think catching too
many errors here is a better trade than catching none.

>>> To prevent future API breaks, I propose documenting turning %load-hook
>>> into a keyword argument procedure with #:allow-other-keys, as
>>> something like:
>>>
>>>    (lambda* (file-name #:key (depth the-default) #:allow-other-keys)
>>>      ...)
>>>
>>> and in the documentation mention that more keywords may be added in
>>> the future (and hence #:allow-other-keys).
>>>
>>> I think it's quite plausible that there will be more such arguments in
>>> the future!
>> That sounds like a good idea, helas as I understand, with the
>> current
>> solution, everything needs to be kept as fixed positional arguments so
>> we can make sense of them on the C side (which accepts a list of 1 or
>> more items, expected to be given in order).  So unless you have other
>> ideas that would also ensure backward-compatibility concern on the C
>> side, I'd leave it as is for now
>
> The C side doesn't expect anything -- the C side only _calls_ the load
> hook, it doesn't implement the load hook, as far as I can tell.
>
> More concretely, as I understand it, all you need to do on the C-side
> is replacing
>
>> +  scm_call_2(hook, full_filename, depth);
>
> by
>
>> +  scm_call_3(hook, full_filename,  scm_from_utf8_keyword("depth"),
>   depth);
>
> (using SCM_KEYWORD instead might be preferred).

Thanks for the concrete example.  I think I was thinking in terms of
scm_primitive_load, which is the one carrying the extra information
provided to the %load-hook (e.g. the depth) during its recursive calls.
Since we're stuck with positional arguments for scm_primitive_load for
backward compatibility, I see little value having a more flexible arity
for the %load-hook (they will have to evolve together if they do, as I
see it).

So while it sounds good "on paper", in practice it appears it'd provide
little value, unless I'm missing something.  Or did you have concrete
ideas of what extra arguments may make sense to have for a %load-hook
procedure that wouldn't need to be passed through a modified
scm_primitive_load procedure?

Thanks for your continued feedback.

-- 
Maxim