define-record doesn't respect scope rules

[Mario Domenech Goulart]

It seems that somehow record definitions don't respect scope rules. Here's a simple example:

$ csi -n

CHICKEN
(c)2008-2010 The Chicken Team
(c)2000-2007 Felix L. Winkelmann
Version 4.6.3 
linux-unix-gnu-x86 [ manyargs dload ptables ]
compiled 2010-11-12 on mario (Linux)

#;1> (let () (define-record foo bar))
#;2> (make-foo 1)
#<foo>

I expected an unbound variable error when calling make-foo out of the let body.

[Moritz Heidkamp]

The above code expands to this:

(##core#let
  ()
  (##core#begin
    (##core#set!
      make-foo
      (##core#lambda (bar) (##sys#make-structure (##core#quote foo) bar)))
    (##core#set!
      foo?
      (##core#lambda (x) (##sys#structure? x (##core#quote foo))))
    (##core#begin
      (##core#set!
        foo-bar-set!
        (##core#lambda
          (x val)
          (##core#check (##sys#check-structure x (##core#quote foo)))
          (##sys#block-set! x 1 val)))
      (##core#set!
        foo-bar
        (##core#lambda
          (x)
          (##core#check (##sys#check-structure x (##core#quote foo)))
          (##sys#block-ref x 1))))))

This suggests that (let () (begin (define foo 1) which expands to (##core#let () (##core#begin (##core#set! foo 1))) would also leak the foo binding but somehow this is not the case.

[felix winkelmann]

Sorry, I have no idea yet why it behaves like this.

[sjamaan]

I was going to say it is because the generated symbols are not renamed, but changing that didn't fix the bug.

[felix winkelmann]

I think the problem is that ##sys#canonicalize-body compares the starting expressions of bodies with an eq? on the result of lookup. The expansion of define-record expands into definitions with a renamed version of define, and the lookup returns the global toplevel definition of define which does not compare eq? with 'define. Just a hunch, but this must somehow be the reason (this or something similar). Perhaps we should use a different lookup routine in this case: just eq? comparison with the symbol or it's macro-alias (if it exists).

[sjamaan]

I'm not sure if I understand that yet. Anyway, here's some more info:

Adding debugging info shows that define-record is expanded in a syntax environment containing the default bindings. This example shows an empty syntax environment:

(define-syntax foo (syntax-rules () ((_ x) (define x 1))))
(let () (foo a))
(print a)

And hence it throws an error when you run it. However, this gives the same problem as the original problem:

(let-syntax ((foo (syntax-rules () ((_ x) (begin (define x 1))))))
  (let () (foo a))
  (print "inside: " a))

(print "toplevel: " a)

This prints two lines. The a leaks out here too.

[sjamaan]

Felix, of course you were correct: the eq? in canonicalize-body is wrong.

The reason is that the current syntax environment contains a macro (a list containing the expansion handling procedure and the syntax env of that handler), which can't be eq?ed to a macro name of course.

I fixed this by looking up the value in the current syntax env and if it's a macro comparing its handler to all handlers in the core syntax env. If the handler is found, use the name that handler is bound to in the core syntax env.

If the name is not found in the current syntax env, the name is just returned as that name would be looked up in the core syntax env by the macro expander, so it's either already the core name, or simply not a macro at all.

I added a couple of nasty tests I could come up with. Hopefully this provides complete coverage, but probably not :)

[felix winkelmann]

Replying to sjamaan:

I fixed this by looking up the value in the current syntax env and if it's a macro comparing its handler to all handlers in the core syntax env. If the handler is found, use the name that handler is bound to in the core syntax env.

If the name is not found in the current syntax env, the name is just returned as that name would be looked up in the core syntax env by the macro expander, so it's either already the core name, or simply not a macro at all.

I think it would be a bit simpler to just remember the SE-entries generated in the initial macro-env (I changed ##sys#extend-macro-environment recently to return the entry) and directly compare the resolved name with the entry. What do you think?

[sjamaan]

That's a little too abstract for me to grok. Could you post a patch that does this?

Anyway, if it's simpler and it passes the tests in my patch, it's probably better!

[felix winkelmann]

I recently committed a change similar to Peter's (and not necessarily better, but it seems to work and passes the test-cases).

[felix winkelmann]

Milestone 4.7.0 deleted

[felix winkelmann]

Milestone 4.8.0 deleted