FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-121654: Add PyType_Freeze() function by vstinner · Pull Request #122457 · python/cpython · GitHub

/ cpython Public

gh-121654: Add PyType_Freeze() function - #122457

Merged
vstinner merged 11 commits into
python:mainfrom
vstinner:type_freeze
Oct 25, 2024
ConversationCommits (11)ChecksFiles changed
Merged

gh-121654: Add PyType_Freeze() function#122457
vstinner merged 11 commits into
python:mainfrom
vstinner:type_freeze

Conversation

vstinner commented Jul 30, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

vstinner requested review from a team, encukou and markshannon as code owners July 30, 2024 12:49
bedevere-app Bot mentioned this pull request Jul 30, 2024
Closed

vstinner commented Jul 30, 2024

Copy link
Copy Markdown
Member Author

cc @encukou @da-woods

da-woods commented Jul 30, 2024

Copy link
Copy Markdown
Contributor

Looks sensible to me.

encukou reviewed Aug 1, 2024
View reviewed changes

encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It'll take me some time to review this in depth, unfortunately.
Here are some initial thoughts for what I'd want to check.

Comment thread Doc/c-api/type.rst
Comment thread Objects/typeobject.c Outdated

vstinner commented Aug 26, 2024

Copy link
Copy Markdown
Member Author

@encukou: Please review my updated PR. I modified my PR to check __mro__ instead of __bases__.

I also rebased the PR on the main branch to fix some merge conflicts.

encukou reviewed Aug 27, 2024
View reviewed changes

encukou left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good, thank you!
I have a few suggestions, and the C API WG should vote on new stable API, but I hope those are just formalities.

Comment thread Doc/c-api/type.rst Outdated
Comment thread Doc/c-api/type.rst
Comment thread Lib/test/test_capi/test_type.py
vstinner and others added 2 commits August 27, 2024 14:13
Update Doc/c-api/type.rst
0155362 
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Update Lib/test/test_capi/test_type.py
1c1fa7e 
Co-authored-by: Petr Viktorin <encukou@gmail.com>
vstinner mentioned this pull request Aug 27, 2024
Closed

vstinner commented Aug 27, 2024

Copy link
Copy Markdown
Member Author

the C API WG should vote on new stable API, but I hope those are just formalities.

I created capi-workgroup/decisions#40

vstinner commented Sep 23, 2024

Copy link
Copy Markdown
Member Author

@zooba: I added to the doc:

The type must not be used before it's made immutable. For example, type
instances must not be created before the type is made immutable.

Is it what you expected?

vstinner commented Sep 30, 2024

Copy link
Copy Markdown
Member Author

@encukou @serhiy-storchaka: Would you mind to review my PyType_Freeze() implementation?

vstinner commented Sep 30, 2024

Copy link
Copy Markdown
Member Author

See also PR gh-124789 implementing @zooba's Py_tp_create_callback slot idea.

vstinner commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

@encukou @serhiy-storchaka @erlend-aasland @zooba: Would you mind to review this PR? The API was approved by the C API Working Group.

serhiy-storchaka approved these changes Oct 19, 2024
View reviewed changes
Comment thread Lib/test/test_capi/test_type.py Outdated
Comment thread Objects/typeobject.c
Comment on lines +11342 to +11346
if (!PyTuple_Check(mro)) {
Py_DECREF(mro);
PyErr_SetString(PyExc_TypeError, "unable to get the type MRO");
return -1;
}

serhiy-storchaka Oct 19, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How difficult to add test for this case?

vstinner Oct 19, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure that it's possible to create a type with a MRO which is not a tuple. Using a metaclass, it's possible to override the MRO, but internally, Python converts mro() result into a tuple.

I added this check since type_get_mro() returns None if type->tp_mro is NULL. But I have no idea how to create a type with type->tp_mro=NULL. It's more a sanity check.

vstinner merged commit db96327 into python:main Oct 25, 2024
vstinner deleted the type_freeze branch October 25, 2024 09:12

vstinner commented Oct 25, 2024

Copy link
Copy Markdown
Member Author

Merged. Thanks for reviews.

da-woods commented Oct 26, 2024

Copy link
Copy Markdown
Contributor

Thanks Victor. I'll look at using this after the next CPython release

AraHaan commented Oct 30, 2024

Copy link
Copy Markdown
Contributor

I mean technically you can use it now if you manually build the main branch.

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
pythongh-121654: Add PyType_Freeze() function (python#122457)
533a40a 
Co-authored-by: Petr Viktorin <encukou@gmail.com>
sergey-miryanov reviewed May 13, 2025
View reviewed changes
Comment thread Objects/typeobject.c
i = 1;
}
for (; i<PyTuple_GET_SIZE(bases); i++) {
PyTypeObject *b = (PyTypeObject*)PyTuple_GET_ITEM(bases, i);

sergey-miryanov May 13, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry for the necro-posting.
IIUC only improperly constructed tuples can store NULL items. Is this such a case? If so, is the following check redundant?

vstinner May 16, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't know, I just moved the code. The test was already there before.

sergey-miryanov May 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Got it, thanks!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sergey-miryanov sergey-miryanov left review comments
encukou encukou left review comments
serhiy-storchaka serhiy-storchaka approved these changes
erlend-aasland erlend-aasland approved these changes
markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL