request review & testing: syncase

request review & testing: syncase

[Andy Wingo]

Hey folks,

I've rebased the syncase branch on top of current master, and fixed
everything I know about. Now you can have macros in modules that expand
to references to identifiers that are local to the module that the macro
was defined in.

I mean to say:

   (define-module (foo)
     #:use-module (ice-9 syncase)
     #:export (bar))

   (define-syntax bar
     (syntax-rules ()
       ((_ y)
        (kar (frob y)))))

   (define kar car)

   (define-syntax frob
     (syntax-rules ()
       ((_ y)
        y)))

   (define-module (baz)
     #:use-module (foo))

   (bar '(a b c d))
    => a

Give it a look-see, a whirl, a what-have-you! I'd like to merge this one
in at some point within the next week or two. I'll wait for an OK from
Ludo or Neil before doing so, though.

Peace,

Andy
-- 
http://wingolog.org/

Re: request review & testing: syncase

[Neil Jerram]

Andy Wingo <wingo@pobox.com> writes:

> Hey folks,
>
> I've rebased the syncase branch on top of current master, and fixed
> everything I know about. Now you can have macros in modules that expand
> to references to identifiers that are local to the module that the macro
> was defined in.
>
> I mean to say:
>
>    (define-module (foo)
>      #:use-module (ice-9 syncase)
>      #:export (bar))
>
>    (define-syntax bar
>      (syntax-rules ()
>        ((_ y)
>         (kar (frob y)))))
>
>    (define kar car)
>
>    (define-syntax frob
>      (syntax-rules ()
>        ((_ y)
>         y)))
>
>    (define-module (baz)
>      #:use-module (foo))
>
>    (bar '(a b c d))
>     => a

Cool.  Does it work for defmacro and define-macro too?

> Give it a look-see, a whirl, a what-have-you! I'd like to merge this one
> in at some point within the next week or two. I'll wait for an OK from
> Ludo or Neil before doing so, though.

Thanks.  Starting to review in detail now...

     Neil

Re: request review & testing: syncase

[Neil Jerram]

Andy Wingo <wingo@pobox.com> writes:

> I've rebased the syncase branch on top of current master, and fixed
> everything I know about. Now you can have macros in modules that expand
> to references to identifiers that are local to the module that the macro
> was defined in.

"serialize module information into syncase's output -- getting ready
for hygiene"

Renaming (ice-9 annotate) to (ice-9 expand-support)... is that really
compelling enough for the risk of upsetting someone (if there is
anyone!) who is using (ice-9 annotate) ?

So far, not understanding why we don't generate (@ ...) or (@@
... directly), but perhaps that will become clear.

"add modules to syntax objects (part 1, intermediate step)"

Out of interest, why use a vector here, whereas in the previous commit
you used structs?

"finish bootstrap to syntax-objects with modules"

OK.  I see now why the vector representation didn't matter.  I hadn't
realized before that psyntax.scm required itself to bootstrap.

"thread the module through syntax-case's expansion"

+SCM_DEFINE (scm_procedure_module, "procedure-module", 1, 0, 0,
+ (SCM proc),
+ "Return the module that was current when this procedure was defined.\n"
+ "Free variables in this procedure are resolved relative to the\n"
+ "procedure's module.")

Can you delete the second sentence?  I think the concept of how
variables are resolved belongs elsewhere, and mentioning it here feels
slightly worrying - suggesting (to me, very slightly, and no matter
how strange that would be) that proper resolution might only happen
for procedures for which procedure-module is called.

+#define FUNC_NAME s_scm_procedure_module
+{
+ SCM_VALIDATE_PROC (SCM_ARG1, proc);

Could you use scm_env_module () here to simplify the code?

"eval-closure-module, here hopefully not for long"

OK.

"more work on modules and hygiene, not finished yet, alas."

Hmm, "eval closures are so 1990s": I think I agree, but I believe the
idea of eval closures was to allow other top levels than modules (and
the root).  In case anyone out there is actually using a non-module
eval closure, are we inadvertently removing support for using syncase
with such top levels?

+ ;(debug-disable 'debug 'procnames)
+ ;(read-disable 'positions)

Is that temporary?

-;; The following lines are necessary only if we start making changes
-;; (use-syntax sc-expand)
-;; (load-from-path "ice-9/psyntax")

Am I correct in thinking that we have Makefile rules to automatically
do the needed generation when psyntax.scm changes?

"houston, we have hygiene"

Hooray!

- (build-global-reference (source-annotation (car e)) value mod)
+ (build-global-reference (source-annotation (car e)) value
+ (if (syntax-object? (car e))
+ (syntax-object-module (car e))
+ mod))

Didn't understand this change, and I'm not sure if the changelog
covers it.  Can you explain more?

"hygienic compilation"

OK.  Am I correct in thinking that this is just an optimization -
i.e. that the VM would correctly interpret the @ and @@ macros later
on if they weren't intercepted here?

"@ and @@ as primitive macros"

+ ASSERT_SYNTAX (scm_ilength (expr) == 3, s_bad_expression, expr);
+ ASSERT_SYNTAX (scm_ilength (scm_cadr (expr)) > 0, s_bad_expression, expr);

I think it would be helpful to also assert that the caddr is a
symbol.  (Also in @@)

"fix handling of pre-modules errors in the vm"

OK.

"fix hygiene + modules + local macros"

- #:export (pmatch ppat))
+ #:export (pmatch))
;; FIXME: shouldn't have to export ppat...

Can remove the FIXME too now!

Overall...

Great stuff!

I believe this implementation confirms the point that we're relying on
(define-module ...) to have a lexical effect, and hence (given that
define-module isn't actually syntax) on the fact that Guile reads and
evaluates top-level forms from a file or the REPL sequentially.  I
think we should just make that explicit somewhere, for anyone looking
in future at a different way of reading files.  How does the compiler
handle this, does it look specifically for define-module forms when
reading?

Regards,
        Neil

Re: request review & testing: syncase

[Andy Wingo]

Hi Neil,

Thanks for the review!

On Mon 13 Apr 2009 11:05, Neil Jerram <neil@ossau.uklinux.net> writes:

> "serialize module information into syncase's output -- getting ready
> for hygiene"
>
> Renaming (ice-9 annotate) to (ice-9 expand-support)... is that really
> compelling enough for the risk of upsetting someone (if there is
> anyone!) who is using (ice-9 annotate) ?

(ice-9 annotate) is new, it's only been in for a month or two. So it's
not possible that anyone uses it :)

> So far, not understanding why we don't generate (@ ...) or (@@
> ... directly), but perhaps that will become clear.

That's because syntax-case operates natively on "syntax objects", which
already carry information with them regarding what piece of code
introduced them into an expansion. Adding modules to that information
was a natural fit, and allowed existing predicates to keep on working
(e.g. identifier?).

Also, using a part of existing Scheme (namely, @ and @@) introduces
hygiene problems of its own. One can imagine sandbox modules in which
certain imported macros are available, but @ and @@ are not available.

(This latter point is moot in the current implementation, but that will
change; the expansion stripper will do different, appropriate things
for the memoizer and for the compiler.)

> "add modules to syntax objects (part 1, intermediate step)"
>
> Out of interest, why use a vector here, whereas in the previous commit
> you used structs?

Structs are actually vectors, within syncase at least. (This is all
within a big letrec IIRC, so these definitions don't escape the
expansion definitions.)

> "finish bootstrap to syntax-objects with modules"
>
> OK.  I see now why the vector representation didn't matter.  I hadn't
> realized before that psyntax.scm required itself to bootstrap.

That's the magical thing about psyntax.scm, that it's an expander that
expands itself. Also the source of much consternation ;-)

> "thread the module through syntax-case's expansion"
>
> +SCM_DEFINE (scm_procedure_module, "procedure-module", 1, 0, 0,
> + (SCM proc),
> + "Return the module that was current when this procedure was defined.\n"
> + "Free variables in this procedure are resolved relative to the\n"
> + "procedure's module.")
>
> Can you delete the second sentence?

Sure.

> +#define FUNC_NAME s_scm_procedure_module
> +{
> + SCM_VALIDATE_PROC (SCM_ARG1, proc);
>
> Could you use scm_env_module () here to simplify the code?

Ooh, good point. Done.

> "more work on modules and hygiene, not finished yet, alas."
>
> Hmm, "eval closures are so 1990s": I think I agree, but I believe the
> idea of eval closures was to allow other top levels than modules (and
> the root).  In case anyone out there is actually using a non-module
> eval closure, are we inadvertently removing support for using syncase
> with such top levels?

Hm, I think so. Given that Guile assumes that eval closure lookup is
idempotent, module binder procedures should have expressive power
similar to that of eval closures. I would like to remove eval closures
altogether.

> + ;(debug-disable 'debug 'procnames)
> + ;(read-disable 'positions)
>
> Is that temporary?

It was intended to be ;-) Good catch, fixed locally.

> -;; The following lines are necessary only if we start making changes
> -;; (use-syntax sc-expand)
> -;; (load-from-path "ice-9/psyntax")
>
> Am I correct in thinking that we have Makefile rules to automatically
> do the needed generation when psyntax.scm changes?

Yes. If you touch psyntax.scm and then make, you'll see it.

> - (build-global-reference (source-annotation (car e)) value mod)
> + (build-global-reference (source-annotation (car e)) value
> + (if (syntax-object? (car e))
> + (syntax-object-module (car e))
> + mod))
>
> Didn't understand this change, and I'm not sure if the changelog
> covers it.  Can you explain more?

Sure. Consider a form produced by an expansion: `(a b c)'. The origin of
the list structure could be different than the origin of the parts; for
example a pmatch macro expansion would produce a (ppat ...) form whose
contents depend on the source text, and thus the expanded form "belongs"
to the source text, but the ppat identifier was introduced by the
expansion, and thus "belongs" to the module that pmatch was defined in.

The macro the above diff comes from, syntax-type, operates both on leaf
nodes and at one level removed from the leaf node. That is to say, it
can process identifiers, for dealing with identifier-syntax, and forms,
for dealing with macro expansion. The above diff deals with a form like:

  (foo . rest)

which should be expanded out if `foo' is a macro. If `foo' is not a
macro, and not lexically bound, it is a call to a global procedure. But
that global procedure should be looked up to relative to the macro that
introduced it, not to the macro that introduced the enclosing form.

The diff says, if `foo' was introduced via hygienic expansion, scope it
relative to the macro that introduced it. (It could have been introduced
via datum->syntax-object, in which case we just give it the scope of the
containing expression, if any.)

Note that this case would fall through nicely if syntax-type recursed on
its car, but for various reasons (efficiency, clarity regarding macro
expansions) it doesn't, so we have to put in the special case.

> "hygienic compilation"
>
> OK.  Am I correct in thinking that this is just an optimization -
> i.e. that the VM would correctly interpret the @ and @@ macros later
> on if they weren't intercepted here?

No, the VM would compile to a call to the result of doing the runtime
lookup. If the thing being looked up is actually a macro, you need to
look it up at compile time -- at runtime a macro is not a procedure, so
you would get an error.

Like this, with today's guile:

    scheme@(guile-user)> ((lambda (x) (x 1 2)) if)
    ERROR: In procedure vm-run:
    ERROR: VM: Wrong type to apply: #<primitive-builtin-macro! if> [IP offset: 7]

Contrast to the behavior under the interpreter:

    scheme@(guile-user)> ,o interp #t
    scheme@(guile-user)> ((lambda (x) (x 1 2)) if)
    $2 = 2

In the future, I'm not sure what to do -- if we always use the syncase
expander, a bare "if" will yield a syntax error at expansion time.
Scheme people claim this is the correct behavior, though I would prefer
that macros have first-class values.

> "@ and @@ as primitive macros"
>
> + ASSERT_SYNTAX (scm_ilength (expr) == 3, s_bad_expression, expr);
> + ASSERT_SYNTAX (scm_ilength (scm_cadr (expr)) > 0, s_bad_expression, expr);
>
> I think it would be helpful to also assert that the caddr is a
> symbol.  (Also in @@)

Done.

> "fix hygiene + modules + local macros"
>
> - #:export (pmatch ppat))
> + #:export (pmatch))
> ;; FIXME: shouldn't have to export ppat...
>
> Can remove the FIXME too now!

Indeed!

> I believe this implementation confirms the point that we're relying on
> (define-module ...) to have a lexical effect, and hence (given that
> define-module isn't actually syntax) on the fact that Guile reads and
> evaluates top-level forms from a file or the REPL sequentially.  I
> think we should just make that explicit somewhere, for anyone looking
> in future at a different way of reading files.

That is to say, the compiler has R5RS semantics: expressions are
logically evaluated one-by-one. Adding compilation to this mix should
not affect the semantics. (That's why the compiler can compile most all
existing Guile code -- with a couple of caveats below.)

I'm not sure exactly what you mean by "lexical" effect; if you mean that
that compiling one expression can affect the next in the read sequence.
This is true. In the define-module case, what happens is that you have
code that is evaluated both at expansion time and run time -- so the
expansion-time code (that does a set-current-module) gets run before the
next expression is compiled. The compiler doesn't need to scan for
define-module; it can primitive-eval forms at expansion time.

But there is a catch, a bug really: currently we read in a whole file at
once. This is the wrong thing, because expansion of a form may alter the
current reader. We should read, expand, and compile forms one-by-one.

Cheers,

Andy
-- 
http://wingolog.org/