ENH: Deprecate writeable broadcast_array (numpy#12609)

When the base is not an array (or generally when the flag of the base
array was toggled), it is OK to allow setting the writeable flag to
True, as long as any ancestor (especially the last one) is writeable.

This commit also slightly change the behaviour of the base attribute. 

---

* ENH: Deprecate writeable broadcast_array

* ENH: Make writeable flag enabling more reliable for non-array bases

When the base is not an array (or generally when the flag of the base
array was toggled), it is OK to allow setting the writeable flag to
True, as long as any ancestor (especially the last one) is writeable.

* Update doc/release/1.17.0-notes.rst

Co-Authored-By: Sebastian Berg <[email protected]>

* Update doc/release/1.17.0-notes.rst

Co-Authored-By: Sebastian Berg <[email protected]>

* Update numpy/lib/tests/test_stride_tricks.py

Co-Authored-By: Sebastian Berg <[email protected]>

* Update numpy/core/tests/test_multiarray.py

Co-Authored-By: Sebastian Berg <[email protected]>

* DOC: improve warning (from review)

doc/release/1.17.0-notes.rst

@@ -50,6 +50,21 @@ always incorrect. If the old behavior was intended, it can be preserved without
 a warning by using ``nonzero(atleast_1d(arr))`` instead of ``nonzero(arr)``.
 In a future release, it is most likely this will raise a `ValueError`.
 
+Writing to the result of `numpy.broadcast_arrays` will warn
+-----------------------------------------------------------
+
+Commonly `numpy.broadcast_arrays` returns a writeable array with internal
+overlap, making it unsafe to write to. A future version will set the
+``writeable`` flag to ``False``, and require users to manually set it to
+``True`` if they are sure that is what they want to do. Now writing to it will
+emit a deprecation warning with instructions to set the ``writeable`` flag
+``True``.  Note that if one were to inspect the flag before setting it, one
+would find it would already be ``True``.  Explicitly setting it, though, as one
+will need to do in future versions, clears an internal flag that is used to
+produce the deprecation warning. To help alleviate confusion, an additional
+`FutureWarning` will be emitted when accessing the ``writeable`` flag state to
+clarify the contradiction.
+
 
 Future Changes
 ==============
@@ -148,6 +163,17 @@ Previously, ``can_cast`` returned `True` for almost all inputs for
 from a structured dtype to a regular one.  This has been fixed, making it
 more consistent with actual casting using, e.g., the ``.astype`` method.
 
+``arr.writeable`` can be switched to true slightly more often
+-------------------------------------------------------------
+
+In rare cases, it was not possible to switch an array from not writeable
+to writeable, although a base array is writeable. This can happen if an
+intermediate ``arr.base`` object is writeable. Previously, only the deepest
+base object was considered for this decision. However, in rare cases this
+object does not have the necessary information. In that case switching to
+writeable was never allowed. This has now been fixed.
+
+
 C API changes
 =============
 

numpy/core/src/multiarray/arrayobject.c

@@ -655,13 +655,11 @@ NPY_NO_EXPORT int
 array_might_be_written(PyArrayObject *obj)
 {
     const char *msg =
-        "Numpy has detected that you (may be) writing to an array returned\n"
-        "by numpy.diagonal. This code will likely break in a future numpy\n"
-        "release -- see numpy.diagonal docs for details. The quick fix is\n"
-        "to make an explicit copy (e.g., do arr.diagonal().copy()).";
+        "Numpy has detected that you (may be) writing to an array with\n"
+        "overlapping memory from np.broadcast_arrays. If this is intentional\n"
+        "set the WRITEABLE flag True or make a copy immediately before writing.";
     if (PyArray_FLAGS(obj) & NPY_ARRAY_WARN_ON_WRITE) {
-        /* 2012-07-17, 1.7 */
-        if (DEPRECATE_FUTUREWARNING(msg) < 0) {
+        if (DEPRECATE(msg) < 0) {
             return -1;
         }
         /* Only warn once per array */

numpy/core/src/multiarray/common.c

@@ -632,8 +632,18 @@ _IsWriteable(PyArrayObject *ap)
         ap = (PyArrayObject *)base;
         base = PyArray_BASE(ap);
 
+        if (PyArray_ISWRITEABLE(ap)) {
+            /*
+             * If any base is writeable, it must be OK to switch, note that
+             * bases are typically collapsed to always point to the most
+             * general one.
+             */
+            return NPY_TRUE;
+        }
+
         if (base == NULL || PyArray_CHKFLAGS(ap, NPY_ARRAY_OWNDATA)) {
-            return (npy_bool) PyArray_ISWRITEABLE(ap);
+            /* there is no further base to test the writeable flag for */
+            return NPY_FALSE;
         }
         assert(!PyArray_CHKFLAGS(ap, NPY_ARRAY_OWNDATA));
     }

numpy/core/src/multiarray/flagsobject.c

@@ -7,6 +7,7 @@
 #define NPY_NO_DEPRECATED_API NPY_API_VERSION
 #define _MULTIARRAYMODULE
 #include "numpy/arrayobject.h"
+#include "arrayobject.h"
 #include "numpy/arrayscalars.h"
 
 #include "npy_config.h"
@@ -201,21 +202,36 @@ arrayflags_dealloc(PyArrayFlagsObject *self)
     static PyObject * \
     arrayflags_ ## lower ## _get(PyArrayFlagsObject *self) \
     { \
-        PyObject *item; \
-        item = ((self->flags & (UPPER)) == (UPPER)) ? Py_True : Py_False; \
-        Py_INCREF(item); \
-        return item; \
+        return PyBool_FromLong((self->flags & (UPPER)) == (UPPER)); \
     }
 
+static char *msg = "future versions will not create a writeable "
+    "array from broadcast_array. Set the writable flag explicitly to "
+    "avoid this warning.";
+
+#define _define_get_warn(UPPER, lower) \
+    static PyObject * \
+    arrayflags_ ## lower ## _get(PyArrayFlagsObject *self) \
+    { \
+        if (self->flags & NPY_ARRAY_WARN_ON_WRITE) { \
+            if (PyErr_Warn(PyExc_FutureWarning, msg) < 0) {\
+                return NULL; \
+            } \
+        }\
+        return PyBool_FromLong((self->flags & (UPPER)) == (UPPER)); \
+    }
+
+
 _define_get(NPY_ARRAY_C_CONTIGUOUS, contiguous)
 _define_get(NPY_ARRAY_F_CONTIGUOUS, fortran)
 _define_get(NPY_ARRAY_WRITEBACKIFCOPY, writebackifcopy)
 _define_get(NPY_ARRAY_OWNDATA, owndata)
 _define_get(NPY_ARRAY_ALIGNED, aligned)
-_define_get(NPY_ARRAY_WRITEABLE, writeable)
-_define_get(NPY_ARRAY_ALIGNED|
+_define_get(NPY_ARRAY_WRITEABLE, writeable_no_warn)
+_define_get_warn(NPY_ARRAY_WRITEABLE, writeable)
+_define_get_warn(NPY_ARRAY_ALIGNED|
             NPY_ARRAY_WRITEABLE, behaved)
-_define_get(NPY_ARRAY_ALIGNED|
+_define_get_warn(NPY_ARRAY_ALIGNED|
             NPY_ARRAY_WRITEABLE|
             NPY_ARRAY_C_CONTIGUOUS, carray)
 
@@ -398,6 +414,40 @@ arrayflags_writeable_set(PyArrayFlagsObject *self, PyObject *obj)
     return 0;
 }
 
+static int
+arrayflags_warn_on_write_set(PyArrayFlagsObject *self, PyObject *obj)
+{
+    /*
+     * This code should go away in a future release, so do not mangle the
+     * array_setflags function with an extra kwarg
+     */
+    int ret;
+    if (obj == NULL) {
+        PyErr_SetString(PyExc_AttributeError,
+                "Cannot delete flags _warn_on_write attribute");
+        return -1;
+    }
+    ret = PyObject_IsTrue(obj);
+    if (ret > 0) {
+        if (!(PyArray_FLAGS((PyArrayObject*)self->arr) & NPY_ARRAY_WRITEABLE)) {
+            PyErr_SetString(PyExc_ValueError,
+                        "cannot set '_warn_on_write' flag when 'writable' is "
+                        "False");
+            return -1;
+        }
+        PyArray_ENABLEFLAGS((PyArrayObject*)self->arr, NPY_ARRAY_WARN_ON_WRITE);
+    }
+    else if (ret < 0) {
+        return -1;
+    }
+    else {
+        PyErr_SetString(PyExc_ValueError,
+                        "cannot clear '_warn_on_write', set "
+                        "writeable True to clear this private flag");
+        return -1;
+    }
+    return 0;
+}
 
 static PyGetSetDef arrayflags_getsets[] = {
     {"contiguous",
@@ -436,6 +486,14 @@ static PyGetSetDef arrayflags_getsets[] = {
         (getter)arrayflags_writeable_get,
         (setter)arrayflags_writeable_set,
         NULL, NULL},
+    {"_writeable_no_warn",
+        (getter)arrayflags_writeable_no_warn_get,
+        (setter)NULL,
+        NULL, NULL},
+    {"_warn_on_write",
+        (getter)NULL,
+        (setter)arrayflags_warn_on_write_set,
+        NULL, NULL},
     {"fnc",
         (getter)arrayflags_fnc_get,
         NULL,
@@ -623,7 +681,7 @@ arrayflags_setitem(PyArrayFlagsObject *self, PyObject *ind, PyObject *item)
              ((n==1) && (strncmp(key, "U", n) == 0))) {
         return arrayflags_updateifcopy_set(self, item);
     }
-    else if (((n==14) && (strncmp(key, "WRITEBACKIFCOPY", n) == 0)) ||
+    else if (((n==15) && (strncmp(key, "WRITEBACKIFCOPY", n) == 0)) ||
              ((n==1) && (strncmp(key, "X", n) == 0))) {
         return arrayflags_writebackifcopy_set(self, item);
     }
@@ -648,19 +706,25 @@ static PyObject *
 arrayflags_print(PyArrayFlagsObject *self)
 {
     int fl = self->flags;
+    const char *_warn_on_write = "";
 
+    if (fl & NPY_ARRAY_WARN_ON_WRITE) {
+        _warn_on_write = "  (with WARN_ON_WRITE=True)";
+    }
     return PyUString_FromFormat(
                         "  %s : %s\n  %s : %s\n"
+                        "  %s : %s\n  %s : %s%s\n"
                         "  %s : %s\n  %s : %s\n"
-                        "  %s : %s\n  %s : %s\n"
-                        "  %s : %s",
+                        "  %s : %s\n",
                         "C_CONTIGUOUS",    _torf_(fl, NPY_ARRAY_C_CONTIGUOUS),
                         "F_CONTIGUOUS",    _torf_(fl, NPY_ARRAY_F_CONTIGUOUS),
                         "OWNDATA",         _torf_(fl, NPY_ARRAY_OWNDATA),
                         "WRITEABLE",       _torf_(fl, NPY_ARRAY_WRITEABLE),
+                        _warn_on_write,
                         "ALIGNED",         _torf_(fl, NPY_ARRAY_ALIGNED),
                         "WRITEBACKIFCOPY", _torf_(fl, NPY_ARRAY_WRITEBACKIFCOPY),
-                        "UPDATEIFCOPY",    _torf_(fl, NPY_ARRAY_UPDATEIFCOPY));
+                        "UPDATEIFCOPY",    _torf_(fl, NPY_ARRAY_UPDATEIFCOPY)
+    );
 }
 
 static int

numpy/core/src/multiarray/methods.c

@@ -6,6 +6,7 @@
 #define NPY_NO_DEPRECATED_API NPY_API_VERSION
 #define _MULTIARRAYMODULE
 #include "numpy/arrayobject.h"
+#include "arrayobject.h"
 #include "numpy/arrayscalars.h"
 
 #include "arrayfunction_override.h"
@@ -2527,6 +2528,7 @@ array_setflags(PyArrayObject *self, PyObject *args, PyObject *kwds)
                     }
                 }
                 PyArray_ENABLEFLAGS(self, NPY_ARRAY_WRITEABLE);
+                PyArray_CLEARFLAGS(self, NPY_ARRAY_WARN_ON_WRITE);
             }
             else {
                 fa->flags = flagback;
@@ -2539,9 +2541,9 @@ array_setflags(PyArrayObject *self, PyObject *args, PyObject *kwds)
         }
         else {
             PyArray_CLEARFLAGS(self, NPY_ARRAY_WRITEABLE);
+            PyArray_CLEARFLAGS(self, NPY_ARRAY_WARN_ON_WRITE);
         }
     }
-
     Py_RETURN_NONE;
 }
 

numpy/core/tests/test_multiarray.py

@@ -110,6 +110,39 @@ def test_writeable(self):
         self.a[0] = 5
         self.a[0] = 0
 
+    def test_writeable_any_base(self):
+        # Ensure that any base being writeable is sufficient to change flag;
+        # this is especially interesting for arrays from an array interface.
+        arr = np.arange(10)
+
+        class subclass(np.ndarray):
+            pass
+
+        # Create subclass so base will not be collapsed, this is OK to change
+        view1 = arr.view(subclass)
+        view2 = view1[...]
+        arr.flags.writeable = False
+        view2.flags.writeable = False
+        view2.flags.writeable = True  # Can be set to True again.
+
+        arr = np.arange(10)
+
+        class frominterface:
+            def __init__(self, arr):
+                self.arr = arr
+                self.__array_interface__ = arr.__array_interface__
+
+        view1 = np.asarray(frominterface)
+        view2 = view1[...]
+        view2.flags.writeable = False
+        view2.flags.writeable = True
+
+        view1.flags.writeable = False
+        view2.flags.writeable = False
+        with assert_raises(ValueError):
+            # Must assume not writeable, since only base is not:
+            view2.flags.writeable = True
+
     def test_writeable_from_readonly(self):
         # gh-9440 - make sure fromstring, from buffer on readonly buffers
         # set writeable False
@@ -191,6 +224,15 @@ def test_writeable_from_c_data(self):
             with assert_warns(DeprecationWarning):
                 arr.flags.writeable = True
 
+    def test_warnonwrite(self):
+        a = np.arange(10)
+        a.flags._warn_on_write = True
+        with warnings.catch_warnings(record=True) as w:
+            warnings.filterwarnings('always')
+            a[1] = 10
+            a[2] = 10
+            # only warn once
+            assert_(len(w) == 1)
 
     def test_otherflags(self):
         assert_equal(self.a.flags.carray, True)
@@ -2961,8 +3003,6 @@ def test_diagonal(self):
         assert_equal(b.diagonal(0, 2, 1), [[0, 3], [4, 7]])
 
     def test_diagonal_view_notwriteable(self):
-        # this test is only for 1.9, the diagonal view will be
-        # writeable in 1.10.
         a = np.eye(3).diagonal()
         assert_(not a.flags.writeable)
         assert_(not a.flags.owndata)

numpy/core/tests/test_ufunc.py

@@ -564,6 +564,18 @@ def test_broadcast(self):
         b = np.arange(3).reshape((3, 1, 1))
         assert_raises(ValueError, umt.inner1d, a, b)
 
+        # Writing to a broadcasted array with overlap should warn, gh-2705
+        a = np.arange(2)
+        b = np.arange(4).reshape((2, 2))
+        u, v = np.broadcast_arrays(a, b)
+        assert_equal(u.strides[0], 0)
+        x = u + v
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter("always")
+            u += v
+            assert_equal(len(w), 1)
+            assert_(x[0,0]  != u[0, 0])
+
     def test_type_cast(self):
         msg = "type cast"
         a = np.arange(6, dtype='short').reshape((2, 3))

numpy/lib/stride_tricks.py

@@ -121,18 +121,18 @@ def _broadcast_to(array, shape, subok, readonly):
     if any(size < 0 for size in shape):
         raise ValueError('all elements of broadcast shape must be non-'
                          'negative')
-    needs_writeable = not readonly and array.flags.writeable
-    extras = ['reduce_ok'] if needs_writeable else []
-    op_flag = 'readwrite' if needs_writeable else 'readonly'
+    extras = []
     it = np.nditer(
         (array,), flags=['multi_index', 'refs_ok', 'zerosize_ok'] + extras,
-        op_flags=[op_flag], itershape=shape, order='C')
+        op_flags=['readonly'], itershape=shape, order='C')
     with it:
         # never really has writebackifcopy semantics
         broadcast = it.itviews[0]
     result = _maybe_view_as_subclass(array, broadcast)
-    if needs_writeable and not result.flags.writeable:
+    # In a future version this will go away
+    if not readonly and array.flags._writeable_no_warn:
         result.flags.writeable = True
+        result.flags._warn_on_write = True
     return result
 
 
@@ -222,8 +222,15 @@ def broadcast_arrays(*args, **kwargs):
     broadcasted : list of arrays
         These arrays are views on the original arrays.  They are typically
         not contiguous.  Furthermore, more than one element of a
-        broadcasted array may refer to a single memory location.  If you
-        need to write to the arrays, make copies first.
+        broadcasted array may refer to a single memory location. If you need
+        to write to the arrays, make copies first. While you can set the
+        ``writable`` flag True, writing to a single output value may end up
+        changing more than one location in the output array.
+
+        .. deprecated:: 1.17
+            The output is currently marked so that if written to, a deprecation
+            warning will be emitted. A future version will set the
+            ``writable`` flag False so writing to it will raise an error.
 
     Examples
     --------
@@ -260,7 +267,5 @@ def broadcast_arrays(*args, **kwargs):
         # Common case where nothing needs to be broadcasted.
         return args
 
-    # TODO: consider making the results of broadcast_arrays readonly to match
-    # broadcast_to. This will require a deprecation cycle.
     return [_broadcast_to(array, shape, subok=subok, readonly=False)
             for array in args]