From e6956af1fdf10c42934f3b35d453ca1a0cdb211a Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Sun, 9 Jul 2023 17:40:11 +0200 Subject: [PATCH] gh-106572: Deprecate PyObject_SetAttr(v, name, NULL) If the value is NULL, PyObject_SetAttr() and PyObject_SetAttrString() emit a DeprecationWarning in Python Development Mode or if Python is built in debug mode. weakref proxy_setattr() calls PyObject_DelAttr() if value is NULL. --- Doc/c-api/object.rst | 19 +++++-- Doc/whatsnew/3.13.rst | 11 ++++ Lib/test/test_capi/test_misc.py | 18 +++++++ ...-07-10-13-39-56.gh-issue-106572.3ZQjCa.rst | 9 ++++ Modules/_testcapimodule.c | 15 ++++++ Objects/object.c | 51 ++++++++++++++++--- Objects/weakrefobject.c | 8 ++- 7 files changed, 118 insertions(+), 13 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2023-07-10-13-39-56.gh-issue-106572.3ZQjCa.rst diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index 6fc5b2d14dd327..d642e70b717ff3 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -122,9 +122,13 @@ Object Protocol return ``0`` on success. This is the equivalent of the Python statement ``o.attr_name = v``. - If *v* is ``NULL``, the attribute is deleted. This behaviour is deprecated - in favour of using :c:func:`PyObject_DelAttr`, but there are currently no - plans to remove it. + If *v* is ``NULL``, emit a :exc:`DeprecationWarning` in :ref:`Python + Development Mode ` or if :ref:`Python is built in debug mode + `. + + .. deprecated:: 3.13 + Calling ``PyObject_SetAttrString(o, attr_name, NULL)`` is deprecated: + ``PyObject_DelAttr(o, attr_name)`` must be used instead. .. c:function:: int PyObject_SetAttrString(PyObject *o, const char *attr_name, PyObject *v) @@ -134,8 +138,13 @@ Object Protocol return ``0`` on success. This is the equivalent of the Python statement ``o.attr_name = v``. - If *v* is ``NULL``, the attribute is deleted, but this feature is - deprecated in favour of using :c:func:`PyObject_DelAttrString`. + If *v* is ``NULL``, emit a :exc:`DeprecationWarning` in :ref:`Python + Development Mode ` or if :ref:`Python is built in debug mode + `. + + .. deprecated:: 3.13 + Calling ``PyObject_SetAttrString(o, attr_name, NULL)`` is deprecated: + ``PyObject_DelAttrString(o, attr_name)`` must be used instead. .. c:function:: int PyObject_GenericSetAttr(PyObject *o, PyObject *name, PyObject *value) diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index dc020682ac1edd..f3f4145086ee06 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -795,6 +795,17 @@ Deprecated :c:func:`PyWeakref_GetRef` on Python 3.12 and older. (Contributed by Victor Stinner in :gh:`105927`.) +* Deprecate :c:func:`PyObject_SetAttr` and :c:func:`PyObject_SetAttrString` to + remove an attribute (if *value* is ``NULL``): ``PyObject_DelAttr(v, name)`` + and ``PyObject_DelAttrString(v, name)`` must be used instead. If the + development mode is enabled or if Python is built in debug mode, these + deprecated functions now emit a :exc:`DeprecationWarning` if *value* is + ``NULL``. When :c:func:`PyObject_SetAttr` is called with a ``NULL`` value, + it's unclear if the caller wants to remove the attribute on purpose, or if + the value is ``NULL`` because of an error. Such API is error prone and should + be avoided. + (Contributed by Victor Stinner in :gh:`106572`.) + Removed ------- diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 9cbd5506bf6dea..c37c96a74af305 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -1115,6 +1115,24 @@ class Data(_testcapi.ObjExtraData): del d.extra self.assertIsNone(d.extra) + def test_setattr(self): + class MyType: + pass + + PyObject_SetAttr = _testcapi.PyObject_SetAttr + + obj = MyType() + PyObject_SetAttr(obj, "attr", 123) + self.assertEqual(obj.attr, 123) + # PyObject_SetAttr(obj, name, NULL) emits a DeprecationWarning + # in the Python Development Mode or if Python is built in debug mode + if support.Py_DEBUG or sys.flags.dev_mode: + with self.assertWarns(DeprecationWarning): + PyObject_SetAttr(obj, "attr") + else: + PyObject_SetAttr(obj, "attr") + self.assertFalse(hasattr(obj, "attr")) + @requires_limited_api class TestHeapTypeRelative(unittest.TestCase): diff --git a/Misc/NEWS.d/next/C API/2023-07-10-13-39-56.gh-issue-106572.3ZQjCa.rst b/Misc/NEWS.d/next/C API/2023-07-10-13-39-56.gh-issue-106572.3ZQjCa.rst new file mode 100644 index 00000000000000..6c865bc58a61a5 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2023-07-10-13-39-56.gh-issue-106572.3ZQjCa.rst @@ -0,0 +1,9 @@ +Deprecate :c:func:`PyObject_SetAttr` and :c:func:`PyObject_SetAttrString` to +remove an attribute (if *value* is ``NULL``): ``PyObject_DelAttr(v, name)`` and +``PyObject_DelAttrString(v, name)`` must be used instead. If the development +mode is enabled or if Python is built in debug mode, these deprecated functions +now emit a :exc:`DeprecationWarning` if *value* is ``NULL``. When +:c:func:`PyObject_SetAttr` is called with a ``NULL`` value, it's unclear if the +caller wants to remove the attribute on purpose, or if the value is ``NULL`` +because of an error. Such API is error prone and should be avoided. +Patch by Victor Stinner. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index dd2c9c72e53787..1a9d35a58be549 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3464,6 +3464,20 @@ test_weakref_capi(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args)) } +static PyObject * +test_pyobject_setattr(PyObject *Py_UNUSED(module), PyObject *args) +{ + PyObject *obj, *name, *value = NULL; + if (!PyArg_ParseTuple(args, "OO|O", &obj, &name, &value)) { + return NULL; + } + if (PyObject_SetAttr(obj, name, value) < 0) { + return NULL; + } + Py_RETURN_NONE; +} + + static PyMethodDef TestMethods[] = { {"set_errno", set_errno, METH_VARARGS}, {"test_config", test_config, METH_NOARGS}, @@ -3609,6 +3623,7 @@ static PyMethodDef TestMethods[] = { {"function_set_kw_defaults", function_set_kw_defaults, METH_VARARGS, NULL}, {"check_pyimport_addmodule", check_pyimport_addmodule, METH_VARARGS}, {"test_weakref_capi", test_weakref_capi, METH_NOARGS}, + {"PyObject_SetAttr", test_pyobject_setattr, METH_VARARGS}, {NULL, NULL} /* sentinel */ }; diff --git a/Objects/object.c b/Objects/object.c index d30e048335ab63..cf8c0a789c2558 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -926,8 +926,11 @@ PyObject_HasAttrString(PyObject *v, const char *name) return ok; } -int -PyObject_SetAttrString(PyObject *v, const char *name, PyObject *w) +// Forward declaration +static int object_set_attr(PyObject *v, PyObject *name, PyObject *value); + +static int +object_set_attr_string(PyObject *v, const char *name, PyObject *w) { PyObject *s; int res; @@ -937,15 +940,32 @@ PyObject_SetAttrString(PyObject *v, const char *name, PyObject *w) s = PyUnicode_InternFromString(name); if (s == NULL) return -1; - res = PyObject_SetAttr(v, s, w); + res = object_set_attr(v, s, w); Py_XDECREF(s); return res; } +int +PyObject_SetAttrString(PyObject *v, const char *name, PyObject *w) +{ + if (w == NULL +#ifndef Py_DEBUG + && _Py_GetConfig()->dev_mode +#endif + && PyErr_WarnEx(PyExc_DeprecationWarning, + "PyObject_SetAttrString(v, name, NULL) is deprecated: " + "use PyObject_DelAttrString(v, name) instead", + 1) < 0) + { + return -1; + } + return object_set_attr_string(v, name, w); +} + int PyObject_DelAttrString(PyObject *v, const char *name) { - return PyObject_SetAttrString(v, name, NULL); + return object_set_attr_string(v, name, NULL); } int @@ -1156,8 +1176,8 @@ PyObject_HasAttr(PyObject *v, PyObject *name) return 1; } -int -PyObject_SetAttr(PyObject *v, PyObject *name, PyObject *value) +static int +object_set_attr(PyObject *v, PyObject *name, PyObject *value) { PyTypeObject *tp = Py_TYPE(v); int err; @@ -1205,10 +1225,27 @@ PyObject_SetAttr(PyObject *v, PyObject *name, PyObject *value) return -1; } +int +PyObject_SetAttr(PyObject *v, PyObject *name, PyObject *value) +{ + if (value == NULL +#ifndef Py_DEBUG + && _Py_GetConfig()->dev_mode +#endif + && PyErr_WarnEx(PyExc_DeprecationWarning, + "PyObject_SetAttr(v, name, NULL) is deprecated: " + "use PyObject_DelAttr(v, name) instead", + 1) < 0) + { + return -1; + } + return object_set_attr(v, name, value); +} + int PyObject_DelAttr(PyObject *v, PyObject *name) { - return PyObject_SetAttr(v, name, NULL); + return object_set_attr(v, name, NULL); } PyObject ** diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index e9563729bf82ba..913564ed9379f6 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -485,7 +485,13 @@ proxy_setattr(PyObject *proxy, PyObject *name, PyObject *value) if (!proxy_check_ref(obj)) { return -1; } - int res = PyObject_SetAttr(obj, name, value); + int res; + if (value != NULL) { + res = PyObject_SetAttr(obj, name, value); + } + else { + res = PyObject_DelAttr(obj, name); + } Py_DECREF(obj); return res; }