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

[Maxime Devos]

Op 11-10-2023 om 04:36 schreef Maxim Cournoyer:
> Onderwerp:
> Re: [PATCH v4 4/4] load: Display modules depth in output when using 
> %load-verbosely.
> Van:
> Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Datum:
> 11-10-2023 04:36
>
> Aan:
> Maxime Devos <maximedevos@telenet.be>
> CC:
> guile-devel@gnu.org
>
>
> 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.

I think the opposite -- if the load hook really wants to catch its own 
wrong-number-of-args, it can just do that by inserting an appropriate 
'catch', whereas the load hook cannot remove the overly wide implicitly 
catch of primitive_load.

Implicitly catching the errors is also an API violation, as it is not 
documented that primitive-load, load-in-vicinity, etc., implicitly 
suppress errors.

(Also, you forgot doing the arity checks in ice-9/boot-9.scm; there are 
users of %load-hook outside boot-9.scm.)

Implicitly suppressing errors makes it harder to detect that there is a 
bug (with would normally have been reported by the error) and makes it 
harder to debug the bug.  Which is kind of the opposite of what 
exceptions are for.

For example, consider the procedure:

;; For compatibility with Guile<3.X.Y, don't assume depth
;; information is available.
(define* (handle file-name #:key depth #:allow-other-keys)
  (and depth
       (format #t "The file name length is ~a and the depth is ~a"
                  (string-length) ; oops forgot file-name
                  depth)))

When doing (handle "a.scm" #:depth 0), we will get a
wrong-number-of-args because of the bogus string-length, and as such the 
code will do (handle "a.scm"), which is a no-op and as such the 
exception/bug report about the use of string-length is suppressed.

I think that a slight amount of backwards compatibility is a way better 
trade than suppressing bug reports/exceptions.  If mostly proper arity 
detection turns out to be too difficult, I would rather have that the 
code simply uses the new (incompatible) API unconditionally than having 
Guile suppress bug reports/exceptions (*).

(*) Not all exceptions are bug reports, but many are.

About not 100% guaranteed, consider

;; foo has arity 1 because identity has arity 1
(define foo (compose identity identity))

(foo) ; -> wrong-number-of-arguments
(foo 0) ; -> 0
(foo 0 0) ; -> wrong-number-of-arguments

yet

(arity (compose identity identity)) ; -> 0 or more arguments

so 'procedure-minimum-arity' and the like can produce incorrect arguments.

Note that (compose identity identity) is something like (ignoring 
multiple return values for simplicity, as we're just using 'identity' here):

   (lambda args
     (identity (apply identity args)))

so Guile records this as '0 or more arguments'.

As such, the arities recorded by Guile can be larger as a set (but not 
smaller) than the actual arities.

My guess what's happening is that ice-9/boot-9.scm is not yet compiled 
when you noticed "arity=(0 0 #t)' and that such Guile approximated the 
arity as 'anything'. (the #t means 'has rest argument').

The proposal is that you would do:

  * Guile says 'rest = false' and 'req + opt <= 1', only pass a single 
arguemnt.
  * otherwise, pass file name + the keyword arguments

Then, old code with arity information (and not the bogus 'compose'-kind 
of arity information) will work correctly, and all new code will work 
correctly.

> > > > 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).

We are not stuck with the positional arguments.  There is no backwards 
incompatibility on the Scheme side (besides the initial 'add rest 
arguments' or 'add keyword arguments' thing'), and on the C side you 
could simply define scm_primitive_load2, late scm_primitive_load3, etc., 
or the hack that you are using currently.

Also, they don't have to evolve together -- they are related, but there 
exist other users of %load-hook too, and you can add arguments to 
primitive-load without adding them to %load-hook and conversely.

For example, consider the 'depth' argument that you are adding (*) to 
%load-hook.

(*) While currently you added a depth argument to scm_primitive_load, 
you could have used the parameter object '%current-module-load-path' 
directly instead, avoiding the messy argument parsing without creating 
ABI incompatibilities or versioned functions, if I'm not missing anything.

> 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?

What you're missing is that this is for future compatibility, we don't 
need concrete ideas yet.  The value is future backwards compatibility, 
in that we only need to introduce the backwards compatibility once, and 
that in the future we can add arguments to the hook without any fear. 
(At least, backwards compatibility on the Scheme side.)

Also, I don't think I need to provide any hypothetical arguments for 
%load-hook in context of primitive_load, because there are also other 
users of %load-hook, e.g. load-in-vicinity.

I think I don't have to provide an idea for extra arguments -- just 
think of what you are doing currently and imagine that sometimes in the 
future someone else will do something similar.

Here are some proposals:

  * #:module or #:module-name (for things loaded via #:use-module)
  * #:last-modification-time (primitive-load can do 'stat')
  * #:language
  * #:interpreted? or #:compiled?
  * #:precompiled?     (in case of primitive-load, always #false, IIUC)

Best regards,
Maxime Devos.

OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature