Re: [PATCH] Per-module reader

[Neil Jerram]

address@hidden (Ludovic Courtès) writes:

> Hi,
>
> The patch below adds a `#:reader' keyword to `define-module' that allows
> modules to specify the reader that should be used to interpret them:
>
>   (define-module (the-module)
>     #:reader the-reader-for-my-favorite-syntax)

All looks good, except for concerns about (i) performance; (ii)
whether #:reader fits nicely in the big picture with the transformer
option; (iii) one possible bug.

> This way, module implementors can decide to use whatever syntax variant
> they prefer, while not having any side-effect on the code being read
> from other files or modules.  For illustration, here is an example that
> makes use of my `guile-reader' thing:
>
> --8<---------------cut here---------------start------------->8---
> (define-module (module-with-reader)
>   #:reader (let* ((elisp-char-tr
>                  (make-token-reader #\?
>                                     (token-reader-procedure
>                                      (standard-token-reader 'character)))))
>            (make-reader (cons elisp-char-tr
>                               (map standard-token-reader
>                                    '(whitespace
>                                      sexp string number colon-keyword
>                                      semicolon-comment
>                                      symbol-upper-case symbol-lower-case
>                                      quote-quasiquote-unquote)))))
>   #:use-module (system reader))

This is nice; but concerns about whether it fits with transformer are:

- Transformer is specified with #:use-syntax followed by module name,
  which is a lot different from your syntax.  I prefer your syntax,
  because use-syntax requires the transformer procedure to have the
  same name as the module it's defined in, which seems rubbish.  But
  on the other hand perhaps there is a reason for forcing the
  reader/transformer to be defined in another module - are we missing
  some subtlety here?

- As well as the inline definition option shown here, does it also
  work if the #:reader arg is a proc defined in one of the used
  modules?  I guess it can't possibly work in the circular reference
  case -- i.e. where module A uses a reader defined in module B, but B
  also uses A and B was loaded first -- so this is probably deemed
  user error - but how is that error presented to the user?

- Does it make sense to have both #:reader and #:use-syntax?  Should
  we reimplement use-syntax as a special case of #:reader?  Does it
  make sense that the places where your reader affects libguile
  (i.e. primitive-load) are different from the places where the module
  transformer affects libguile (i.e. the various versions of eval)?

> Index: ice-9/boot-9.scm
> ===================================================================
> RCS file: /cvsroot/guile/guile/guile-core/ice-9/boot-9.scm,v
> retrieving revision 1.351
> diff -u -B -b -p -r1.351 boot-9.scm
> --- ice-9/boot-9.scm  31 Jul 2005 23:36:50 -0000      1.351
> +++ ice-9/boot-9.scm  13 Sep 2005 12:28:08 -0000
> @@ -1185,7 +1185,8 @@
>    (make-record-type 'module
>                   '(obarray uses binder eval-closure transformer name kind
>                     duplicates-handlers duplicates-interface
> -                   observers weak-observers observer-id)
> +                   observers weak-observers observer-id
> +                   reader)
>                   %print-module))
>  
>  ;; make-module &opt size uses binder
> @@ -1221,7 +1222,8 @@
>                                         uses binder #f #f #f #f #f #f
>                                         '()
>                                         (make-weak-value-hash-table 31)
> -                                       0)))
> +                                       0
> +                                       read)))

Init value "read" here prevents your optimization in load.c from
taking effect - so don't you want #f ?

> Index: libguile/load.c
> ===================================================================
> RCS file: /cvsroot/guile/guile/guile-core/libguile/load.c,v
> retrieving revision 1.86
> diff -u -B -b -p -r1.86 load.c
> --- libguile/load.c   23 May 2005 19:57:20 -0000      1.86
> +++ libguile/load.c   13 Sep 2005 12:28:08 -0000
> @@ -82,15 +82,29 @@ SCM_DEFINE (scm_primitive_load, "primiti
>      scm_call_1 (hook, filename);
>  
>    { /* scope */
> +    SCM module = SCM_BOOL_F, reader = SCM_BOOL_F;
>      SCM port = scm_open_file (filename, scm_from_locale_string ("r"));
>      scm_frame_begin (SCM_F_FRAME_REWINDABLE);
>      scm_i_frame_current_load_port (port);
>  
>      while (1)
>        {
> -     SCM form = scm_read (port);
> +     SCM form;
> +
> +     if (scm_current_module () != module)
> +       {
> +         module = scm_current_module ();
> +         reader = SCM_MODULE_READER (module);
> +       }

I'm worried that this could reduce performance noticeably, and it
seems unnecessarily inefficient.  So ... <pause> ...

Surely the low level concept here is that an alternative reader is a
property of a port?  In that case, I would expect that

- the switch to the alternative reader would be implemented inside
  scm_read, using a new field on scm_t_port which can be set/read
  using set-port-reader!/port-reader

- the module level #:reader option would be implemented by
  define-module calling set-port-reader! on current-load-port.  (Note
  BTW that define-module should also do this when called to switch
  context to an already existing module.)

Note that this also raises the question of what to do when someone
types (define-module (module-with-reader)) at the REPL ... perhaps
define-module should set the reader of the current input port also?

> Index: libguile/modules.h
> ===================================================================
> RCS file: /cvsroot/guile/guile/guile-core/libguile/modules.h,v
> retrieving revision 1.28
> diff -u -B -b -p -r1.28 modules.h
> --- libguile/modules.h        31 Jul 2005 23:36:14 -0000      1.28
> +++ libguile/modules.h        13 Sep 2005 12:28:08 -0000
> @@ -45,6 +45,7 @@ SCM_API scm_t_bits scm_module_tag;
>  #define scm_module_index_binder              2
>  #define scm_module_index_eval_closure        3
>  #define scm_module_index_transformer 4
> +#define scm_module_index_reader        12

Why 12?  From the context I'd expect 5.

Regards,
        Neil