Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#1227 closed defect (fixed)

parameterize bug

Reported by: Mario Domenech Goulart Owned by:
Priority: major Milestone: 4.11.0
Component: core libraries Version: 4.10.x
Keywords: Cc: initerm@…
Estimated difficulty:

Description

The following message was sent by Joo ChurlSoo? by e-mail:

The followin seems to be a bug.

CHICKEN
(c) 2008-2015, The CHICKEN Team
(c) 2000-2007, Felix L. Winkelmann
Version 4.10.0 (rev b259631)
windows-mingw32-x86 [ manyargs dload ptables ]
compiled 2015-08-04 on yves.more-magic.net (Linux)
#;1>
(define a (make-parameter 1 number->string))
(define b (make-parameter 2 number->string))
(list (a) (b))

#;2> #;3> ("1" "2")
#;4> (parameterize ((a 10) (b 20) (a 30)) (list (a) (b)))
("30" "20")
#;5> (list (a) (b))
("10" "2")
#;6> (a 1)
"1"
#;7> (list (a) (b))
("1" "2")
#;8> (parameterize ((a 10) (b 20) (a 'abc)) (list (a) (b)))

Error: (number->string) bad argument type: abc
#;8> (list (a) (b))
("10" "20")
#;9> (a 1) (b 2)
"1"
#;10> "2"
#;11> (parameterize ((a 10) (b 20) (a 30) (a 40) (b 30)) (list (a) (b)))
("40" "30")
#;12> (list (a) (b))
("30" "20")

Change History (3)

comment:1 Changed 5 years ago by sjamaan

This is going to be very tricky to fix in an efficient manner, if you assume any parameterize may fail/throw an exception.

AFAICT, the only truly reliable way to do that is to actually expand it in the "naive" way, exactly as if you had a nested parameterize call for every variable, like so:

#;1> (define a (make-parameter 1 number->string))
(define b (make-parameter 2 number->string))
(list (a) (b)) 
("1" "2")

#;2>
(parameterize ((a 10))
  (parameterize ((b 20))
    (parameterize ((a 'abc))
      (list (a) (b)))))
Error: (number->string) bad argument type: abc
#;3> (list (a) (b))
("1" "2")

Unfortunately, if we did this, it would mean one dynamic-wind per parameterized variable. Dynamic-wind is pretty inefficient, so if you're setting several variables at once, this will slow things down a lot. Anything else needs to deal with nonlocal exits and of course exception handling.

However, there are still a few improvements we can make!

First, the behaviour of (parameterize ((a 10) (b 20) (a 30)) (list (a) (b))) can be fixed relatively easy by performing the restores of the variables in reverse (or perhaps storing the parameters in a list and performing some sort of deduplication, but that's of course much trickier).

OK, so looking at it again I think it can be fixed after all.

Our main problem is the following:

#;1> (define a (make-parameter 1 number->string))
(define b (make-parameter 2 number->string))
#;2> (list (a) (b))
("1" "2")
(parameterize ((a 10) (b 'abc)) (list (a) (b)))
Error: (number->string) bad argument type: abc
#;3> (list (a) (b))
("10" "2")

This could in theory be fixed by first calculating all the converted values in a big let and only after they've been converted, set the parameters to the new value. This would require changing the internal "hidden" API that's used. Currently there's no way to extract the converter from a parameter; you can only set the parameter to a value, optionally passing it through the converter.

Currently, the expansion looks something like this:

(let ((a2 a) (b4 b))
  (let ((g6 10) (g8 'abc))
    (let ((mode #f))
      (let ((swap (lambda ()
                    (let ((t (a2))) (a2 g6 mode) (set! g6 t))
                    ;; If this raises an exception, a2 won't be restored
                    (let ((t (b4))) (b4 g8 mode) (set! g8 t))
                    (set! mode #t))))
        (dynamic-wind swap (lambda () (list (a) (b))) swap)))))

So to make this work, we'd need something like the following (inventing a new parameter API as I go along):

(let ((a2 a) (b4 b))
  (let ((g6 10) (g8 'abc))
    (let ((mode #f))
      (let ((swap (lambda ()
                    (let ((new-a (if mode g6 (a2 g6 'convert-only)))
                          (new-b (if mode g8 (b4 g8 'convert-only)))
                      ;; Remember current values
                      (set! g6 (a2))
                      (set! g8 (b4))
                      ;; Now we can safely set the values
                      (a2 new-a 'set-only)
                      (b4 new-b 'set-only)
                      (set! mode #t))))
        (dynamic-wind swap (lambda () (list (a) (b))) swap)))))

Here we really should also use two flavours of swap like I described above: the "after" setting the parameters in reverse order, to avoid setting a to the incorrect value if it's parameterized twice in the same expression. If we do this, we also don't need to mess about with the mode variable, which looks a bit iffy to me. As I understand it, mode won't get toggled if something breaks halfway through. On the other hand, if that happens we're screwed anyway, because half the temp variables that are being swapped will have the old values and the other half the new values.

Last edited 5 years ago by sjamaan (previous) (diff)

comment:2 Changed 5 years ago by sjamaan

Milestone: someday4.11.0

comment:3 Changed 5 years ago by evhan

Resolution: fixed
Status: newclosed

Fixed by a94b69e and 9a63e35.

Last edited 5 years ago by evhan (previous) (diff)
Note: See TracTickets for help on using tickets.