gh-134786: Py_TPFLAGS_MANAGED_WEAKREF and Py_TPFLAGS_MANAGED_DICT must be used only if Py_TPFLAGS_HAVE_GC set

[sergey-miryanov]

According to discussion Py_TPFLAGS_MANAGED_WEAKREF and Py_TPFLAGS_MANAGED_DICT must be used only if Py_TPFLAGS_HAVE_GC is set.

• Issue: Types with Py_TPFLAGS_MANAGED_WEAKREF but not Py_TPFLAGS_HAVE_GC crash when creating a weak reference #134786

---

📚 Documentation preview 📚: https://cpython-previews--135863.org.readthedocs.build/

[sergey-miryanov]

@markshannon Please take a look.

[chris-se]

As the person who reported #134786: I would suggest making the documentation more explicit and replacing should with must here, especially since the code now errors out if Py_TPFLAGS_HAVE_GC is not set.

[Starbuck5]

As the person who reported #134786: I would suggest making the documentation more explicit and replacing should with must here, especially since the code now errors out if Py_TPFLAGS_HAVE_GC is not set.

+1

Additionally, this PR puts this into a WhatsNew entry for 3.15. This is not a new thing, this is a documentation fix + fix for the runtime to not allow these invalid types to be created. So I think the added whatsnew is misleading.

[sergey-miryanov]

I'm not sure if we should support Py_TPFLAGS_MANAGED_DICT without Py_TPFLAGS_HAVE_GC, because Py_TPFLAGS_MANAGED_DICT implies Py_TPFLAGS_HAVE_GC. However, I have added it, though, to confirm that it is doable.

I think the current version needs some updates and clarification in the docs. First, though, I would like to get some feedback on the overall approach. Is this the right way to do after all?

Please take a look if someone interested.

[sergey-miryanov]

@markshannon Please take a look.

[efimov-mikhail]

Maybe we should rename this PR?

[sergey-miryanov]

Maybe we should rename this PR?

I forgot to apply changes :(

[markshannon]

Once the remaining comment has been addressed, I'm happy to merge this.

For the record, the requirements may change in the future:
For Py_TPFLAGS_MANAGED_DICT things are clear: Py_TPFLAGS_HAVE_GC must be set.
For Py_TPFLAGS_MANAGED_WEAKREF things are more complex. The current implementation requires that Py_TPFLAGS_HAVE_GC to be set, but there is no fundamental reason why the VM cannot manage the weakrefs of objects that cannot be part of cycles.

[sergey-miryanov]

For Py_TPFLAGS_MANAGED_WEAKREF things are more complex. The current implementation requires that Py_TPFLAGS_HAVE_GC to be set, but there is no fundamental reason why the VM cannot manage the weakrefs of objects that cannot be part of cycles.

I had such commit 42c0932, we decided to revert it for now. Maybe we should open a separate PR for it?

[kumaraditya303]

If it isn't too much work, it is worth adding a test for this.

[sergey-miryanov]

@kumaraditya303 I have added test.

[sergey-miryanov]

@markshannon @kumaraditya303 It is ready to review, could you please take a look.

[sergey-miryanov]

Thanks all!