Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate PyObject_SetAttr(obj, name, NULL): PyObject_DelAttr(obj, name) must be used instead #106572

Closed
vstinner opened this issue Jul 9, 2023 · 6 comments

Comments

@vstinner
Copy link
Member

vstinner commented Jul 9, 2023

When 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.

In 2005, this feature was already deprecated in the documentation in issue #69887 by commit 45be8d6.

I propose to now emit a DeprecationWarning and schedule the removal of this feature in Python 3.15. If too many C extensions rely on this current behavior, we can consider removing the feature later.

Linked PRs

@vstinner
Copy link
Member Author

vstinner commented Jul 9, 2023

@vstinner
Copy link
Member Author

vstinner commented Jul 9, 2023

I wrote PR #105373 to remove the deprecation in the documentation, but @serhiy-storchaka wrote that the deprecation should stay: #105374 (comment)

Copy of his message:


The Gmane link no longer work, the working links to the discussion (it was split on several threads) are
https://mail.python.org/archives/list/python-dev@python.org/thread/OSKS23WDHU67NQCANPNSAPHATZ2J3W7T/
https://mail.python.org/archives/list/python-dev@python.org/thread/UJI3KJQVOY7VELTDJOBTBWM3TYDHEYGD/

The problems with the current behavior:

  1. It looks confusing that some of *Set* functions do removing, while other raise SystemError or crash. Ideally, all such functions should either support deletion by specifying NULL, or raise SystemError. A crash is acceptable too, for performance.
  2. It is a bug magnet. It is easy to get the NULL pointer instead of a pointer to object (just forgot to check for error) and delete instead of set.

Changing the current behavior is a long process. M.-A. Lemburg specified the required procedure. It is interesting, that if we started it in 3.6, it would already be finished.

@vstinner
Copy link
Member Author

vstinner commented Jul 9, 2023

I reported this error prone API as capi-workgroup/problems#47

@vstinner
Copy link
Member Author

vstinner commented Jul 9, 2023

We should consider deprecating similar APIs used to delete an item/attribute:

  • PyObject_SetItem(obj, key, NULL): use PyObject_DelItem(obj, key) instead
  • PyMapping_SetItemString(obj, key, NULL): use PyMapping_DelItemString(obj, key) instead
  • PySequence_SetItem(obj, index, NULL): use PySequence_DelItem(obj, index) instead
  • PySequence_SetSlice(obj, i1, i2, NULL): use PySequence_DelSlice(obj, i1, i2) instead

vstinner added a commit to vstinner/cpython that referenced this issue Jul 10, 2023
* Convert PyObject_DelAttr() and PyObject_DelAttrString() macros to
  functions.
* Add PyObject_DelAttr() and PyObject_DelAttrString() functions to
  the stable ABI.
* Replace PyObject_SetAttr(obj, name, NULL) with
  PyObject_DelAttr(obj, name).
* weakref proxy_setattr() calls PyObject_DelAttr() if value is NULL.
vstinner added a commit to vstinner/cpython that referenced this issue Jul 10, 2023
* Convert PyObject_DelAttr() and PyObject_DelAttrString() macros to
  functions.
* Add PyObject_DelAttr() and PyObject_DelAttrString() functions to
  the stable ABI.
* Replace PyObject_SetAttr(obj, name, NULL) with
  PyObject_DelAttr(obj, name).
* weakref proxy_setattr() calls PyObject_DelAttr() if value is NULL.
vstinner added a commit to vstinner/cpython that referenced this issue Jul 10, 2023
* Convert PyObject_DelAttr() and PyObject_DelAttrString() macros to
  functions.
* Add PyObject_DelAttr() and PyObject_DelAttrString() functions to
  the stable ABI.
* Replace PyObject_SetAttr(obj, name, NULL) with
  PyObject_DelAttr(obj, name).
* weakref proxy_setattr() calls PyObject_DelAttr() if value is NULL.
@serhiy-storchaka
Copy link
Member

Unfortunately, adding a warning will break binary compatibility. Code compiled with older Python version which uses PyObject_DelAttr() calls PyObject_SetAttr(), because PyObject_DelAttr() was implemented as a macro.

We should first implement PyObject_DelAttr() as a separate function, then in some distant future we decide to break binary compatibility with Python older than 3.13 and add a warning, then error, in PyObject_SetAttr().

vstinner added a commit to vstinner/cpython that referenced this issue Jul 10, 2023
* Convert PyObject_DelAttr() and PyObject_DelAttrString() macros to
  functions.
* Add PyObject_DelAttr() and PyObject_DelAttrString() functions to
  the stable ABI.
* Replace PyObject_SetAttr(obj, name, NULL) with
  PyObject_DelAttr(obj, name).
vstinner added a commit to vstinner/cpython that referenced this issue Jul 10, 2023
* Convert PyObject_DelAttr() and PyObject_DelAttrString() macros to
  functions.
* Add PyObject_DelAttr() and PyObject_DelAttrString() functions to
  the stable ABI.
* Replace PyObject_SetAttr(obj, name, NULL) with
  PyObject_DelAttr(obj, name).
vstinner added a commit to vstinner/cpython that referenced this issue Jul 10, 2023
* Convert PyObject_DelAttr() and PyObject_DelAttrString() macros to
  functions.
* Add PyObject_DelAttr() and PyObject_DelAttrString() functions to
  the stable ABI.
* Replace PyObject_SetAttr(obj, name, NULL) with
  PyObject_DelAttr(obj, name).
vstinner added a commit that referenced this issue Jul 11, 2023
* Convert PyObject_DelAttr() and PyObject_DelAttrString() macros to
  functions.
* Add PyObject_DelAttr() and PyObject_DelAttrString() functions to
  the stable ABI.
* Replace PyObject_SetAttr(obj, name, NULL) with
  PyObject_DelAttr(obj, name).
vstinner added a commit to vstinner/cpython that referenced this issue Jul 11, 2023
* Convert PyObject_DelAttr() and PyObject_DelAttrString() macros to
  functions.
* Add PyObject_DelAttr() and PyObject_DelAttrString() functions to
  the stable ABI.
* Replace PyObject_SetAttr(obj, name, NULL) with
  PyObject_DelAttr(obj, name).
* weakref proxy_setattr() calls PyObject_DelAttr() if value is NULL.
vstinner added a commit to vstinner/cpython that referenced this issue Jul 11, 2023
If the alue 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.
vstinner added a commit to vstinner/cpython that referenced this issue Jul 11, 2023
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.
@vstinner
Copy link
Member Author

I close the issue: see #106573 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants