Skip to content

gh-134819: Add sys.set_object_tags and sys.get_object_tags #135073

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
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Jun 3, 2025

@corona10 corona10 changed the title gh-134819: Add sys.set_object_tags and sys.get_object_tags [WIP] gh-134819: Add sys.set_object_tags and sys.get_object_tags Jun 3, 2025
@corona10 corona10 changed the title [WIP] gh-134819: Add sys.set_object_tags and sys.get_object_tags gh-134819: Add sys.set_object_tags and sys.get_object_tags Jun 3, 2025
@corona10 corona10 marked this pull request as ready for review June 3, 2025 12:56
@corona10
Copy link
Member Author

corona10 commented Jun 3, 2025

@vstinner @colesbury @Fidget-Spinner @ZeroIntensity
Before making more progress, including documentation stuff.
I would like to get feedback on the current implementation.

{
assert(object != NULL);
if (strcmp(tag, "immortal") == 0) {
_Py_SetImmortal(object);
Copy link
Member

Choose a reason for hiding this comment

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

This definitely isn't safe on its own. (Trust me, I've gone down quite the rabbithole in getting arbitrary object immortalization working. It's extraordinarily complex to do safely.)

Copy link
Member Author

@corona10 corona10 Jun 3, 2025

Choose a reason for hiding this comment

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

Hmm, do you think that we should not allow setting "immortal" tag at this moment?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, at least for now. sys.set_object_tags is allowed to ignore tags, right?

Copy link
Member Author

@corona10 corona10 Jun 3, 2025

Choose a reason for hiding this comment

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

Yeah, we will ignore (and it's intended behavior)

@vstinner
Copy link
Member

vstinner commented Jun 3, 2025

Before making more progress, including documentation stuff. I would like to get feedback on the current implementation.

As I wrote in the issue, I dislike this API and I prefer the current design: one function per object attribute, such as sys._is_immortal().

@corona10
Copy link
Member Author

corona10 commented Jun 3, 2025

As I wrote in the issue, I dislike this API and I prefer the current design: one function per object attribute, such as sys._is_immortal()

The problem is that the alternative implementation also needs to provide the same functions to pass the unittests and avoid people from checking attributes based on the CPython-only function. So this API will remove unnecessary additional APIs that are only for checking the CPython implementation detail.

I also dislike that we are exposing CPython implementation detail to the pure Python function.
(C API is fine.. It's highly intertwined with CPython.)

@corona10
Copy link
Member Author

corona10 commented Jun 3, 2025

So if you think that we're fine with exposing CPython implementation details through Python and C API. I can happily drop this proposal.

@colesbury
Copy link
Contributor

I agree with @vstinner here and still feel the same way as in my comment on the issue. I am also not in a rush to expose these APIs at the Python level.

@ZeroIntensity
Copy link
Member

I'm indifferent. I see arguments for both sides that both make sense.

In general, I think we do need a way to expose unstable APIs to Python the same way we do in C. It's definitely not fun to have to set up a C extension just so you can get something that is useful at a Python level (e.g., deferred reference counting). I think Donghee makes a good point that prefixing with _ isn't a sufficient way to do that. Rather than getting hung up on how object implementation details will be exposed, I think we should focus on the bigger picture. How should we expose implementation details to Python, regardless of objects in specific?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants