Bug fix: adding back !is_alias<Class>(ptr) that were accidentally omitted. (#2958)

* Bug fix: adding back `!is_alias<Class>(ptr)` that were accidentally omitted.

* Introducing PYBIND11_SH_AVL, PYBIND11_SH_DEF macros. Applying PYBIND11_SH_DEF to test_factory_constructors.py to complete test coverage.

* Using PYBIND11_SH_DEF in test_methods_and_attributes.cpp, for more complete test coverage.

* Using PYBIND11_SH_DEF in test_multiple_inheritance.cpp, for more complete test coverage.

* Cleaning up test_classh_mock.cpp.

* Better explanations for PYBIND11_SH_AVL, PYBIND11_SH_DEF.

* Disabling 3.10-dev builds.

Author: rwgk

.github/workflows/ci.yml

@@ -23,7 +23,8 @@ jobs:
         - 3.5
         - 3.6
         - 3.9
-        - 3.10-dev
+        # Broken b/o https://github.com/pytest-dev/pytest/issues/8539
+        # - 3.10-dev
         - pypy2
         - pypy3
 

README_smart_holder.rst

@@ -101,28 +101,48 @@ holder:
        py::classh<Foo>(m, "Foo");
    }
 
-There are three small differences compared to classic pybind11:
+There are three small differences compared to Classic pybind11:
 
 - ``#include <pybind11/smart_holder.h>`` is used instead of
   ``#include <pybind11/pybind11.h>``.
 
+- The ``PYBIND11_SMART_HOLDER_TYPE_CASTERS(Foo)`` macro is needed.
+
 - ``py::classh`` is used instead of ``py::class_``.
 
-- The ``PYBIND11_SMART_HOLDER_TYPE_CASTERS(Foo)`` macro is needed.
+To the 2nd bullet point, the ``PYBIND11_SMART_HOLDER_TYPE_CASTERS`` macro
+needs to appear in all translation units with pybind11 bindings that involve
+Python⇄C++ conversions for `Foo`. This is the biggest inconvenience of the
+Conservative mode. Practically, at a larger scale it is best to work with a
+pair of `.h` and `.cpp` files for the bindings code, with the macros in the
+`.h` files.
 
-To the 2nd bullet point, ``py::classh<Foo>`` is simply a shortcut for
+To the 3rd bullet point, ``py::classh<Foo>`` is simply a shortcut for
 ``py::class_<Foo, py::smart_holder>``. The shortcut makes it possible to
-switch to using ``py::smart_holder`` without messing up the indentation of
-existing code. However, when migrating code that uses ``py::class_<Foo,
-std::shared_ptr<Foo>>``, currently ``std::shared_ptr<Foo>`` needs to be
-removed manually when switching to ``py::classh`` (#HelpAppreciated this
-could probably be avoided with a little bit of template metaprogramming).
+switch to using ``py::smart_holder`` without disturbing the indentation of
+existing code.
+
+When migrating code that uses ``py::class_<Bar, std::shared_ptr<Bar>>``
+there are two alternatives. The first one is to use ``py::classh<Bar>``:
+
+.. code-block:: diff
+
+   - py::class_<Bar, std::shared_ptr<Bar>>(m, "Bar");
+   + py::classh<Bar>(m, "Bar");
+
+This is clean and simple, but makes it difficult to fall back to Classic
+mode if needed. The second alternative is to replace ``std::shared_ptr<Bar>``
+with ``PYBIND11_SH_AVL(Bar)``:
 
-To the 3rd bullet point, the macro also needs to appear in other translation
-units with pybind11 bindings that involve Python⇄C++ conversions for
-`Foo`. This is the biggest inconvenience of the Conservative mode. Practically,
-at a larger scale it is best to work with a pair of `.h` and `.cpp` files
-for the bindings code, with the macros in the `.h` files.
+.. code-block:: diff
+
+   - py::class_<Bar, std::shared_ptr<Bar>>(m, "Bar");
+   + py::class_<Bar, PYBIND11_SH_AVL(Bar)>(m, "Bar");
+
+The ``PYBIND11_SH_AVL`` macro substitutes ``py::smart_holder``
+in Conservative mode, or ``std::shared_ptr<Bar>`` in Classic mode.
+See tests/test_classh_mock.cpp for an example. Note that the macro is also
+designed to not disturb the indentation of existing code.
 
 
 Progressive mode
@@ -132,47 +152,63 @@ To work in Progressive mode:
 
 - Add ``-DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT`` to the compilation commands.
 
-- Remove any ``std::shared_ptr<...>`` holders from existing ``py::class_``
-  instantiations.
+- Remove or replace (see below) ``std::shared_ptr<...>`` holders.
 
 - Only if custom smart-pointers are used: the
-  `PYBIND11_TYPE_CASTER_BASE_HOLDER` macro is needed [`example
-  <https://github.com/pybind/pybind11/blob/2f624af1ac8571d603df2d70cb54fc7e2e3a356a/tests/test_multiple_inheritance.cpp#L72>`_].
+  `PYBIND11_TYPE_CASTER_BASE_HOLDER` macro is needed (see
+  tests/test_smart_ptr.cpp for examples).
 
 Overall this is probably easier to work with than the Conservative mode, but
 
 - the macro inconvenience is shifted from ``py::smart_holder`` to custom
-  smart-pointers (but probably much more rarely needed).
+  smart-pointer holders (which are probably much more rare).
 
 - it will not interoperate with other extensions built against master or
   stable, or extensions built in Conservative mode (see the cross-module
   compatibility section below).
 
+When migrating code that uses ``py::class_<Bar, std::shared_ptr<Bar>>`` there
+are the same alternatives as for the Conservative mode (see previous section).
+An additional alternative is to use the ``PYBIND11_SH_DEF(...)`` macro:
+
+.. code-block:: diff
+
+   - py::class_<Bar, std::shared_ptr<Bar>>(m, "Bar");
+   + py::class_<Bar, PYBIND11_SH_DEF(Bar)>(m, "Bar");
+
+The ``PYBIND11_SH_DEF`` macro substitutes ``py::smart_holder`` only in
+Progressive mode, or ``std::shared_ptr<Bar>`` in Classic or Conservative
+mode. See tests/test_classh_mock.cpp for an example. Note that the
+``PYBIND11_SMART_HOLDER_TYPE_CASTERS`` macro is never needed in combination
+with the ``PYBIND11_SH_DEF`` macro, which is an advantage compared to the
+``PYBIND11_SH_AVL`` macro. Please review tests/test_classh_mock.cpp for a
+concise overview of all available options.
+
 
-Transition from Conservative to Progressive mode
-------------------------------------------------
+Transition from Classic to Progressive mode
+-------------------------------------------
 
 This still has to be tried out more in practice, but in small-scale situations
 it may be feasible to switch directly to Progressive mode in a break-fix
 fashion. In large-scale situations it seems more likely that an incremental
 approach is needed, which could mean incrementally converting ``py::class_``
-to ``py::classh`` including addition of the macros, then flip the switch,
-and convert ``py::classh`` back to ``py:class_`` combined with removal of the
-macros if desired (at that point it will work equivalently either way). It
-may be smart to delay the final cleanup step until all third-party projects
-of interest have made the switch, because then the code will continue to
-work in either mode.
+to ``py::classh`` and using the family of related macros, then flip the switch
+to Progressive mode, and convert ``py::classh`` back to ``py:class_`` combined
+with removal of the macros if desired (at that point it will work equivalently
+either way). It may be smart to delay the final cleanup step until all
+third-party projects of interest have made the switch, because then the code
+will continue to work in all modes.
 
 
-Using py::classh but with fallback to classic pybind11
-------------------------------------------------------
+Using py::smart_holder but with fallback to Classic pybind11
+------------------------------------------------------------
 
-This could be viewed as super-conservative mode, for situations in which
-compatibility with classic pybind11 (without smart_holder) is needed for
-some period of time. The main idea is to enable use of ``py::classh``
-and the associated ``PYBIND11_SMART_HOLDER_TYPE_CASTERS`` macro while
-still being able to build the same code with classic pybind11. Please see
-tests/test_classh_mock.cpp for an example.
+For situations in which compatibility with Classic pybind11
+(without smart_holder) is needed for some period of time, fallback
+to Classic mode can be enabled by copying the ``BOILERPLATE`` code
+block from tests/test_classh_mock.cpp. This code block provides mock
+implementations of ``py::classh`` and the family of related macros
+(e.g. ``PYBIND11_SMART_HOLDER_TYPE_CASTERS``).
 
 
 Classic / Conservative / Progressive cross-module compatibility
@@ -268,7 +304,7 @@ inherit from ``py::trampoline_self_life_support``, for example:
        ...
    };
 
-This is the only difference compared to classic pybind11. A fairly
+This is the only difference compared to Classic pybind11. A fairly
 minimal but complete example is tests/test_class_sh_trampoline_unique_ptr.cpp.
 
 
@@ -277,7 +313,7 @@ Ideas for the long-term
 
 The macros are clearly an inconvenience in many situations. Highly
 speculative: to avoid the need for the macros, a potential approach would
-be to combine the classic implementation (``type_caster_base``) with
+be to combine the Classic implementation (``type_caster_base``) with
 the ``smart_holder_type_caster``, but this will probably be very messy and
 not great as a long-term solution. The ``type_caster_base`` code is very
 complex already. A more maintainable approach long-term could be to work

include/pybind11/detail/init.h

@@ -179,7 +179,7 @@ template <
 void construct(value_and_holder &v_h, std::unique_ptr<Cpp<Class>, D> &&unq_ptr, bool need_alias) {
     auto *ptr = unq_ptr.get();
     no_nullptr(ptr);
-    if (Class::has_alias && need_alias)
+    if (Class::has_alias && need_alias && !is_alias<Class>(ptr))
         throw type_error("pybind11::init(): construction failed: returned std::unique_ptr pointee "
                          "is not an alias instance");
     auto smhldr
@@ -209,7 +209,7 @@ template <
 void construct(value_and_holder &v_h, std::shared_ptr<Cpp<Class>> &&shd_ptr, bool need_alias) {
     auto *ptr = shd_ptr.get();
     no_nullptr(ptr);
-    if (Class::has_alias && need_alias)
+    if (Class::has_alias && need_alias && !is_alias<Class>(ptr))
         throw type_error("pybind11::init(): construction failed: returned std::shared_ptr pointee "
                          "is not an alias instance");
     auto smhldr     = type_caster<Cpp<Class>>::template smart_holder_from_shared_ptr(shd_ptr);

include/pybind11/pybind11.h

@@ -1217,12 +1217,30 @@ template <typename T>
 
 using default_holder_type = std::unique_ptr<T>;
 
+#    ifndef PYBIND11_SH_AVL
+#        define PYBIND11_SH_AVL(...) std::shared_ptr<__VA_ARGS__> // "Smart_Holder if AVaiLable"
+// -------- std::shared_ptr(...) -- same length by design, to not disturb the indentation
+// of existing code.
+#    endif
+
+#    define PYBIND11_SH_DEF(...) std::shared_ptr<__VA_ARGS__> // "Smart_Holder if DEFault"
+// -------- std::shared_ptr(...) -- same length by design, to not disturb the indentation
+// of existing code.
+
 #    define PYBIND11_TYPE_CASTER_BASE_HOLDER(T, ...)
 
 #else
 
 using default_holder_type = smart_holder;
 
+#    ifndef PYBIND11_SH_AVL
+#        define PYBIND11_SH_AVL(...) ::pybind11::smart_holder // "Smart_Holder if AVaiLable"
+// -------- std::shared_ptr(...) -- same length by design, to not disturb the indentation
+// of existing code.
+#    endif
+
+#    define PYBIND11_SH_DEF(...) ::pybind11::smart_holder // "Smart_Holder if DEFault"
+
 // This define could be hidden away inside detail/smart_holder_type_casters.h, but is kept here
 // for clarity.
 #    define PYBIND11_TYPE_CASTER_BASE_HOLDER(T, ...)                                              \

include/pybind11/smart_holder.h

@@ -8,6 +8,12 @@
 #include "detail/smart_holder_type_casters.h"
 #include "pybind11.h"
 
+#undef PYBIND11_SH_AVL // Undoing #define in pybind11.h
+
+#define PYBIND11_SH_AVL(...) ::pybind11::smart_holder // "Smart_Holder if AVaiLable"
+// ---- std::shared_ptr(...) -- same length by design, to not disturb the indentation
+// of existing code.
+
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 
 // Supports easier switching between py::class_<T> and py::class_<T, py::smart_holder>:

tests/test_classh_mock.cpp

@@ -14,6 +14,12 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 template <typename type_, typename... options>
 using classh = class_<type_, options...>;
 PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
+#    ifndef PYBIND11_SH_AVL
+#        define PYBIND11_SH_AVL(...) std::shared_ptr<__VA_ARGS__> // "Smart_Holder if AVaiLable"
+#    endif
+#    ifndef PYBIND11_SH_DEF
+#        define PYBIND11_SH_DEF(...) std::shared_ptr<__VA_ARGS__> // "Smart_Holder if DEFault"
+#    endif
 #    ifndef PYBIND11_SMART_HOLDER_TYPE_CASTERS
 #        define PYBIND11_SMART_HOLDER_TYPE_CASTERS(...)
 #    endif
@@ -24,23 +30,42 @@ PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
 // BOILERPLATE END
 
 namespace {
-struct Foo0 {};
-struct Foo1 {};
-struct Foo2 {};
+struct FooUc {};
+struct FooUp {};
+struct FooSa {};
+struct FooSc {};
+struct FooSp {};
 } // namespace
 
-PYBIND11_TYPE_CASTER_BASE_HOLDER(Foo1, std::shared_ptr<Foo1>)
-PYBIND11_SMART_HOLDER_TYPE_CASTERS(Foo2)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(FooUp)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(FooSp)
+
+PYBIND11_TYPE_CASTER_BASE_HOLDER(FooSa, std::shared_ptr<FooSa>)
 
 TEST_SUBMODULE(classh_mock, m) {
-    // Uses std::unique_ptr<Foo0> as holder in conservative mode, py::smart_holder in progressive
-    // mode (if available).
-    py::class_<Foo0>(m, "Foo0").def(py::init<>());
+    // Please see README_smart_holder.rst, in particular section
+    // Classic / Conservative / Progressive cross-module compatibility
+
+    // Uses std::unique_ptr<FooUc> as holder in Classic or Conservative mode, py::smart_holder in
+    // Progressive mode.
+    py::class_<FooUc>(m, "FooUc").def(py::init<>());
+
+    // Uses std::unique_ptr<FooUp> as holder in Classic mode, py::smart_holder in Conservative or
+    // Progressive mode.
+    py::classh<FooUp>(m, "FooUp").def(py::init<>());
+
+    // Always uses std::shared_ptr<FooSa> as holder.
+    py::class_<FooSa, std::shared_ptr<FooSa>>(m, "FooSa").def(py::init<>());
 
-    // Always uses std::shared_ptr<Foo1> as holder.
-    py::class_<Foo1, std::shared_ptr<Foo1>>(m, "Foo1").def(py::init<>());
+    // Uses std::shared_ptr<FooSc> as holder in Classic or Conservative mode, py::smart_holder in
+    // Progressive mode.
+    py::class_<FooSc, PYBIND11_SH_DEF(FooSc)>(m, "FooSc").def(py::init<>());
+    // -------------- std::shared_ptr<FooSc> -- same length by design, to not disturb the
+    // indentation of existing code.
 
-    // Uses py::smart_holder if available, or std::unique_ptr<Foo2> if only pybind11 classic is
-    // available.
-    py::classh<Foo2>(m, "Foo2").def(py::init<>());
+    // Uses std::shared_ptr<FooSp> as holder in Classic mode, py::smart_holder in Conservative or
+    // Progressive mode.
+    py::class_<FooSp, PYBIND11_SH_AVL(FooSp)>(m, "FooSp").def(py::init<>());
+    // -------------- std::shared_ptr<FooSp> -- same length by design, to not disturb the
+    // indentation of existing code.
 }

tests/test_classh_mock.py

@@ -6,6 +6,8 @@
 def test_foobar():
     # Not really testing anything in particular. The main purpose of this test is to ensure the
     # suggested BOILERPLATE code block in test_classh_mock.cpp is correct.
-    assert m.Foo0()
-    assert m.Foo1()
-    assert m.Foo2()
+    assert m.FooUc()
+    assert m.FooUp()
+    assert m.FooSa()
+    assert m.FooSc()
+    assert m.FooSp()

tests/test_factory_constructors.cpp

@@ -139,11 +139,6 @@ class TestFactoryHelper {
     static std::shared_ptr<TestFactory3> construct3(int a) { return std::shared_ptr<TestFactory3>(new TestFactory3(a)); }
 };
 
-PYBIND11_TYPE_CASTER_BASE_HOLDER(TestFactory3, std::shared_ptr<TestFactory3>)
-PYBIND11_TYPE_CASTER_BASE_HOLDER(TestFactory4, std::shared_ptr<TestFactory4>)
-PYBIND11_TYPE_CASTER_BASE_HOLDER(TestFactory5, std::shared_ptr<TestFactory5>)
-PYBIND11_TYPE_CASTER_BASE_HOLDER(TestFactory7, std::shared_ptr<TestFactory7>)
-
 TEST_SUBMODULE(factory_constructors, m) {
 
     // Define various trivial types to allow simpler overload resolution:
@@ -188,7 +183,7 @@ TEST_SUBMODULE(factory_constructors, m) {
     auto c4a = [c](pointer_tag, TF4_tag, int a) { (void) c; return new TestFactory4(a);};
 
     // test_init_factory_basic, test_init_factory_casting
-    py::class_<TestFactory3, std::shared_ptr<TestFactory3>> pyTestFactory3(m, "TestFactory3");
+    py::class_<TestFactory3, PYBIND11_SH_DEF(TestFactory3)> pyTestFactory3(m, "TestFactory3");
     pyTestFactory3
         .def(py::init([](pointer_tag, int v) { return TestFactoryHelper::construct3(v); }))
         .def(py::init([](shared_ptr_tag) { return TestFactoryHelper::construct3(); }));
@@ -212,12 +207,12 @@ TEST_SUBMODULE(factory_constructors, m) {
         ;
 
     // test_init_factory_casting
-    py::class_<TestFactory4, TestFactory3, std::shared_ptr<TestFactory4>>(m, "TestFactory4")
+    py::class_<TestFactory4, TestFactory3, PYBIND11_SH_DEF(TestFactory4)>(m, "TestFactory4")
         .def(py::init(c4a)) // pointer
         ;
 
     // Doesn't need to be registered, but registering makes getting ConstructorStats easier:
-    py::class_<TestFactory5, TestFactory3, std::shared_ptr<TestFactory5>>(m, "TestFactory5");
+    py::class_<TestFactory5, TestFactory3, PYBIND11_SH_DEF(TestFactory5)>(m, "TestFactory5");
 
     // test_init_factory_alias
     // Alias testing
@@ -238,7 +233,7 @@ TEST_SUBMODULE(factory_constructors, m) {
 
     // test_init_factory_dual
     // Separate alias constructor testing
-    py::class_<TestFactory7, PyTF7, std::shared_ptr<TestFactory7>>(m, "TestFactory7")
+    py::class_<TestFactory7, PyTF7, PYBIND11_SH_DEF(TestFactory7)>(m, "TestFactory7")
         .def(py::init(
             [](int i) { return TestFactory7(i); },
             [](int i) { return PyTF7(i); }))

tests/test_factory_constructors.py

@@ -280,10 +280,11 @@ def get(self):
     assert not g1.has_alias()
     with pytest.raises(TypeError) as excinfo:
         PythFactory7(tag.shared_ptr, tag.invalid_base, 14)
-    assert (
-        str(excinfo.value)
-        == "pybind11::init(): construction failed: returned holder-wrapped instance is not an "
-        "alias instance"
+    assert str(excinfo.value) in (
+        "pybind11::init(): construction failed: returned holder-wrapped instance is not an "
+        "alias instance",
+        "pybind11::init(): construction failed: returned std::shared_ptr pointee is not an "
+        "alias instance",
     )
 
     assert [i.alive() for i in cstats] == [13, 7]