Repository: cython/cython
Pull request: Memoryviews work with cache_builtins=False
Now they make their own tiny cache of the relevant exceptions
at module initialization so they can still access these without
the GIL.

closes https://github.com/cython/cython/issues/3406

I guess this slightly breaks the contract of the cache_builtins option, that you should be able to monkey-patch builtin stuff (including exceptions). With that said - it's an implementation detail that it's written in Cython. If it was written in C then it'd just use [the standard exception definitions in the headers](https://docs.python.org/3/c-api/exceptions.html#standard-exceptions) and the user wouldn't be able to change them anyway.

Problem statement:
MemoryView implementation incompatible with Options.cache_builtins = False
Disabling the `cache_builtins` option in setup.py:
```
Options.cache_builtins = False
```
causes code that uses `MemoryView` to fail to compile:
```
Error compiling Cython file:
------------------------------------------------------------
...
    if not is_slice:

        if start < 0:
            start += shape
        if not 0 <= start < shape:
            _err_dim(IndexError, "Index out of bounds (axis %d)", dim)
                    ^
------------------------------------------------------------

View.MemoryView:832:21: Accessing Python global or builtin not allowed without gil
```

(There are several such errors building the file, all stemming from doing a lookup on the exception type without the GIL held.)

To fix this, the GIL should be locked before calling _err_dim.

Hints from issue discussion:
I've done this a tiny bit differently to how you've suggested, since I think the whole point of `_err_dim` was avoiding writing duplicate `with gil:` code but the basic diagnosis was good.
Great, thanks for the quick turnaround! Your way will prevent someone from mocking `ValueError`, but on the other hand I can't see anyone wanting to actually _do_ that, so it should work!
> Your way will prevent someone from mocking ValueError, but on the other hand I can't see anyone wanting to actually do that, so it should work

I was just adding a comment to that effect in the PR description when you replied:

>I guess this slightly breaks the contract of the cache_builtins option, that you should be able to monkey-patch builtin stuff (including exceptions). With that said - it's an implementation detail that it's written in Cython. If it was written in C then it'd just use the standard exception definitions in the headers and the user wouldn't be able to change them anyway.

I think the "implementation detail" excuse makes it OK but we'll see if it makes it past review...

Commit message: Disabled test

It was affecting static state and breaking subsequent tests

Patch to review:
diff --git a/Cython/Utility/MemoryView.pyx b/Cython/Utility/MemoryView.pyx
index 5d7a525d3fd..8d396335a8e 100644
--- a/Cython/Utility/MemoryView.pyx
+++ b/Cython/Utility/MemoryView.pyx
@@ -94,8 +94,11 @@ cdef extern from "<stdlib.h>":
     void free(void *) nogil
     void *memcpy(void *dest, void *src, size_t n) nogil
 
-
-
+# cache the errors used so access to them doesn't need the GIL
+# with Options.cache_builtins
+cdef __pyx_memoryview_IndexError = IndexError
+cdef __pyx_memoryview_ValueError = ValueError
+cdef __pyx_memoryview_MemoryError = MemoryError
 
 #
 ### cython.array class
@@ -829,13 +832,13 @@ cdef int slice_memviewslice(
         if start < 0:
             start += shape
         if not 0 <= start < shape:
-            _err_dim(IndexError, "Index out of bounds (axis %d)", dim)
+            _err_dim(__pyx_memoryview_IndexError, "Index out of bounds (axis %d)", dim)
     else:
         # index is a slice
         negative_step = have_step != 0 and step < 0
 
         if have_step and step == 0:
-            _err_dim(ValueError, "Step may not be zero (axis %d)", dim)
+            _err_dim(__pyx_memoryview_ValueError, "Step may not be zero (axis %d)", dim)
 
         # check our bounds and set defaults
         if have_start:
@@ -896,7 +899,7 @@ cdef int slice_memviewslice(
             if new_ndim == 0:
                 dst.data = (<char **> dst.data)[0] + suboffset
             else:
-                _err_dim(IndexError, "All dimensions preceding dimension %d "
+                _err_dim(__pyx_memoryview_IndexError, "All dimensions preceding dimension %d "
                                      "must be indexed and not sliced", dim)
         else:
             suboffset_dim[0] = new_ndim
@@ -954,7 +957,7 @@ cdef int transpose_memslice({{memviewslice_name}} *memslice) nogil except 0:
         shape[i], shape[j] = shape[j], shape[i]
 
         if memslice.suboffsets[i] >= 0 or memslice.suboffsets[j] >= 0:
-            _err(ValueError, "Cannot transpose memoryview with indirect dimensions")
+            _err(__pyx_memoryview_ValueError, "Cannot transpose memoryview with indirect dimensions")
 
     return 1
 
@@ -1221,7 +1224,7 @@ cdef void *copy_data_to_temp({{memviewslice_name}} *src,
 
     result = malloc(size)
     if not result:
-        _err(MemoryError, NULL)
+        _err(__pyx_memoryview_MemoryError, NULL)
 
     # tmpslice[0] = src
     tmpslice.data = <char *> result
@@ -1250,7 +1253,7 @@ cdef void *copy_data_to_temp({{memviewslice_name}} *src,
 @cname('__pyx_memoryview_err_extents')
 cdef int _err_extents(int i, Py_ssize_t extent1,
                              Py_ssize_t extent2) except -1 with gil:
-    raise ValueError("got differing extents in dimension %d (got %d and %d)" %
+    raise __pyx_memoryview_ValueError("got differing extents in dimension %d (got %d and %d)" %
                                                         (i, extent1, extent2))
 
 @cname('__pyx_memoryview_err_dim')
@@ -1297,7 +1300,7 @@ cdef int memoryview_copy_contents({{memviewslice_name}} src,
                 _err_extents(i, dst.shape[i], src.shape[i])
 
         if src.suboffsets[i] >= 0:
-            _err_dim(ValueError, "Dimension %d is not direct", i)
+            _err_dim(__pyx_memoryview_ValueError, "Dimension %d is not direct", i)
 
     if slices_overlap(&src, &dst, ndim, itemsize):
         # slices overlap, copy to temp, copy temp to dst
diff --git a/runtests.py b/runtests.py
index c126dd349f7..f3c151a9883 100755
--- a/runtests.py
+++ b/runtests.py
@@ -878,7 +878,7 @@ def setUp(self):
         from Cython.Compiler import Options
         self._saved_options = [
             (name, getattr(Options, name))
-            for name in ('warning_errors', 'clear_to_none', 'error_on_unknown_names', 'error_on_uninitialized')
+            for name in dir(Options) if not name.startswith("_")
         ]
         self._saved_default_directives = list(Options.get_directive_defaults().items())
         Options.warning_errors = self.warning_errors
@@ -1048,6 +1048,9 @@ def run_cython(self, test_directory, module, targetdir, incdir, annotate,
         if 'allow_unknown_names' in self.tags['tag']:
             from Cython.Compiler import Options
             Options.error_on_unknown_names = False
+        if 'no_cache_builtins' in self.tags['tag']:
+            from Cython.Compiler import Options
+            Options.cache_builtins = False
 
         try:
             CompilationOptions
diff --git a/tests/bugs.txt b/tests/bugs.txt
index e853b45263b..1dd9015a0d6 100644
--- a/tests/bugs.txt
+++ b/tests/bugs.txt
@@ -55,3 +55,9 @@ pyregr.test_urllibnet
 
 # Inlined generators
 inlined_generator_expressions
+
+# The test works fine. However setting Options.cache_builtins
+# affects what's stored in the builtins scope which isn't reset
+# between tests so it breaks a lot of subsequent stuff. There's
+# too much static-state to see an easy way round it.
+memoryview.memoryview_cache_builtins
diff --git a/tests/compile/fused_redeclare_T3111.pyx b/tests/compile/fused_redeclare_T3111.pyx
index 3dfed9f38b0..8607225c53a 100644
--- a/tests/compile/fused_redeclare_T3111.pyx
+++ b/tests/compile/fused_redeclare_T3111.pyx
@@ -24,16 +24,16 @@ def foo(dtype_t[:] a, dtype_t_out[:, :] b):
 _WARNINGS = """
 22:10: 'cpdef_method' redeclared
 33:10: 'cpdef_cname_method' redeclared
-446:72: Argument evaluation order in C function call is undefined and may not be as expected
-446:72: Argument evaluation order in C function call is undefined and may not be as expected
-749:34: Argument evaluation order in C function call is undefined and may not be as expected
-749:34: Argument evaluation order in C function call is undefined and may not be as expected
-943:27: Ambiguous exception value, same as default return value: 0
-943:27: Ambiguous exception value, same as default return value: 0
-974:29: Ambiguous exception value, same as default return value: 0
-974:29: Ambiguous exception value, same as default return value: 0
-1002:46: Ambiguous exception value, same as default return value: 0
-1002:46: Ambiguous exception value, same as default return value: 0
-1092:29: Ambiguous exception value, same as default return value: 0
-1092:29: Ambiguous exception value, same as default return value: 0
+449:72: Argument evaluation order in C function call is undefined and may not be as expected
+449:72: Argument evaluation order in C function call is undefined and may not be as expected
+752:34: Argument evaluation order in C function call is undefined and may not be as expected
+752:34: Argument evaluation order in C function call is undefined and may not be as expected
+946:27: Ambiguous exception value, same as default return value: 0
+946:27: Ambiguous exception value, same as default return value: 0
+977:29: Ambiguous exception value, same as default return value: 0
+977:29: Ambiguous exception value, same as default return value: 0
+1005:46: Ambiguous exception value, same as default return value: 0
+1005:46: Ambiguous exception value, same as default return value: 0
+1095:29: Ambiguous exception value, same as default return value: 0
+1095:29: Ambiguous exception value, same as default return value: 0
 """
diff --git a/tests/memoryview/memoryview_cache_builtins.pxd b/tests/memoryview/memoryview_cache_builtins.pxd
new file mode 100644
index 00000000000..0de6ba64c4c
--- /dev/null
+++ b/tests/memoryview/memoryview_cache_builtins.pxd
@@ -0,0 +1 @@
+include "memoryview.pxd"
diff --git a/tests/memoryview/memoryview_cache_builtins.pyx b/tests/memoryview/memoryview_cache_builtins.pyx
new file mode 100644
index 00000000000..5e7c59e5e46
--- /dev/null
+++ b/tests/memoryview/memoryview_cache_builtins.pyx
@@ -0,0 +1,7 @@
+# mode: run
+# tag: no_cache_builtins
+
+# memoryview was broken with cache_builtins=False (the exception throwing code needed the GIL)
+# this just repeats the tests with it off
+
+include "memoryview.pyx"


Code review comments:
diff_hunk: @@ -878,7 +878,7 @@ def setUp(self):
         from Cython.Compiler import Options
         self._saved_options = [
             (name, getattr(Options, name))
-            for name in ('warning_errors', 'clear_to_none', 'error_on_unknown_names', 'error_on_uninitialized')
+            for name in dir(Options) if not name.startswith("_")
path: runtests.py
text: @user1:
I actually kept it explicit at the time which options could be set in tests, but wouldn't object to making it dynamic if it gets in the way. Seems better to use `vars()`, though, instead of `dir()` + `getattr()`.

@author:
Adding `cache_builtins` to runtests.py turned out to be a disaster because the contents of the builtins scope isn't reset between tests. I've rolled this back and added a very simple srctree test instead.

diff_hunk: @@ -1221,7 +1224,7 @@ cdef void *copy_data_to_temp({{memviewslice_name}} *src,
 
     result = malloc(size)
     if not result:
-        _err(MemoryError, NULL)
+        _err(__pyx_memoryview_MemoryError, NULL)
path: Cython/Utility/MemoryView.pyx
text: @user1:
This is just `PyErr_NoMemory()`, right?

@author:
Yes but it'd need a `with gil` block

diff_hunk: @@ -94,8 +94,11 @@ cdef extern from "<stdlib.h>":
     void free(void *) nogil
     void *memcpy(void *dest, void *src, size_t n) nogil
 
-
-
+# cache the errors used so access to them doesn't need the GIL
+# with Options.cache_builtins
+cdef __pyx_memoryview_IndexError = IndexError
path: Cython/Utility/MemoryView.pyx
text: @user1:
Why not just declare `PyExc_IndexError` and friends as external `object`, and use those?

@author:
extern `object` isn't allowed but I think this way makes sense so I've declared them as `PyObject*` and added suitable casts inside `_err` and `_err_dim`

@user1:
Did you push that change?

@author:
No. It looks like I'd stashed it and then forgotten about it. Done now.