Opened 15 months ago

Closed 7 months 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 10 months ago.
Initial attempt to teach compiler and evaluator about toplevel

Download all attachments as: .zip

Change History (18)

comment:1 Changed 15 months ago by ai-artisan

  • Summary changed from record types defined inside a closure lives beyond the closure to record types defined inside a body lives beyond the body

comment:2 follow-up: Changed 15 months ago by mario

comment:3 in reply to: ↑ 2 Changed 15 months ago by ai-artisan

yes but the bug still exists at 4.11.0

comment:4 Changed 15 months 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 15 months 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 15 months ago by sjamaan

  • Milestone changed from someday to 5.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 15 months ago by sjamaan

  • Summary changed from record types defined inside a body lives beyond the body to CHICKEN allows misplaced internal defines, which live on outside the body

comment:8 Changed 15 months 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 15 months ago by sjamaan (previous) (diff)

comment:9 Changed 15 months ago by sjamaan

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

comment:10 Changed 15 months ago by sjamaan

This triggers the bug:

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

(print x) ; => #<procedure (x)> in CHICKEN and Gauche, error in other Schemes

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

Last edited 15 months ago by sjamaan (previous) (diff)

comment:11 Changed 15 months ago by sjamaan

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

comment:12 Changed 14 months ago by sjamaan

  • Estimated difficulty set to hard

comment:13 Changed 11 months 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 10 months 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 10 months ago by sjamaan

Initial attempt to teach compiler and evaluator about toplevel

comment:15 Changed 10 months ago by sjamaan

Now all that remains is to add some tests.

comment:16 Changed 8 months 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 7 months ago by sjamaan

  • Resolution set to fixed
  • Status changed from new to closed

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.