Skip to content
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

C API: _PyObject_VisitManagedDict() function should be public #107073

Closed
vstinner opened this issue Jul 22, 2023 · 10 comments
Closed

C API: _PyObject_VisitManagedDict() function should be public #107073

vstinner opened this issue Jul 22, 2023 · 10 comments
Labels
topic-C-API type-bug An unexpected behavior, bug, or error

Comments

@vstinner
Copy link
Member

vstinner commented Jul 22, 2023

tl; dr: _PyObject_VisitManagedDict() and _PyObject_ClearManagedDict() function should be public.

If a C extension implements a type as a heap type, the type must support the GC protocol: it must implement visit and clear functions, and the dealloc function must DECREF the type.

It seems like if a heap type uses the new Py_TPFLAGS_MANAGED_DICT flag, the visit function must call _PyObject_VisitManagedDict() and the clear function must call _PyObject_ClearManagedDict(). Correct me if I'm wrong.

Problem: _PyObject_VisitManagedDict() and _PyObject_ClearManagedDict() functions are private. IMO they must be public.

Either Py_TPFLAGS_MANAGED_DICT flag must be private, or it should be public and all related helper functions should be public as well.

Py_TPFLAGS_MANAGED_DICT was added to Python 3.11.

_PyObject_VisitManagedDict() and _PyObject_ClearManagedDict() functions were added to Python 3.12.

In Python 3.12, typing.TypeVar, typing.ParamSpec, typing.TypeVarTuple are implemented with Py_TPFLAGS_MANAGED_DICT (and use the 2 helper functions).

In Python 3.13, _asyncio.Future and _asyncio.Task are also implemented with Py_TPFLAGS_MANAGED_DICT (and use the 2 helper functions).

Note: In Objects/typeobject.c, subtype_traverse() calls _PyObject_VisitManagedDict() in some cases, and subtype_clear() calls _PyObject_ClearManagedDict() in some cases.

Linked PRs

@vstinner vstinner added type-bug An unexpected behavior, bug, or error topic-C-API labels Jul 22, 2023
@vstinner
Copy link
Member Author

cc @brandtbucher @iritkatriel: Tell me if I'm correct. If yes, I can create a PR to fix this issue.

@vstinner
Copy link
Member Author

The doc doesn't say much about how a heap type should be implemented:

@vstinner
Copy link
Member Author

pybind11 (C++) uses a strange hack using *_PyObject_GetDictPtr() expression (note the * at the start) which uses the private _PyObject_GetDictPtr() function:
https://github.com/pybind/pybind11/blob/8d08dc64ca300342852ceaa7b1d65fe9f69dab06/include/pybind11/detail/class.h#L523-L539

traverse:

    PyObject *&dict = *_PyObject_GetDictPtr(self);
    Py_VISIT(dict);

clear:

    PyObject *&dict = *_PyObject_GetDictPtr(self);
    Py_CLEAR(dict);

By the way, should we also make _PyObject_GetDictPtr() function public?

@vstinner
Copy link
Member Author

cc @colesbury @ambv

vstinner added a commit to vstinner/cpython that referenced this issue Sep 1, 2023
Make PyObject_VisitManagedDict() and PyObject_ClearManagedDict()
functions public.

* Rename _PyObject_VisitManagedDict() to PyObject_VisitManagedDict().
* Rename _PyObject_ClearManagedDict() to PyObject_ClearManagedDict().
* Document these functions.
vstinner added a commit to vstinner/cpython that referenced this issue Sep 1, 2023
Make PyObject_VisitManagedDict() and PyObject_ClearManagedDict()
functions public in Python 3.12 C API.

* Rename _PyObject_VisitManagedDict() to PyObject_VisitManagedDict().
* Rename _PyObject_ClearManagedDict() to PyObject_ClearManagedDict().
* Document these functions.
@vstinner
Copy link
Member Author

vstinner commented Sep 1, 2023

I'm not sure if _PyObject_GetDictPtr() should be made public or not.

vstinner added a commit to vstinner/cpython that referenced this issue Sep 4, 2023
Make PyObject_VisitManagedDict() and PyObject_ClearManagedDict()
functions public in Python 3.13 C API.

* Rename _PyObject_VisitManagedDict() to PyObject_VisitManagedDict().
* Rename _PyObject_ClearManagedDict() to PyObject_ClearManagedDict().
* Document these functions.
vstinner added a commit to vstinner/cpython that referenced this issue Sep 13, 2023
Make PyObject_VisitManagedDict() and PyObject_ClearManagedDict()
functions public in Python 3.13 C API.

* Rename _PyObject_VisitManagedDict() to PyObject_VisitManagedDict().
* Rename _PyObject_ClearManagedDict() to PyObject_ClearManagedDict().
* Document these functions.
vstinner added a commit to vstinner/cpython that referenced this issue Sep 13, 2023
Make PyObject_VisitManagedDict() and PyObject_ClearManagedDict()
functions public in Python 3.13 C API.

* Rename _PyObject_VisitManagedDict() to PyObject_VisitManagedDict().
* Rename _PyObject_ClearManagedDict() to PyObject_ClearManagedDict().
* Document these functions.
@markshannon
Copy link
Member

Just to be clear, the root problem here is that if a class is defined using PyType_FromSpec() and Py_TPFLAGS_MANAGED_DICT is set, there is no way to implement tp_visit and tp_clear.

That can be fixed by adding _PyObject_VisitManagedDict() and _PyObject_ClearManagedDict() (or something with equivalent functionality) to the API so that tp_visit and tp_clear can be implemented.

However, PyType_FromSpec() inherits many of the flaws of statically declaring PyTypeObjects.
We should offer a better way to build classes, so maybe add a PyUnstable_ prefix, as we want to come up with a better approach?

@markshannon
Copy link
Member

_PyObject_GetDictPtr() should not be made public.
We should remove it, but not while anyone still uses tp_dictoffset.

@markshannon
Copy link
Member

If the PyUnstable_ prefix had existed when these functions were added, I would have used it.

Having managed dicts is stable, so the functions will continue to have meaning if even the implementation changes.
In that sense they are stable.

However, we want to move the responsibility for visiting and clearing from C extensions to the VM. Once we have done that we will no longer need _PyObject_VisitManagedDict() and _PyObject_ClearManagedDict(), so there is an argument for making them PyUnstable.

On balance, I think they should be public. Making object layout the full responsibility of the VM won't happen for a few releases and even when it does it will need a longish deprecation to remove all the old API. So, we might as well add _PyObject_VisitManagedDict() and _PyObject_ClearManagedDict() to the stable API.

vstinner added a commit that referenced this issue Oct 2, 2023
Make PyObject_VisitManagedDict() and PyObject_ClearManagedDict()
functions public in Python 3.13 C API.

* Rename _PyObject_VisitManagedDict() to PyObject_VisitManagedDict().
* Rename _PyObject_ClearManagedDict() to PyObject_ClearManagedDict().
* Document these functions.
@vstinner
Copy link
Member Author

vstinner commented Oct 3, 2023

However, we want to move the responsibility for visiting and clearing from C extensions to the VM.

Py_VISIT() should only be called once by the GC. So either calling PyObject_VisitManagedDict() should be the responsibility of the C extension, or the VM. But it cannot be both. And it cannot change without adding a new type flag, otherwise it would break the backward compatibility, since PyObject_VisitManagedDict() is public in Python 3.13. No?

On balance, I think they should be public. Making object layout the full responsibility of the VM won't happen for a few releases

That sounds like a good practical trade-off.

@vstinner
Copy link
Member Author

vstinner commented Oct 3, 2023

I added PyObject_VisitManagedDict() and PyObject_ClearManagedDict() functions to pythoncapi-compat for Python 3.11 and Python 3.12 using _PyObject_GetDictPtr(): python/pythoncapi-compat@a594354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants