Opened 12 years ago
Closed 12 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)
Change History (16)
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 follow-up: 7 Changed 12 years ago by
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 follow-up: 11 Changed 12 years ago by
See also #613; I think we were right in the assessment that global-alias-hook is evil and must die.
comment:7 follow-up: 8 Changed 12 years ago by
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 follow-up: 9 Changed 12 years ago by
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 follow-up: 12 Changed 12 years ago by
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.
Changed 12 years ago by
Attachment: | 0001-Fix-944-by-making-the-behvior-of-macro-renamed-defin.patch added |
---|
comment:10 Changed 12 years ago by
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 Changed 12 years ago by
comment:12 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Could you post here the actual error and how you produce it?