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-101696: Make sure immutable types have a valid version tag #101742

Closed
wants to merge 1 commit into from
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions Objects/typeobject.c
View file
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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