Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#504 closed defect (fixed)

define-syntax in begin-for-syntax body ineffective

Reported by: felix winkelmann Owned by: felix winkelmann
Priority: major Milestone: 4.9.0
Component: expander Version: 4.6.x
Keywords: define-syntax begin-for-syntax Cc: sjamaan
Estimated difficulty:

Description

(begin-for-syntax (define-syntax ...)) doesn't seem to work. I assume this is caused by the use of parameterize in eval/meta (eval.scm): restoring ##sys#macro-environment drops any new syntax definitions made.

Attachments (1)

meta-macros.diff (326 bytes) - added by sjamaan 10 years ago.
Fix for the interpreter

Download all attachments as: .zip

Change History (11)

comment:1 Changed 10 years ago by sjamaan

Shouldn't macros be evaluated with the meta-macro-environment as their macro environment? (ie, use eval/meta while expanding macro definitions)

comment:2 Changed 10 years ago by sjamaan

Never mind the idiot; that's exactly what it's being used for

comment:3 Changed 10 years ago by sjamaan

OK, I got it to work in the interpreter. Here's a simple testcase:

(module foo (listify)
  (import chicken scheme)
  (begin-for-syntax
   (define-syntax call-it-123
     (syntax-rules ()
       ((_ x)
        '(x 'x 1 2 3)))))
  (define-syntax listify
    (lambda (e r c)
      (call-it-123 list))))

(import foo)
(print (listify))

The patch works because while expanding the body of a macro, the macro environment should be that of the meta-macro-environment, but inside the thunk that is the second argument to dynamic-wind the macro-environment has already been tweaked to point to what was previously the meta-macro-environment. (at least, I think that's what's happening)

I can't get it to work in the compiler for some reason. I've replaced the compiler's eval/meta to have the same code as in eval.scm (which is more careful in restoring the various environments and works differently to a regular parameterize), but that didn't help :(

Changed 10 years ago by sjamaan

Attachment: meta-macros.diff added

Fix for the interpreter

comment:4 Changed 10 years ago by sjamaan

The example fragment is expected to print (list 1 2 3) by the way

comment:5 in reply to:  3 Changed 10 years ago by sjamaan

Replying to sjamaan:

I can't get it to work in the compiler for some reason.

Actually, strike that. I got it to work in the compiler too, but I needed to install it to see the effects. Stupid testcases :(
(actually, I needed to recompile chicken with itself after doing this, because it segfaulted initially. There may be a bug in there somewhere, unless this is expected)

I've committed and pushed a fix to the expander-simplifications branch (revision 216f2181)

comment:6 Changed 10 years ago by felix winkelmann

I can't follow. I already made the change. Do you refer to my changed code? I thought this was fixed already. Our coordination sucks.

comment:7 in reply to:  6 Changed 10 years ago by sjamaan

Replying to felix:

I can't follow. I already made the change. Do you refer to my changed code? I thought this was fixed already.

I wasn't aware you had fixed it, and unaware of that I was actually working on your fixed code. The fix didn't work for me. Also, you didn't patch the compiler; just the interpreter. My changeset adds a test, too. Please give it a try.

Our coordination sucks.

It would be a good idea if you put a comment on a ticket or close it at the same time you're pushing a fix.

comment:8 Changed 10 years ago by sjamaan

Resolution: fixed
Status: newclosed

This works now

comment:9 Changed 10 years ago by felix winkelmann

Milestone: 4.7.04.8.0

Milestone 4.7.0 deleted

comment:10 Changed 9 years ago by felix winkelmann

Milestone: 4.8.04.9.0

Milestone 4.8.0 deleted

Note: See TracTickets for help on using tickets.