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

Thanks, LGTM.

@shihai1991
Copy link
Member

@shihai1991 shihai1991 commented Feb 9, 2022

whatsnew/3.11.rst shoud be updated?

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Feb 9, 2022

whatsnew/3.11.rst shoud be updated?

Yes, good catch.

@encukou
Copy link
Member Author

@encukou 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

@vstinner vstinner 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?

@encukou
Copy link
Member Author

@encukou 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

@erlend-aasland erlend-aasland commented Feb 11, 2022

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

@vstinner vstinner commented Feb 11, 2022

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 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!

Doc/c-api/type.rst Outdated Show resolved Hide resolved
Doc/c-api/type.rst Outdated Show resolved Hide resolved
Doc/c-api/type.rst Outdated Show resolved Hide resolved
: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

@vstinner vstinner Feb 11, 2022

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

@encukou encukou Feb 11, 2022

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 Feb 11, 2022
Co-authored-by: Victor Stinner <vstinner@python.org>
Copy link
Member

@vstinner vstinner left a comment

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
13 checks passed
@encukou encukou deleted the pytype_getmodulebydef branch Feb 11, 2022
@encukou
Copy link
Member Author

@encukou encukou commented Feb 11, 2022

Thank you!

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.

None yet

6 participants