bug#26058: utf16->string and utf32->string don't conform to R6RS

bug#26058: utf16->string and utf32->string don't conform to R6RS

["Taylan Ulrich Bayırlı/Kammer"]

See the R6RS Libraries document page 10.  The differences:

- R6RS supports reading a BOM.

- R6RS mandates an endianness argument to specify the behavior at the
  absence of a BOM.

- R6RS allows an optional third argument 'endianness-mandatory' to
  explicitly ignore any possible BOM.

Here's a quick patch on top of master.  I didn't test it thoroughly...


===File
/home/taylan/src/guile/guile-master/0001-Fix-R6RS-utf16-string-and-utf32-string.patch===
From f51cd1d4884caafb1ed0072cd77c0e3145f34576 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Taylan=20Ulrich=20Bay=C4=B1rl=C4=B1/Kammer?=
 <taylanbayirli@gmail.com>
Date: Fri, 10 Mar 2017 22:36:55 +0100
Subject: [PATCH] Fix R6RS utf16->string and utf32->string.

* module/rnrs/bytevectors.scm (read-bom16, read-bom32): New procedures.
(r6rs-utf16->string, r6rs-utf32->string): Ditto.
---
 module/rnrs/bytevectors.scm | 52 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/module/rnrs/bytevectors.scm b/module/rnrs/bytevectors.scm
index 9744359f0..997a8c9cb 100644
--- a/module/rnrs/bytevectors.scm
+++ b/module/rnrs/bytevectors.scm
@@ -69,7 +69,9 @@
            bytevector-ieee-double-native-set!
 
            string->utf8 string->utf16 string->utf32
-           utf8->string utf16->string utf32->string))
+           utf8->string
+           (r6rs-utf16->string . utf16->string)
+           (r6rs-utf32->string . utf32->string)))
 
 
 (load-extension (string-append "libguile-" (effective-version))
@@ -80,4 +82,52 @@
       `(quote ,sym)
       (error "unsupported endianness" sym)))
 
+(define (read-bom16 bv)
+  (let ((c0 (bytevector-u8-ref bv 0))
+        (c1 (bytevector-u8-ref bv 1)))
+    (cond
+     ((and (= c0 #xFE) (= c1 #xFF))
+      'big)
+     ((and (= c0 #xFF) (= c1 #xFE))
+      'little)
+     (else
+      #f))))
+
+(define r6rs-utf16->string
+  (case-lambda
+    ((bv default-endianness)
+     (let ((bom-endianness (read-bom16 bv)))
+       (if (not bom-endianness)
+           (utf16->string bv default-endianness)
+           (substring/shared (utf16->string bv bom-endianness) 1))))
+    ((bv endianness endianness-mandatory?)
+     (if endianness-mandatory?
+         (utf16->string bv endianness)
+         (r6rs-utf16->string bv endianness)))))
+
+(define (read-bom32 bv)
+  (let ((c0 (bytevector-u8-ref bv 0))
+        (c1 (bytevector-u8-ref bv 1))
+        (c2 (bytevector-u8-ref bv 2))
+        (c3 (bytevector-u8-ref bv 3)))
+    (cond
+     ((and (= c0 #x00) (= c1 #x00) (= c2 #xFE) (= c3 #xFF))
+      'big)
+     ((and (= c0 #xFF) (= c1 #xFE) (= c2 #x00) (= c3 #x00))
+      'little)
+     (else
+      #f))))
+
+(define r6rs-utf32->string
+  (case-lambda
+    ((bv default-endianness)
+     (let ((bom-endianness (read-bom32 bv)))
+       (if (not bom-endianness)
+           (utf32->string bv default-endianness)
+           (substring/shared (utf32->string bv bom-endianness) 1))))
+    ((bv endianness endianness-mandatory?)
+     (if endianness-mandatory?
+         (utf32->string bv endianness)
+         (r6rs-utf32->string bv endianness)))))
+
 ;;; bytevector.scm ends here
-- 
2.11.0

============================================================

bug#26058: utf16->string and utf32->string don't conform to R6RS

[Andy Wingo]

On Sat 11 Mar 2017 13:19, taylanbayirli@gmail.com ("Taylan Ulrich "Bayırlı/Kammer"") writes:

> See the R6RS Libraries document page 10.  The differences:
>
> - R6RS supports reading a BOM.
>
> - R6RS mandates an endianness argument to specify the behavior at the
>   absence of a BOM.
>
> - R6RS allows an optional third argument 'endianness-mandatory' to
>   explicitly ignore any possible BOM.
>
> Here's a quick patch on top of master.  I didn't test it thoroughly...

Hi,

this is a tricky area that is not so amenable to quick patches :) Have
you looked into what Guile already does for byte-order marks?  Can you
explain how the R6RS specification relates to this?

  https://www.gnu.org/software/guile/manual/html_node/BOM-Handling.html

Andy

bug#26058: utf16->string and utf32->string don't conform to R6RS

[Taylan Ulrich Bayırlı/Kammer]

Andy Wingo <wingo@pobox.com> writes:

> Hi,
>
> this is a tricky area that is not so amenable to quick patches :) Have
> you looked into what Guile already does for byte-order marks?  Can you
> explain how the R6RS specification relates to this?
>
>   https://www.gnu.org/software/guile/manual/html_node/BOM-Handling.html
>
> Andy

Hmm, interesting.  I noticed the utf{16,32}->string procedures ignoring
a BOM at the start of the given bytevector, but didn't look at it from a
ports perspective.

TL;DR of the below though: the R6RS semantics offer a strict enrichment
of the feature-set of the utfX->string procedures relative to the Guile
semantics, so at most we would end up with spurious features.  (The
optional ability to handle any possible BOM at the start of the
bytevector, with a fall-back endianness in case none is found.)


That said, let's see...

If I do a textual read from a port, I already get a string and not a
bytevector, so the behavior of utfX->string operations is irrelevant.

If I do binary I/O, the following situations are possible:

1. I'm guaranteed to get any possible bytes that happen to form a valid
   BOM at the start of the stream as-is in the returned bytevector; the
   binary I/O interface doesn't see such bytes as anything special, as
   it could simply be coincidence that the stream starts with such
   bytes.

2. I'm guaranteed *not* to get bytes that form a BOM at the start of the
   stream; instead they're consumed to set the port encoding for any
   future text I/O.

3. The behavior is unspecified and either of the above may happen.

In the case of #1, it's probably good for utfX->string procedures to be
able to handle BOMs, but also allow explicitly ignoring any possible
BOM.  The R6RS semantics cover this.

In the case of #2, the utfX->string procedures don't need to be able to
handle BOMs as far as we're talking about passing them bytevectors
returned by port I/O, but it also doesn't hurt if they optionally
support it.  The R6RS semantics are fine here as well I think.

As for #3... first of all it's bad IMO; the behavior ought to be
specified. :-) But in any case, the additional features of the R6RS
semantics won't hurt.

WDYT?  As far as I understand the page you linked, Guile currently
implements #3, which I think is unfortunate but can kinda understand
too.  In any case, the additional R6RS features won't hurt, right?

Taylan

bug#26058: utf16->string and utf32->string don't conform to R6RS

[Andy Wingo]

On Mon 13 Mar 2017 19:10, taylanbayirli@gmail.com (Taylan Ulrich "Bayırlı/Kammer") writes:

> If I do binary I/O, the following situations are possible:
>
> 1. I'm guaranteed to get any possible bytes that happen to form a valid
>    BOM at the start of the stream as-is in the returned bytevector; the
>    binary I/O interface doesn't see such bytes as anything special, as
>    it could simply be coincidence that the stream starts with such
>    bytes.
>
> 2. I'm guaranteed *not* to get bytes that form a BOM at the start of the
>    stream; instead they're consumed to set the port encoding for any
>    future text I/O.
>
> 3. The behavior is unspecified and either of the above may happen.

(1).  But I thought this bug was about using a bytevector as a source
and then doing textual I/O on it, no?

Andy

bug#26058: utf16->string and utf32->string don't conform to R6RS

[Taylan Ulrich Bayırlı/Kammer]

Andy Wingo <wingo@pobox.com> writes:

> On Mon 13 Mar 2017 19:10, taylanbayirli@gmail.com (Taylan Ulrich "Bayırlı/Kammer") writes:
>
>> If I do binary I/O, the following situations are possible:
>>
>> 1. I'm guaranteed to get any possible bytes that happen to form a valid
>>    BOM at the start of the stream as-is in the returned bytevector; the
>>    binary I/O interface doesn't see such bytes as anything special, as
>>    it could simply be coincidence that the stream starts with such
>>    bytes.
>
> (1).  But I thought this bug was about using a bytevector as a source
> and then doing textual I/O on it, no?

I have a feeling we're somehow talking past each other. :-) As far as
I'm concerned, the bug is just that the procedures don't conform to the
specification.

It would of course be good if the behavior of these procedures was
somehow in harmony with the behavior of I/O operations, but I don't see
any issues arising from adopting the R6RS behavior of the procedures
utf16->string and utf32->string.  Do you?

Taylan

bug#26058: utf16->string and utf32->string don't conform to R6RS

[Andy Wingo]

On Tue 14 Mar 2017 16:03, taylanbayirli@gmail.com (Taylan Ulrich "Bayırlı/Kammer") writes:

> Andy Wingo <wingo@pobox.com> writes:
>
>> On Mon 13 Mar 2017 19:10, taylanbayirli@gmail.com (Taylan Ulrich "Bayırlı/Kammer") writes:
>>
>>> If I do binary I/O, the following situations are possible:
>>>
>>> 1. I'm guaranteed to get any possible bytes that happen to form a valid
>>>    BOM at the start of the stream as-is in the returned bytevector; the
>>>    binary I/O interface doesn't see such bytes as anything special, as
>>>    it could simply be coincidence that the stream starts with such
>>>    bytes.
>>
>> (1).  But I thought this bug was about using a bytevector as a source
>> and then doing textual I/O on it, no?
>
> I have a feeling we're somehow talking past each other. :-) As far as
> I'm concerned, the bug is just that the procedures don't conform to the
> specification.
>
> It would of course be good if the behavior of these procedures was
> somehow in harmony with the behavior of I/O operations, but I don't see
> any issues arising from adopting the R6RS behavior of the procedures
> utf16->string and utf32->string.  Do you?

Adopting the behavior is more or less fine.  If it can be done while
relying on the existing behavior, that is better than something ad-hoc
in a module.

Andy

bug#26058: utf16->string and utf32->string don't conform to R6RS

[Taylan Ulrich Bayırlı/Kammer]

Andy Wingo <wingo@pobox.com> writes:

> Adopting the behavior is more or less fine.  If it can be done while
> relying on the existing behavior, that is better than something ad-hoc
> in a module.

You mean somehow leveraging the existing BOM handling code of Guile
(found in the ports code) would be preferable to reimplementing it like
in this patch, correct?

In that light, I had this attempt:

(define r6rs-utf16->string
  (case-lambda
    ((bv default-endianness)
     (let* ((binary-port (open-bytevector-input-port bv))
            (transcoder (make-transcoder (utf-16-codec)))
            (utf16-port (transcoded-port binary-port transcoder)))
       ;; XXX how to set default-endianness for a port?
       (get-string-all utf16-port)))
    ((bv endianness endianness-mandatory?)
     (if endianness-mandatory?
         (utf16->string bv endianness)
         (r6rs-utf16->string bv endianness)))))

As commented in the first branch of the case-lambda, this does not yet
make use of the 'default-endianness' parameter to tell the port
transcoder (or whoever) what to do in case no BOM is found in the
stream.

From what I can tell, Guile is currently hardcoded to *transparently*
default to big-endian in ports.c, port_clear_stream_start_for_bom_read.

Is there a way to detect when Guile was unable to find a BOM?  (In that
case one could set the endianness explicitly to the desired default.)

Or do you see another way to implement this?

Thanks for the feedback!
Taylan


P.S.: Huge congrats on the big release. :-)

bug#26058: utf16->string and utf32->string don't conform to R6RS

[Mark H Weaver]

Hi Taylan,

taylanbayirli@gmail.com (Taylan Ulrich "Bayırlı/Kammer") writes:

> Andy Wingo <wingo@pobox.com> writes:
>
>> Adopting the behavior is more or less fine.  If it can be done while
>> relying on the existing behavior, that is better than something ad-hoc
>> in a module.

In general, I agree with Andy's sentiment that it would be better to
avoid redundant BOM handling code, and moreover, I appreciate his
reluctance to apply a fix without careful consideration of our existing
BOM semantics.

However, as Taylan discovered, Guile does not provide a mechanism to
specify a default endianness of a UTF-16 or UTF-32 port in case a BOM is
not found.  I see no straightforward way to implement these R6RS
interfaces using ports.

We could certainly add such a mechanism if needed, but I see another
problem with this approach: the expense of creating and later collecting
a bytevector port object would be a very heavy burden to place on these
otherwise fairly lightweight operations.  Therefore, I would prefer to
avoid that implementation strategy for these operations.

Although BOM handling for ports is quite complex with many subtle points
to consider, detecting a BOM at the beginning of a bytevector is so
trivial that I personally have no objection to this tiny duplication of
logic.

Therefore, my preference would be to adopt code similar to that proposed
by Taylan, although I believe it can, and should, be further simplified:

> diff --git a/module/rnrs/bytevectors.scm b/module/rnrs/bytevectors.scm
> index 9744359f0..997a8c9cb 100644
> --- a/module/rnrs/bytevectors.scm
> +++ b/module/rnrs/bytevectors.scm
> @@ -69,7 +69,9 @@
>             bytevector-ieee-double-native-set!
>  
>             string->utf8 string->utf16 string->utf32
> -           utf8->string utf16->string utf32->string))
> +           utf8->string
> +           (r6rs-utf16->string . utf16->string)
> +           (r6rs-utf32->string . utf32->string)))
>  
>  
>  (load-extension (string-append "libguile-" (effective-version))
> @@ -80,4 +82,52 @@
>        `(quote ,sym)
>        (error "unsupported endianness" sym)))
>  
> +(define (read-bom16 bv)
> +  (let ((c0 (bytevector-u8-ref bv 0))
> +        (c1 (bytevector-u8-ref bv 1)))
> +    (cond
> +     ((and (= c0 #xFE) (= c1 #xFF))
> +      'big)
> +     ((and (= c0 #xFF) (= c1 #xFE))
> +      'little)
> +     (else
> +      #f))))

We should gracefully handle the case of an empty bytevector, returning
an empty string without error in that case.

Also, we should use a single 'bytevector-u16-ref' operation to check for
the BOM.  Pick an arbitrary endianness for the operation (big-endian?),
and compare the resulting integer with both #xFEFF and #xFFFE.  That
way, the code will be simpler and more efficient.  Note that our VM has
dedicated instructions for these multi-byte bytevector accessors, and
there will be fewer comparison operations as well.  Similarly for the
utf32 case.

What do you think?

> +(define r6rs-utf16->string
> +  (case-lambda
> +    ((bv default-endianness)
> +     (let ((bom-endianness (read-bom16 bv)))
> +       (if (not bom-endianness)
> +           (utf16->string bv default-endianness)
> +           (substring/shared (utf16->string bv bom-endianness) 1))))

Better to use plain 'substring' here, I think.  The machinery of shared
substrings is more expensive, and unnecessary in this case.

Otherwise, it looks good to me.  Would you like to propose a revised
patch?

Andy, what do you think?

       Mark