bug#38263: Bug in srfi-11 (?)

bug#38263: Bug in srfi-11 (?)

[Tim Gesthuizen via Bug reports for GUILE, GNU's Ubiquitous Extension Language]

Hi,
I was tinkering with srfi-11 and was wondering whether the following
is correct:

> scheme@(guile-user)> ,expand (let-values (((a b c) (values 1 2 3))
> 				  ((d . e) (values 4 5)))
> 		       (list a b c d e))
> $26 = ((@@ (srfi srfi-11) call-with-values)
>  (lambda () (values 1 2 3))
>  (lambda (t-1dff1b83541ce327-16e
>           t-1dff1b83541ce327-16f
>           t-1dff1b83541ce327-170)
>    ((@@ (srfi srfi-11) call-with-values)
>     (lambda () (values 4 5))
>     (lambda (d . e)
>       (let ((t-1dff1b83541ce327-171 d)
>             (t-1dff1b83541ce327-172 e)
>             (a t-1dff1b83541ce327-16e)
>             (b t-1dff1b83541ce327-16f)
>             (c t-1dff1b83541ce327-170))
>         (list a b c d e))))))

This differs from what the comment above the macro definition claims
to expand to.
It seems like the author forgot that he matched the temporaries before
the variables in srfi-11.scm:94.

> diff --git a/module/srfi/srfi-11.scm b/module/srfi/srfi-11.scm
> index 22bda21a2..13a2ffc4d 100644
> --- a/module/srfi/srfi-11.scm
> +++ b/module/srfi/srfi-11.scm
> @@ -95,13 +95,13 @@
>                                 (let lp ((vars (syntax vars)))
>                                   (syntax-case vars ()
>                                     ((id . rest)
> -                                    (acons (syntax id)
> -                                           (car
> +                                    (acons (car
>                                              (generate-temporaries (syntax (id))))
> +                                           (syntax id)
>                                             (lp (syntax rest))))
> -                                   (id (acons (syntax id)
> -                                              (car
> +                                   (id (acons (car
>                                                 (generate-temporaries (syntax (id))))
> +                                              (syntax id)
>                                                '())))))
>                                ((id ...) ids)
>                                ((tmp ...) tmps))

The code "works" anyhow because the lambdas are all nested and the
inner ones capture the parameters of the outer ones.
Which got me thinking why all the messing with temporaries is
neccessary at all. Why is

> (define-syntax let-values
>   (lambda (x)
>     (syntax-case x ()
>       ((_ (clauses ...) b0 b1 ...)
>        (let lp ((clauses #'(clauses ...)))
> 	 (if (null? clauses)
> 	     #'(begin b0 b1 ...)
> 	     (syntax-case (car clauses) ()
> 	       ((args exp)
> 		(with-syntax ((inner (lp (cdr clauses))))
> 		  #'(call-with-values (lambda () exp)
> 		      (lambda args inner)))))))))))

not sufficient? I would not consider my self a Scheme expert and it
could be that I just missed something.
It would be nice if someone could verify whether this is a bug or not.
I am using Guile 2.2.6 on Guix.

Tim.

bug#38263: Bug in srfi-11

[Tim Gesthuizen via Bug reports for GUILE, GNU's Ubiquitous Extension Language]

Hi,
I had a look today into the srfi-11 specificiation. It requires that the
variables are bound to fresh locations so let me rephrase the bug:

> (let ((a 1)
>       (b (let-values (((a . b) (values 2 3))
>                        (c (begin (set! a 9) 4)))
>            (list a b c))))
>   (cons a b))

Evaluates to `(1 9 (3) (4))` while it should evaluate to
`(9 2 (3) (4))`.

Hope this helps,
Tim.

bug#38263: Bug in srfi-11

[Mark H Weaver]

Hi Tim,

Tim Gesthuizen <tim.gesthuizen@yahoo.de> wrote:

> Hi,
> I had a look today into the srfi-11 specificiation. It requires that the
> variables are bound to fresh locations so let me rephrase the bug:
>
>> (let ((a 1)
>>       (b (let-values (((a . b) (values 2 3))
>>                       (c (begin (set! a 9) 4)))
>>            (list a b c))))
>>   (cons a b))
>
> Evaluates to `(1 9 (3) (4))` while it should evaluate to
> `(9 2 (3) (4))`.

I agree that this example indicates a bug in Guile's 'let-values'
implementation (which was written by Andy Wingo in August 2009), but I
disagree that it should evaluate to '(9 2 (3) (4)).  I think that your
example should raise an error, because at the point where (set! a 9) is
found, neither of the 'a' variables are in scope.

     Regards,
       Mark

bug#38263: Bug in srfi-11

[Mark H Weaver]

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

Hi Tim,

> Mark H Weaver writes:
>> I agree that this example indicates a bug in Guile's 'let-values'
>> implementation (which was written by Andy Wingo in August 2009), but I
>> disagree that it should evaluate to '(9 2 (3) (4)).  I think that your
>> example should raise an error, because at the point where (set! a 9) is
>> found, neither of the 'a' variables are in scope.
>
> Oops, that `let` should have been a `let*` (Moving the first a into
> scope). But if you could verify that what I described is a bug I would
> like to propose a patch.

I agree that it's a bug, and that if you change 'let' to 'let*' in your
previous example, the result should be '(9 2 (3) (4)).

I took a quick look, and I believe the fix is simply to swap 'new-var'
and 'new-tmp' on line 95 of srfi-11.scm.  See the attached patch.  Does
it fix the problems you're seeing?

     Thanks,
       Mark



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Fix 'let-values' where <formals> is an improper list --]
[-- Type: text/x-patch, Size: 1537 bytes --]

From 4657b95713facffcde685b578ed19dbeb45624d0 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Sun, 24 Nov 2019 14:46:45 -0500
Subject: [PATCH] Fix 'let-values' where <formals> is an improper list.

Fixes <https://bugs.gnu.org/38263>.
Reported by Tim Gesthuizen <tim.gesthuizen@yahoo.de>.

* module/srfi/srfi-11.scm (let-values): Swap 'new-tmp' and 'new-var' in
the pair pattern, to match the code that creates those pairs.
---
 module/srfi/srfi-11.scm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/module/srfi/srfi-11.scm b/module/srfi/srfi-11.scm
index 22bda21a2..42b8527ba 100644
--- a/module/srfi/srfi-11.scm
+++ b/module/srfi/srfi-11.scm
@@ -1,6 +1,7 @@
 ;;; srfi-11.scm --- let-values and let*-values
 
-;; Copyright (C) 2000, 2001, 2002, 2004, 2006, 2009 Free Software Foundation, Inc.
+;; Copyright (C) 2000, 2001, 2002, 2004, 2006, 2009,
+;;   2019 Free Software Foundation, Inc.
 ;;
 ;; This library is free software; you can redistribute it and/or
 ;; modify it under the terms of the GNU Lesser General Public
@@ -91,7 +92,7 @@
                     (syntax (call-with-values (lambda () exp)
                               (lambda (new-tmp ...) inner))))))
                ((vars exp)
-                (with-syntax ((((new-tmp . new-var) ...)
+                (with-syntax ((((new-var . new-tmp) ...)
                                (let lp ((vars (syntax vars)))
                                  (syntax-case vars ()
                                    ((id . rest)
-- 
2.24.0

bug#38263: Bug in srfi-11

[Tim Gesthuizen via Bug reports for GUILE, GNU's Ubiquitous Extension Language]

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

Hello again,

the attached patch also adds a unit test.
I am not into how Guile is organized: Is there anything keeping us from
adding the change?

Tim.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-srfi-11-Do-not-expose-variables-to-later-clauses.patch --]
[-- Type: text/x-patch, Size: 1954 bytes --]

From 99d8fb795932eb92b7d5fb09115b6691f4bfe66d Mon Sep 17 00:00:00 2001
From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
Date: Tue, 3 Dec 2019 18:50:37 +0100
Subject: [PATCH] srfi-11: Do not expose variables to later clauses

The current implementation of srfi-11s let-values allows later clauses
to access and modify variables bound in earlier clauses when the clause
is not a proper list.

* module/srfi/srfi-11.scm (let-values): Fix switched variable names.
* test-suite/tests/srfi-11.test (let-values): Add test checking that the
  variable cannot be changed in later clauses.
---
 module/srfi/srfi-11.scm       | 2 +-
 test-suite/tests/srfi-11.test | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/module/srfi/srfi-11.scm b/module/srfi/srfi-11.scm
index 22bda21a2..7afac9c5f 100644
--- a/module/srfi/srfi-11.scm
+++ b/module/srfi/srfi-11.scm
@@ -91,7 +91,7 @@
                     (syntax (call-with-values (lambda () exp)
                               (lambda (new-tmp ...) inner))))))
                ((vars exp)
-                (with-syntax ((((new-tmp . new-var) ...)
+                (with-syntax ((((new-var . new-tmp) ...)
                                (let lp ((vars (syntax vars)))
                                  (syntax-case vars ()
                                    ((id . rest)
diff --git a/test-suite/tests/srfi-11.test b/test-suite/tests/srfi-11.test
index 40563dc18..9bfaa4300 100644
--- a/test-suite/tests/srfi-11.test
+++ b/test-suite/tests/srfi-11.test
@@ -74,7 +74,14 @@
 	'(unbound-variable . ".*")
       (let-values (((x) (values 1))
 		   ((y) (values (1+ x))))
-	#f))))
+	#f))
+
+    (pass-if "first binding with rest invisible to second expr"
+      (let* ((a 1)
+             (b (let-values (((a . b) (values 2 3))
+                             (c (begin (set! a 9) 4)))
+                  (list a b c))))
+        (equal? (cons a b) '(9 2 (3) (4)))))))
 
 ;;
 ;; let*-values
-- 
2.24.0

bug#38263: Bug in srfi-11

[Andy Wingo]

Applied to both branches.  Thanks for the patch and thanks also to Mark
for the review and initial patch!

In the future, Tim, if you have larger patches, we should work out
copyright assignment paperwork.  But less-than-10-line patches are fine
without.  Let me know if this is of interest to you.  Cheers :)

Andy

On Tue 03 Dec 2019 19:15, Tim Gesthuizen via "Bug reports for GUILE, GNU's Ubiquitous Extension Language" <bug-guile@gnu.org> writes:

> Hello again,
>
> the attached patch also adds a unit test.
> I am not into how Guile is organized: Is there anything keeping us from
> adding the change?
>
> Tim.
>
> From 99d8fb795932eb92b7d5fb09115b6691f4bfe66d Mon Sep 17 00:00:00 2001
> From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
> Date: Tue, 3 Dec 2019 18:50:37 +0100
> Subject: [PATCH] srfi-11: Do not expose variables to later clauses
>
> The current implementation of srfi-11s let-values allows later clauses
> to access and modify variables bound in earlier clauses when the clause
> is not a proper list.
>
> * module/srfi/srfi-11.scm (let-values): Fix switched variable names.
> * test-suite/tests/srfi-11.test (let-values): Add test checking that the
>   variable cannot be changed in later clauses.
> ---
>  module/srfi/srfi-11.scm       | 2 +-
>  test-suite/tests/srfi-11.test | 9 ++++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/module/srfi/srfi-11.scm b/module/srfi/srfi-11.scm
> index 22bda21a2..7afac9c5f 100644
> --- a/module/srfi/srfi-11.scm
> +++ b/module/srfi/srfi-11.scm
> @@ -91,7 +91,7 @@
>                      (syntax (call-with-values (lambda () exp)
>                                (lambda (new-tmp ...) inner))))))
>                 ((vars exp)
> -                (with-syntax ((((new-tmp . new-var) ...)
> +                (with-syntax ((((new-var . new-tmp) ...)
>                                 (let lp ((vars (syntax vars)))
>                                   (syntax-case vars ()
>                                     ((id . rest)
> diff --git a/test-suite/tests/srfi-11.test b/test-suite/tests/srfi-11.test
> index 40563dc18..9bfaa4300 100644
> --- a/test-suite/tests/srfi-11.test
> +++ b/test-suite/tests/srfi-11.test
> @@ -74,7 +74,14 @@
>  	'(unbound-variable . ".*")
>        (let-values (((x) (values 1))
>  		   ((y) (values (1+ x))))
> -	#f))))
> +	#f))
> +
> +    (pass-if "first binding with rest invisible to second expr"
> +      (let* ((a 1)
> +             (b (let-values (((a . b) (values 2 3))
> +                             (c (begin (set! a 9) 4)))
> +                  (list a b c))))
> +        (equal? (cons a b) '(9 2 (3) (4)))))))
>  
>  ;;
>  ;; let*-values