Repository: ultrajson/ultrajson (language: python)

Issue:
Surrogates fix fails tests with PyPy on Windows
### What did you do?

Built wheels on CI using https://github.com/ultrajson/ultrajson/blob/main/.github/workflows/deploy.yml.

### What did you expect to happen?

All wheels built.

### What actually happened?

Failed to build PyPy wheels on Windows due to `test_decode_surrogate_characters` failures:

* https://github.com/ultrajson/ultrajson/actions/runs/2507627808

Before PR https://github.com/ultrajson/ultrajson/pull/550 was merged, wheels were successfully built.

* https://github.com/ultrajson/ultrajson/actions/runs/2495845358

### What versions are you using?

* OS: Windows
* Python: PyPy
* UltraJSON: `main` after https://github.com/ultrajson/ultrajson/pull/550 merged.

PR https://github.com/ultrajson/ultrajson/pull/547 is partly to blame for removing PyPy + Windows from the CI.

---

Temporarily enabling a full Python x OS matrix, all pass before https://github.com/ultrajson/ultrajson/pull/550:

https://github.com/hugovk/ultrajson/actions/runs/2511636309

After the merge, all pass except `pypy-3.7` and `pypy3.8` on `windows-latest`:

https://github.com/hugovk/ultrajson/actions/runs/2511595253



Discussion / hints:
Hmm, interesting. And it's not just the `wchar_t` merging behaviour I mentioned in the PR, but all lone surrogates seem to get replaced with U+FFFD. That's odd.
This smells like a bug in PyPy. I have verified that `decode_string` is performing exactly as expected. So for example in the test case with input `"\\uD83D\\uD83D\\uDCA9"`, the result is a three-element `wchar_t` with values D83D, D83D, and DCA9 on 16-bit `wchar_t` platforms. When this is fed to CPython's `PyUnicode_FromWideChar`, it results in the expected output, a lone surrogate and U+1F4A9. PyPy instead produces U+FFFD and U+1F4A9. However, based on a glance at the relevant source code, I couldn't tell where the replacement occurs.

The fix is likely the same as for that broken `wchar_t` merging of raw surrogates anyway: moving away from `wchar_t` entirely. `Py_UCS4` and `PyUnicode_FromKindAndData` seem like the obvious route; that interface did not exist yet when the `wchar_t` code was written. I'll give this a try soon. Will be interesting to see its performance as well.
I have a working implementation with `Py_UCS4` which passes all tests (including the ones that were skipped in #550) on `windows-latest` and `ubuntu-latest` with CPython 3.10 and PyPy 3.8. The code is very messy currently though; in particular, it imports `Python.h` in the lib to get the `Py_UCS4` typedef. It's an unsigned int of at least 32 bit, and while the obvious type for that is `uint32_t` (and that is indeed what CPython uses), there's no guarantee of that. Any ideas how to best approach that?
I believe that abandoning `wchar_t` in favour of `uint32_t` is the trendy, albeit reluctant, choice to make in order to make Windows behave with unicode. I personally don't mind if we just hardcode the assumption that `Py_UCS4` is `uint32_t` and either use `uint32_t` directly in `lib` or have our own `typedef JSUCHAR uint32_t` in `lib` which we use instead. I can't think of any good reason why Python would want to set `Py_USC4` to anything else.
Well, PyPy has a `typedef unsigned int Py_UCS4` for some reason (and either assumes that `int` is at least 32 bit or simply doesn't support other platforms, I guess), so there's that...

Maybe we can add a clever check somewhere to ensure that the types match during compilation. Something like this, functionally (as this doesn't actually work, I think):

```c
#if sizeof(Py_UCS4) != sizeof(uint32_t)
#error "Py_UCS4 does not match uint32_t
#endif
```
> Well, PyPy has a typedef unsigned int Py_UCS4 for some reason (and either assumes that int is at least 32 bit or simply doesn't support other platforms, I guess), so there's that...

What? Why? [splutter splutter splutter]

---

Yeah, that check would be useful. It don't think the test failures would be that self explainatory if `Py_UCS4` became something else.
So I haven't done assertions at compile-time before. The clean way to do that is `_Static_assert`, which was added only in C11. CPython 3.11 requires C11 anyway, but previous versions don't, so we probably shouldn't rely on that just yet. There's a wide range of hacks to emulate it in older C, some of which no longer work. I think I'll go with the one that's also used in Linux (specifically in `vbox_vmmdev_types.h`):

```c
typedef char assert_py_ucs4_is_jsuint32[1 - 2*!!(sizeof(Py_UCS4) == sizeof(JSUINT32))];
```

Slightly cursed, but hey, at least it works. :-)
That's so ugly that it's comical. We've got to have it! (Honestly, I don't even want to know how that works.)
The funny thing is that it's actually one of the cleaner ways to do this. I suppose `(condition) ? 1 : -1` would make the meaning more obvious, but that's no fun. `!!` negates the result of the comparison twice to get a 0 (if the comparison evaluates to false) or 1 (if it's true). Then this tries to define an array type with either size 1 or -1. The latter is obviously an error, and the compiler gets angry. And if it passes and the type is never referenced anywhere, it should get optimised out and never end up in the binary. The abyss stared back...

There's an older, uglier version:

```c
#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
```

The actual error is the exact same reasoning, and the `(void)sizeof(...)` thing is there for ... [reasons](https://scaryreasoner.wordpress.com/2009/02/28/checking-sizeof-at-compile-time/). This no longer compiles with current GCC though (fortunately).

Aanyway, enough about this. I'll prepare a PR.

Edit: Actually had the logic messed up there; a true comparison is equal to 1, a false one 0. The condition is what must be true *for compilation to fail* in this ... thing. A single exclamation mark should invert the meaning.

Fix patch:
diff --git a/lib/ultrajson.h b/lib/ultrajson.h
index 170214ff..eda221aa 100644
--- a/lib/ultrajson.h
+++ b/lib/ultrajson.h
@@ -54,7 +54,6 @@ tree doesn't have cyclic references.
 #define __ULTRAJSON_H__
 
 #include <stdio.h>
-#include <wchar.h>
 
 // Max decimals to encode double floating point numbers with
 #ifndef JSON_DOUBLE_MAX_DECIMALS
@@ -318,7 +317,7 @@ EXPORTFUNCTION char *JSON_EncodeObject(JSOBJ obj, JSONObjectEncoder *enc, char *
 
 typedef struct __JSONObjectDecoder
 {
-  JSOBJ (*newString)(void *prv, wchar_t *start, wchar_t *end);
+  JSOBJ (*newString)(void *prv, JSUINT32 *start, JSUINT32 *end);
   void (*objectAddKey)(void *prv, JSOBJ obj, JSOBJ name, JSOBJ value);
   void (*arrayAddItem)(void *prv, JSOBJ obj, JSOBJ value);
   JSOBJ (*newTrue)(void *prv);
diff --git a/lib/ultrajsondec.c b/lib/ultrajsondec.c
index caf15ccc..47485a6b 100644
--- a/lib/ultrajsondec.c
+++ b/lib/ultrajsondec.c
@@ -41,7 +41,6 @@ Numeric decoder derived from from TCL library
 #include <assert.h>
 #include <string.h>
 #include <limits.h>
-#include <wchar.h>
 #include <stdlib.h>
 #include <errno.h>
 #include <stdint.h>
@@ -58,8 +57,8 @@ struct DecoderState
 {
   char *start;
   char *end;
-  wchar_t *escStart;
-  wchar_t *escEnd;
+  JSUINT32 *escStart;
+  JSUINT32 *escEnd;
   int escHeap;
   int lastType;
   JSUINT32 objDepth;
@@ -361,14 +360,12 @@ static const JSUINT8 g_decoderLookup[256] =
 static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds)
 {
   int index;
-  wchar_t *escOffset;
-  wchar_t *escStart;
+  JSUINT32 *escOffset;
+  JSUINT32 *escStart;
   size_t escLen = (ds->escEnd - ds->escStart);
   JSUINT8 *inputOffset;
   JSUTF16 ch = 0;
-#if WCHAR_MAX >= 0x10FFFF
   JSUINT8 *lastHighSurrogate = NULL;
-#endif
   JSUINT8 oct;
   JSUTF32 ucs;
   ds->lastType = JT_INVALID;
@@ -380,11 +377,11 @@ static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds
 
     if (ds->escHeap)
     {
-      if (newSize > (SIZE_MAX / sizeof(wchar_t)))
+      if (newSize > (SIZE_MAX / sizeof(JSUINT32)))
       {
         return SetError(ds, -1, "Could not reserve memory block");
       }
-      escStart = (wchar_t *)ds->dec->realloc(ds->escStart, newSize * sizeof(wchar_t));
+      escStart = (JSUINT32 *)ds->dec->realloc(ds->escStart, newSize * sizeof(JSUINT32));
       if (!escStart)
       {
         ds->dec->free(ds->escStart);
@@ -394,18 +391,18 @@ static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds
     }
     else
     {
-      wchar_t *oldStart = ds->escStart;
-      if (newSize > (SIZE_MAX / sizeof(wchar_t)))
+      JSUINT32 *oldStart = ds->escStart;
+      if (newSize > (SIZE_MAX / sizeof(JSUINT32)))
       {
         return SetError(ds, -1, "Could not reserve memory block");
       }
-      ds->escStart = (wchar_t *) ds->dec->malloc(newSize * sizeof(wchar_t));
+      ds->escStart = (JSUINT32 *) ds->dec->malloc(newSize * sizeof(JSUINT32));
       if (!ds->escStart)
       {
         return SetError(ds, -1, "Could not reserve memory block");
       }
       ds->escHeap = 1;
-      memcpy(ds->escStart, oldStart, escLen * sizeof(wchar_t));
+      memcpy(ds->escStart, oldStart, escLen * sizeof(JSUINT32));
     }
 
     ds->escEnd = ds->escStart + newSize;
@@ -438,14 +435,14 @@ static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds
         inputOffset ++;
         switch (*inputOffset)
         {
-          case '\\': *(escOffset++) = L'\\'; inputOffset++; continue;
-          case '\"': *(escOffset++) = L'\"'; inputOffset++; continue;
-          case '/':  *(escOffset++) = L'/';  inputOffset++; continue;
-          case 'b':  *(escOffset++) = L'\b'; inputOffset++; continue;
-          case 'f':  *(escOffset++) = L'\f'; inputOffset++; continue;
-          case 'n':  *(escOffset++) = L'\n'; inputOffset++; continue;
-          case 'r':  *(escOffset++) = L'\r'; inputOffset++; continue;
-          case 't':  *(escOffset++) = L'\t'; inputOffset++; continue;
+          case '\\': *(escOffset++) = '\\'; inputOffset++; continue;
+          case '\"': *(escOffset++) = '\"'; inputOffset++; continue;
+          case '/':  *(escOffset++) = '/';  inputOffset++; continue;
+          case 'b':  *(escOffset++) = '\b'; inputOffset++; continue;
+          case 'f':  *(escOffset++) = '\f'; inputOffset++; continue;
+          case 'n':  *(escOffset++) = '\n'; inputOffset++; continue;
+          case 'r':  *(escOffset++) = '\r'; inputOffset++; continue;
+          case 't':  *(escOffset++) = '\t'; inputOffset++; continue;
 
           case 'u':
           {
@@ -494,7 +491,6 @@ static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds
               inputOffset ++;
             }
 
-#if WCHAR_MAX >= 0x10FFFF
             if ((ch & 0xfc00) == 0xdc00 && lastHighSurrogate == inputOffset - 6 * sizeof(*inputOffset))
             {
               // Low surrogate immediately following a high surrogate
@@ -502,16 +498,13 @@ static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds
               *(escOffset-1) = (((*(escOffset-1) - 0xd800) <<10) | (ch - 0xdc00)) + 0x10000;
             }
             else
-#endif
             {
-              *(escOffset++) = (wchar_t) ch;
+              *(escOffset++) = (JSUINT32) ch;
             }
-#if WCHAR_MAX >= 0x10FFFF
             if ((ch & 0xfc00) == 0xd800)
             {
               lastHighSurrogate = inputOffset;
             }
-#endif
             break;
           }
 
@@ -523,7 +516,7 @@ static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds
 
       case 1:
       {
-        *(escOffset++) = (wchar_t) (*inputOffset++);
+        *(escOffset++) = (JSUINT32) (*inputOffset++);
         break;
       }
 
@@ -537,7 +530,7 @@ static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds
         }
         ucs |= (*inputOffset++) & 0x3f;
         if (ucs < 0x80) return SetError (ds, -1, "Overlong 2 byte UTF-8 sequence detected when decoding 'string'");
-        *(escOffset++) = (wchar_t) ucs;
+        *(escOffset++) = (JSUINT32) ucs;
         break;
       }
 
@@ -560,7 +553,7 @@ static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds
         }
 
         if (ucs < 0x800) return SetError (ds, -1, "Overlong 3 byte UTF-8 sequence detected when encoding string");
-        *(escOffset++) = (wchar_t) ucs;
+        *(escOffset++) = (JSUINT32) ucs;
         break;
       }
 
@@ -584,20 +577,7 @@ static FASTCALL_ATTR JSOBJ FASTCALL_MSVC decode_string ( struct DecoderState *ds
 
         if (ucs < 0x10000) return SetError (ds, -1, "Overlong 4 byte UTF-8 sequence detected when decoding 'string'");
 
-#if WCHAR_MAX == 0xffff
-        if (ucs >= 0x10000)
-        {
-          ucs -= 0x10000;
-          *(escOffset++) = (wchar_t) (ucs >> 10) + 0xd800;
-          *(escOffset++) = (wchar_t) (ucs & 0x3ff) + 0xdc00;
-        }
-        else
-        {
-          *(escOffset++) = (wchar_t) ucs;
-        }
-#else
-        *(escOffset++) = (wchar_t) ucs;
-#endif
+        *(escOffset++) = (JSUINT32) ucs;
         break;
       }
     }
@@ -810,14 +790,14 @@ JSOBJ JSON_DecodeObject(JSONObjectDecoder *dec, const char *buffer, size_t cbBuf
   /*
   FIXME: Base the size of escBuffer of that of cbBuffer so that the unicode escaping doesn't run into the wall each time */
   struct DecoderState ds;
-  wchar_t escBuffer[(JSON_MAX_STACK_BUFFER_SIZE / sizeof(wchar_t))];
+  JSUINT32 escBuffer[(JSON_MAX_STACK_BUFFER_SIZE / sizeof(JSUINT32))];
   JSOBJ ret;
 
   ds.start = (char *) buffer;
   ds.end = ds.start + cbBuffer;
 
   ds.escStart = escBuffer;
-  ds.escEnd = ds.escStart + (JSON_MAX_STACK_BUFFER_SIZE / sizeof(wchar_t));
+  ds.escEnd = ds.escStart + (JSON_MAX_STACK_BUFFER_SIZE / sizeof(JSUINT32));
   ds.escHeap = 0;
   ds.prv = dec->prv;
   ds.dec = dec;
diff --git a/python/JSONtoObj.c b/python/JSONtoObj.c
index 55000b58..fbb3fb6f 100644
--- a/python/JSONtoObj.c
+++ b/python/JSONtoObj.c
@@ -59,9 +59,18 @@ static void Object_arrayAddItem(void *prv, JSOBJ obj, JSOBJ value)
   return;
 }
 
-static JSOBJ Object_newString(void *prv, wchar_t *start, wchar_t *end)
+/*
+Check that Py_UCS4 is the same as JSUINT32, else Object_newString will fail.
+Based on Linux's check in vbox_vmmdev_types.h.
+This should be replaced with
+  _Static_assert(sizeof(Py_UCS4) == sizeof(JSUINT32));
+when C11 is made mandatory (CPython 3.11+, PyPy ?).
+*/
+typedef char assert_py_ucs4_is_jsuint32[1 - 2*!(sizeof(Py_UCS4) == sizeof(JSUINT32))];
+
+static JSOBJ Object_newString(void *prv, JSUINT32 *start, JSUINT32 *end)
 {
-  return PyUnicode_FromWideChar (start, (end - start));
+  return PyUnicode_FromKindAndData (PyUnicode_4BYTE_KIND, (Py_UCS4 *) start, (end - start));
 }
 
 static JSOBJ Object_newTrue(void *prv)


Test patch:
diff --git a/tests/test_ujson.py b/tests/test_ujson.py
index 66d8d0c9..d50ebbf9 100644
--- a/tests/test_ujson.py
+++ b/tests/test_ujson.py
@@ -1,4 +1,3 @@
-import ctypes
 import datetime as dt
 import decimal
 import io
@@ -515,10 +514,6 @@ def test_encode_surrogate_characters():
     assert ujson.dumps({"\ud800": "\udfff"}, ensure_ascii=False, sort_keys=True) == out2
 
 
-@pytest.mark.xfail(
-    hasattr(sys, "pypy_version_info") and os.name == "nt",
-    reason="This feature needs fixing! See #552",
-)
 @pytest.mark.parametrize(
     "test_input, expected",
     [
@@ -543,10 +538,6 @@ def test_encode_surrogate_characters():
     ],
 )
 def test_decode_surrogate_characters(test_input, expected):
-    # FIXME Wrong output (combined char) on platforms with 16-bit wchar_t
-    if test_input == '"\uD83D\uDCA9"' and ctypes.sizeof(ctypes.c_wchar) == 2:
-        pytest.skip("Raw surrogate pairs are not supported with 16-bit wchar_t")
-
     assert ujson.loads(test_input) == expected
     assert ujson.loads(test_input.encode("utf-8", "surrogatepass")) == expected