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

C API: Add replacements for PyObject_HasAttr() etc #108511

Closed
serhiy-storchaka opened this issue Aug 26, 2023 · 4 comments
Closed

C API: Add replacements for PyObject_HasAttr() etc #108511

serhiy-storchaka opened this issue Aug 26, 2023 · 4 comments
Labels
topic-C-API type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 26, 2023

Feature or enhancement

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

#75753
#106672

Proposal:

Functions PyDict_GetItem(), PyDict_GetItemString(), PyMapping_HasKey(), PyMapping_HasKeyString(), PyObject_HasAttr(), PyObject_HasAttrString() and PySys_GetObject() have a flaw -- they clear any error raised inside the function, including important and critical errors. They cannot be fixed, because the user code which use them do not handle errors. There are replacements free from this flaw for PyDict_GetItem() (PyDict_GetItemWithError() and PyDict_GetItemRef()) and, in some applications, to PyDict_GetItemString() (PyDict_GetItemRefString()).

We need new functions similar to PyMapping_HasKey(), PyMapping_HasKeyString(), PyObject_HasAttr(), PyObject_HasAttrString() which return three-state value (1 - yes, 0 -- no, and -1 --error). What should be their names? Add the WithError suffix? Add the Ex sufix? Add the 2 suffix?

Linked PRs

@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement topic-C-API labels Aug 26, 2023
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Sep 6, 2023
Add the following functions:

* PyObject_HasAttrWithError()
* PyObject_HasAttrStringWithError()
* PyMapping_HasKeyWithError()
* PyMapping_HasKeyStringWithError()
@gvanrossum
Copy link
Member

In the PR you propose new names of the form PyMapping_HasKeyWithError (etc.). That's a rather long name. Does this follow a convention we use elsewhere in the C API? (I understand that the "ideal" name is already taken by a similar function that has a design flaw.)

@serhiy-storchaka
Copy link
Member Author

There is a single precedence: the PyDict_GetItem()/PyDict_GetItemWithError() pair. Adding PyObject_HasAttrWithError() and PyMapping_HasKeyWithError() will make it an established convention.

I asked about names in https://discuss.python.org/t/replacements-for-pymapping-haskey-pymapping-haskeystring-pyobject-hasattr-and-pyobject-hasattrstring/32589, and WithError looks like the least bad suffix.

We can also use PyMapping_Contains(), similar to the existing PyDict_Contains(). But I do not know alternative name to PyObject_HasAttrWithError().

Other approach was used for the PyArg_Parse*() functions. The original functions had a flaw (the worked with size as int instead of Py_ssize_t), so new functions like _PyArg_ParseTuple_SizeT() was introduced, and then defines conditionally redefined PyArg_ParseTuple to _PyArg_ParseTuple_SizeT. I don't like how it ended -- eventually ABI compatibility was broken. In 3.12 PyArg_ParseTuple raises an exception, and in 3.13 it is the same as _PyArg_ParseTuple_SizeT (crashing or working incorrectly if the caller code was not fixed). So if we take this approach, we can get a user code which skipped Python versions and was not changed to handle errors returned by new PyObject_HasAttr(). It will just take more time.

@gvanrossum
Copy link
Member

Okay, I think just introducing the new names now seems the right thing, and it looks like WithError is the best suffix we can think of. I don't like the macro juggling approach (though in the past I might have) so let's just live with the new and old API together, and encourage users by only peaceful means to update their code.

@vstinner
Copy link
Member

vstinner commented Sep 15, 2023

I didn't like when define PY_SSIZE_T_CLEAN was added. Most C extensions had to add it. Not all of them. It's hard to know if a C extension was modernized or not.

I didn't like when it was removed: is it ok to remove if you keep Python 3.6 support? Is it going to break stuffs?

IMO this dark macro magic is hard to understand as an user.

serhiy-storchaka added a commit that referenced this issue Sep 17, 2023
…H-109025)

Add the following functions:

* PyObject_HasAttrWithError()
* PyObject_HasAttrStringWithError()
* PyMapping_HasKeyWithError()
* PyMapping_HasKeyStringWithError()
csm10495 pushed a commit to csm10495/cpython that referenced this issue Sep 28, 2023
…ors (pythonGH-109025)

Add the following functions:

* PyObject_HasAttrWithError()
* PyObject_HasAttrStringWithError()
* PyMapping_HasKeyWithError()
* PyMapping_HasKeyStringWithError()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants