Macro-renamed definitions in begin bodies don't get seen by later forms (but only in modules)

[sjamaan]

Reported by Andy on IRC:

(module foo (bar)
  (import chicken scheme)
  (define-syntax bar
    (er-macro-transformer 
      (lambda (e r c)
        `(,(r 'begin)
           (,(r 'define) ,(r 'req) 1)
           (,(r 'display) ,(r 'req))))))))

This gives an error, while at toplevel it's okay. I think it should just print "1", but maybe there's a good reason why it doesn't?

Also happens with IR macros:

[felix winkelmann]

Could you post here the actual error and how you produce it?

[sjamaan]

Sorry, it was late. The actual code that reproduces the error is:

(module foo ()
   (import chicken scheme)
   (define-syntax bar
     (er-macro-transformer
       (lambda (e r c)
          `(,(r 'begin)
             (,(r 'define) ,(r 'req) 1)
             (,(r 'print) ,(r 'req))))))
   (bar))

The error is:

Warning: reference to possibly unbound identifier `req'

Error: module unresolved: foo

[felix winkelmann]

The "req" will probably not be seen outside of the expansion, but the reference in "display" should definitely work.

I have no idea what could be the reason for this.

[sjamaan]

The problem appears to be that the "define" macro calls ##sys#register-export with the macro-aliased name, so something like "req15" ends up in the module-exist-list. Later, the evaluator sees ##core#set! or the variable reference and calls ##sys#alias-global-hook on it, which returns the original identifier which was macro-aliased. This can't be found in the module-exist-list and that causes the module to be unresolvable.

The bug is in the way the alias-global-hook is invoked; it shouldn't be resolving the identifier to its bare form; this causes an unhygienic lookup. I'm not sure how to fix it yet.

BTW: I think ##sys#register-export is badly named; it doesn't register an exported identifier; it merely adds it to the list of identifiers known to be defined in the module (module-exist-list).

[sjamaan]

I've tested a couple of Schemes, and it looks like Chicken is doing something pretty wrong with generated macro names. Try the following on the REPL:

#;1> (define-syntax foo (syntax-rules () ((_) (begin (define bar 1) (display bar) (newline)))))
#;2> bar  ;; This causes an error to be thrown.  In Chicken, it just results in 1

[sjamaan]

See also #613; I think we were right in the assessment that global-alias-hook is evil and must die.

[megane]

Replying to sjamaan:

I've tested a couple of Schemes, and it looks like Chicken is doing something pretty wrong with generated macro names. Try the following on the REPL:

#;1> (define-syntax foo (syntax-rules () ((_) (begin (define bar 1) (display bar) (newline)))))
#;2> bar  ;; This causes an error to be thrown.  In Chicken, it just results in 1

I think the example should be:

#;1> (define-syntax foo (syntax-rules () ((_) (begin (define bar 1) (display bar) (newline)))))
#;2> (foo)
1
#;3> bar
1

[megane]

Replying to megane:

Works inside a module:

(module
 m1
 *
 (import chicken scheme)
 (define-syntax foo
  (syntax-rules ()
    ((_) (begin
	   (define bar 1)
	   (display bar)
	   (newline)))))
 (foo)
 (display bar))

;; Warning: reference to possibly unbound identifier `bar'

;; Error: module unresolved: m1

[sjamaan]

Replying to megane:

Replying to megane:

Works inside a module:
;; Warning: reference to possibly unbound identifier `bar'

;; Error: module unresolved: m1

It looks like we're all equally confused... That's the original bugreport!

Also, I re-tested and it turns out MIT Scheme has a similar bug:

1 ]=> (define-syntax foo (syntax-rules () ((_) (begin (define bar 1) (display bar) (newline)))))

;Value: foo

1 ]=> (foo)

;Unbound variable: bar

And Gauche behaves like Chicken:

gosh> (define-syntax foo (syntax-rules () ((_) (begin (define bar 1) (display bar) (newline)))))
#<undef>
gosh> (foo)
1
#<undef>
gosh> bar
1

Chibi and Scheme48 print 1 and then raise an exception that bar is undefined after typing "bar".
I'm still debating what the correct behavior is. Maybe (define bar 1) should really define bar at toplevel, which should be visible to the user? It doesn't feel right, though.

R5RS says: "Note that a define at top level may or may not introduce a binding; see section 5.2"

If we allow the definition to leak out and overwrite the user's code, the "fix" is actually rather trivial. I'll attach one shortly.

[felix winkelmann]

The patch looks good, but as I understand it, definitions that are results of expansions should not leak into the toplevel. What is the correct behavior and what is not is probably open to debate. Making them leak might be more useful and more intuitive.

[felix winkelmann]

Replying to sjamaan:

See also #613; I think we were right in the assessment that global-alias-hook is evil and must die.

It is necessary, since it applies the actual module-prefix.

[megane]

Replying to sjamaan:

R5RS says: "Note that a define at top level may or may not introduce a binding; see section 5.2"

I think it says there that it should create a binding if there doesn't exist one already.

​http://www.schemers.org/Documents/Standards/R5RS/HTML/r5rs-Z-H-2.html#%_toc_%_sec_5.2.1

So in the syntax-rules case it should create a binding because bar was not an argument to the macro and syntax-rules is supposed to be hygienic.

AFAUI the original example works correctly at the toplevel, but not in modules, and the syntax-rules case works in modules, but not at the toplevel.

[sjamaan]

Section 5.2 is about "define" proper, it doesn't deal with defines from macros. I don't see a difference in behavior between the syntax-rules version and the er-macro version here.

[sjamaan]

The fix from comment:8 was committed and pushed as revision 7bef21de09d279b0413ca94dbef978665c7a0f1c. I think this change is sufficient to consider this issue as "fixed".