Undo attr.h, detail/class.h changes made under PR #3923

[rwgk]

Description

See https://github.com/python/cpython/issues/92678 for background.

Suggested changelog entry:

[rwgk]

Thanks @Skylion007! Leaving this in draft mode for the minute, waiting for feedback here: https://github.com/python/cpython/issues/96046#issuecomment-1219759686

[Skylion007]

I was worried GC was not implemented properly with our use of the new API (see https://github.com/pybind/pybind11/issues/4092), but it sounds like that the upstream CPython has been updated so it is fine now?

[rwgk]

I was worried GC was not implemented properly with our use of the new API (see #4092), but it sounds like that the upstream CPython has been updated so it is fine now?

Who could give us authoritative advice what to do here?

A: Merge this PR (because the old way of doing things evidently works again with Python 3.11, thanks to @markshannon), wait for the Python 3.11.0 final release, but then soon after start testing against 3.12 (maybe brining back what this PR removes, as a starting point)?

B: Drop this PR because all required changes were made in Python 3.11 in the meantime?

C: Change this PR to make more/different adjustments for 3.11?

@Fidget-Spinner (b/o #4092)

@vstinner @tiran (b/o https://github.com/python/cpython/issues/96046)

@markshannon @pablogsal (b/o https://github.com/python/cpython/issues/92678)

[vstinner]

I suggest you waiting until https://github.com/python/cpython/pull/96047 is merged, and then test pybind11 again on a Python containing this change.

[rwgk]

I suggest you waiting until python/cpython#96047 is merged, and then test pybind11 again on a Python containing this change.

Looks like it's working! All details below. The command at the very bottom runs to completion (no more segfault). This is using the pybind11 v2.10.0 release.

@vstinner should I just close this PR?

---

I tested with this https://github.com/python/cpython commit:

commit 4f7f83b5bdd61f01a5ad49fb4df61cab3607ab99 (HEAD -> 3.11, origin/3.11)

git archive --format=tar.gz -o $HOME/cpython-3.11-4f7f83b5bdd61f01a5ad49fb4df61cab3607ab99.tar.gz --prefix=cpython-3.11-4f7f83b5bdd61f01a5ad49fb4df61cab3607ab99/ 3.11

Dockerfile

FROM fedora:36
RUN dnf -y install gcc
RUN dnf -y install g++
RUN dnf -y install gdb
RUN dnf -y install libffi-devel
RUN dnf -y install openssl-devel
RUN dnf -y install sqlite-devel
RUN dnf -y install git
RUN dnf -y install vim
RUN dnf -y install qpdf-devel

In the running docker container:

tar zxvf /ghome/cpython-3.11-4f7f83b5bdd61f01a5ad49fb4df61cab3607ab99.tar.gz
cd cpython-3.11-4f7f83b5bdd61f01a5ad49fb4df61cab3607ab99/
./configure --prefix=/Python-3.11-4f7f83b5bdd61f01a5ad49fb4df61cab3607ab99
make install -j 16
cd ..
/Python-3.11-4f7f83b5bdd61f01a5ad49fb4df61cab3607ab99/bin/python3.11 -m venv env
source /env/bin/activate
python -m pip install --upgrade pip
python -m pip install pytest
python -m pip install IPython
python -m pip install sphinx sphinx_issues sphinx_design sphinx_rtd_theme
git clone https://github.com/pikepdf/pikepdf
cd pikepdf/
python -m pip install .
cd docs/
/env/bin/sphinx-build . ../html

[rwgk]

FWIW, I ran a super-simple leak check: while True loop and top command. See diff below. It's all a bit make-shift, but I do NOT have any evidence that there is a leak (using cpython 3.11 @ commit 4f7f83b5bdd61f01a5ad49fb4df61cab3607ab99, pybind11 master).

This adds to my belief that we can keep using Py_TPFLAGS_MANAGED_DICT with Python 3.11, as released with pybind11 v2.10.0, which seems nice for continuity.

diff --git a/tests/test_multiple_inheritance.py b/tests/test_multiple_inheritance.py
index 3a1d88d7..4ad67569 100644
--- a/tests/test_multiple_inheritance.py
+++ b/tests/test_multiple_inheritance.py
@@ -261,13 +261,24 @@ def test_mi_static_properties():
         assert d.static_value == 0


-# Requires PyPy 6+
-def test_mi_dynamic_attributes():
[email protected]("vanilla_type", (m.VanillaDictMix1, m.VanillaDictMix2))
+def test_mi_dynamic_attr_simple(vanilla_type):
     """Mixing bases with and without dynamic attribute support"""
-
-    for d in (m.VanillaDictMix1(), m.VanillaDictMix2()):
+    while True:
+        d = vanilla_type()
         d.dynamic = 1
         assert d.dynamic == 1
+        break
+
+
[email protected]("vanilla_type", (m.VanillaDictMix1, m.VanillaDictMix2))
+def test_mi_dynamic_attr_ref_cycle(vanilla_type):
+    """Mixing bases with and without dynamic attribute support"""
+    while True:
+        d = vanilla_type()
+        d.ref_cycle = d
+        assert d.ref_cycle is d
+        #break


 def test_mi_unaligned_base():

[henryiii]

What's the status of this?

[rwgk]

What's the status of this?

I believe we don't need this, I just kept it around because it's not clear to me.

From memory, it was initially needed because of a change A in Python, but then it triggered a bug B in Python, then there was a rollback of change A so it is no longer needed, but also the triggered bug B is fixed, so we can keep it anyway.

That's all I know. It may be an oversimplification or even misrepresentation. I don't have time to dig into Python 3.11 and 3.12 to truly understand the situation and how it evolved.

Last activity here was 2 months ago: Closing, I figure we're fine.