Skip to content

gh-105227: Add PyType_GetDict() #105747

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

Merged
merged 10 commits into from
Jul 10, 2023
Merged

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Jun 13, 2023

This compensates for static builtin types having tp_dict set to NULL.


📚 Documentation preview 📚: https://cpython-previews--105747.org.readthedocs.build/

@ericsnowcurrently
Copy link
Member Author

@ericsnowcurrently ericsnowcurrently removed the request for review from markshannon June 13, 2023 20:57
Comment on lines 160 to 161
**<<>>**: Names in double angle brackets should be initially set to
``NULL`` and treated as read-only after initialization.
Copy link
Member

Choose a reason for hiding this comment

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

Only marking up tp_dict implies it's the only one in this category. At least tp_bases & tp_mro should be marked too.

@encukou
Copy link
Member

encukou commented Jun 14, 2023

@Yhg1s needs to approve backporting to 3.12

replacement for accessing :c:member:`~PyTypeObject.tp_dict` directly.
The returned dictionary must be treated as read-only.

This function isn't intended for general use. It's meant for
Copy link
Member

Choose a reason for hiding this comment

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

If it's not meant for general use, would it be better to add it as unstable API?

Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I think it makes more sense to say this is for general use.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Can you update Doc/data/refcounts.dat, so we get a "Return value: New reference" markup in the docs?

Comment on lines 60 to 64
This function isn't intended for general use. It's meant for
specific embedding and language-binding cases, where direct access
to the dict is necessary and indirect access (e.g. via the proxy)
isn't adequate. Extension modules may continue to use ``tp_dict``,
directly or indirectly, when setting up their own types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe elevate this paragraph to a .. note:, or maybe a .. warning:?

The new :c:func:`PyType_GetDict` provides the dictionary for the given type
object that is normally exposed by ``cls.__dict__``. Normally it's
sufficient to use :c:member:`~PyTypeObject.tp_dict`, but for the static
builtin types ``tp_dict`` is now always ``NULL``. ``PyType_GetDict()``
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting nit:

Suggested change
builtin types ``tp_dict`` is now always ``NULL``. ``PyType_GetDict()``
builtin types :c:member:`!tp_dict` is now always ``NULL``. :c:func:`!PyType_GetDict()`

Copy link
Member

@Yhg1s Yhg1s left a comment

Choose a reason for hiding this comment

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

Approving the new API as RM: 👍

replacement for accessing :c:member:`~PyTypeObject.tp_dict` directly.
The returned dictionary must be treated as read-only.

This function isn't intended for general use. It's meant for
Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I think it makes more sense to say this is for general use.

@tacaswell
Copy link
Contributor

tacaswell commented Jun 20, 2023

The problem has also been fixed on the PyQt6 side without this PR (I must plead ignorance of exactly what those changes are), so I am not sure how critical this PR is.

Sorry, I last track of which branch I had built CPython from 🐑 .

What I meant to say is:

I can confirm that the latest version of pyqt6 works with this branch so this change is critical.

@philthompson10
Copy link

PyQt now uses this call. Strictly speaking it could do the C equivalent of 'cls.dict' but the call is much nicer.

hrnciar pushed a commit to fedora-python/cpython that referenced this pull request Jun 21, 2023
This should make pyqt6 build with Python 3.12.
For more infor see: python#105747
hrnciar pushed a commit to fedora-python/cpython that referenced this pull request Jun 21, 2023
This patch should make pyqt6 build with Python 3.12.
For more info see: python#105747
@ericsnowcurrently
Copy link
Member Author

I'm not available enough to merge this for a few weeks, so anyone that feels comfortable doing so is welcome to in the meantime. 😄

@The-Compiler
Copy link
Contributor

Any updates on this? This currently blocks me (with qutebrowser, via PyQt) from testing Python 3.12 on CI.

@encukou
Copy link
Member

encukou commented Jul 10, 2023

Oh dear, I didn't realis a few weeks is all the time until Beta. And I was busy so it didn't reach the top of my TODO list.
I'll push some tests and a doc update.

@encukou encukou merged commit a840806 into python:main Jul 10, 2023
@miss-islington
Copy link
Contributor

Thanks @ericsnowcurrently for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 10, 2023
This compensates for static builtin types having `tp_dict` set to `NULL`.

(cherry picked from commit a840806)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-bot
Copy link

GH-106600 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jul 10, 2023
encukou added a commit that referenced this pull request Jul 10, 2023
gh-105227: Add PyType_GetDict() (GH-105747)

This compensates for static builtin types having `tp_dict` set to `NULL`.

(cherry picked from commit a840806)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@The-Compiler
Copy link
Contributor

Oh whoops, I wasn't even realizing how close to the Beta 4 I was commenting (apparently it was scheduled for yesterday, but not released yet?)!

Glad to hear this still made it in, thanks so much @ericsnowcurrently and @encukou!

@Yhg1s
Copy link
Member

Yhg1s commented Jul 11, 2023

FWIW, this PR was one of the reasons 3.12b4 wasn't cut yesterday :)


.. versionchanged:: 3.12

Internals detail: For static builtin types, this is always ``NULL``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the post merge review, but FYI, there's a directive for implementation details: .. impl-detail::

@ericsnowcurrently
Copy link
Member Author

Thanks for wrapping this up, @encukou!

@ericsnowcurrently ericsnowcurrently deleted the pytype-getdict branch July 17, 2023 15:44
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.

9 participants