bug#24425: [PATCH] Don’t cast Unicode to 8-bit when casing unibyte strings

bug#24425: [PATCH] Don’t cast Unicode to 8-bit when casing unibyte strings

[Michal Nazarewicz]

Currently, when operating on unibyte strings and buffers, if casing
ASCII character results in a Unicode character the result is forcefully
converted to 8-bit by masking all but the eight least significant bits.
This has awkward results such as:

	(let ((table (make-char-table 'case-table)))
	  (set-char-table-parent table (current-case-table))
	  (set-case-syntax-pair ?I ?ı table)
	  (set-case-syntax-pair ?İ ?i table)
	  (with-case-table table
	    (concat (upcase "istanabul") " " (downcase "IRMA"))))
	=> "0STANABUL 1rma"

Change the code so that ASCII characters being cased to Unicode
characters are left unchanged when operating on unibyte data.  In other
words, aforementioned example will produce:

	=> "iSTANBUL "Irma"

Arguably this isn’t correct either but it’s less wrong and ther’s not
much we can do when the strings are unibyte.

Note that casify_object had a ‘(c >= 0 && c < 256)’ condition but since
CHAR_TO_BYTE8 (and thus MAKE_CHAR_UNIBYTE) happily casts Unicode
characters to 8-bit (i.e. c & 0xFF), this never triggered for discussed
case.

* src/casefiddle.c (casify_object, casify_region): When dealing with
unibyte data, don’t attempt to store Unicode characters in the result.
---
 src/casefiddle.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

 Unless there are objections, I’ll commit it in a few days.

diff --git a/src/casefiddle.c b/src/casefiddle.c
index 2d32f49..247cc6f 100644
--- a/src/casefiddle.c
+++ b/src/casefiddle.c
@@ -71,8 +71,8 @@ casify_object (enum case_action flag, Lisp_Object obj)
 	{
 	  if (! inword)
 	    c = upcase1 (c1);
-	  if (! multibyte)
-	    MAKE_CHAR_UNIBYTE (c);
+	  if (! multibyte && CHAR_BYTE8_P (c))
+	    c = CHAR_TO_BYTE8 (c);
 	  XSETFASTINT (obj, c | flags);
 	}
       return obj;
@@ -93,18 +93,19 @@ casify_object (enum case_action flag, Lisp_Object obj)
 	  c1 = c;
 	  if (inword && flag != CASE_CAPITALIZE_UP)
 	    c = downcase (c);
-	  else if (!uppercasep (c)
-		   && (!inword || flag != CASE_CAPITALIZE_UP))
-	    c = upcase1 (c1);
+	  else if (!inword || flag != CASE_CAPITALIZE_UP)
+	    c = upcase (c1);
 	  if ((int) flag >= (int) CASE_CAPITALIZE)
 	    inword = (SYNTAX (c) == Sword);
 	  if (c != c1)
 	    {
-	      MAKE_CHAR_UNIBYTE (c);
-	      /* If the char can't be converted to a valid byte, just don't
-		 change it.  */
-	      if (c >= 0 && c < 256)
-		SSET (obj, i, c);
+	      if (CHAR_BYTE8_P (c))
+		c = CHAR_TO_BYTE8 (c);
+	      else if (!ASCII_CHAR_P (c))
+		/* If the char can't be converted to a valid byte, just don't
+		   change it.  */
+		continue;
+	      SSET (obj, i, c);
 	    }
 	}
       return obj;
@@ -250,8 +251,11 @@ casify_region (enum case_action flag, Lisp_Object b, Lisp_Object e)
 
 	  if (! multibyte)
 	    {
-	      MAKE_CHAR_UNIBYTE (c);
-	      FETCH_BYTE (start_byte) = c;
+	      /* If the char can't be converted to a valid byte, just don't
+		 change it.  */
+	      if (ASCII_CHAR_P (c) ||
+		  (CHAR_BYTE8_P (c) && ((c = CHAR_TO_BYTE8 (c)), true)))
+		FETCH_BYTE (start_byte) = c;
 	    }
 	  else if (ASCII_CHAR_P (c2) && ASCII_CHAR_P (c))
 	    FETCH_BYTE (start_byte) = c;
-- 
2.8.0.rc3.226.g39d4020

bug#24425: [PATCH] Don’t cast Unicode to 8-bit when casing unibyte strings

[Eli Zaretskii]

> From: Michal Nazarewicz <mina86@mina86.com>
> Date: Tue, 13 Sep 2016 00:46:07 +0200
> 
> Currently, when operating on unibyte strings and buffers, if casing
> ASCII character results in a Unicode character the result is forcefully
> converted to 8-bit by masking all but the eight least significant bits.
> This has awkward results such as:
> 
>         (let ((table (make-char-table 'case-table)))
>           (set-char-table-parent table (current-case-table))
>           (set-case-syntax-pair ?I ?ı table)
>           (set-case-syntax-pair ?İ ?i table)
>           (with-case-table table
>             (concat (upcase "istanabul") " " (downcase "IRMA"))))
>         => "0STANABUL 1rma"
> 
> Change the code so that ASCII characters being cased to Unicode
> characters are left unchanged when operating on unibyte data.  In other
> words, aforementioned example will produce:
> 
>         => "iSTANBUL "Irma"
> 
> Arguably this isn’t correct either but it’s less wrong and ther’s not
> much we can do when the strings are unibyte.

Thanks, but I don't think it's TRT to fix this in a way that produces
a semi-broken result.  Second-guessing what the user/caller means and
silently producing results that only make sense if the guess was
correct is about the worst thing we could do in these dark-corner
situations.

Currently, case changes in unibyte characters and strings are only
well defined for pure ASCII text; if the input or the result is not
pure ASCII, we produce "undefined behavior".  In particular, case
tables are not set at all for unibyte characters, because it's not
text, it's a byte stream.  Either we decide that we don't want to
support case changes in unibyte non-ASCII characters, and we stick to
the current behavior (or maybe even signal an error, except that I'm
afraid that would break too many things); or we decide we want to
support this use case, but then do it properly.  Properly means that
upcasing "istanbul" in the above example will produce "İSTANBUL", not
"iSTANBUL", and downcasing "IRMA" will produce "ırma".  Yes, these are
multibyte strings produced from unibyte input, but I think it's the
only result we can claim to be correct for a supported use case.
(Such a change could still break some code somewhere, but at least
it's a defendable breakage.)

> Note that casify_object had a ‘(c >= 0 && c < 256)’ condition but since
> CHAR_TO_BYTE8 (and thus MAKE_CHAR_UNIBYTE) happily casts Unicode
> characters to 8-bit (i.e. c & 0xFF), this never triggered for discussed
> case.

We could convert that condition into an eassert, if we are certain the
condition should never trigger.  But that's an aside.

Thanks.

bug#24425: [PATCH] Don’t cast Unicode to 8-bit when casing unibyte strings

[Michal Nazarewicz]

On Tue, Sep 13 2016, Eli Zaretskii wrote:
> Currently, case changes in unibyte characters and strings are only
> well defined for pure ASCII text; if the input or the result is not
> pure ASCII, we produce "undefined behavior".

Would the following (not tested) make sense then:

diff --git a/src/casefiddle.c b/src/casefiddle.c
index 2d32f49..4dc2357 100644
--- a/src/casefiddle.c
+++ b/src/casefiddle.c
@@ -89,23 +89,19 @@ casify_object (enum case_action flag, Lisp_Object obj)
       for (i = 0; i < size; i++)
 	{
 	  c = SREF (obj, i);
-	  MAKE_CHAR_MULTIBYTE (c);
 	  c1 = c;
-	  if (inword && flag != CASE_CAPITALIZE_UP)
-	    c = downcase (c);
-	  else if (!uppercasep (c)
-		   && (!inword || flag != CASE_CAPITALIZE_UP))
-	    c = upcase1 (c1);
-	  if ((int) flag >= (int) CASE_CAPITALIZE)
-	    inword = (SYNTAX (c) == Sword);
-	  if (c != c1)
+	  if (ASCII_CHAR_P (c))
 	    {
-	      MAKE_CHAR_UNIBYTE (c);
-	      /* If the char can't be converted to a valid byte, just don't
-		 change it.  */
-	      if (c >= 0 && c < 256)
-		SSET (obj, i, c);
+	      if (inword && flag != CASE_CAPITALIZE_UP)
+		c = downcase (c);
+	      else if (!uppercasep (c)
+		       && (!inword || flag != CASE_CAPITALIZE_UP))
+		c = upcase1 (c1);
 	    }
+	  if ((int) flag >= (int) CASE_CAPITALIZE)
+	    inword = (SYNTAX (c) == Sword);
+	  if (c != c1 && ASCII_CHAR_P (c))
+	    SSET (obj, i, c);
 	}
       return obj;
     }
@@ -230,8 +226,9 @@ casify_region (enum case_action flag, Lisp_Object b, Lisp_Object e)
       else
 	{
 	  c = FETCH_BYTE (start_byte);
-	  MAKE_CHAR_MULTIBYTE (c);
 	  len = 1;
+	  if (!ASCII_CHAR_P (c))
+	    goto done;
 	}
       c2 = c;
       if (inword && flag != CASE_CAPITALIZE_UP)
@@ -239,9 +236,6 @@ casify_region (enum case_action flag, Lisp_Object b, Lisp_Object e)
       else if (!uppercasep (c)
 	       && (!inword || flag != CASE_CAPITALIZE_UP))
 	c = upcase1 (c);
-      if ((int) flag >= (int) CASE_CAPITALIZE)
-	inword = ((SYNTAX (c) == Sword)
-		  && (inword || !syntax_prefix_flag_p (c)));
       if (c != c2)
 	{
 	  last = start;
@@ -250,8 +244,8 @@ casify_region (enum case_action flag, Lisp_Object b, Lisp_Object e)
 
 	  if (! multibyte)
 	    {
-	      MAKE_CHAR_UNIBYTE (c);
-	      FETCH_BYTE (start_byte) = c;
+	      if (ASCII_CHAR_P (c))
+		FETCH_BYTE (start_byte) = c;
 	    }
 	  else if (ASCII_CHAR_P (c2) && ASCII_CHAR_P (c))
 	    FETCH_BYTE (start_byte) = c;
@@ -280,6 +274,10 @@ casify_region (enum case_action flag, Lisp_Object b, Lisp_Object e)
 		}
 	    }
 	}
+    done:
+      if ((int) flag >= (int) CASE_CAPITALIZE)
+	inword = ((SYNTAX (c) == Sword)
+		  && (inword || !syntax_prefix_flag_p (c)));
       start++;
       start_byte += len;
     }

If working on non-ASCII characters isn’t supported we might just as well
skip all the logic that handles non-ASCII unibyte characters.

> Properly means that upcasing "istanbul" in the above example will
> produce "İSTANBUL", not "iSTANBUL", and downcasing "IRMA" will produce
> "ırma".

I thought about that but then another corner case is "istanbul\xff"
which is a unibyte string with 8-bit bytes.

I have no strong feelings either way so I’m happy just leaving it as is
as well.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

bug#24425: [PATCH] Don’t cast Unicode to 8-bit when casing unibyte strings

[Eli Zaretskii]

> From: Michal Nazarewicz <mina86@mina86.com>
> Cc: 24425@debbugs.gnu.org
> Date: Thu, 15 Sep 2016 16:23:54 +0200
> 
> On Tue, Sep 13 2016, Eli Zaretskii wrote:
> > Currently, case changes in unibyte characters and strings are only
> > well defined for pure ASCII text; if the input or the result is not
> > pure ASCII, we produce "undefined behavior".
> 
> Would the following (not tested) make sense then:

AFAIU, it would disallow handling unibyte text by setting up case
tables for 8-bit characters in their multibyte representation,
i.e. above #x3FFF00.  I'd rather not lose that, although I don't think
I've ever seen that used.

> > Properly means that upcasing "istanbul" in the above example will
> > produce "İSTANBUL", not "iSTANBUL", and downcasing "IRMA" will produce
> > "ırma".
> 
> I thought about that but then another corner case is "istanbul\xff"
> which is a unibyte string with 8-bit bytes.

And what is the problem in that case?

> I have no strong feelings either way so I’m happy just leaving it as is
> as well.

That is fine with me.

Was there some real-life use case where you bumped into this?  If so,
maybe we should discuss that use case, perhaps the solution, if we
need one, is something other than what we talked about until now.

bug#24425: [PATCH] Don’t cast Unicode to 8-bit when casing unibyte strings

[Michal Nazarewicz]

>> I thought about that but then another corner case is "istanbul\xff"
>> which is a unibyte string with 8-bit bytes.

On Thu, Sep 15 2016, Eli Zaretskii wrote:
> And what is the problem in that case?

Disregard.  It’s actually fine.

>> I have no strong feelings either way so I’m happy just leaving it as
>> is as well.

> That is fine with me.
>
> Was there some real-life use case where you bumped into this?  If so,
> maybe we should discuss that use case, perhaps the solution, if we
> need one, is something other than what we talked about until now.

There’s no real-life use case I’ve stumbled upon.

I’m playing around with src/casefiddle.c adding support for various
corner cases (such as fish becoming Fish or FISH) and was surprised by
(upcase "istanbul") when testing Turkish support.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»