glycerine/zygomys

(gensym) always return the same symbol after defmac

qjpcpu opened this issue · 12 comments

(defmac aaa [] (gensym) ^())
(aaa)
(gensym) ;; then gensym always returns the same symbol generated in aaa

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

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

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> 

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>

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

@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

in fact this is a bug of glisp, ref the commit
qjpcpu/glisp@76d4223

@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
                }

@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

@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.

The stack seems okay.

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

@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.