Opened 8 years ago

Closed 8 years ago

#1309 closed defect (fixed)

CHICKEN allows misplaced internal defines, which live on outside the body

Reported by: ai-artisan Owned by:
Priority: critical Milestone: 5.0
Component: core libraries Version: 4.11.0
Keywords: record type define Cc:
Estimated difficulty: hard

Description

(require-extension r7rs)

(let ()

(define-record-type bug

(make-bug)
bug?

)

)

(define b (make-bug))
(display (bug? b))
(newline)

;;output: #t

Attachments (1)

0001-Let-macros-know-if-they-run-at-toplevel.patch (38.5 KB) - added by sjamaan 8 years ago.
Initial attempt to teach compiler and evaluator about toplevel

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by ai-artisan

Summary: record types defined inside a closure lives beyond the closurerecord types defined inside a body lives beyond the body

comment:2 Changed 8 years ago by Mario Domenech Goulart

comment:3 in reply to:  2 Changed 8 years ago by ai-artisan

yes but the bug still exists at 4.11.0

comment:4 Changed 8 years ago by sjamaan

A simpler test case that triggers this bug:

(let () (##core#begin (##core#set! lala 1) (##core#begin (define (x) 1))))
(print x) ; => #<procedure (x)>

comment:5 Changed 8 years ago by sjamaan

Same without the ##core stuff, by the way. I suspect this is some internal define logic that goes wrong. Happens when compiling, too.

comment:6 Changed 8 years ago by sjamaan

Milestone: someday5.0

Gauche behaves identically to CHICKEN. Racket, Guile, Gambit and Scheme48 give an error that the define is in an expression context (after adding an expression after it).

I guess technically, CHICKEN is correct here (in the sense that R5RS undefined behaviour allows this). It is confusing though, so we may want to disallow this. It's a potentially breaking change, so I think if we decide to change this, we should do so in CHICKEN 5.

comment:7 Changed 8 years ago by sjamaan

Summary: record types defined inside a body lives beyond the bodyCHICKEN allows misplaced internal defines, which live on outside the body

comment:8 Changed 8 years ago by sjamaan

In other words, the following program which uses only standard R5RS constructs is invalid according to R5RS:

(define lala 2)
(let ()
   (begin 
     (set! lala 1)
     (begin (define (x) 1) 2)))
(print x) ; => #<procedure (x)> in CHICKEN and Gauche, error in other Schemes
Last edited 8 years ago by sjamaan (previous) (diff)

comment:9 Changed 8 years ago by sjamaan

The set! is just a red herring; any expression there triggers this bug

comment:10 Changed 8 years ago by sjamaan

This triggers the bug:

(let ()
   1
   (begin
     (define (x) 1)
     2))

Removing the 1 or the superfluous begin (which should have no effect) causes the bug to disappear.

Version 0, edited 8 years ago by sjamaan (next)

comment:11 Changed 8 years ago by sjamaan

See also #1297 (which I just closed as a duplicate of this ticket)

comment:12 Changed 8 years ago by sjamaan

Estimated difficulty: hard

comment:13 Changed 8 years ago by sjamaan

OK, this is pretty bad. We can hack in a special case for begin, but that won't fix it in general, because something like (print (define x 1)) can also occur inside the body and won't be detected. Interestingly, this seems to be how Gauche does it, because the following program runs:

(print (define (x) 1))
(print (x))
;; Prints #<undef> followed by 1

I had a look at how Scheme48 does it:

Their definitions are disabled completely, except at toplevel and when parsing internal defines. Any other place will trigger an error. Their definition of define is:

(define-expander 'define
  (lambda (op op-node exp env)
    (syntax-violation 'define
		      (if (destructure-define exp)
			  "definition in expression context"
			  "ill-formed definition")
		      exp)))

When toplevel is walked, they handle define and friends specially. This also allows them to treat define differently from set!.

Gambit seems to take a similar approach:

(define (##comp-define cte src tail? subexpr?)
  (if (or subexpr?
          (##not ##allow-inner-global-define?))
      (##raise-expression-parsing-exception
       'ill-placed-define
       src)
      ...))

In other words, ##comp-define knows if it's being processed as a toplevel expression or as a sub-expression. There should never be a define in a subexpression (internal defines are transformed to letrec so those are gone after processing a lambda, for example).

Whichever approach we take, a proper fix for this will require quite a bit of rework and hardcoding all defining forms into the compiler (which we have to do already anyway, to make internal defines work).

I have to agree with the Scheme48 authors: ; The horror of internal defines

comment:14 Changed 8 years ago by sjamaan

OK, this is pretty bizarre: in several places in the tests, we use functors and modules in an expression context.

(let ((foo (module blah () 1))) foo)
=> 1

In this regard, I'm going to say module is like eval in that it resets the state to a fresh "toplevel context" for the evaluation/compilation of its body. Very weird!

Changed 8 years ago by sjamaan

Initial attempt to teach compiler and evaluator about toplevel

comment:15 Changed 8 years ago by sjamaan

Now all that remains is to add some tests.

comment:16 Changed 8 years ago by sjamaan

Because there's no r7rs egg for CHICKEN 5 yet, here's a testcase that contains just the minimum required code for r7rs define-record-type:

(begin-for-syntax

  (define (macro-handler name)
    (cond ((assq name (##sys#macro-environment)) => caddr)
          (else #f)))

  (define (wrap-er-macro-transformer name handler)
    (er-macro-transformer
     (let ((orig (macro-handler name)))
       (lambda (x r c)
         (let ((e (##sys#current-environment)))
           (handler x r c (lambda (x*) (orig x* '() e)))))))))

(let ()
  (define-record-type bug
    (make-bug)
    bug?) )

(define b (make-bug))
(display (bug? b))
(newline)

comment:17 Changed 8 years ago by sjamaan

Resolution: fixed
Status: newclosed

This is fixed in the chicken-5 branch, by the following set of revisions:

  • 61241e5d58299264ff0a8a7318288906ff710660
  • d345e514c10956a7e95267dfd027725f89394122
  • c9220247dbcdf6fd39697b428cfd40068244219a
  • bd0aa1c6088d865988cf5afc2f53dad36cbe2d3b
Note: See TracTickets for help on using tickets.