gh-101696: Make sure immutable types have a valid version tag#101742
gh-101696: Make sure immutable types have a valid version tag#101742erlend-aasland wants to merge 1 commit intopython:mainfrom
Conversation
|
I guess we can skip NEWS, as this is strictly internal. |
erlend-aasland
requested review from
ericsnowcurrently
and removed request for
markshannon
|
I'll review this by tomorrow. |
ericsnowcurrently
left a comment
ericsnowcurrently
left a comment
There was a problem hiding this comment.
LGTM
FWIW, it would be nice if there were a test that covered the original problem from gh-101696 (i.e. fail without the fix and pass with it), whether by crashing or some other means. Would you mind adding something like that in this PR.
| if (_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) { | ||
| CHECK(type->tp_version_tag != 0); | ||
| } | ||
| else { | ||
| CHECK(type->tp_version_tag == 0); | ||
| } |
There was a problem hiding this comment.
This check is worth adding, regardless of the rest of this PR, right?
| /* 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; | ||
| } |
There was a problem hiding this comment.
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
-
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. ↩
There was a problem hiding this comment.
Note that this PR does not special-case static types; the condition is for immutable types.
|
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be put in the comfy chair! |
The original problem was uncovered by I could of course try to add a |
|
Closing, as per discussion on the issue. |
4 participants
Reverts #101697
_PyStaticType_Deallocdoes not invalidate type version tag #101696