[PATCH v4 0/4] Add module depth information to %load-verbosely output

[PATCH v4 0/4] Add module depth information to %load-verbosely output

[Maxim Cournoyer]

This change was made to support investigating cyclic module dependencies
that sometimes happen in GNU Guix and are difficult to
comprehend/debug.  For more context, see:
<https://issues.guix.gnu.org/65716>.

Changes in v4:
- Remove with-output-to-port in %load-announce and adjust doc

Changes in v3:
- Replace PAD-COUNT with DEPTH in VISUAL-DEPTH guard.

Changes in v2:
- Guard against negative pad count when computing 'visual-depth'

Maxim Cournoyer (4):
  (ice-9 boot-9): Fix typo.
  .dir-locals: Set c-basic-offset to 2 for c-mode.
  guix.scm: Update guile package native inputs.
  load: Display modules depth in output when using %load-verbosely.

 .dir-locals.el                  |  1 +
 .guix/modules/guile-package.scm |  3 +-
 NEWS                            |  8 +++
 THANKS                          |  1 +
 doc/guile-api.alist             |  4 +-
 doc/ref/api-evaluation.texi     | 62 ++++++++++++++++++-----
 libguile/load.c                 | 89 +++++++++++++++++++++++++++------
 libguile/load.h                 |  4 +-
 module/ice-9/boot-9.scm         | 38 ++++++++------
 9 files changed, 164 insertions(+), 46 deletions(-)


base-commit: 8441d8ff5671db690eb239cfea4dcfdee6d6dcdb
-- 
2.41.0

[PATCH v4 1/4] (ice-9 boot-9): Fix typo.

[Maxim Cournoyer]

* module/ice-9/boot-9.scm (module-use-interfaces!): Fix typo in doc string.
---

(no changes since v1)

 module/ice-9/boot-9.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/module/ice-9/boot-9.scm b/module/ice-9/boot-9.scm
index a5f2eea9b..897d8d01c 100644
--- a/module/ice-9/boot-9.scm
+++ b/module/ice-9/boot-9.scm
@@ -2927,7 +2927,7 @@ uses)."
 
 (define (module-use-interfaces! module interfaces)
   "Same as MODULE-USE!, but only notifies module observers after all
-interfaces are added to the inports list."
+interfaces are added to the imports list."
   (let* ((cur (module-uses module))
          (new (let lp ((in interfaces) (out '()))
                 (if (null? in)
-- 
2.41.0

[PATCH v4 2/4] .dir-locals: Set c-basic-offset to 2 for c-mode.

[Maxim Cournoyer]

* .dir-locals.el (c-mode): Set c-basic-offset to 2.
---

(no changes since v1)

 .dir-locals.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.dir-locals.el b/.dir-locals.el
index 908670479..f63bdc8a3 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -3,6 +3,7 @@
 ((nil             . ((fill-column . 72)
                      (tab-width   .  8)))
  (c-mode          . ((c-file-style . "gnu")
+                     (c-basic-offset . 2)
                      (indent-tabs-mode . nil)))
  (scheme-mode
   . ((indent-tabs-mode . nil)
-- 
2.41.0

[PATCH v4 3/4] guix.scm: Update guile package native inputs.

[Maxim Cournoyer]

* guix.scm (guile) [native-inputs]: Replace texlive-base with
texlive-scheme-basic.  Add git:send-email.
---

(no changes since v1)

 .guix/modules/guile-package.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/.guix/modules/guile-package.scm b/.guix/modules/guile-package.scm
index 41710547d..eac3e0fc7 100644
--- a/.guix/modules/guile-package.scm
+++ b/.guix/modules/guile-package.scm
@@ -112,10 +112,11 @@
                      gnu-gettext
                      flex
                      texinfo
-                     texlive-base                 ;for "make pdf"
+                     texlive-scheme-basic ;for "make pdf"
                      texlive-epsf
                      gperf
                      git
+                     `(,git "send-email") ;for convenience
                      gdb
                      strace
                      readline
-- 
2.41.0

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

[Maxim Cournoyer]

* NEWS: Update news.
* THANKS: Add myself.
* doc/guile-api.alist (%load-announce, %load-hook): Add DEPTH argument.
* doc/ref/api-evaluation.texi (Loading): Document new
DEPTH argument for the primitive-load, primitive-load-path and
%load-hook procedures.  Update %load-hook example.  Document
%load-verbosely.
* libguile/load.c (scm_loc_load_hook): Update doc.
(hook_args_data): New struct.
(call_hook_2_body, call_hook_1_handler, call_hook): New procedures.
(scm_primitive_load): Modify to accept a single list of arguments, like
for scm_primitive_load_path, so to accept an optional DEPTH argument.
Call hook via the 'call_hook' procedure.
(scm_primitive_load_path): Accept a third optional DEPTH argument.  Call
hook via the 'call_hook' procedure.  Pass depth to the
'scm_primitive_load' procedure call.
* libguile/load.h (scm_primitive_load)
(scm_primitive_load_path): Add 'depth' to argument name.
* module/ice-9/boot-9.scm (%load-announce): Accept the second DEPTH
argument, and use it to display the modules loaded hierarchically.  Use
format instead of display.
(%current-module-load-depth): New parameter.
(resolve-module): Use it.
(try-module-autoload): Call primitive-load-path with it.
(load-in-vicinity): Invoke %load-hook with it.

Reviewed-by: Maxime Devos <maximedevos@telenet.be>
---

Changes in v4:
- Remove with-output-to-port in %load-announce and adjust doc

Changes in v3:
- Replace PAD-COUNT with DEPTH in VISUAL-DEPTH guard.

Changes in v2:
- Guard against negative pad count when computing 'visual-depth'

 NEWS                        |  8 ++++
 THANKS                      |  1 +
 doc/guile-api.alist         |  4 +-
 doc/ref/api-evaluation.texi | 62 ++++++++++++++++++++------
 libguile/load.c             | 89 +++++++++++++++++++++++++++++++------
 libguile/load.h             |  4 +-
 module/ice-9/boot-9.scm     | 36 +++++++++------
 7 files changed, 160 insertions(+), 44 deletions(-)

diff --git a/NEWS b/NEWS
index b319404d7..b8b12f1f6 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,14 @@ definitely unused---this is notably the case for modules that are only
 used at macro-expansion time, such as (srfi srfi-26).  In those cases,
 the compiler reports it as "possibly unused".
 
+** The %load-hook procedure is now applied with an extra 'depth' argument
+
+This argument is used to show the depth level of the module being load
+in the output when setting %load-verbosely to #t, which makes it easier
+to inspect which module caused others to be loaded.  It is hoped to be
+useful when troubleshooting tricky top-level module circular
+dependencies.
+
 * Bug fixes
 
 ** (ice-9 suspendable-ports) incorrect UTF-8 decoding
diff --git a/THANKS b/THANKS
index aa4877e95..546f79b45 100644
--- a/THANKS
+++ b/THANKS
@@ -5,6 +5,7 @@ Contributors since the last release:
 	    Rob Browning
         Tristan Colgate-McFarlane
           Aleix Conchillo Flaqué
+          Maxim Cournoyer
         Ludovic Courtès
           Jason Earl
            Paul Eggert
diff --git a/doc/guile-api.alist b/doc/guile-api.alist
index a1616149f..20c900166 100644
--- a/doc/guile-api.alist
+++ b/doc/guile-api.alist
@@ -37,9 +37,9 @@
 (%init-rdelim-builtins (groups Scheme) (scan-data "#<primitive-procedure %init-rdelim-builtins>"))
 (%init-rw-builtins (groups Scheme) (scan-data "#<primitive-procedure %init-rw-builtins>"))
 (%library-dir (groups Scheme) (scan-data "#<primitive-procedure %library-dir>"))
-(%load-announce (groups Scheme) (scan-data "#<procedure %load-announce (file)>"))
+(%load-announce (groups Scheme) (scan-data "#<procedure %load-announce (file depth)>"))
 (%load-extensions (groups Scheme) (scan-data ""))
-(%load-hook (groups Scheme) (scan-data "#<procedure %load-announce (file)>"))
+(%load-hook (groups Scheme) (scan-data "#<procedure %load-announce (file depth)>"))
 (%load-path (groups Scheme) (scan-data ""))
 (%load-verbosely (groups Scheme) (scan-data ""))
 (%make-void-port (groups Scheme) (scan-data "#<primitive-procedure %make-void-port>"))
diff --git a/doc/ref/api-evaluation.texi b/doc/ref/api-evaluation.texi
index 7c08e2494..2cccf0481 100644
--- a/doc/ref/api-evaluation.texi
+++ b/doc/ref/api-evaluation.texi
@@ -865,14 +865,20 @@ calling @code{load-compiled} on the resulting file is equivalent to
 calling @code{load} on the source file.
 @end deffn
 
-@deffn {Scheme Procedure} primitive-load filename
+@deffn {Scheme Procedure} primitive-load filename [depth]
 @deffnx {C Function} scm_primitive_load (filename)
 Load the file named @var{filename} and evaluate its contents in the
 top-level environment.  @var{filename} must either be a full pathname or
 be a pathname relative to the current directory.  If the variable
 @code{%load-hook} is defined, it should be bound to a procedure that
 will be called before any code is loaded.  See the documentation for
-@code{%load-hook} later in this section.
+@code{%load-hook} later in this section.  An optional second argument,
+@var{depth}, can be specified to track the depth at which modules are
+loaded.
+
+For compatibility with Guile 3.9 and earlier, the C function takes only
+one argument, which can be either a string (the file name) or an
+argument list.
 @end deffn
 
 @deftypefn {C Function} SCM scm_c_primitive_load (const char *filename)
@@ -905,20 +911,48 @@ change occurs at the right time.
 @end defvar
 
 @defvar %load-hook
-A procedure to be called @code{(%load-hook @var{filename})} whenever a
-file is loaded, or @code{#f} for no such call.  @code{%load-hook} is
-used by all of the loading functions (@code{load} and
-@code{primitive-load}, and @code{load-from-path} and
+A procedure to be called @code{(%load-hook @var{filename} @var{depth})}
+whenever a file is loaded, or @code{#f} for no such call.
+@code{%load-hook} is used by all of the loading functions (@code{load}
+and @code{primitive-load}, and @code{load-from-path} and
 @code{primitive-load-path} documented in the next section).
 
-For example an application can set this to show what's loaded,
+The default @code{%load-hook} is bound to a procedure that does
+something like:
+
+@example
+(define (%load-hook file depth)
+  (when %load-verbosely
+    (let* ((pad-count (- 3 (string-length (number->string depth))))
+           (pad (if (> pad-count 0)
+                    (make-string pad-count #\space)
+                    ""))
+           (visual-depth (if (> depth 0)
+                             (make-string depth #\space)
+                             "")))
+      (format (current-warning-port)
+              ";;; loading ~a~a ~a~a~%" pad depth visual-depth file)
+      (force-output (current-warning-port)))))
+@end example
+
+@vindex %load-verbosely, to enable default %load-hook output
+As you can see from the above procedure, an application can thus set the
+@code{%load-verbosely} variable to @code{#t} to enable the default load
+hook output, which produces something like:
 
 @example
-(set! %load-hook (lambda (filename)
-                   (format #t "Loading ~a ...\n" filename)))
-(load-from-path "foo.scm")
-@print{} Loading /usr/local/share/guile/site/foo.scm ...
+@print{};;; loading   0 guix/gnu/packages/abiword.scm
+@print{};;; loading   1  guix/build-system/glib-or-gtk.scm
+@print{};;; loading   2   guix/build/glib-or-gtk-build-system.scm
+@print{};;; loading   3    guix/build/gnu-build-system.scm
+@print{};;; loading   4     guix/build/gremlin.scm
+@print{};;; loading   5      guix/elf.scm
 @end example
+
+The number corresponds to the depth at which the module was loaded,
+which is a recursive process.  The indentation of the file name loaded
+corresponds to that depth value, to make it easy to visually discern
+which module caused others to be loaded.
 @end defvar
 
 @deffn {Scheme Procedure} current-load-port
@@ -969,7 +1003,7 @@ It's better to use @code{add-to-load-path} than to modify
 @code{%load-path} directly, because @code{add-to-load-path} takes care
 of modifying the path both at compile-time and at run-time.
 
-@deffn {Scheme Procedure} primitive-load-path filename [exception-on-not-found]
+@deffn {Scheme Procedure} primitive-load-path filename [exception-on-not-found] [depth]
 @deffnx {C Function} scm_primitive_load_path (filename)
 Search @code{%load-path} for the file named @var{filename} and
 load it into the top-level environment.  If @var{filename} is a
@@ -983,7 +1017,9 @@ second argument, @var{exception-on-not-found}.  If it is @code{#f},
 @code{#f} will be returned.  If it is a procedure, it will be called
 with no arguments.  (This allows a distinction to be made between
 exceptions raised by loading a file, and exceptions related to the
-loader itself.)  Otherwise an error is signaled.
+loader itself.)  Otherwise an error is signaled.  An optional third
+argument, @var{depth}, can be specified to track the depth at which modules are
+loaded.
 
 For compatibility with Guile 1.8 and earlier, the C function takes only
 one argument, which can be either a string (the file name) or an
diff --git a/libguile/load.c b/libguile/load.c
index 34e7934b9..094b6d985 100644
--- a/libguile/load.c
+++ b/libguile/load.c
@@ -72,35 +72,92 @@
 \f
 /* Loading a file, given an absolute filename.  */
 
-/* Hook to run when we load a file, perhaps to announce the fact somewhere.
-   Applied to the full name of the file.  */
+/* Hook to run when we load a file, perhaps to announce the fact
+   somewhere.  Applied to the full name of the file and (since 3.10) an
+   optional depth counter.  */
 static SCM *scm_loc_load_hook;
 
 /* The current reader (a fluid).  */
 static SCM the_reader = SCM_BOOL_F;
 
+struct hook_args_data {
+    SCM filename;
+    SCM depth;
+};
+
+static SCM call_hook_2_body(void *data) {
+    struct hook_args_data *args_data = data;
+    scm_call_2(*scm_loc_load_hook, args_data->filename, args_data->depth);
+    return SCM_BOOL_T;
+}
+
+static SCM call_hook_1_handler(void *data, SCM key, SCM args ) {
+    struct hook_args_data *args_data = data;
+    scm_call_1(*scm_loc_load_hook, args_data->filename);
+    return SCM_BOOL_T;
+}
+
+/* Helper to call %load-hook with the correct number of arguments. */
+static void call_hook (SCM hook, SCM filename, SCM depth) {
+  if (scm_is_false (hook))
+    return;
+
+  struct hook_args_data args_data;
+  args_data.filename = filename;
+  args_data.depth = depth;
+
+  /* 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);
+}
 
-SCM_DEFINE (scm_primitive_load, "primitive-load", 1, 0, 0, 
-           (SCM filename),
+SCM_DEFINE (scm_primitive_load, "primitive-load", 0, 0, 1,
+            (SCM args),
 	    "Load the file named @var{filename} and evaluate its contents in\n"
 	    "the top-level environment. The load paths are not searched;\n"
 	    "@var{filename} must either be a full pathname or be a pathname\n"
 	    "relative to the current directory.  If the  variable\n"
 	    "@code{%load-hook} is defined, it should be bound to a procedure\n"
 	    "that will be called before any code is loaded.  See the\n"
-	    "documentation for @code{%load-hook} later in this section.")
+	    "documentation for @code{%load-hook} later in this section.\n"
+            "A second optional argument can be used to specify the depth\n"
+            "at which the module was loaded.")
 #define FUNC_NAME s_scm_primitive_load
 {
+  SCM filename;
+  SCM depth;
   SCM hook = *scm_loc_load_hook;
   SCM ret = SCM_UNSPECIFIED;
 
+  if (scm_is_string (args)) {
+      /* C code written for 3.9 and earlier expects this function to
+         take a single argument (the file name).  */
+      filename = args;
+      depth = scm_from_int(0);
+    }
+  else {
+    /* Starting from 3.10, this function takes 1 required and 1 optional
+       arguments. */
+    long len;
+
+    SCM_VALIDATE_LIST_COPYLEN (SCM_ARG1, args, len);
+    if (len < 1 || len > 2)
+      scm_error_num_args_subr (FUNC_NAME);
+
+    filename = SCM_CAR (args);
+    SCM_VALIDATE_STRING (SCM_ARG1, filename);
+
+    depth = len > 1 ? SCM_CADR (args) : scm_from_int(0);
+  }
+
   SCM_VALIDATE_STRING (1, filename);
   if (scm_is_true (hook) && scm_is_false (scm_procedure_p (hook)))
     SCM_MISC_ERROR ("value of %load-hook is neither a procedure nor #f",
 		    SCM_EOL);
 
-  if (!scm_is_false (hook))
-    scm_call_1 (hook, filename);
+  call_hook (hook, filename, depth);
 
   {
     SCM port;
@@ -1163,11 +1220,13 @@ SCM_DEFINE (scm_primitive_load_path, "primitive-load-path", 0, 0, 1,
             "depending on the optional second argument,\n"
             "@var{exception_on_not_found}.  If it is @code{#f}, @code{#f}\n"
             "will be returned.  If it is a procedure, it will be called\n"
-            "with no arguments.  Otherwise an error is signaled.")
+            "with no arguments.  Otherwise an error is signaled.\n\n"
+            "A third optional argument may be provided to track module depth.")
 #define FUNC_NAME s_scm_primitive_load_path
 {
   SCM filename, exception_on_not_found;
   SCM full_filename, compiled_thunk;
+  SCM depth;
   SCM hook = *scm_loc_load_hook;
   struct stat stat_source, stat_compiled;
   int found_stale_compiled_file = 0;
@@ -1182,21 +1241,24 @@ SCM_DEFINE (scm_primitive_load_path, "primitive-load-path", 0, 0, 1,
 	 single argument (the file name).  */
       filename = args;
       exception_on_not_found = SCM_UNDEFINED;
+      depth = scm_from_int (0);
     }
   else
     {
-      /* Starting from 1.9, this function takes 1 required and 1 optional
-	 argument.  */
+      /* Starting from 1.9, this function takes 1 required and 1
+	 optional arguments.  From 3.10, this function takes 1 required
+	 and 2 optional arguments.  */
       long len;
 
       SCM_VALIDATE_LIST_COPYLEN (SCM_ARG1, args, len);
-      if (len < 1 || len > 2)
+      if (len < 1 || len > 3)
 	scm_error_num_args_subr (FUNC_NAME);
 
       filename = SCM_CAR (args);
       SCM_VALIDATE_STRING (SCM_ARG1, filename);
 
       exception_on_not_found = len > 1 ? SCM_CADR (args) : SCM_UNDEFINED;
+      depth = len > 2 ? SCM_CADDR (args) : scm_from_int (0);
     }
 
   if (SCM_UNBNDP (exception_on_not_found))
@@ -1252,8 +1314,7 @@ SCM_DEFINE (scm_primitive_load_path, "primitive-load-path", 0, 0, 1,
                         scm_list_1 (filename));
     }
 
-  if (!scm_is_false (hook))
-    scm_call_1 (hook, full_filename);
+  call_hook(hook, full_filename, depth);
 
   if (scm_is_true (compiled_thunk))
     return scm_call_0 (compiled_thunk);
@@ -1264,7 +1325,7 @@ SCM_DEFINE (scm_primitive_load_path, "primitive-load-path", 0, 0, 1,
       if (scm_is_true (freshly_compiled))
         return scm_call_0 (scm_load_thunk_from_file (freshly_compiled));
       else
-        return scm_primitive_load (full_filename);
+        return scm_primitive_load (scm_list_2 (full_filename, depth));
     }
 }
 #undef FUNC_NAME
diff --git a/libguile/load.h b/libguile/load.h
index 25f67b87b..d03019b44 100644
--- a/libguile/load.h
+++ b/libguile/load.h
@@ -27,7 +27,7 @@
 \f
 SCM_API SCM scm_parse_path (SCM path, SCM tail);
 SCM_API SCM scm_parse_path_with_ellipsis (SCM path, SCM base);
-SCM_API SCM scm_primitive_load (SCM filename);
+SCM_API SCM scm_primitive_load (SCM filename_and_depth);
 SCM_API SCM scm_c_primitive_load (const char *filename);
 SCM_API SCM scm_sys_package_data_dir (void);
 SCM_API SCM scm_sys_library_dir (void);
@@ -36,7 +36,7 @@ SCM_API SCM scm_sys_global_site_dir (void);
 SCM_API SCM scm_sys_site_ccache_dir (void);
 SCM_API SCM scm_search_path (SCM path, SCM filename, SCM rest);
 SCM_API SCM scm_sys_search_load_path (SCM filename);
-SCM_API SCM scm_primitive_load_path (SCM filename_and_exception_on_not_found);
+SCM_API SCM scm_primitive_load_path (SCM filename_and_exception_on_not_found_and_depth);
 SCM_API SCM scm_c_primitive_load_path (const char *filename);
 SCM_INTERNAL SCM scm_sys_warn_auto_compilation_enabled (void);
 SCM_INTERNAL void scm_init_load_path (void);
diff --git a/module/ice-9/boot-9.scm b/module/ice-9/boot-9.scm
index 897d8d01c..a1177e172 100644
--- a/module/ice-9/boot-9.scm
+++ b/module/ice-9/boot-9.scm
@@ -2236,15 +2236,18 @@ name extensions listed in %load-extensions."
 (define %load-verbosely #f)
 (define (assert-load-verbosity v) (set! %load-verbosely v))
 
-(define (%load-announce file)
-  (if %load-verbosely
-      (with-output-to-port (current-warning-port)
-        (lambda ()
-          (display ";;; ")
-          (display "loading ")
-          (display file)
-          (newline)
-          (force-output)))))
+(define (%load-announce file depth)
+  (when %load-verbosely
+    (let* ((pad-count (- 3 (string-length (number->string depth))))
+           (pad (if (> pad-count 0)
+                    (make-string pad-count #\space)
+                    ""))
+           (visual-depth (if (> depth 0)
+                             (make-string depth #\space)
+                             "")))
+      (format (current-warning-port)
+              ";;; loading ~a~a ~a~a~%" pad depth visual-depth file)
+      (force-output (current-warning-port)))))
 
 (set! %load-hook %load-announce)
 
@@ -3250,6 +3253,10 @@ deterministic."
     (set-module-declarative?! m (user-modules-declarative?))
     m))
 
+;;; This parameter is used to track the depth at which modules are
+;;; loaded.
+(define %current-module-load-depth (make-parameter -1))
+
 ;; NOTE: This binding is used in libguile/modules.c.
 ;;
 (define resolve-module
@@ -3272,8 +3279,10 @@ deterministic."
              already)
             (autoload
              ;; Try to autoload the module, and recurse.
-             (try-load-module name version)
-             (resolve-module name #f #:ensure ensure))
+             (parameterize ((%current-module-load-depth
+                             (1+ (%current-module-load-depth))))
+               (try-load-module name version)
+               (resolve-module name #f #:ensure ensure)))
             (else
              ;; No module found (or if one was, it had no public interface), and
              ;; we're not autoloading. Make an empty module if #:ensure is true.
@@ -3584,7 +3593,8 @@ but it fails to load."
                        (call/ec
                         (lambda (abort)
                           (primitive-load-path (in-vicinity dir-hint name)
-                                               abort)
+                                               abort
+                                               (%current-module-load-depth))
                           (set! didit #t)))))))
                 (lambda () (set-autoloaded! dir-hint name didit)))
               didit))))))
@@ -4406,7 +4416,7 @@ when none is available, reading FILE-NAME with READER."
       (if compiled
           (begin
             (if %load-hook
-                (%load-hook abs-file-name))
+                (%load-hook abs-file-name (%current-module-load-depth)))
             (compiled))
           (start-stack 'load-stack
                        (primitive-load abs-file-name)))))
-- 
2.41.0

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1919 bytes --]

Something I didn't notice previously:

Op 25-09-2023 om 16:29 schreef Maxim Cournoyer:
> +  if (scm_is_string (args)) {
> +      /* C code written for 3.9 and earlier expects this function to
> +         take a single argument (the file name).  */
> +      filename = args;
> +      depth = scm_from_int(0);
> +    }

IIUC, args is always a list and never a string.  However, it might be a 
list of one element (being a string) or two elements.

 >+  else {
 >+    /* Starting from 3.10, this function takes 1 required and 1 >optional
 >+       arguments. */
 >+    long len;
 >+
 >+    SCM_VALIDATE_LIST_COPYLEN (SCM_ARG1, args, len);
 >+    if (len < 1 || len > 2)
 >+      scm_error_num_args_subr (FUNC_NAME);
 >+
 >+    filename = SCM_CAR (args);
 >+    SCM_VALIDATE_STRING (SCM_ARG1, filename);
 >+
 >+    depth = len > 1 ? SCM_CADR (args) : scm_from_int(0);

While I suppose this would work, you are using the rest argument to 
manually implement optional elements.  I think it would be quite a bit 
simpler to just use the optional elements instead. (I.e., set opt=1 
instead of rst=1 and keep req=1 in SCM_DEFINE).

Adding an extra argument is, however, a C API and ABI break, so I'd 
rename it to scm_primitive_load2 and let scm_primitive_load be a small 
wrapper for scm_primitive_load2.

... now I think about it and re-read the C comments, maybe the (lack of) 
C API/ABI break is why you are using rest arguments here?  I think a new 
function is cleaner and less risky though -- I don't think you are 
supposed to do (primitive-load (list "a" 0)) in Scheme yet this code 
allows that.

Likely the same applies to s_scm_primitive_load_path.

> * doc/guile-api.alist (%load-announce, %load-hook): Add DEPTH argument.

Technically backwards-incompatible but I don't know any actual users of 
%load-hook etc. outside Guile itself.

Best regards,
Maxime Devos

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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:

> Something I didn't notice previously:
>
> Op 25-09-2023 om 16:29 schreef Maxim Cournoyer:
>> +  if (scm_is_string (args)) {
>> +      /* C code written for 3.9 and earlier expects this function to
>> +         take a single argument (the file name).  */
>> +      filename = args;
>> +      depth = scm_from_int(0);
>> +    }
>
> IIUC, args is always a list and never a string.  However, it might be
> a list of one element (being a string) or two elements.

Surprisingly perhaps, this works, I guess because a string is an array.
See the 2009 commit 31ab99de563027fe2bceb60bbd712407fcaf868e which
exploits the same feature.

>>+  else {
>>+    /* Starting from 3.10, this function takes 1 required and 1 >optional
>>+       arguments. */
>>+    long len;
>>+
>>+    SCM_VALIDATE_LIST_COPYLEN (SCM_ARG1, args, len);
>>+    if (len < 1 || len > 2)
>>+      scm_error_num_args_subr (FUNC_NAME);
>>+
>>+    filename = SCM_CAR (args);
>>+    SCM_VALIDATE_STRING (SCM_ARG1, filename);
>>+
>>+    depth = len > 1 ? SCM_CADR (args) : scm_from_int(0);
>
> While I suppose this would work, you are using the rest argument to
> manually implement optional elements.  I think it would be quite a bit
> simpler to just use the optional elements instead. (I.e., set opt=1
> instead of rst=1 and keep req=1 in SCM_DEFINE).
>
> Adding an extra argument is, however, a C API and ABI break, so I'd
> rename it to scm_primitive_load2 and let scm_primitive_load be a small
> wrapper for scm_primitive_load2.
>
> ... now I think about it and re-read the C comments, maybe the (lack
> of) C API/ABI break is why you are using rest arguments here?  I think
> a new function is cleaner and less risky though -- I don't think you
> are supposed to do (primitive-load (list "a" 0)) in Scheme yet this
> code allows that.
>
> Likely the same applies to s_scm_primitive_load_path.

Yes, the extra complications on the C side are to preserve backward
compatibility.

>> * doc/guile-api.alist (%load-announce, %load-hook): Add DEPTH argument.
>
> Technically backwards-incompatible but I don't know any actual users
> of %load-hook etc. outside Guile itself.

%load-hook is carefully crafted as to accept both one or two arguments
(for backward compatibility).  I didn't pay attention to %load-announce
and I'm surprised it's a public API, as its sole function seems to be
the default %load-hook value.  

-- 
Thanks,
Maxim

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1970 bytes --]



Op 02-10-2023 om 18:13 schreef Maxim Cournoyer:
>> Something I didn't notice previously:
>>
>> Op 25-09-2023 om 16:29 schreef Maxim Cournoyer:
>>> +  if (scm_is_string (args)) {
>>> +      /* C code written for 3.9 and earlier expects this function to
>>> +         take a single argument (the file name).  */
>>> +      filename = args;
>>> +      depth = scm_from_int(0);
>>> +    }
>> IIUC, args is always a list and never a string.  However, it might be
>> a list of one element (being a string) or two elements.
> Surprisingly perhaps, this works, I guess because a string is an array.
> See the 2009 commit 31ab99de563027fe2bceb60bbd712407fcaf868e which
> exploits the same feature.

While apparently it works, that does not look like a feature to me but 
rather an accident, though I don't know what hypothetical feature you 
are referring to.  Also, sure strings are arrays, but you don't seem to 
be using that anywhere, so I'm not following.

> %load-hook is carefully crafted as to accept both one or two arguments
> (for backward compatibility).  I didn't pay attention to %load-announce
> and I'm surprised it's a public API, as its sole function seems to be
> the default %load-hook value.  

The default value might be, but it's supposed to be overwritable, and 
according to the old API it's a one-argument procedure, so if there is 
an old application/library setting it to something accepting only a 
single argument, there is your API break.

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!

Best regards,
Maxime Devos.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Maxim Cournoyer]

Hello!

Maxime Devos <maximedevos@telenet.be> writes:

> Op 02-10-2023 om 18:13 schreef Maxim Cournoyer:
>>> Something I didn't notice previously:
>>>
>>> Op 25-09-2023 om 16:29 schreef Maxim Cournoyer:
>>>> +  if (scm_is_string (args)) {
>>>> +      /* C code written for 3.9 and earlier expects this function to
>>>> +         take a single argument (the file name).  */
>>>> +      filename = args;
>>>> +      depth = scm_from_int(0);
>>>> +    }
>>> IIUC, args is always a list and never a string.  However, it might be
>>> a list of one element (being a string) or two elements.
>> Surprisingly perhaps, this works, I guess because a string is an array.
>> See the 2009 commit 31ab99de563027fe2bceb60bbd712407fcaf868e which
>> exploits the same feature.
>
> While apparently it works, that does not look like a feature to me but
> rather an accident, though I don't know what hypothetical feature you
> are referring to.

I meant feature here as behavior.

> Also, sure strings are arrays, but you don't seem
> to be using that anywhere, so I'm not following.

That was just an uneducated guess about how the underlying C procedure
translated from the macros.  You can safely forget about it.

>> %load-hook is carefully crafted as to accept both one or two arguments
>> (for backward compatibility).  I didn't pay attention to %load-announce
>> and I'm surprised it's a public API, as its sole function seems to be
>> the default %load-hook value.  
>
> The default value might be, but it's supposed to be overwritable, and
> according to the old API it's a one-argument procedure, so if there is
> an old application/library setting it to something accepting only a
> single argument, there is your API break.

%load-announce is replaced whole via something like:

  set! %load-hook my-proc

placed early in your program.  Whether my-proc is a 1 or 2 arguments
procedure, it works with the proposed change (it's been tested).

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

-- 
Thanks,
Maxim

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 3068 bytes --]



Op 04-10-2023 om 03:42 schreef Maxim Cournoyer:
>>> %load-hook is carefully crafted as to accept both one or two arguments
>>> (for backward compatibility).  I didn't pay attention to %load-announce
>>> and I'm surprised it's a public API, as its sole function seems to be
>>> the default %load-hook value.
>>
>> The default value might be, but it's supposed to be overwritable, and
>> according to the old API it's a one-argument procedure, so if there is
>> an old application/library setting it to something accepting only a
>> single argument, there is your API break.
> 
> %load-announce is replaced whole via something like:
> 
>    set! %load-hook my-proc
> 
> placed early in your program.  Whether my-proc is a 1 or 2 arguments
> procedure, it works with the proposed change (it's been tested).

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

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

Best regards,
Maxime Devos

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 9396 bytes --]



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.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Maxim Cournoyer]

[-- Attachment #1: Type: text/plain, Size: 6423 bytes --]

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.

OK, I agree!

I tried using scm_program_minimum_arity in libguile/load.c like so:

--8<---------------cut here---------------start------------->8---
modified   libguile/load.c
@@ -52,6 +52,7 @@
 #include "loader.h"
 #include "modules.h"
 #include "pairs.h"
+#include "procprop.h"
 #include "procs.h"
 #include "read.h"
 #include "srfi-13.h"
@@ -80,21 +81,26 @@ static SCM *scm_loc_load_hook;
 /* The current reader (a fluid).  */
 static SCM the_reader = SCM_BOOL_F;
 
-struct hook_args_data {
-    SCM filename;
-    SCM depth;
-};
+/* Predicate to check whether HOOK is of the new type, accepting extra
+   keyword arguments and a rest argument. */
+static SCM new_hook_p (SCM hook) {
+  SCM arity;
+  uint nreqs;                   /* number of required arguments */
+  uint nopts;                   /* number of optional arguments */
+  SCM restp;                    /* accepts rest argument? */
 
-static SCM call_hook_2_body(void *data) {
-    struct hook_args_data *args_data = data;
-    scm_call_2(*scm_loc_load_hook, args_data->filename, args_data->depth);
-    return SCM_BOOL_T;
-}
+  arity = scm_procedure_minimum_arity(hook);
 
-static SCM call_hook_1_handler(void *data, SCM key, SCM args ) {
-    struct hook_args_data *args_data = data;
-    scm_call_1(*scm_loc_load_hook, args_data->filename);
+  /* In case the hook has no arity information, simply assumes it uses
+     the newer interface. */
+  if (scm_is_eq(SCM_BOOL_F, arity))
     return SCM_BOOL_T;
+
+  nreqs = scm_to_uint(scm_car(arity));
+  nopts = scm_to_uint(scm_cadr(arity));
+  restp = scm_caddr(arity);
+
+  return scm_from_bool(!restp && (nreqs + nopts) <= 1);
 }
 
 /* Helper to call %load-hook with the correct number of arguments. */
@@ -102,15 +108,11 @@ static void call_hook (SCM hook, SCM filename, SCM depth) {
   if (scm_is_false (hook))
     return;
 
-  struct hook_args_data args_data;
-  args_data.filename = filename;
-  args_data.depth = depth;
-
-  /* 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);
+  if (new_hook_p(hook)) {
+    scm_call_3(hook, filename, scm_from_utf8_keyword("depth"), depth);
+  } else {
+    scm_call_1(hook, filename);
+  }
 }
 
 SCM_DEFINE (scm_primitive_load, "primitive-load", 0, 0, 1,
--8<---------------cut here---------------end--------------->8---

Unfortunately, it fails to build with an error that hints that perhaps
using scm_procedure_minimum_arity that early is not supported?

--8<---------------cut here---------------start------------->8---
Making all in module
make[2]: Entering directory '/home/maxim/src/guile/module'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/home/maxim/src/guile/module'
Making all in stage0
make[2]: Entering directory '/home/maxim/src/guile/stage0'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/home/maxim/src/guile/stage0'
Making all in stage1
make[2]: Entering directory '/home/maxim/src/guile/stage1'
  BOOTSTRAP(stage1) GUILEC ice-9/boot-9.go
;;; note: source file /home/maxim/src/guile/module/ice-9/boot-9.scm
;;;       newer than compiled /home/maxim/src/guile/stage1/ice-9/boot-9.go
;;; found fresh compiled file at /home/maxim/src/guile/stage0/ice-9/boot-9.go
guile: uncaught exception:
In procedure private-lookup: Module named (system vm program) does not exist
Cannot exit gracefully when init is in progress; aborting.
make[2]: *** [Makefile:2512: ice-9/boot-9.go] Aborted
make[2]: Leaving directory '/home/maxim/src/guile/stage1'
make[1]: *** [Makefile:2205: all-recursive] Error 1
make[1]: Leaving directory '/home/maxim/src/guile'
make: *** [Makefile:2090 : all] Erreur 2
--8<---------------cut here---------------end--------------->8---

See the attached patch file below if you'd like to experiment
with it.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-on-top-of-v4-arity-check-wip.patch --]
[-- Type: text/x-patch, Size: 5359 bytes --]

From a0463d1b2aee68587e4183df1be3a16ba445ad3c Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Sat, 21 Oct 2023 23:12:08 -0400
Subject: [PATCH] on top of v4 arity check wip

---
 doc/ref/api-evaluation.texi |  6 ++---
 libguile/load.c             | 44 +++++++++++++++++++------------------
 module/ice-9/boot-9.scm     |  5 +++--
 3 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/doc/ref/api-evaluation.texi b/doc/ref/api-evaluation.texi
index 2cccf0481..6cc32ca17 100644
--- a/doc/ref/api-evaluation.texi
+++ b/doc/ref/api-evaluation.texi
@@ -872,8 +872,8 @@ top-level environment.  @var{filename} must either be a full pathname or
 be a pathname relative to the current directory.  If the variable
 @code{%load-hook} is defined, it should be bound to a procedure that
 will be called before any code is loaded.  See the documentation for
-@code{%load-hook} later in this section.  An optional second argument,
-@var{depth}, can be specified to track the depth at which modules are
+@code{%load-hook} later in this section.  An optional keyword argument,
+@var{#:depth}, can be specified to track the depth at which modules are
 loaded.
 
 For compatibility with Guile 3.9 and earlier, the C function takes only
@@ -921,7 +921,7 @@ The default @code{%load-hook} is bound to a procedure that does
 something like:
 
 @example
-(define (%load-hook file depth)
+(define* (%load-hook file #:key (depth 0) #:allow-other-keys)
   (when %load-verbosely
     (let* ((pad-count (- 3 (string-length (number->string depth))))
            (pad (if (> pad-count 0)
diff --git a/libguile/load.c b/libguile/load.c
index 094b6d985..3af7d8844 100644
--- a/libguile/load.c
+++ b/libguile/load.c
@@ -52,6 +52,7 @@
 #include "loader.h"
 #include "modules.h"
 #include "pairs.h"
+#include "procprop.h"
 #include "procs.h"
 #include "read.h"
 #include "srfi-13.h"
@@ -80,21 +81,26 @@ static SCM *scm_loc_load_hook;
 /* The current reader (a fluid).  */
 static SCM the_reader = SCM_BOOL_F;
 
-struct hook_args_data {
-    SCM filename;
-    SCM depth;
-};
+/* Predicate to check whether HOOK is of the new type, accepting extra
+   keyword arguments and a rest argument. */
+static SCM new_hook_p (SCM hook) {
+  SCM arity;
+  uint nreqs;                   /* number of required arguments */
+  uint nopts;                   /* number of optional arguments */
+  SCM restp;                    /* accepts rest argument? */
 
-static SCM call_hook_2_body(void *data) {
-    struct hook_args_data *args_data = data;
-    scm_call_2(*scm_loc_load_hook, args_data->filename, args_data->depth);
-    return SCM_BOOL_T;
-}
+  arity = scm_procedure_minimum_arity(hook);
 
-static SCM call_hook_1_handler(void *data, SCM key, SCM args ) {
-    struct hook_args_data *args_data = data;
-    scm_call_1(*scm_loc_load_hook, args_data->filename);
+  /* In case the hook has no arity information, simply assumes it uses
+     the newer interface. */
+  if (scm_is_eq(SCM_BOOL_F, arity))
     return SCM_BOOL_T;
+
+  nreqs = scm_to_uint(scm_car(arity));
+  nopts = scm_to_uint(scm_cadr(arity));
+  restp = scm_caddr(arity);
+
+  return scm_from_bool(!restp && (nreqs + nopts) <= 1);
 }
 
 /* Helper to call %load-hook with the correct number of arguments. */
@@ -102,15 +108,11 @@ static void call_hook (SCM hook, SCM filename, SCM depth) {
   if (scm_is_false (hook))
     return;
 
-  struct hook_args_data args_data;
-  args_data.filename = filename;
-  args_data.depth = depth;
-
-  /* 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);
+  if (new_hook_p(hook)) {
+    scm_call_3(hook, filename, scm_from_utf8_keyword("depth"), depth);
+  } else {
+    scm_call_1(hook, filename);
+  }
 }
 
 SCM_DEFINE (scm_primitive_load, "primitive-load", 0, 0, 1,
diff --git a/module/ice-9/boot-9.scm b/module/ice-9/boot-9.scm
index a1177e172..ad9ace8c8 100644
--- a/module/ice-9/boot-9.scm
+++ b/module/ice-9/boot-9.scm
@@ -2236,7 +2236,7 @@ name extensions listed in %load-extensions."
 (define %load-verbosely #f)
 (define (assert-load-verbosity v) (set! %load-verbosely v))
 
-(define (%load-announce file depth)
+(define* (%load-announce file #:key (depth 0) #:allow-other-keys)
   (when %load-verbosely
     (let* ((pad-count (- 3 (string-length (number->string depth))))
            (pad (if (> pad-count 0)
@@ -4416,7 +4416,8 @@ when none is available, reading FILE-NAME with READER."
       (if compiled
           (begin
             (if %load-hook
-                (%load-hook abs-file-name (%current-module-load-depth)))
+                (%load-hook abs-file-name
+                            #:depth (%current-module-load-depth)))
             (compiled))
           (start-stack 'load-stack
                        (primitive-load abs-file-name)))))

base-commit: 8441d8ff5671db690eb239cfea4dcfdee6d6dcdb
prerequisite-patch-id: 73b2c2e4cdba4082690935d77d8b9df1175d82c8
prerequisite-patch-id: a4a96623fa9d08a3032734e30e01808c1f646956
prerequisite-patch-id: f983f023aedefef1c1ecfb07e99555a57ac2b62a
prerequisite-patch-id: 4745e61c60672254d05ee5936b0750bb0a904a47
-- 
2.41.0


[-- Attachment #3: Type: text/plain, Size: 2711 bytes --]


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

Ugh.  Thanks for spotting that.

[...]

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

[...]

I'm convinced here again, thanks for all the details and thoughtful
examples!  I've made the change, and given the above error encountered
trying to go fancy with the arity, I've just thrown the towel on that
and accepted a backward incompatible change for user hooks arity, that
I've documented in NEWS.  A v5 revision of this series will be sent
soon, and you'll be CC'd on it.

-- 
Thanks,
Maxim