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.

Sign up for GitHub

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

Jump to bottom

gh-121654: Add PyType_Freeze() function #122457

Merged
merged 11 commits into from
Oct 25, 2024
Merged

gh-121654: Add PyType_Freeze() function #122457

merged 11 commits into from
Oct 25, 2024

Conversation

vstinner
Copy link
Member

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

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

cc @encukou @da-woods

@da-woods
Copy link
Contributor

Looks sensible to me.

encukou
encukou reviewed Aug 1, 2024
View reviewed changes
Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

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

Doc/c-api/type.rst Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
@vstinner
Copy link
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
encukou reviewed Aug 27, 2024
View reviewed changes
Copy link
Member

@encukou encukou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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.

Doc/c-api/type.rst Outdated Show resolved Hide resolved
Doc/c-api/type.rst Show resolved Hide resolved
Lib/test/test_capi/test_type.py Show resolved Hide resolved
vstinner and others added 2 commits August 27, 2024 14:13
@vstinner vstinner mentioned this pull request Aug 27, 2024
Closed
@vstinner
Copy link
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
Copy link
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
Copy link
Member Author

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

@vstinner
Copy link
Member Author

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

@vstinner
Copy link
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
serhiy-storchaka approved these changes Oct 19, 2024
View reviewed changes
Lib/test/test_capi/test_type.py Outdated Show resolved Hide resolved
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;
}
Copy link
Member

Choose a reason for hiding this comment

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

How difficult to add test for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 vstinner merged commit db96327 into python:main Oct 25, 2024
37 checks passed
@vstinner vstinner deleted the type_freeze branch October 25, 2024 09:12
@vstinner
Copy link
Member Author

Merged. Thanks for reviews.

@da-woods
Copy link
Contributor

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

@AraHaan
Copy link
Contributor

AraHaan commented Oct 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@encukou encukou encukou left review comments

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@markshannon 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.
6 participants
@vstinner @da-woods @AraHaan @encukou @serhiy-storchaka @erlend-aasland