Quite sneaky bug in SRFI-9

[Andreas Rottmann <a.rottmann@gmx.at>]

[-- Attachment #1: Type: text/plain, Size: 1951 bytes --]

Hi!   After being puzzled for some time, I found the root cause 
for an issue with my code was actually a quite serious bug in 
SRFI-9's `define-inlinable' (here we go again -- but this time 
it's an actual bug, I promise ;-)):  The current implementation of 
`define-inlinable' duplicates actual parameters passed to the 
macros it generates, for example (note the `(find-next-solution! s 
10000)' expression):  scheme@(guile-user)> (tree-il->scheme 
(macroexpand '(solution? (find-next-solution! s 10000))))
$6 = (if ((@@ (srfi srfi-9) struct?) (find-next-solution! s 10000))
         ((@@ (srfi srfi-9) eq?) 
              ((@@ (srfi srfi-9) struct-vtable) (find-next-solution! s 10000))
              (@@ (dorodango solver) solution))
          #f)


Attached is a patch to fix this (along with a test case), but I guess
it's not as good as it could be:

- It doesn't preserve the current error-at-syntax-time behavior when an
  argument count mismatch occurs (this makes the first two "constructor"
  testcases in srfi-9.test fail, and obsolete).  I don't know if this is
  considered important.

- It's certainly not as efficient as the current, broken implementation.
  Ideally, the expansion would be a `let' block instead of the
  `call-with-values' invocation.  I'd guess the former might be more
  amendable for optimization, but perhaps I'm wrong here.  I didn't
  implement the solution using `let', as I believe it cannot be done in
  a straightforward way (i.e. by exploiting the pattern matcher only)
  with Guile's current syntax-case implementation.

Has anyone done benchmarks of the benefits of the current (broken)
SRFI-9 accessors and predicates vs. regular procedures on real code?
Given the argument duplication issue, it might be the case that
`define-inlinable' actually slowed things down :-).

Perhaps removing `define-inlinable' entirely would be an acceptable
alternative fix for this issue?

Anyway, here's the patch:



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: srfi-9-fix2.diff --]
[-- Type: text/x-diff, Size: 2561 bytes --]

From: Andreas Rottmann <a.rottmann@gmx.at>
Subject: Fix argument duplication `define-inlinable' in SRFI-9

* modules/srfi/srfi-9.scm (define-inlinable): Don't duplicate argument
  expressions in the expansion of the macros generated.
* test-suite/tests/srfi-9.test (constructor): Adapted.
  (side-effecting arguments): New test for the argument duplication
  issue.

---
 module/srfi/srfi-9.scm       |    5 +++--
 test-suite/tests/srfi-9.test |   22 +++++++++-------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/module/srfi/srfi-9.scm b/module/srfi/srfi-9.scm
index fad570b..34dbb53 100644
--- a/module/srfi/srfi-9.scm
+++ b/module/srfi/srfi-9.scm
@@ -87,8 +87,9 @@
              (define-syntax name
                (lambda (x)
                  (syntax-case x ()
-                   ((_ formals ...)
-                    #'(begin body ...))
+                   ((_ . arguments)
+                    #'(call-with-values (lambda () (values . arguments))
+                        (lambda (formals ...) body ...)))
                    (_
                     (identifier? x)
                     #'proc-name))))))))))
diff --git a/test-suite/tests/srfi-9.test b/test-suite/tests/srfi-9.test
index f8006c4..6766482 100644
--- a/test-suite/tests/srfi-9.test
+++ b/test-suite/tests/srfi-9.test
@@ -38,21 +38,10 @@
 
 (with-test-prefix "constructor"
 
-  ;; Constructors are defined using `define-integrable', meaning that direct
-  ;; calls as in `(make-foo)' lead to a compile-time psyntax error, hence the
-  ;; distinction below.
-
-  (pass-if-exception "foo 0 args (inline)" exception:syntax-pattern-unmatched
-     (compile '(make-foo) #:env (current-module)))
-  (pass-if-exception "foo 2 args (inline)" exception:syntax-pattern-unmatched
-     (compile '(make-foo 1 2) #:env (current-module)))
-
   (pass-if-exception "foo 0 args" exception:wrong-num-args
-     (let ((make-foo make-foo))
-       (make-foo)))
+    (make-foo))
   (pass-if-exception "foo 2 args" exception:wrong-num-args
-     (let ((make-foo make-foo))
-       (make-foo 1 2))))
+    (make-foo 1 2)))
 
 (with-test-prefix "predicate"
 
@@ -94,6 +83,13 @@
   (pass-if-exception "set-y! on bar" exception:wrong-type-arg
      (set-y! b 99)))
 
+(with-test-prefix "side-effecting arguments"
+
+    (pass-if "predicate"
+      (let ((x 0))
+        (and (foo? (begin (set! x (+ x 1)) f))
+             (= x 1)))))
+
 (with-test-prefix "non-toplevel"
     
   (define-record-type :frotz (make-frotz a b) frotz?
-- 
tg: (df12979..) t/srfi-9-fix2 (depends on: stable-2.0)

[-- Attachment #3: Type: text/plain, Size: 63 bytes --]


Regards, Rotty
-- 
Andreas Rottmann -- <http://rotty.yi.org/>