GeezerVison

		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

encukou Jun 14, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Yhg1s Jun 15, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou, ericsnowcurrently, and erlend-aasland reacted with thumbs up emoji

All reactions

👍 3 reactions

erlend-aasland reviewed

Jun 14, 2023

View reviewed changes

erlend-aasland left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/c-api/type.rst Outdated

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.

erlend-aasland Jun 13, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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:?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Misc/NEWS.d/next/C API/2023-06-13-14-24-55.gh-issue-105227.HDL9aF.rst Outdated

		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()``

erlend-aasland Jun 13, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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()`

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Yhg1s reviewed

Jun 15, 2023

View reviewed changes

Yhg1s left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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: 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/c-api/type.rst Outdated

		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

Yhg1s Jun 15, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou, ericsnowcurrently, and erlend-aasland reacted with thumbs up emoji

All reactions

👍 3 reactions

tacaswell commented Jun 20, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

~~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.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

philthompson10 commented Jun 20, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hrnciar pushed a commit to fedora-python/cpython that referenced this pull request

Jun 21, 2023


          00402: Add PyType_GetDict()

8c20bb1

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


          00402: Add PyType_GetDict()

f11d97d

This patch should make pyqt6 build with Python 3.12.
For more info see: python#105747

ericsnowcurrently commented Jun 28, 2023

Copy link

Copy Markdown

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. 😄

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

The-Compiler commented Jul 10, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou commented Jul 10, 2023

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou added 6 commits

July 10, 2023 17:14


          Docs: Mark this for general use (better than reading tp_dict directly!)

ac9a071


          Linkify the NEWS entry


          Add a rudimentary test

23b3a41


          Docs: Revert the <<>> in the summary -- the info is in prose for now

7ed5d86


          Merge in the main branch

8d9b9ff


          Formatting nit

97e1d9c

encukou merged commit a840806 into python:main

Jul 10, 2023

miss-islington commented Jul 10, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting core review label

Jul 10, 2023

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jul 10, 2023


          pythongh-105227: Add PyType_GetDict() (pythonGH-105747)

7771a2d

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

(cherry picked from commit a840806)

Co-authored-by: Eric Snow <[email protected]>
Co-authored-by: Petr Viktorin <[email protected]>

bedevere-bot commented Jul 10, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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


          [3.12] gh-105227: Add PyType_GetDict() (GH-105747) (#106600)

41057b2

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 <[email protected]>
Co-authored-by: Petr Viktorin <[email protected]>

The-Compiler commented Jul 11, 2023

Copy link

Copy Markdown

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Yhg1s commented Jul 11, 2023

Copy link

Copy Markdown

Member

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

The-Compiler and erlend-aasland reacted with heart emoji

All reactions

❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reviewed

Jul 11, 2023

View reviewed changes

Comment thread

Doc/c-api/typeobj.rst


		.. versionchanged:: 3.12

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

erlend-aasland Jul 11, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently commented Jul 17, 2023

Copy link

Copy Markdown

Member Author

Thanks for wrapping this up, @encukou!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently deleted the pytype-getdict branch

July 17, 2023 15:44

Reviewers

encukou

encukou left review comments

Yhg1s

Yhg1s left review comments

erlend-aasland

erlend-aasland left review comments

ronaldoussoren

Awaiting requested review from ronaldoussoren

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

ericsnowcurrently commented Jun 13, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently commented Jun 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

encukou commented Jun 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yhg1s left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philthompson10 commented Jun 20, 2023

Uh oh!

ericsnowcurrently commented Jun 28, 2023

Uh oh!

The-Compiler commented Jul 10, 2023

Uh oh!

encukou commented Jul 10, 2023

Uh oh!

miss-islington commented Jul 10, 2023

Uh oh!

bedevere-bot commented Jul 10, 2023

Uh oh!

The-Compiler commented Jul 11, 2023

Uh oh!

Yhg1s commented Jul 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented Jul 17, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ericsnowcurrently commented Jun 13, 2023 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

tacaswell commented Jun 20, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.