Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with syntax-rules expander? #532

Closed
pclouds opened this issue Oct 8, 2019 · 33 comments
Closed

Problem with syntax-rules expander? #532

pclouds opened this issue Oct 8, 2019 · 33 comments

Comments

@pclouds
Copy link
Contributor

pclouds commented Oct 8, 2019

This is a simplified code from '(chibi monad environment)'

(import (scheme base))

(define-syntax abc
  (syntax-rules ()
    ((_ n ((field get put) ...))
     (define-record-type n
         (make-state field ... %props)
         state?
       (field get put) ...
       (%props get-props set-props!)))))

(abc foo ((port env-port env-port-set!)
          (output env-output env-output-set!)))

The expansion should be something like this, which I tested was working

(define-record-type foo
    (make-state port output %props)
    state?
  (port env-port env-port-set!)
  (output env-output env-output-set!)
  (%props get-props set-props!))

But instead I got

*** ERROR: rtd-constructor: field-specs contains unrecognized field name: #<identifier r7rs.user#%props.f77c7100>

Not sure what was the problem (might as well be me...)

@shirok
Copy link
Owner

shirok commented Oct 8, 2019

I've seen something similar to that related to define-record-type expansion. It was written before er-macro-expander and doing lots of kludge with define-macro. Let me see...

@shirok
Copy link
Owner

shirok commented Oct 8, 2019

Ah, I see. We unwrap identifiers to make field names (in fieldspecs->slotspecs, libsrc/gauche/record.scm), since records are also Gauche classes and it assumes slot names are symbols. But then we can't match up that slot with the arguments given to the constructor spec, which became an identifier.

If we also unwrap the constructor arguments, it works in in this example, but it becomes unhygienic and will fail on more complex cases. My idea is to replace them with gensym-ed names.

@Hamayama
Copy link
Contributor

Hamayama commented Oct 8, 2019

I have a question.

define-record-type has a function which generates accessor names automatically.

So, I think hygienic macro (syntax-rules) can't realize define-record-type.

In above case, if (%props get-props set-props!) changed to (%props),
what accessor name should be generated?

@shirok
Copy link
Owner

shirok commented Oct 9, 2019

Hayamaya: Yes, automatic generation of names do not work well with hygiene.
I think there has been a debate related to it in some srfi discussion---may be srfi-150.
At this moment, I believe it's in the undefined territory; each implementation chooses what it thinks work.

@pclouds
Copy link
Contributor Author

pclouds commented Oct 9, 2019

I'm totally ok with keeping the current behavior as-is since it's undefined. Although if someone has a hack to get it work, I'd appreciate it. I'm trying to run srfi-159 code from chibi, then remove chibi sepcific bits to make gauche support (scheme show).

@shirok
Copy link
Owner

shirok commented Oct 9, 2019

Let's keep this open until we find a reasonable solution (either fix it or wontfix).

@pclouds
Copy link
Contributor Author

pclouds commented Oct 9, 2019

Although if someone has a hack to get it work, I'd appreciate it.

Never mind. Found it :D srfi-159 here I come!

@pclouds
Copy link
Contributor Author

pclouds commented Oct 9, 2019

I'm totally ok with keeping the current behavior as-is since it's undefined.

Actually in this case, after staring some more at (chibi monad environment), get-props and set-props! are only used inside the syntax-rules block, not exposed to outside. So however they are renamed internally, it should still work because all references are also renamed consistently.

@shirok
Copy link
Owner

shirok commented Oct 9, 2019

Well, I realized there' a simple bug. Could you try this fix? It's only a missing quote.

diff --git a/libsrc/gauche/record.scm b/libsrc/gauche/record.scm
index ad34ccb..eeb2efe 100644
--- a/libsrc/gauche/record.scm
+++ b/libsrc/gauche/record.scm
@@ -444,7 +444,7 @@
       [#f '()]
       [#t `((,%define-inline ,(sym+ 'make- typename) (,%ctor ,typename)))]
       [((? id? ctor-name) field ...)
-       `((,%define-inline ,ctor-name (,%ctor ,typename ,(list->vector field))))]
+       `((,%define-inline ,ctor-name (,%ctor ,typename ',(list->vector field))))]
       [(? id? ctor-name)
        `((,%define-inline ,ctor-name (,%ctor ,typename)))]
       [x (error "invalid constructor spec" ctor-spec)]))

@pclouds
Copy link
Contributor Author

pclouds commented Oct 9, 2019

Yes that fixed it! I'm seeing another problem with syntax-rules, but unlikely related to records. I'll open another one if I can reduce the problem to some small example.

@shirok
Copy link
Owner

shirok commented Oct 9, 2019

Quote strips syntactic information. As far as we follow srfi-99's syntactic/procedural layer design, we have to quote field descriptions to pass to procedural layer, so it should be the right thing.
However, it also means field names doesn't keep hygiene (e.g. if two macros introduce new fields with the same name, they'll conflict each other). Notably, I suspect it is not compatible with srfi-150---I should check it. Anyway it's a different issue.

@pclouds
Copy link
Contributor Author

pclouds commented Oct 10, 2019

I can't wrap my head around this, and attempting to create a smaller example did not work out so well. So if anyone's interested, if you checkout https://github.com/pclouds/Gauche/commits/sigh and run

> ~/w/gauche/src $ LD_LIBRARY_PATH=. ./gosh -ftest abc.scm 
*** ERROR: unbound variable: #<identifier chibi.monad.environment#tell.e96cdb80>
    While loading "./abc.scm" at line 3

The "tell" is an internal macro defined by define-environment-monad in lib/chibi/show/base.scm. Now if you uncomment the these two in base.scm

  ;(tell: tell llet)
  ;(ask: ask ksa)

which gives the "tell" and "ask" macros public names, the problem is gone.

All I could tell is it starts from

(define (show-run out proc)
  (run (sequence (with! (port out)
                        (col 0)
                        (row 0)
                        (width 78)
                        (radix 10)
                        (pad-char #\space)
                        (output output-default)
                        (string-width substring-length))
                 proc)))

and gets to this part (in enviroment.scm)

       (define-syntax u
         (syntax-rules ooo ()
           ((u (prop value) ooo)
            (lambda (st)
              (tell st 'prop value) ooo
              st))))

This is NOT a problem for my work on porting srfi-159 because I removed this monad macro anyway (and probably will replace it with srfi-165). This is just in case it shows a bug in syntax-rules

@pclouds
Copy link
Contributor Author

pclouds commented Oct 11, 2019

There's definitely something wrong with macro expansion. I've updated 'sigh' branch to pprint the expansion.

The gist of this is nested macros ('u' aka 'with!' calling 'tell' macro, which should expands to '%tell' function; both 'tell' and '%tell' should be renamed) that are all defined by the top level macro define-monad-environment. A simple example shows that all macros are expanded normally.

However, pprint the macro expansion of 'with!' shows this

(lambda
 (st.0)
 (begin (tell st.0 (quote port) (quote #f)) (tell st.0 (quote col) (quote 0))
  (tell st.0 (quote row) (quote 0)) (tell st.0 (quote width) (quote 78))
  (tell st.0 (quote radix) (quote 10))
  (tell st.0 (quote pad-char) (quote #\space))
  (tell st.0 (quote output) (quote #f))
  (tell st.0 (quote string-width) (quote #f)) st.0))

"tell" is still here (and it does not look renamed either) while it should have been expanded down to '%tell'. This is probably why 'tell' is considered unbounded when executed.

@shirok
Copy link
Owner

shirok commented Oct 11, 2019

There's a hidden feature that dumps input and output of each macro expansion. It could provide some clue. Evaluate the following:

(with-module gauche.internal (set! *trace-macro* #t))

(I just found you have to autoload pprint first to make it work---just evaluate pprint)

@pclouds
Copy link
Contributor Author

pclouds commented Oct 11, 2019

Shiro oh Shiro! Had I known about *trace-macro* I would have sworn a lot less at define-environment-monad macro, which at some point I considered worse than reading assembly (though admiteddly I learned a bit more after understanding it).

It did not help much though, for some reason Gauche stopped expanding at with! and did not consider the macros inside (i.e. tell.0). Perhaps some information was lost and it did not consider tell.0 a macro anymore, I don't know.

This is the last step

Macro input>>>
(with! (port out) (col 0) (row 0) (width 78) (radix 10) (pad-char #\space)
 (output output-default) (string-width substring-length))

Macro output<<<
(lambda (st.0) (tell.0 st.0 (quote port) out) (tell.0 st.0 (quote col) 0)
 (tell.0 st.0 (quote row) 0) (tell.0 st.0 (quote width) 78)
 (tell.0 st.0 (quote radix) 10) (tell.0 st.0 (quote pad-char) #\space)
 (tell.0 st.0 (quote output) output-default)
 (tell.0 st.0 (quote string-width) substring-length) st.0)

only when I give tell macro a public name, preventing renaming then this happens after that

Macro input>>>
(tell.0 st.0 (quote port) out)

Macro output<<<
(env-port-set!.0 st.0 out)

@shirok
Copy link
Owner

shirok commented Oct 11, 2019

I've totally forgotten about *trace-macro*---I think I hacked it up out of frustration of debugging macros but it hadn't thought out well so I didn't make it an official feature. I guess I should've.

pclouds added a commit to pclouds/Gauche that referenced this issue Feb 2, 2020
The environment monad in chibi is created by a macro provided by '(chibi
environment monad)' but that macro does not work in Gauche. Problem with
rtd-constructor, do manual expansion (see shirok#532).

Avoid that macro by providing the expanded version of the monad. In the
future this code is probably replaced with srfi-165.
pclouds added a commit to pclouds/Gauche that referenced this issue Feb 2, 2020
The environment monad in chibi is created by a macro provided by '(chibi
environment monad)' but that macro does not work in Gauche. Problem with
rtd-constructor, do manual expansion (see shirok#532).

Avoid that macro by providing the expanded version of the monad. In the
future this code is probably replaced with srfi-165.
@pclouds
Copy link
Contributor Author

pclouds commented Mar 4, 2020

I looked at this again after srfi-159 has been merged. Even after replacing monad.scm (the expanded version) with define-monad-environment from chibi (i.e. the original version), the problem was gone, all tests passed. So syntax-rules looked innocent.

However, that was the macro defintion and macro expansion in the same .scm file. If I move the define-monad-environment macro to a separate library, problems reappear. Plenty of srfi-159 tests failed with "unbound variable: #<identifier ...>".

So it probably has something to do with exporting symbols (or identifiers or whatever that is) not with hygienic renaming business.

Changes are in my "sigh" branch. The top commit is bad, the one before that good.

@pclouds
Copy link
Contributor Author

pclouds commented Mar 5, 2020

The problem is "fixed" by moving the wrapper macro outside the library. Before pclouds@0b3ddbe

> ~/w/gauche/src $ ./gosh -ftest -usrfi-159
gosh$ (show #f "hi")
*** ERROR: unbound variable: #<identifier srfi-159.monad#tell.63aa6600>
Stack Trace:
_______________________________________
  0  (tell st 'port out)
        [unknown location]
  1  (tell st 'port out)
        [unknown location]
  2  (show-run out proc)
        at "../lib/srfi-159/base.scm":40
  3  (eval expr env)
        at "../lib/gauche/interactive.scm":268

and after it

> ~/w/gauche/src $ ./gosh -ftest -usrfi-159
gosh$ (show #f "hi")
"hi"

tell is a macro defined inside a macro dem which is expanded in macro define-environment-monad, perhaps that messes up some rules...

@pclouds
Copy link
Contributor Author

pclouds commented Mar 5, 2020

Small example to demonstrate. This yyy.scm works

(import (scheme base) (scheme write))
(define-syntax x1
  (syntax-rules ()
    ((x1 y1)
     (x2 x3 y1))))

(define-syntax x2
  (syntax-rules ()
    ((x2 x3 y1)
     (begin
       (define-syntax x3
         (syntax-rules ()
           ((x3 x4) x4)))
       (define-syntax y1
         (syntax-rules ()
           ((y1 y2) (x3 y2))))))))

(x1 bar)
(write (bar 1))

while this yyy.scm does not

(import (scheme base) (scheme write) (xxx))
(x1 bar)
(write (bar 1))

the xxx.scm contains the same part from the first yyy.scm

(define-library (xxx)
  (export x1)
  (import (scheme base))

  (begin
    (define-syntax x1
      (syntax-rules ()
        ((x1 y1)
         (x2 x3 y1))))

    (define-syntax x2
      (syntax-rules ()
        ((x2 x3 y1)
         (begin
           (define-syntax x3
             (syntax-rules ()
               ((x3 x4) x4)))
           (define-syntax y1
             (syntax-rules ()
               ((y1 y2) (x3 y2))))))))))

@Hamayama
Copy link
Contributor

Hamayama commented Mar 5, 2020

I changed yyy1.scm and yyy2.scm as follows.

yyy1.scm

(import (scheme base) (scheme write)
        (only (gauche interactive) apropos))

(define-syntax x1
  (syntax-rules ()
    ((x1 y1)
     (x2 x3 y1))))

(define-syntax x2
  (syntax-rules ()
    ((x2 x3 y1)
     (begin
       (define-syntax x3
         (syntax-rules ()
           ((x3 x4) x4)))
       (define-syntax y1
         (syntax-rules ()
           ((y1 y2) (x3 y2))))))))

(x1 bar)
(write (bar 1))
(newline)
(apropos 'x1)
(apropos 'x2)
(apropos 'x3)
(apropos 'bar)

yyy2.scm

(import (scheme base) (scheme write) (xxx)
        (only (gauche interactive) apropos))
(x1 bar)
;(write (bar 1))
(newline)
(apropos 'x1)
(apropos 'x2)
(apropos 'x3)
(apropos 'bar)

Then, the result is as follows.

> gosh yyy1.scm
1
x1                             (r7rs.user)
x2                             (r7rs.user)
x3                             (r7rs.user)
bar                            (r7rs.user)
> gosh -A. yyy2.scm
x1                             (xxx)
x2                             (xxx)
x3                             (r7rs.user)
bar                            (r7rs.user)

So, in the latter case, x1 and x3 is in the different modules.

This might be a something hint but I don't know this is bug or spec ...

@pclouds
Copy link
Contributor Author

pclouds commented Mar 6, 2020

I didn't know apropos could be used this way, thanks! I don't know if it's bug or spec either but it worked with at least sagittarius and chibi (didn't try more schemes)

@pclouds
Copy link
Contributor Author

pclouds commented Mar 6, 2020

It's a bit clearer with (trace-macro #t)

Macro input>>>
(x1 bar)

Macro output<<<
(x2.0 x3.0 bar)

Macro input>>>
(x2.0 x3.0 bar)

Macro output<<<
(begin
  (define-syntax x3.0
    (syntax-rules ()
      ((x3.0 x4.0)
       x4.0)))
  (define-syntax bar
    (syntax-rules ()
      ((bar y2.0)
       (x3.0 y2.0)))))

Macro input>>>
(bar 1)

Macro output<<<
(x3.0 1)

All the expansions look ok, except that x3.0 is not bounded...

@pclouds
Copy link
Contributor Author

pclouds commented Mar 6, 2020

If we make x3 a procedure instead of a macro, then it works. Sounds to me like nested define-syntax should be exported the same way as define?

(define-library (xxx)
  (export x1)
  (import (scheme base))

  (begin
    (define-syntax x1
      (syntax-rules ()
        ((x1 y1)
         (x2 x3 y1))))

    (define x2
      (syntax-rules ()
        ((x2 x3 y1)
         (begin
           (define (x3 x4) x4)
           (define-syntax y1
             (syntax-rules ()
               ((y1 y2) (x3 y2))))))))))

Output with trace-macro on. It's interesting that the last line is x3.139.0 (renamed again, or?)

Macro input>>>
(x1 bar)

Macro output<<<
(x2.0 x3.0 bar)

Macro input>>>
(x2.0 x3.0 bar)

Macro output<<<
(begin (define (x3.0 x4.0) x4.0)
 (define-syntax bar (syntax-rules () ((bar y2.0) (x3.0 y2.0)))))

Macro input>>>
(bar 1)

Macro output<<<
(x3.139.0 1)

@pclouds
Copy link
Contributor Author

pclouds commented Mar 6, 2020

may be related to #328

@pclouds
Copy link
Contributor Author

pclouds commented Mar 7, 2020

This seems like a hygiene problem.

x3 macro is not renamed when defined in r7rs.user, but it's called renamed by y1 macro. yyy3.scm works even though (I think) it should not

(import (scheme base) (scheme write) (xxx))
(x1 bar)
;(write (bar 1)) ==> expanding to (x3 1)
(write (x3 1))
(newline)

@pclouds
Copy link
Contributor Author

pclouds commented Mar 8, 2020

This fixes the problem here (and still passes the test suite) but I'm too green in this area to even judge if I'm going the right direction. @shirok if you have time please have a look, greatly appreciated.

What I'm trying to do here is instead of registering the nested macro (x3) in the expanded context (yyy.scm) I try to keep the macro in macro-definition environment (xxx.scm) instead because other macros in the same level (e.g. y1) will refer to it that way. I try to use the same rename trick in pass1/define to hopefully not mess everything else up.

One thing I'm sure could go wrong is how to detect this case, wrapped-identifier? works here but it's probably too loose.

diff --git a/src/compile-1.scm b/src/compile-1.scm
index 19865a439..cd35f0802 100644
--- a/src/compile-1.scm
+++ b/src/compile-1.scm
@@ -711,23 +711,29 @@
     ($const-undef)))
 
 (define-pass1-syntax (define-syntax form cenv) :null
   (check-toplevel form cenv)
   ;; Temporary: we use the old compiler's syntax-rules implementation
   ;; for the time being.
   (match form
     [(_ name expr)
      (let* ([cenv (cenv-add-name cenv (variable-name name))]
             [transformer (pass1/eval-macro-rhs 'define-syntax expr cenv)])
-       ;; See the "Hygiene alert" in pass1/define.
-       (%insert-syntax-binding (cenv-module cenv) (unwrap-syntax name)
-                               transformer)
+       (if (wrapped-identifier? name)
+         (begin
+           (%rename-toplevel-identifier! name)
+           (%insert-syntax-binding (identifier-module name)
+                                   (unwrap-syntax name)
+                                   transformer))
+         ;; See the "Hygiene alert" in pass1/define.
+         (%insert-syntax-binding (cenv-module cenv) (unwrap-syntax name)
+                                 transformer))
        ($const-undef))]
     [_ (error "syntax-error: malformed define-syntax:" form)]))
 
 ;; Experimental
 (define-pass1-syntax (define-hybrid-syntax form cenv) :gauche
   (pass1-define-hybrid-syntax form cenv))
 (define-pass1-syntax (define-inline/syntax form cenv) :gauche ;deprecated
   (pass1-define-hybrid-syntax form cenv))
 
 (define (pass1-define-hybrid-syntax form cenv)

@shirok
Copy link
Owner

shirok commented Mar 9, 2020

Your fix does make sense, though I need to look more closely.

@shirok
Copy link
Owner

shirok commented Mar 10, 2020

Ok, the problem is certainly unwrap-syntax. It strips the information that x3 is inserted in the module xxx and treat it as if it were defined in yyy.scm. We need to do the same thing as pass1/define does with "hygiene alert" section, and your fix exactly does it.

@shirok
Copy link
Owner

shirok commented Mar 11, 2020

Does 28f229a address the original issue?

@pclouds
Copy link
Contributor Author

pclouds commented Mar 11, 2020

Works beautifully. Thanks! An immediate effect of this is gauche users can now use (chibi monad environment) from snow if they want to :)

@pclouds pclouds closed this as completed Mar 11, 2020
@shirok
Copy link
Owner

shirok commented Mar 11, 2020

Thanks for your work to identify the problem.

@Hamayama
Copy link
Contributor

Great work!
I found a similar issue (2014) . ( just a information )
okuoku/yuni#1

@shirok
Copy link
Owner

shirok commented Mar 12, 2020

Aha. And the "fix" of that particular case 0f85b90 is pretty much the same as the fix of this one. The old fix didn't involve cross-module reference, and back then modification of pass1/define fixed that particular issue, but we should've done the same thing for define-syntax and define-macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants