-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
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
Open
sonnyding1
wants to merge
3
commits into
python:main
Choose a base branch
from
sonnyding1:doc_tp_versions_used
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PyTypeObjectif 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.
That is easily said, but you are too late. The static use case of
PyTypeObjectis 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
PyTypeObjectfrom 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.
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.
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.
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 toPyType_Ready.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 = 0to 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:
PyTypeObjectcontains 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.tp_versions_used. (Basically, promotetp_versions_usedto a public API.)Otherwise, unless another core dev speaks up and is strongly +1 on documenting
tp_versions_usedin 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.
I'm either +1 or -1 here because we have a precedent, that is
tp_watchedandtp_version_tag. Both are documented as "internal, do not use". However I fail to see how it could actually really help in initializing. How do you deal with those fields as they are only documented for internal use? I mean, all of them should 0 by default but I'm surprised that the compiler doesn't complain then. Since they are internal, we should maybe first indicate the default values they should take instead of saying that they are "internal" only, just so that users can initialize them. So yes, I would prefer that we document the default values and which ones are internals.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.
Yeah, I saw those fields. I think they were added during a time when there was much more emphasis on static types.
It's only an issue when compiling with
-Wmissing-field-initializers. But even then, it tells you exactly which fields you need to initialize, and the default values will have to be zero for anyPyTypeObjectfield we add (because otherwise we'd break code).I'd prefer we add a note that says
PyTypeObjectcontains some internal fields, and that the user should just initialize them to zero.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 also prefer that. What's important is the default values to be known to users that are worried about it. Because if it's an internal field, nothing would forbid us to use another default value!