Opened 9 years ago

Closed 9 years ago

#944 closed defect (fixed)

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

Reported by: sjamaan Owned by: sjamaan
Priority: major Milestone: 4.9.0
Component: expander Version: 4.8.x
Keywords: expander, macros, hygiene, modules Cc: andyjpb
Estimated difficulty:

Description

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:

Attachments (1)

0001-Fix-944-by-making-the-behvior-of-macro-renamed-defin.patch (3.3 KB) - added by sjamaan 9 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by felix winkelmann

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

comment:2 Changed 9 years ago by 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

comment:3 Changed 9 years ago by 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.

comment:4 Changed 9 years ago by 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).

comment:5 Changed 9 years ago by 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

comment:6 Changed 9 years ago by sjamaan

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

comment:7 in reply to:  5 ; Changed 9 years ago by 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

comment:8 in reply to:  7 ; Changed 9 years ago by 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

comment:9 in reply to:  8 ; Changed 9 years ago by 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.

comment:10 Changed 9 years ago by 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.

comment:11 in reply to:  6 Changed 9 years ago by 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.

comment:12 in reply to:  9 Changed 9 years ago by 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.

comment:13 Changed 9 years ago by 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.

comment:14 Changed 9 years ago by 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".

comment:15 Changed 9 years ago by sjamaan

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.