Summary

define-record doesn't respect scope rules

Metadata

Attachments

Description

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.

Changes and comments

[2010-12-09 18:15:47 UTC] syn wrote:

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.

[2010-12-10 09:39:59 UTC] felix changed status from new to assigned

[2010-12-10 09:39:59 UTC] felix set owner to felix

[2010-12-10 09:39:59 UTC] felix wrote:

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

[2010-12-10 15:40:33 UTC] sjamaan wrote:

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

[2010-12-11 13:11:58 UTC] felix wrote:

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

[2010-12-11 21:42:55 UTC] sjamaan wrote:

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.

[2010-12-12 13:50:27 UTC] sjamaan wrote:

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 :)

[2010-12-12 13:50:57 UTC] sjamaan attached fix-canonicalize-body.patch (description=#f)

[2010-12-16 18:15:43 UTC] felix wrote:

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?

[2010-12-16 18:15:43 UTC] felix changed component from unknown to expander

[2010-12-16 18:59:08 UTC] sjamaan wrote:

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!

[2010-12-20 12:49:38 UTC] felix changed status from assigned to closed

[2010-12-20 12:49:38 UTC] felix set resolution to fixed

[2010-12-20 12:49:38 UTC] felix wrote:

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

[2011-06-01 09:00:41 UTC] felix changed milestone from 4.7.0 to 4.8.0

[2011-06-01 09:00:41 UTC] felix wrote:

Milestone 4.7.0 deleted

[2012-09-24 21:47:48 UTC] felix changed milestone from 4.8.0 to 4.9.0

[2012-09-24 21:47:48 UTC] felix wrote:

Milestone 4.8.0 deleted