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

gh-106320: Remove private _PyType_Lookup() function #108600

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 29, 2023

Move the private function to the internal C API (pycore_object.h).

Move the private function to the internal C API (pycore_object.h).
@vstinner
Copy link
Member Author

@serhiy-storchaka: I vaguely recall that you considered to expose a similar API to the public C API. Is it still the case? Or what is the PyObject_GetOptionalAttr() API that you added to Python 3.13?

@vstinner
Copy link
Member Author

@serhiy-storchaka
Copy link
Member

No, I did not work with _PyType_Lookup(). It seems to me, that _PyType_Lookup() to PyObject_GetAttr() as PyDict_GetItem() to PyObject_GetItem() -- convenient, but flawed by design. And it is not use the descriptor machinery. It is used in many essential parts of the code, and I think that there is a large chance that it is used in third-party projects like Cython. It is difficult if possible to reimplement it with a public API.

Perhaps we should first add a public and better designed variants of _PyType_Lookup() and _PyObject_LookupSpecial() before making them less accessible to third-party developers.

@vstinner
Copy link
Member Author

think that there is a large chance that it is used in third-party projects like Cython

Results of a code search on PyPI top 5,000 projects (2023-07-04).

Affected projects (20):

  • Cython (0.29.36)
  • GDAL (3.7.0)
  • M2Crypto (0.39.0)
  • catboost (1.2)
  • cvxpy (1.3.2)
  • dlib (19.24.2)
  • gevent (22.10.2)
  • mecab-python3 (1.0.6)
  • onnx (1.14.0)
  • onnxoptimizer (0.3.13)
  • onnxsim (0.4.33)
  • osmium (3.6.0)
  • praat-parselmouth (0.4.3)
  • pybind11 (2.10.4)
  • pygraphviz (1.11)
  • python-crfsuite (0.9.9)
  • pythonnet (3.0.1)
  • scipy (1.11.1)
  • sentencepiece (0.1.99)
  • yappi (1.4.0)

@vstinner
Copy link
Member Author

Perhaps we should first add a public

If a private function is widely used and it makes sense to expose it to the public C API, sure, go ahead.

and better designed variants of _PyType_Lookup() and _PyObject_LookupSpecial() before making them less accessible to third-party developers.

Which aspects of the API you would like to change in _PyType_Lookup()?

@serhiy-storchaka
Copy link
Member

Which aspects of the API you would like to change in _PyType_Lookup()?

First of all, it contains PyErr_Clear(). We should get rid of such functions.

If it start returning an error, there are variants of the interface: return a pointer with the following calling of PyErr_Occurred() if it is NULL, or return -1/0/1 and save the reference by provided pointer like in PyObject_GetOptionalAttr(), or return a pointer and set the error flag by provided pointer like in find_name_in_mro() and some other private function. It can also return in what type the name was found if there is a need.

Then you may want to return a new reference, although I am not sure that there is a problem with a borrowing reference here. But we should investigate.

It may be turn out that _PyType_Lookup() is too low level, and that all use cases can be covered by few higher level wrappers. Then we can even remove it from internal API.

@vstinner
Copy link
Member Author

vstinner commented Sep 4, 2023

I close my issue. I prefer to keep _PyType_Lookup() for now.

Well, apparently, it's still interesting to consider adding a public flavor of this API.

@vstinner vstinner closed this Sep 4, 2023
@vstinner vstinner deleted the remove_pytype_lookup branch September 4, 2023 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants