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

gh-101696: Make sure immutable types have a valid version tag by erlend-aasland · Pull Request #101742 · python/cpython · GitHub

/ cpython Public

gh-101696: Make sure immutable types have a valid version tag #101742

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

Closed
erlend-aasland wants to merge 1 commit into from
Conversation 11 Commits 1 Checks 0 Files changed
Closed

gh-101696: Make sure immutable types have a valid version tag #101742

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension .c  (1) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
15 changes: 13 additions & 2 deletions Objects/typeobject.c
Show comments
View file
Open in desktop
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
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,13 @@ _PyType_CheckConsistency(PyTypeObject *type)
CHECK(type->tp_traverse != NULL);
}

if (_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) {
CHECK(type->tp_version_tag != 0);
}
else {
CHECK(type->tp_version_tag == 0);
}
Comment on lines +232 to +237

ericsnowcurrently Feb 9, 2023

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

This check is worth adding, regardless of the rest of this PR, right?

erlend-aasland Feb 9, 2023

Copy link
Copy Markdown
Contributor 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

Yes

if (type->tp_flags & Py_TPFLAGS_DISALLOW_INSTANTIATION) {
CHECK(type->tp_new == NULL);
CHECK(PyDict_Contains(type->tp_dict, &_Py_ID(__new__)) == 0);
Expand Down Expand Up @@ -4469,8 +4476,6 @@ _PyStaticType_Dealloc(PyTypeObject *type)
}

type->tp_flags &= ~Py_TPFLAGS_READY;
type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG;
type->tp_version_tag = 0;

if (type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
_PyStaticType_ClearWeakRefs(type);
Expand Down Expand Up @@ -6971,6 +6976,12 @@ PyType_Ready(PyTypeObject *type)
type->tp_flags |= Py_TPFLAGS_IMMUTABLETYPE;
}

/* All immutable types must have a static valid version tag */
if (type->tp_flags & Py_TPFLAGS_IMMUTABLETYPE) {
type->tp_version_tag = next_version_tag++;
type->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG;
}
Comment on lines +6979 to +6983

ericsnowcurrently Feb 9, 2023

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

tl;dr I'm on board with this.

As I mentioned in gh-101696, this is good as it covers non-builtin static types (AKA non-_Py_TPFLAGS_STATIC_BUILTIN), e.g. in community extension modules. 1 (The switch to Py_TPFLAGS_IMMUTABLETYPE seems fine.)

FWIW, the already merged fix may fit a little better with the vague idea (in my head) that we should be special-casing static types in the runtime as little as possible (or otherwise in more explicit ways, like the dedicated _PyStaticType_Dealloc()). In that spirit, rather than modifying PyType_Ready(), I was going to suggest we could find a way to apply _PyStaticType_Dealloc() to all static types, not just the builtin ones, which might be a good idea regardless. However, that feels like overkill for fixing this issue. Plus, we're already special-casing static types here in PyType_Ready(). (Perhaps my mental model is misaligned...)

Bottom line: dropping the original fix and adding the fix you have here is worth it since it handles non-builtin static types too.

Footnotes

  1. One could argue that non-builtin (non-core) static types should be discouraged (basically, they already are) and we shouldn't bother adding extra maintenance burden--however small--to accommodate them. However, that's not where the community is at nor will be for a long time and there's no reason to be so hard-line about it, especially in a case like this where the additional maintenance burden is essentially zero. ↩

erlend-aasland Feb 9, 2023

Copy link
Copy Markdown
Contributor 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

Note that this PR does not special-case static types; the condition is for immutable types.

if (type_ready(type) < 0) {
type->tp_flags &= ~Py_TPFLAGS_READYING;
return -1;
Expand Down
Back | FazBrowse Home | New Git URL