glycerine / zygomys

Zygo is a Lisp interpreter written in 100% Go. Central use case: dynamically compose Go struct trees in a zygo script, then invoke compiled Go functions on those trees. Makes Go reflection easy.
https://github.com/glycerine/zygomys/wiki
BSD 2-Clause "Simplified" License
1.71k stars 81 forks source link

(gensym) always return the same symbol after defmac #54

Closed qjpcpu closed 2 years ago

qjpcpu commented 2 years ago
(defmac aaa [] (gensym) ^())
(aaa)
(gensym) ;; then gensym always returns the same symbol generated in aaa
qjpcpu commented 2 years ago

current environment's next symbol can't be updated any more, if env duplicated for generate macro

glycerine commented 2 years ago

I can reproduce this but I'm busy at the moment. Do you have a fix too?

glycerine commented 2 years ago

A temporary workaround appears to be using gensym while specifying a prefix like "hello" in this example:

zygo> (defmac aaa [] (gensym "hello") ^())
(defmac aaa [] (gensym "hello") ^())
zygo> (gensym)
(gensym)
__gensym256
zygo> (gensym "hi")
(gensym "hi")
hi257
zygo> (gensym "there")
(gensym "there")
there258
zygo> (gensym)
(gensym)
__gensym259
zygo> (aaa)
(aaa)
zygo> (gensym)
(gensym)
__gensym260
zygo> (gensym)
(gensym)
__gensym261
zygo> (gensym)
(gensym)
__gensym262
zygo> (gensym)
(gensym)
__gensym263
zygo> (gensym "hello")
(gensym "hello")
hello264
zygo> (gensym "hello")
(gensym "hello")
hello265
zygo> 
glycerine commented 2 years ago

Experimenting... each prefix can get stuck on its own, but alternating between prefixes seems to unstick the stuck prefix

zygo> (defmac bbb [] (gensym) (gensym "hello") ^())
(defmac bbb [] (gensym) (gensym "hello") ^())
zygo> (gensym)
(gensym)
__gensym253
zygo> (gensym)
(gensym)
__gensym254
zygo> (bbb)
(bbb)
zygo> (gensym)
(gensym)
__gensym255
zygo> (gensym)
(gensym)
__gensym255
zygo> (gensym)
(gensym)
__gensym255
zygo> (gensym "hello")
(gensym "hello")
hello255
zygo> (gensym "hello")
(gensym "hello")
hello256
zygo> (gensym "hello")
(gensym "hello")
hello256
zygo> (gensym)
(gensym)
__gensym256
zygo> (gensym)
(gensym)
__gensym257
zygo> (gensym)
(gensym)
__gensym258
zygo>
glycerine commented 2 years ago

Fixed by forcing env.GenSymbol to always increment env.nextsymbol. Integers are cheap.

qjpcpu commented 2 years ago

@glycerine your commit can't fix it, nextsymbol should be a global counter. below is a cornor case: I want two macro to define dynamic function, the new1 always generate a new function to println "function1", and new2 generate a new function to println "function2", but execute new2 would overwrite the function of new1.

zygo> (defmac new1 [] (let [name (gensym "func")] (println name) ^(defn ~name [] (println "function1"))))
zygo> (defmac new2 [] (let [name (gensym "func")] (println name) ^(defn ~name [] (println "function2"))))
zygo> (new1)
func255
zygo> (func255)
function1
zygo> (new2)
func255       // expect func256 not func255
zygo> (func255)
function2
qjpcpu commented 2 years ago

in fact this is a bug of glisp, ref the commit https://github.com/qjpcpu/glisp/commit/76d42238c094355165b4f8e07743ed5d2fbd6066

glycerine commented 2 years ago

@qjpcpu I'm sorry but your last comment is too terse to be understood. You'll have to explain more to convey your meaning.

I invite you to try the following fix and report back if it solves your problem and does not create other problems. I don't see any problems in the test suite, but I duplicated the environment long ago because applying the macro definition was messing up the stack. If, after some additional experience, you find this works okay, then I would be happy to change it.

diff --git a/zygo/generator.go b/zygo/generator.go
index 9b24e16..9cbcbcb 100644
--- a/zygo/generator.go
+++ b/zygo/generator.go
@@ -632,8 +632,14 @@ func (gen *Generator) GenerateCallBySymbol(sym *SexpSymbol, args []Sexp, orig Se
        if found {
                // calling Apply on the current environment will screw up
                // the stack, creating a duplicate environment is safer
-               env := gen.env.Duplicate()
-               expr, err := env.Apply(macro, args)
+               //env := gen.env.Duplicate()
+               //expr, err := env.Apply(macro, args)
+
+               // try without the env.Duplicate. Issue 54 user wants macros
+               // to be able to arbitrarily alter the current environment,
+               // and have those side effects be visible.
+               expr, err := gen.env.Apply(macro, args)
+
                if err != nil {
                        return err
                }
qjpcpu commented 2 years ago

@glycerine emm, the fix would screw up the stack, it's better to drop it. And I think my example case would hardly occur in reality, so your previous commit is enough.

Thanks

glycerine commented 2 years ago

@qjpcpu I don't have any present evidence that the fix would screw up the stack. That problem may have gotten fixed when the generator code came up to speed. I was just concerned that my test suite was not exhaustive enough to unconver stack mess-ups if they were still present.

Still, if you don't need, it would be less risky to leave it out.

glycerine commented 2 years ago

The stack seems okay.

v6.0.3 uses the proposed change to macro application above. So macros can alter their environment now.

glycerine commented 1 year ago

@qjpcpu wrote:

emm, the fix would screw up the stack, it's better to drop it. And I think my example case would hardly occur in reality, so your previous commit is enough.

So yeah, the stack screw up was too much. I've brought back in the protection I had for that in v6.0.6. That will disable part of issue/54 features, but @qjpcpu, the requestor of the feature, said felt that was the proper tradeoff too.