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

bpo-46613: Add PyType_GetModuleByDef to the public & limited API #31081

Merged
merged 7 commits into from
Feb 11, 2022

Conversation

encukou
Copy link
Member

@encukou encukou commented Feb 2, 2022

  • Remove the underscore from the name (everywhere)
  • Add to limited API

https://bugs.python.org/issue46613

…ited API

- Remove the underscore from the name (everywhere)
- Add to limited API
Copy link
Member

@shihai1991 shihai1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM.

@shihai1991
Copy link
Member

whatsnew/3.11.rst shoud be updated?

@erlend-aasland
Copy link
Contributor

whatsnew/3.11.rst shoud be updated?

Yes, good catch.

@encukou
Copy link
Member Author

encukou commented Feb 11, 2022

Here it is!
I can't quite come up with a way to summarize it for people who haven't read up on the topic. Hopefully it's enough.

@vstinner
Copy link
Member

Would it make sense to not add it to the limited API in Python 3.11, try to convert a few third party C extensions using it, and if everything goes well, add it to the limited API?

@encukou
Copy link
Member Author

encukou commented Feb 11, 2022

Would it make sense to not add it to the limited API in Python 3.11, try to convert a few third party C extensions using it, and if everything goes well, add it to the limited API?

It would! OTOH, addition to the limited API is also easy to revert, and the function itself already proved useful in stdlib.
Should I remove the stable ABI addition for now?

@erlend-aasland
Copy link
Contributor

I think that makes sense. AFAICS, only a handful of devs (Christian Heimes, Hai Shi, Petr, Victor, me) have been using this function in the CPython code base. In this case, maybe it would make sense to try it out in the wild first. Who could we ask?

@vstinner
Copy link
Member

Should I remove the stable ABI addition for now?

For Py_NewRef/Py_XNewRef, I chose to add it to the stable ABI immediately. For PyType_GetModuleByDef(), I'm more scared because I introduced a bug and noticed it before your recent review. Sadly, only a minority of 3rd party C extensions are using heap types. Moreover, we discovered issues with heap types defined in the stdlib in Python 3.10:

  • Some heap types didn't implement fully the GC protocol: no Py_TPFLAGS_HAVE_GC flag, or no traverse function, or objects not tracked by the GC
  • Types became mutable: add Py_TPFLAGS_HAVE_GC flag
  • tp_new=NULL didn't work anymore: add Py_TPFLAGS_DISALLOW_INSTANTIATION flag

These bugs made me even more careful than I was previously.

Maybe PyType_GetModuleByDef() is fine, but it's just that heap types in general became "a minefield" if you don't pay attention.

I love heap types, I want to use them everywhere. Maybe we need to enhance the documentation, add more warnings in PEP 630, and even implement new runtime checks (in development/debug mode?).

For example, creating a type with Py_TPFLAGS_HAVE_GC but with no traverse function now raises an exception.

@encukou
Copy link
Member Author

encukou commented Feb 11, 2022

Makes sense, but I'm skeptical of actually finding the issues (ones we can't fix in CPython patch release) unless this is in the limited API. Heap types -- and this function -- aren't very useful for existing projects if they aren't switching to the limited API.
Essentially all of the testing will be on that handful of devs anyway.

more warnings in PEP 630

Those are coming up!

:c:func:`PyModule_GetState()` to get module state from slot methods (such as
:c:member:`~PyTypeObject.tp_init` or :c:member:`~PyNumberMethods.nb_add`)
and other places where a method's defining class cannot be passed using the
:c:type:`PyCMethod` calling convention.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may mention somewhere that this function only works on a type created by PyType_FromModuleAndSpec.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works with any method that attaches the module to the class. Currently there's just one, but I've been adding new functions with extra arguments relatively fast. I'd rather not document one specific function.

encukou and others added 2 commits February 11, 2022 16:12
Co-authored-by: Victor Stinner <vstinner@python.org>
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

It's better with the correct return type in the doc :-)

I hope that we will be able to add it to the limited C API soon!

@encukou encukou merged commit 2049469 into python:main Feb 11, 2022
@encukou encukou deleted the pytype_getmodulebydef branch February 11, 2022 16:22
@encukou
Copy link
Member Author

encukou commented Feb 11, 2022

Thank you!

erlend-aasland pushed a commit to erlend-aasland/devguide that referenced this pull request Sep 8, 2023
…n#31081)

* Make PyType_GetModuleByDef public (remove underscore)

Co-authored-by: Victor Stinner <vstinner@python.org>
AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Sep 13, 2023
…n#31081)

* Make PyType_GetModuleByDef public (remove underscore)

Co-authored-by: Victor Stinner <vstinner@python.org>

GitHub-Issue-Link: python/cpython#90771
erlend-aasland pushed a commit to python/devguide that referenced this pull request Sep 26, 2023
…n#31081)

* Make PyType_GetModuleByDef public (remove underscore)

Co-authored-by: Victor Stinner <vstinner@python.org>

GitHub-Issue-Link: python/cpython#90771
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants