-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
gh-132331: Add tp_versions_used
to PyTypeObject
docs
#132335
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.
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: sobolevn <mail@sobolevn.me>
@@ -2229,6 +2231,17 @@ and :c:data:`PyType_Type` effectively act as defaults.) | |||
.. versionadded:: 3.12 | |||
|
|||
|
|||
.. c:member:: uint16_t PyTypeObject.tp_versions_used | |||
Internal. Do not use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we document something, just to tell people not to use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field is exposed to the user who may use local PyTypeObject instances that are statically initialized (the LinuxCNC project implements that use case several places). If you do not initialize this field, then you get a warning missing initializer for member '_typeobject::tp_versions_used'
.
Therefore, it should be documented (and it needs to be set to zero when statically initialized).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay on the response. Please don't use a static PyTypeObject
if you're running into this problem -- use a heap type (PyType_Spec
) instead, which uses an array of slots rather than a struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use a static
PyTypeObject
if you're running into this problem [snip]
That is easily said, but you are too late. The static use case of PyTypeObject
is already a fact and has been for a long time (like the past 19 years for LinuxCNC). I'm sure other projects also use it in a static fashion.
If you want to hide static use of PyTypeObject
from the users, then you will have to go through a proper deprecation cycle. If you are not prepared to do that or there is no support for deprecation, then you should simply document every field in this structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, the phrase "documenting the field" does not mean that you have to tell its inner secrets to all users.
The only "documenting" you must do is to acknowledge the field's existence, just like all other fields of the PyTypeObject
, so all fields are listed together in the documentation. You may even tell the user that it is "for internal use only" and then be done with it. That is all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to hide static use of PyTypeObject from the users, then you will have to go through a proper deprecation cycle. If you are not prepared to do that or there is no support for deprecation, then you should simply document every field in this structure.
I think this is probably the better approach, but instead of a deprecation with a planned removal, we should just soft deprecate static types.
These days, I'd argue that static types should only stick around in public APIs for compatibility's sake. But for a maintained project like LinuxCNC, it's really not much of a chore to switch over to heap types, especially since it will help in the long run.
You may even tell the user that it is "for internal use only" and then be done with it. That is all.
The problem is that we don't really do this anywhere else, as far as I know (and if we do, we shouldn't!). It doesn't make much sense to me to document something as "this is only documented so C++ users know to set this to zero so they can avoid a warning". (It also makes it much, much more difficult to remove tp_versions_used
if it's documented.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is probably the better approach, but instead of a deprecation with a planned removal, we should just soft deprecate static types.
Static use is documented and it specifically mentions PyTypeObject
. Even though it contains a warning about 'static types must be compiled for a specific Python minor version', there is no mention that users should avoid it.
And with soft deprecation: from PEP378: A soft deprecation can be used when using an API which should no longer be used to write new code, but it remains safe to continue using it in existing code. The API remains documented and tested, but will not be developed further (no enhancement).
With soft deprecation, you a) cannot add a new field to this structure any more because of the "no enhancement" clause and b) you cannot use soft deprecation to "hide" a field and maintain compatibility because of the "remains documented" clause.
The simple solution is that you document this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static use is documented and it specifically mentions PyTypeObject. Even though it contains a warning about 'static types must be compiled for a specific Python minor version', there is no mention that users should avoid it.
Yes, you're right that static types need to be discouraged more in the documentation. That's a planned thing to do, but nobody has gotten around to it yet. But, that page does contain several other downsides of static types.
With soft deprecation, you a) cannot add a new field to this structure any more because of the "no enhancement" clause and b) you cannot use soft deprecation to "hide" a field and maintain compatibility because of the "remains documented" clause.
I meant soft deprecating public use of PyTypeObject
. We still want to change it, but we can soft deprecate doing things like passing static types to PyType_Ready
.
The simple solution is that you document this field.
I'm trying to be reasonable, but I've explained several times now why I'm strongly against doing so. This is a private field that users shouldn't touch. If you want to avoid the warning, just add .tp_versions_used = 0
to your structure initialization -- C users will implicitly do that anyway (we can't just add a field that needs a non-zero default value without breaking things), so it's fine to do it in C++ too.
I have two alternative proposals that don't involve switching to heap types:
- Document that
PyTypeObject
contains some internal fields that should be initialized to zero. That way, we don't have to document internal fields while still letting people know what to do. - Add a proper section (not just "do not use") on type versions and put that in the docs alongside
tp_versions_used
. (Basically, promotetp_versions_used
to a public API.)
Otherwise, unless another core dev speaks up and is strongly +1 on documenting tp_versions_used
in this way, this won't go any further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not document something that's supposed to be internal.
As far as I can tell, the motivation here is so that it's easier to statically initialize a PyTypeObject
, but this kind of thing is exactly why you shouldn't use static types; use heap types, which avoid ABI compatibility or structure definition issues. A library for the Julia language had a similar issue a little while ago and they solved it by switching to heap types.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Using heap types is not a free lunch. Here is an example of severe regression in the stdlib, coming from conversion from a static type: #114682 |
|
Fixes #132331. Add
tp_versions_used
to the tp slots section, as well as a description of the field itself. I've read through #113462 and its corresponding PR to understand how this field came about, but please let me know if there are any inaccuracies, thanks!tp_versions_used
was added toPyTypeObject
but is not documented #132331📚 Documentation preview 📚: https://cpython-previews--132335.org.readthedocs.build/