Repository: numpy/numpy
Pull request: BUG: fix handling of copy keyword argument when calling __array__
Closes #25916.

This applies @rgommers draft changes from the issue description and adds a couple more fixes on top.

* Replaces internal usages of `never_copy` with `copy` that can be `-1` (corresponding to `None`), `0` (`False`), and `1` (`True`). This allows the array coercion machinery and `PyArray_FromArrayAttr_int` to support passing `copy=True` (although right now that will never happen).

* Only passes a copy keyword if copy is not the default value.

* Fixes a bug in checking for the error message

* Removes an incorrect usage of `Py_SetRef` (sorry about suggesting that in the first place 😬 )

* Fixes some reference counting issues

* Tests that the old deprecated calling convention still works.

Problem statement:
`__array__` copy keyword changes incomplete
It's still very early in the morning and I may simply not be awake yet, but it looks to me like the `__array__` changes in gh-25168 are incomplete and don't have a regression test for the old signature:
```python
In [1]: import numpy as np

In [2]: class MyArr:
   ...:     def __init__(self, x):
   ...:         self._x = x
   ...:     def __array__(self, dtype=None):
   ...:         return self._x
   ...: 

In [3]: x = np.arange(3)

In [4]: np.asarray(x)
Out[4]: array([0, 1, 2])

In [5]: np.asarray(MyArr(x))
...
TypeError: MyArr.__array__() got an unexpected keyword argument 'copy'

In [6]: np.__version__
Out[6]: '2.0.0.dev0+git20240301.06b169b'
```

I didn't follow the review of the C code in detail, so I'm probably missing some details, but it doesn't look like the current implementation does what was intended. In particular:
- only pass along the new `copy` keyword if it's not the default value of `None`, to have a smaller impact,
- pass the actual True/False/None values; it now only passes `False` or `None` (`never_copy ? Py_False : Py_None;`),
- when retrying without the new keyword, the `dtype` keyword should still be passed, so the code should pop `copy` and retry with `kwargs` rather than passing NULL.

It looks like the changes should be along these lines (incomplete): 
```diff
$ git diff
diff --git a/numpy/_core/src/multiarray/ctors.c b/numpy/_core/src/multiarray/ctors.c
index 4b9c3add3f..897e141336 100644
--- a/numpy/_core/src/multiarray/ctors.c
+++ b/numpy/_core/src/multiarray/ctors.c
@@ -2442,26 +2442,46 @@ PyArray_FromArrayAttr_int(
 
     PyObject *copy = never_copy ? Py_False : Py_None;
     PyObject *kwargs = PyDict_New();
-    PyDict_SetItemString(kwargs, "copy", copy);
+
+    /*
+     * Only if the value of `copy` isn't the default one, we try to pass it
+     * along; for backwards compatibility we then retry if it fails because the
+     * signature of the __array__ method being called does not have `copy`.
+     */
+    int copy_passed = 0;
+    if (copy != Py_None) {
+        /* XXX: why can `copy` only be False or None, not True? */
+        copy_passed = 1;
+        PyDict_SetItemString(kwargs, "copy", copy);
+    }
     PyObject *args = descr != NULL ? PyTuple_Pack(1, descr) : PyTuple_New(0);
 
     new = PyObject_Call(array_meth, args, kwargs);
 
     if (PyErr_Occurred()) {
+        PyObject *errmsg_substr = PyUnicode_FromString(
+                "__array__() got an unexpected keyword argument 'copy'");
+        if (errmsg_substr == NULL) {
+            return NULL;
+        }
         PyObject *type, *value, *traceback;
         PyErr_Fetch(&type, &value, &traceback);
-        if (PyUnicode_Check(value) && PyUnicode_CompareWithASCIIString(value,
-                    "__array__() got an unexpected keyword argument 'copy'") == 0) {
+        if (PyUnicode_Check(value) && PyUnicode_Contains(value, errmsg_substr) > 0) {
             Py_DECREF(type);
             Py_XDECREF(value);
             Py_XDECREF(traceback);
+            Py_DECREF(errmsg_substr);
             if (PyErr_WarnEx(PyExc_UserWarning,
                              "__array__ should implement 'dtype' and 'copy' keywords", 1) < 0) {
                 return NULL;
             }
-            Py_SETREF(new, PyObject_Call(array_meth, args, NULL));
+            if (copy_passed) { /* try again */
+                PyDict_DelItemString(kwargs, "copy");
+                Py_SETREF(new, PyObject_Call(array_meth, args, kwargs));
+            }
         } else {
             PyErr_Restore(type, value, traceback);
+            Py_DECREF(errmsg_substr);
             return NULL;
         }
     }
```

Cc @mtsokol @ngoldbaum @seberg.

Hints from issue discussion:
This breaks a number of tests in sympy:
```
    def test_issue_14943():
        # Test that __array__ accepts the optional dtype argument
        try:
            from numpy import array
        except ImportError:
            skip('NumPy must be available to test creating matrices from ndarrays')
    
        M = Matrix([[1,2], [3,4]])
>       assert array(M, dtype=float).dtype.name == 'float64'
E       TypeError: MatrixBase.__array__() got an unexpected keyword argument 'copy'

sympy/matrices/tests/test_matrices.py:2967: TypeError
```
At the same time we are also seeing a segfault in CI with the numpy nightly wheels but I don't know if that is related to this (they both seem to happen at the same time):
https://github.com/sympy/sympy/actions/runs/8116952589/job/22188138475?pr=26294

https://groups.google.com/g/sympy/c/OAcu4xV-R7k
> we are also seeing a segfault in CI

I can't reproduce the segfault locally so it is only in CI for now...
> At the same time we are also seeing a segfault in CI with the numpy nightly wheels 

Can you try a CI run where you only use one of nightly numpy/nightly scipy to try to narrow it down a bit?
Almost certainly related, so not sure it is worth digging.  this is the test in question (where it segfaults):
```python
def test_issue_14943():
    # Test that __array__ accepts the optional dtype argument
    try:
        from numpy import array
    except ImportError:
        skip('NumPy must be available to test creating matrices from ndarrays')

    M = Matrix([[1,2], [3,4]])
    assert array(M, dtype=float).dtype.name == 'float64'
```
Yes, the `Py_SETREF(new, PyObject_Call(array_meth, args, NULL));` line seems to be problematic.

Commit message: BUG: fix handling of copy keyword argument when calling __array__

Patch to review:
diff --git a/doc/release/upcoming_changes/25168.change.rst b/doc/release/upcoming_changes/25168.change.rst
index 528027e26dbb..139c15679f6f 100644
--- a/doc/release/upcoming_changes/25168.change.rst
+++ b/doc/release/upcoming_changes/25168.change.rst
@@ -7,3 +7,15 @@ Now `numpy.array` and `numpy.asarray` support three values for ``copy`` paramete
 * ``False`` - Never make a copy. If a copy is required a ``ValueError`` is raised.
 
 The meaning of ``False`` changed as it now raises an exception if a copy is needed.
+
+The ``__array__`` special method now takes a ``copy`` keyword argument.
+-----------------------------------------------------------------------
+
+NumPy will pass ``copy`` to the ``__array__`` special method in situations where
+it would be set to a non-default value (e.g. in a call to
+``np.asarray(some_object, copy=False)``). Currently, if an
+unexpected keyword argument error is raised after this, NumPy will print a
+warning and re-try without the ``copy`` keyword argument. Implementations of
+objects implementing the ``__array__`` protocol should accept a ``copy`` keyword
+argument with the same meaning as when passed to `numpy.array` or
+`numpy.asarray`.
diff --git a/numpy/_core/src/multiarray/array_coercion.c b/numpy/_core/src/multiarray/array_coercion.c
index c2f279ea17c9..25f58adc9a2c 100644
--- a/numpy/_core/src/multiarray/array_coercion.c
+++ b/numpy/_core/src/multiarray/array_coercion.c
@@ -958,7 +958,8 @@ PyArray_AdaptDescriptorToArray(
  *        (Initially it is a pointer to the user-provided head pointer).
  * @param fixed_DType User provided fixed DType class
  * @param flags Discovery flags (reporting and behaviour flags, see def.)
- * @param never_copy Specifies if a copy is allowed during array creation.
+ * @param copy Specifies the copy behavior. -1 is corresponds to copy=None,
+ *        0 to copy=False, and 1 to copy=True in the Python API.
  * @return The updated number of maximum dimensions (i.e. scalars will set
  *         this to the current dimensions).
  */
@@ -968,7 +969,7 @@ PyArray_DiscoverDTypeAndShape_Recursive(
         npy_intp out_shape[NPY_MAXDIMS],
         coercion_cache_obj ***coercion_cache_tail_ptr,
         PyArray_DTypeMeta *fixed_DType, enum _dtype_discovery_flags *flags,
-        int never_copy)
+        int copy)
 {
     PyArrayObject *arr = NULL;
     PyObject *seq;
@@ -1026,7 +1027,7 @@ PyArray_DiscoverDTypeAndShape_Recursive(
             requested_descr = *out_descr;
         }
         arr = (PyArrayObject *)_array_from_array_like(obj,
-                requested_descr, 0, NULL, never_copy);
+                requested_descr, 0, NULL, copy);
         if (arr == NULL) {
             return -1;
         }
@@ -1173,7 +1174,7 @@ PyArray_DiscoverDTypeAndShape_Recursive(
         max_dims = PyArray_DiscoverDTypeAndShape_Recursive(
                 objects[i], curr_dims + 1, max_dims,
                 out_descr, out_shape, coercion_cache_tail_ptr, fixed_DType,
-                flags, never_copy);
+                flags, copy);
 
         if (max_dims < 0) {
             return -1;
@@ -1213,7 +1214,8 @@ PyArray_DiscoverDTypeAndShape_Recursive(
  *        The result may be unchanged (remain NULL) when converting a
  *        sequence with no elements. In this case it is callers responsibility
  *        to choose a default.
- * @param never_copy Specifies that a copy is not allowed.
+ * @param copy Specifies the copy behavior. -1 is corresponds to copy=None,
+ *        0 to copy=False, and 1 to copy=True in the Python API.
  * @return dimensions of the discovered object or -1 on error.
  *         WARNING: If (and only if) the output is a single array, the ndim
  *         returned _can_ exceed the maximum allowed number of dimensions.
@@ -1226,7 +1228,7 @@ PyArray_DiscoverDTypeAndShape(
         npy_intp out_shape[NPY_MAXDIMS],
         coercion_cache_obj **coercion_cache,
         PyArray_DTypeMeta *fixed_DType, PyArray_Descr *requested_descr,
-        PyArray_Descr **out_descr, int never_copy)
+        PyArray_Descr **out_descr, int copy)
 {
     coercion_cache_obj **coercion_cache_head = coercion_cache;
     *coercion_cache = NULL;
@@ -1273,7 +1275,7 @@ PyArray_DiscoverDTypeAndShape(
 
     int ndim = PyArray_DiscoverDTypeAndShape_Recursive(
             obj, 0, max_dims, out_descr, out_shape, &coercion_cache,
-            fixed_DType, &flags, never_copy);
+            fixed_DType, &flags, copy);
     if (ndim < 0) {
         goto fail;
     }
diff --git a/numpy/_core/src/multiarray/array_coercion.h b/numpy/_core/src/multiarray/array_coercion.h
index 6ac381d8f511..9c2eb458240d 100644
--- a/numpy/_core/src/multiarray/array_coercion.h
+++ b/numpy/_core/src/multiarray/array_coercion.h
@@ -40,7 +40,7 @@ PyArray_DiscoverDTypeAndShape(
         npy_intp out_shape[NPY_MAXDIMS],
         coercion_cache_obj **coercion_cache,
         PyArray_DTypeMeta *fixed_DType, PyArray_Descr *requested_descr,
-        PyArray_Descr **out_descr, int never_copy);
+        PyArray_Descr **out_descr, int copy);
 
 NPY_NO_EXPORT PyObject *
 _discover_array_parameters(PyObject *NPY_UNUSED(self),
diff --git a/numpy/_core/src/multiarray/arrayobject.c b/numpy/_core/src/multiarray/arrayobject.c
index 91844fd7605a..86245d9897d6 100644
--- a/numpy/_core/src/multiarray/arrayobject.c
+++ b/numpy/_core/src/multiarray/arrayobject.c
@@ -251,7 +251,7 @@ PyArray_CopyObject(PyArrayObject *dest, PyObject *src_object)
      */
     ndim = PyArray_DiscoverDTypeAndShape(src_object,
             PyArray_NDIM(dest), dims, &cache,
-            NPY_DTYPE(PyArray_DESCR(dest)), PyArray_DESCR(dest), &dtype, 0);
+            NPY_DTYPE(PyArray_DESCR(dest)), PyArray_DESCR(dest), &dtype, 1);
     if (ndim < 0) {
         return -1;
     }
diff --git a/numpy/_core/src/multiarray/common.c b/numpy/_core/src/multiarray/common.c
index 573d0d6063c7..925bc1628295 100644
--- a/numpy/_core/src/multiarray/common.c
+++ b/numpy/_core/src/multiarray/common.c
@@ -119,7 +119,7 @@ PyArray_DTypeFromObject(PyObject *obj, int maxdims, PyArray_Descr **out_dtype)
     int ndim;
 
     ndim = PyArray_DiscoverDTypeAndShape(
-            obj, maxdims, shape, &cache, NULL, NULL, out_dtype, 0);
+            obj, maxdims, shape, &cache, NULL, NULL, out_dtype, 1);
     if (ndim < 0) {
         return -1;
     }
diff --git a/numpy/_core/src/multiarray/ctors.c b/numpy/_core/src/multiarray/ctors.c
index b2b7339978e6..cd3291762efb 100644
--- a/numpy/_core/src/multiarray/ctors.c
+++ b/numpy/_core/src/multiarray/ctors.c
@@ -1421,7 +1421,7 @@ _array_from_buffer_3118(PyObject *memoryview)
  *                       DType may be used, but is not enforced.
  * @param writeable whether the result must be writeable.
  * @param context Unused parameter, must be NULL (should be removed later).
- * @param never_copy Specifies that a copy is not allowed.
+ * @param copy Specifies the copy behavior.
  *
  * @returns The array object, Py_NotImplemented if op is not array-like,
  *          or NULL with an error set. (A new reference to Py_NotImplemented
@@ -1430,7 +1430,7 @@ _array_from_buffer_3118(PyObject *memoryview)
 NPY_NO_EXPORT PyObject *
 _array_from_array_like(PyObject *op,
         PyArray_Descr *requested_dtype, npy_bool writeable, PyObject *context,
-        int never_copy) {
+        int copy) {
     PyObject* tmp;
 
     /*
@@ -1478,7 +1478,7 @@ _array_from_array_like(PyObject *op,
     }
 
     if (tmp == Py_NotImplemented) {
-        tmp = PyArray_FromArrayAttr_int(op, requested_dtype, never_copy);
+        tmp = PyArray_FromArrayAttr_int(op, requested_dtype, copy);
         if (tmp == NULL) {
             return NULL;
         }
@@ -1563,9 +1563,15 @@ PyArray_FromAny_int(PyObject *op, PyArray_Descr *in_descr,
         return NULL;
     }
 
-    ndim = PyArray_DiscoverDTypeAndShape(op,
-            NPY_MAXDIMS, dims, &cache, in_DType, in_descr, &dtype,
-            flags & NPY_ARRAY_ENSURENOCOPY);
+    // Default is copy = None
+    int copy = -1;
+
+    if (flags & NPY_ARRAY_ENSURENOCOPY) {
+        copy = 0;
+    }
+
+    ndim = PyArray_DiscoverDTypeAndShape(
+            op, NPY_MAXDIMS, dims, &cache, in_DType, in_descr, &dtype, copy);
 
     if (ndim < 0) {
         return NULL;
@@ -2408,15 +2414,16 @@ PyArray_FromInterface(PyObject *origin)
  * @param op The Python object to convert to an array.
  * @param descr The desired `arr.dtype`, passed into the `__array__` call,
  *        as information but is not checked/enforced!
- * @param never_copy Specifies that a copy is not allowed.
- *        NOTE: For false it passes `op.__array__(copy=None)`,
- *              for true: `op.__array__(copy=False)`.
+ * @param copy Specifies the copy behavior
+ *        NOTE: For copy == -1 it passes `op.__array__(copy=None)`,
+ *              for copy == 0, `op.__array__(copy=False)`, and
+ *              for copy == 1, `op.__array__(copy=True).
  * @returns NotImplemented if `__array__` is not defined or a NumPy array
  *          (or subclass).  On error, return NULL.
  */
 NPY_NO_EXPORT PyObject *
 PyArray_FromArrayAttr_int(
-        PyObject *op, PyArray_Descr *descr, int never_copy)
+        PyObject *op, PyArray_Descr *descr, int copy)
 {
     PyObject *new;
     PyObject *array_meth;
@@ -2440,36 +2447,59 @@ PyArray_FromArrayAttr_int(
         return Py_NotImplemented;
     }
 
-    PyObject *copy = never_copy ? Py_False : Py_None;
     PyObject *kwargs = PyDict_New();
-    PyDict_SetItemString(kwargs, "copy", copy);
+
+    /*
+     * Only if the value of `copy` isn't the default one, we try to pass it
+     * along; for backwards compatibility we then retry if it fails because the
+     * signature of the __array__ method being called does not have `copy`.
+     */
+    int copy_passed = 0;
+    if (copy != -1) {
+        copy_passed = 1;
+        PyObject *copy_obj = copy == 1 ? Py_True : Py_False;
+        PyDict_SetItemString(kwargs, "copy", copy_obj);
+    }
     PyObject *args = descr != NULL ? PyTuple_Pack(1, descr) : PyTuple_New(0);
 
     new = PyObject_Call(array_meth, args, kwargs);
 
-    if (PyErr_Occurred()) {
+    if (new == NULL) {
+        PyObject *errmsg_substr = PyUnicode_FromString(
+                "__array__() got an unexpected keyword argument 'copy'");
+        if (errmsg_substr == NULL) {
+            return NULL;
+        }
         PyObject *type, *value, *traceback;
         PyErr_Fetch(&type, &value, &traceback);
-        if (PyUnicode_Check(value) && PyUnicode_CompareWithASCIIString(value,
-                    "__array__() got an unexpected keyword argument 'copy'") == 0) {
+        if (PyUnicode_Check(value) && PyUnicode_Contains(value, errmsg_substr) > 0) {
             Py_DECREF(type);
             Py_XDECREF(value);
             Py_XDECREF(traceback);
+            Py_DECREF(errmsg_substr);
             if (PyErr_WarnEx(PyExc_UserWarning,
                              "__array__ should implement 'dtype' and 'copy' keywords", 1) < 0) {
                 return NULL;
             }
-            Py_SETREF(new, PyObject_Call(array_meth, args, NULL));
+            if (copy_passed) { /* try again */
+                PyDict_DelItemString(kwargs, "copy");
+                new = PyObject_Call(array_meth, args, kwargs);
+                if (new == NULL) {
+                    Py_DECREF(kwargs);
+                    return NULL;
+                }
+            }
         } else {
             PyErr_Restore(type, value, traceback);
+            Py_DECREF(errmsg_substr);
+            Py_DECREF(kwargs);
             return NULL;
         }
     }
 
+    Py_DECREF(kwargs);
     Py_DECREF(array_meth);
-    if (new == NULL) {
-        return NULL;
-    }
+
     if (!PyArray_Check(new)) {
         PyErr_SetString(PyExc_ValueError,
                         "object __array__ method not "  \
diff --git a/numpy/_core/src/multiarray/ctors.h b/numpy/_core/src/multiarray/ctors.h
index 36ee1fb38237..d2577f83ef96 100644
--- a/numpy/_core/src/multiarray/ctors.h
+++ b/numpy/_core/src/multiarray/ctors.h
@@ -51,7 +51,7 @@ PyArray_New(
 NPY_NO_EXPORT PyObject *
 _array_from_array_like(PyObject *op,
         PyArray_Descr *requested_dtype, npy_bool writeable, PyObject *context,
-        int never_copy);
+        int copy);
 
 NPY_NO_EXPORT PyObject *
 PyArray_FromAny_int(PyObject *op, PyArray_Descr *in_descr,
@@ -82,7 +82,7 @@ PyArray_FromInterface(PyObject *input);
 
 NPY_NO_EXPORT PyObject *
 PyArray_FromArrayAttr_int(
-        PyObject *op, PyArray_Descr *descr, int never_copy);
+        PyObject *op, PyArray_Descr *descr, int copy);
 
 NPY_NO_EXPORT PyObject *
 PyArray_FromArrayAttr(PyObject *op, PyArray_Descr *typecode,
diff --git a/numpy/_core/tests/test_multiarray.py b/numpy/_core/tests/test_multiarray.py
index 06087ac44328..ee0c1e91205b 100644
--- a/numpy/_core/tests/test_multiarray.py
+++ b/numpy/_core/tests/test_multiarray.py
@@ -8414,9 +8414,9 @@ def test_buffer_interface(self):
         assert np.may_share_memory(arr, res)
 
     def test_array_interfaces(self):
-        # Array interface gives direct memory access (much like a memoryview)
         base_arr = np.arange(10)
 
+        # Array interface gives direct memory access (much like a memoryview)
         class ArrayLike:
             __array_interface__ = base_arr.__array_interface__
 
@@ -8433,8 +8433,6 @@ def test___array__(self):
 
         class ArrayLike:
             def __array__(self, dtype=None, copy=None):
-                # __array__ should return a copy, numpy cannot know this
-                # however.
                 return base_arr
 
         arr = ArrayLike()
@@ -8465,6 +8463,23 @@ def test___array__copy_arg(self):
         with pytest.raises(ValueError):
             np.shares_memory(a, a.__array__(float, copy=False))
 
+        base_arr = np.arange(10)
+
+        class ArrayLikeNoCopy:
+            def __array__(self, dtype=None):
+                return base_arr
+
+        a = ArrayLikeNoCopy()
+
+        # explicitly passing copy=None shouldn't raise a warning
+        arr = np.array(a, copy=None)
+        assert_array_equal(arr, base_arr)
+        assert arr is base_arr
+
+        with pytest.warns(UserWarning, match=("should implement 'dtype' "
+                                              "and 'copy' keywords")):
+            np.array(a, copy=False)
+
     @pytest.mark.parametrize(
             "arr", [np.ones(()), np.arange(81).reshape((9, 9))])
     @pytest.mark.parametrize("order1", ["C", "F", None])


Code review comments:
diff_hunk: @@ -2440,36 +2447,59 @@ PyArray_FromArrayAttr_int(
         return Py_NotImplemented;
     }
 
-    PyObject *copy = never_copy ? Py_False : Py_None;
     PyObject *kwargs = PyDict_New();
-    PyDict_SetItemString(kwargs, "copy", copy);
+
+    /*
+     * Only if the value of `copy` isn't the default one, we try to pass it
+     * along; for backwards compatibility we then retry if it fails because the
+     * signature of the __array__ method being called does not have `copy`.
+     */
+    int copy_passed = 0;
+    if (copy != -1) {
+        copy_passed = 1;
+        PyObject *copy_obj = copy == 1 ? Py_True : Py_False;
+        PyDict_SetItemString(kwargs, "copy", copy_obj);
+    }
     PyObject *args = descr != NULL ? PyTuple_Pack(1, descr) : PyTuple_New(0);
 
     new = PyObject_Call(array_meth, args, kwargs);
 
-    if (PyErr_Occurred()) {
+    if (new == NULL) {
+        PyObject *errmsg_substr = PyUnicode_FromString(
path: numpy/_core/src/multiarray/ctors.c
text: @user1:
We could make this a static variable if it makes a significant difference in speed.

@user1:
Or maybe `PyUnicode_InternFromString`

@author:
done