Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#445 closed defect (fixed)

define-record doesn't respect scope rules

Reported by: Mario Domenech Goulart Owned by: felix winkelmann
Priority: major Milestone: 4.9.0
Component: expander Version: 4.6.x
Keywords: Cc:
Estimated difficulty:

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.

Attachments (1)

fix-canonicalize-body.patch (3.8 KB) - added by sjamaan 9 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by 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.

comment:2 Changed 9 years ago by felix winkelmann

Owner: set to felix winkelmann
Status: newassigned

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

comment:3 Changed 9 years ago by sjamaan

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

comment:4 Changed 9 years ago by 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).

comment:5 Changed 9 years ago by 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.

comment:6 Changed 9 years ago by 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 :)

Changed 9 years ago by sjamaan

Attachment: fix-canonicalize-body.patch added

comment:7 in reply to:  6 Changed 9 years ago by felix winkelmann

Component: unknownexpander

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?

comment:8 Changed 9 years ago by 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!

comment:9 Changed 9 years ago by felix winkelmann

Resolution: fixed
Status: assignedclosed

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

comment:10 Changed 9 years ago by felix winkelmann

Milestone: 4.7.04.8.0

Milestone 4.7.0 deleted

comment:11 Changed 7 years ago by felix winkelmann

Milestone: 4.8.04.9.0

Milestone 4.8.0 deleted

Note: See TracTickets for help on using tickets.