multiple values bug in head / call-with-input-file

multiple values bug in head / call-with-input-file

[Daniel Llorens]

Hello,

I noticed this:

scheme@(guile-user)> (call-with-input-string "hello" (lambda (p) (values 1 2)))
$1 = 1
$2 = 2

but:

scheme@(guile-user)> (call-with-input-file "hello" (lambda (p) (values 1 2)))
$1 = 1

I look up call-with-input-file and I see:

(define (call-with-input-file str proc)
  (let* ((file (open-input-file str))
         (ans (proc file)))
    (close-input-port file)
    ans))

Now, in Guile 1.8:

guile> (let ((a (values 1 2))) a)
1
2

But in Guile 2.0.1:

scheme@(guile-user)> (let ((a (values 1 2))) a)
$1 = 1

The second one breaks call-with-input-file, but probably both should be errors.

Anyway, here is a patch for call-with-input/output-file that avoids the issue.

Regards,
	
	Daniel



From 06f8aea901cd3da68a409a9932757209d91efc40 Mon Sep 17 00:00:00 2001
From: Daniel Llorens <daniel.llorens@bluewin.ch>
Date: Mon, 2 May 2011 14:54:20 +0200
Subject: [PATCH] Fix call-with-input-file, call-with-output-file with multiple values

* module/ice-9/r4rs.scm: Write call-with-input-file, call-with-input-file in terms of dynamic-wind.
---
 module/ice-9/r4rs.scm |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/module/ice-9/r4rs.scm b/module/ice-9/r4rs.scm
index 4d3feba..337e196 100644
--- a/module/ice-9/r4rs.scm
+++ b/module/ice-9/r4rs.scm
@@ -144,10 +144,11 @@ automatically and the value yielded by the procedure is returned.
 If the procedure does not return, then the port will not be closed
 automatically unless it is possible to prove that the port will
 never again be used for a read or write operation."
-  (let* ((file (open-input-file str))
-	 (ans (proc file)))
-    (close-input-port file)
-    ans))
+  (let ((port #f))
+    (dynamic-wind
+      (lambda () (set! port (open-input-file str)))
+      (lambda () (proc port))
+      (lambda () (if port (close-input-port port))))))
 
 (define (call-with-output-file str proc)
   "PROC should be a procedure of one argument, and STR should be a
@@ -160,10 +161,11 @@ automatically and the value yielded by the procedure is returned.
 If the procedure does not return, then the port will not be closed
 automatically unless it is possible to prove that the port will
 never again be used for a read or write operation."
-  (let* ((file (open-output-file str))
-	 (ans (proc file)))
-    (close-output-port file)
-    ans))
+  (let ((port #f))
+    (dynamic-wind
+      (lambda () (set! port (open-output-file str)))
+      (lambda () (proc port))
+      (lambda () (if port (close-output-port port))))))
 
 (define (with-input-from-port port thunk)
   (let* ((swaports (lambda () (set! port (set-current-input-port port)))))
-- 
1.7.1

Re: multiple values bug in head / call-with-input-file

[Mark H Weaver]

Daniel Llorens <daniel.llorens@bluewin.ch> writes:
> scheme@(guile-user)> (call-with-input-string "hello" (lambda (p) (values 1 2)))
> $1 = 1
> $2 = 2
>
> but:
>
> scheme@(guile-user)> (call-with-input-file "hello" (lambda (p) (values 1 2)))
> $1 = 1

Indeed this is suboptimal, and probably a bug.
Thanks for reporting this!

However, your fix is incorrect.  By using dynamic-wind, your patch
significantly changes the semantics of call-with-{input,output}-file.
The docs state:

  If the procedure does not return, then the port will not be closed
  automatically unless it is possible to prove that the port will never
  again be used for a read or write operation.

By using dynamic-wind, you have changed this.  Now, if a continuation is
invoked that causes control to leave the dynamic extent of
call-with-{input,output}-file, the file will be closed.  This is
different from what the docs claim.

Furthermore, if control re-enters that dynamic extent, the file will be
_re-opened_ with a _fresh_ port.  This causes several problems.  First,
the code within proc is probably not expecting that, and will continue
to have copies of the old port around.  To make matters worse:
call-with-input-file will start reading from the beginning of the file
again, and call-with-output-file will _truncate_ the file and resume
writing from the beginning.

I believe the proper fix is something like this:

  (define (call-with-input-file str proc)
    (let ((file (open-input-file str)))
      (call-with-values
        (lambda () (proc file))
        (lambda vals
          (close-input-port file)
          (apply values vals)))))

Would you like to prepare a new patch?

     Best,
      Mark


> From 06f8aea901cd3da68a409a9932757209d91efc40 Mon Sep 17 00:00:00 2001
> From: Daniel Llorens <daniel.llorens@bluewin.ch>
> Date: Mon, 2 May 2011 14:54:20 +0200
> Subject: [PATCH] Fix call-with-input-file, call-with-output-file with multiple values
>
> * module/ice-9/r4rs.scm: Write call-with-input-file, call-with-input-file in terms of dynamic-wind.
> ---
>  module/ice-9/r4rs.scm |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/module/ice-9/r4rs.scm b/module/ice-9/r4rs.scm
> index 4d3feba..337e196 100644
> --- a/module/ice-9/r4rs.scm
> +++ b/module/ice-9/r4rs.scm
> @@ -144,10 +144,11 @@ automatically and the value yielded by the procedure is returned.
>  If the procedure does not return, then the port will not be closed
>  automatically unless it is possible to prove that the port will
>  never again be used for a read or write operation."
> -  (let* ((file (open-input-file str))
> -	 (ans (proc file)))
> -    (close-input-port file)
> -    ans))
> +  (let ((port #f))
> +    (dynamic-wind
> +      (lambda () (set! port (open-input-file str)))
> +      (lambda () (proc port))
> +      (lambda () (if port (close-input-port port))))))
>  
>  (define (call-with-output-file str proc)
>    "PROC should be a procedure of one argument, and STR should be a
> @@ -160,10 +161,11 @@ automatically and the value yielded by the procedure is returned.
>  If the procedure does not return, then the port will not be closed
>  automatically unless it is possible to prove that the port will
>  never again be used for a read or write operation."
> -  (let* ((file (open-output-file str))
> -	 (ans (proc file)))
> -    (close-output-port file)
> -    ans))
> +  (let ((port #f))
> +    (dynamic-wind
> +      (lambda () (set! port (open-output-file str)))
> +      (lambda () (proc port))
> +      (lambda () (if port (close-output-port port))))))
>  
>  (define (with-input-from-port port thunk)
>    (let* ((swaports (lambda () (set! port (set-current-input-port port)))))

Re: multiple values bug in head / call-with-input-file

[Daniel Llorens]

On May 2, 2011, at 19:46, Mark H Weaver wrote:

> Daniel Llorens <daniel.llorens@bluewin.ch> writes:
>> scheme@(guile-user)> (call-with-input-string "hello" (lambda (p) (values 1 2)))
>> $1 = 1
>> $2 = 2
>> 
>> but:
>> 
>> scheme@(guile-user)> (call-with-input-file "hello" (lambda (p) (values 1 2)))
>> $1 = 1
> 
> Indeed this is suboptimal, and probably a bug.
> Thanks for reporting this!
> 
> However, your fix is incorrect.  By using dynamic-wind, your patch
> significantly changes the semantics of call-with-{input,output}-file.

That makes a lot of sense. I copied blindly from with-input..., but of course those don't open or close ports. Thanks for the explanation!

Second try follows. I've noticed the same bug in with-x-to/from-file, so I have attempted to fix those too, in the way of with-x-to/from-string below.

Regards,

	Daniel

From 014552302aad4a3188c60db2f600c2c229c15cde Mon Sep 17 00:00:00 2001
From: Daniel Llorens <daniel.llorens@bluewin.ch>
Date: Mon, 2 May 2011 20:36:33 +0200
Subject: [PATCH] Fix call-with-input-file & relatives for multiple values

* module/ice-9/r4rs.scm (call-with-input-file, call-with-output-file): Rewrite
  with call-with-values.
  (with-input-from-file): use call-with-input-file.
  (with-output-to-file, with-error-to-file): use call-with-output-file.
  Update docstrings to make clear that multiple values may be yielded.
---
 module/ice-9/r4rs.scm |   62 +++++++++++++++++++++++-------------------------
 1 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/module/ice-9/r4rs.scm b/module/ice-9/r4rs.scm
index 4d3feba..f0cc5f6 100644
--- a/module/ice-9/r4rs.scm
+++ b/module/ice-9/r4rs.scm
@@ -140,14 +140,16 @@ already exist. These procedures call PROC
 with one argument: the port obtained by opening the named file for
 input or output.  If the file cannot be opened, an error is
 signalled.  If the procedure returns, then the port is closed
-automatically and the value yielded by the procedure is returned.
+automatically and the value(s) yielded by the procedure is (are) returned.
 If the procedure does not return, then the port will not be closed
 automatically unless it is possible to prove that the port will
 never again be used for a read or write operation."
-  (let* ((file (open-input-file str))
-	 (ans (proc file)))
-    (close-input-port file)
-    ans))
+  (let ((p (open-input-file str)))
+    (call-with-values
+      (lambda () (proc p))
+      (lambda vals
+        (close-input-port port)
+        (apply values vals)))))
 
 (define (call-with-output-file str proc)
   "PROC should be a procedure of one argument, and STR should be a
@@ -156,14 +158,16 @@ already exists. These procedures call PROC
 with one argument: the port obtained by opening the named file for
 input or output.  If the file cannot be opened, an error is
 signalled.  If the procedure returns, then the port is closed
-automatically and the value yielded by the procedure is returned.
+automatically and the value(s) yielded by the procedure is (are) returned.
 If the procedure does not return, then the port will not be closed
 automatically unless it is possible to prove that the port will
 never again be used for a read or write operation."
-  (let* ((file (open-output-file str))
-	 (ans (proc file)))
-    (close-output-port file)
-    ans))
+  (let ((p (open-output-file str)))
+    (call-with-values
+      (lambda () (proc p))
+      (lambda vals
+        (close-output-port p)
+        (apply values vals)))))
 
 (define (with-input-from-port port thunk)
   (let* ((swaports (lambda () (set! port (set-current-input-port port)))))
@@ -177,50 +181,44 @@ never again be used for a read or write operation."
   (let* ((swaports (lambda () (set! port (set-current-error-port port)))))
     (dynamic-wind swaports thunk swaports)))
 
-(define (with-input-from-file file thunk)
-  "THUNK must be a procedure of no arguments, and FILE must be a
+(define (with-input-from-file str thunk)
+  "THUNK must be a procedure of no arguments, and STR must be a
 string naming a file.  The file must already exist. The file is opened for
 input, an input port connected to it is made
 the default value returned by `current-input-port', 
 and the THUNK is called with no arguments.
 When the THUNK returns, the port is closed and the previous
-default is restored.  Returns the value yielded by THUNK.  If an
+default is restored.  Returns the value(s) yielded by THUNK.  If an
 escape procedure is used to escape from the continuation of these
 procedures, their behavior is implementation dependent."
-  (let* ((nport (open-input-file file))
-	 (ans (with-input-from-port nport thunk)))
-    (close-port nport)
-    ans))
+  (call-with-input-file str
+   (lambda (p) (with-input-from-port p thunk))))
 
-(define (with-output-to-file file thunk)
-  "THUNK must be a procedure of no arguments, and FILE must be a
+(define (with-output-to-file str thunk)
+  "THUNK must be a procedure of no arguments, and STR must be a
 string naming a file.  The effect is unspecified if the file already exists. 
 The file is opened for output, an output port connected to it is made
 the default value returned by `current-output-port', 
 and the THUNK is called with no arguments.
 When the THUNK returns, the port is closed and the previous
-default is restored.  Returns the value yielded by THUNK.  If an
+default is restored.  Returns the value(s) yielded by THUNK.  If an
 escape procedure is used to escape from the continuation of these
 procedures, their behavior is implementation dependent."
-  (let* ((nport (open-output-file file))
-	 (ans (with-output-to-port nport thunk)))
-    (close-port nport)
-    ans))
+  (call-with-output-file str
+   (lambda (p) (with-output-to-port p thunk))))
 
-(define (with-error-to-file file thunk)
-  "THUNK must be a procedure of no arguments, and FILE must be a
+(define (with-error-to-file str thunk)
+  "THUNK must be a procedure of no arguments, and STR must be a
 string naming a file.  The effect is unspecified if the file already exists. 
 The file is opened for output, an output port connected to it is made
 the default value returned by `current-error-port', 
 and the THUNK is called with no arguments.
 When the THUNK returns, the port is closed and the previous
-default is restored.  Returns the value yielded by THUNK.  If an
+default is restored.  Returns the value(s) yielded by THUNK.  If an
 escape procedure is used to escape from the continuation of these
 procedures, their behavior is implementation dependent."
-  (let* ((nport (open-output-file file))
-	 (ans (with-error-to-port nport thunk)))
-    (close-port nport)
-    ans))
+  (call-with-output-file str
+   (lambda (p) (with-error-to-port p thunk))))
 
 (define (with-input-from-string string thunk)
   "THUNK must be a procedure of no arguments.
@@ -228,7 +226,7 @@ The test of STRING  is opened for
 input, an input port connected to it is made, 
 and the THUNK is called with no arguments.
 When the THUNK returns, the port is closed.
-Returns the value yielded by THUNK.  If an
+Returns the value(s) yielded by THUNK.  If an
 escape procedure is used to escape from the continuation of these
 procedures, their behavior is implementation dependent."
   (call-with-input-string string
-- 
1.7.4.4

Re: multiple values bug in head / call-with-input-file

[Andy Wingo]

On Mon 02 May 2011 21:09, Daniel Llorens <daniel.llorens@bluewin.ch> writes:

> Second try follows. I've noticed the same bug in with-x-to/from-file,
> so I have attempted to fix those too, in the way of
> with-x-to/from-string below.

Looks very nice, thanks!

We are brushing the copyright assignment limits here; I'll apply this,
but if you are interested in sending further patches to Guile, would you
be OK with assigning copyright to the FSF?  Please let me know, and I'll
send you details in a private mail.

Thanks for the patch, and happy hacking :)

Andy
-- 
http://wingolog.org/

Re: multiple values bug in head / call-with-input-file

[Daniel Llorens]

On May 2, 2011, at 22:55, Andy Wingo wrote:

> We are brushing the copyright assignment limits here; I'll apply this,
> but if you are interested in sending further patches to Guile, would you
> be OK with assigning copyright to the FSF?  Please let me know, and I'll
> send you details in a private mail.

You are too kind; I just copied Mark's fix. Anyway, sure, I'll sign. 

> Thanks for the patch, and happy hacking :)

Thanks to you!

Regards,

	Daniel