Cleanup cast_safe<void> specialization

[laramiel]

Replace explicit specialization of cast_safe with SFINAE.
It's better for SFINAE cases to cover all type-sets rather than mixing SFINAE and explicit specialization.

Extracted from #3674

[rwgk]

Strictly optional, and strictly speaking unrelated to this change, but because this code is touched:

I'd find the code easier to read if lines 1157-1163 were moved down to 1170.

To have this pattern:

if A

if B

if not (A or B)

[laramiel]

Updated for @rwgk suggested order.

[henryiii]

This broke code like this:

#include <pybind11/pybind11.h>
#include <iostream>
namespace py = pybind11;
using std::cout;
using std::endl;
class Animal {
public:
    virtual ~Animal() { }
    virtual void* go() = 0;
};

class PyAnimal : public Animal {
public:
    /* Inherit the constructors */
    using Animal::Animal;
    /* Trampoline (need one for each virtual function) */
    void* go() override {
        PYBIND11_OVERRIDE_PURE(
            void*, /* Return type */
            Animal,      /* Parent class */
            go,          /* Name of function in C++ (must match Python name) */
        );
    }
};

PYBIND11_MODULE(example, m) {
    py::class_<Animal, PyAnimal /* <--- trampoline*/>(m, "Animal")
        .def(py::init<>())
        .def("go", &Animal::go);
}

Example projects that were broken by this are Nvidia's TensorRT.

Can it be fixed quickly or should we revert?

[rwgk]

Wow, 6 months later.
Do you have a link to an error message (for OS, compiler, and the actual error)?
I'll sync with @laramiel to look into this asap.

[henryiii]

$ clang++ -I/usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.10/include/python3.10 -I/usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.10/include/python3.10 -I/Users/henryschreiner/git/software/pybind11/include -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX12.sdk -std=c++11 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX12.sdk -undefined dynamic_lookup example.cpp -o example.cpython-310-darwin.so
example.cpp:18:9: error: cannot initialize return object of type 'void *' with an rvalue of type 'enable_if_t<std::is_same<void, intrinsic_t<void *>>::value, void>' (aka 'void')
        PYBIND11_OVERRIDE_PURE(
        ^~~~~~~~~~~~~~~~~~~~~~~
/Users/henryschreiner/git/software/pybind11/include/pybind11/pybind11.h:2808:5: note: expanded from macro 'PYBIND11_OVERRIDE_PURE'
    PYBIND11_OVERRIDE_PURE_NAME(                                                                  \
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/henryschreiner/git/software/pybind11/include/pybind11/pybind11.h:2770:9: note: expanded from macro 'PYBIND11_OVERRIDE_PURE_NAME'
        PYBIND11_OVERRIDE_IMPL(PYBIND11_TYPE(ret_type), PYBIND11_TYPE(cname), name, __VA_ARGS__); \
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/henryschreiner/git/software/pybind11/include/pybind11/pybind11.h:2736:20: note: expanded from macro 'PYBIND11_OVERRIDE_IMPL'
            return pybind11::detail::cast_safe<ret_type>(std::move(o));                           \
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

And this was opened about two weeks after releasing 2.10.0, it's been sitting in the issues for over two months, and it's been linked in my 2.10.1 wishlist for nearly as long; I've been busy teaching and no one else bothered to look.

It looks like it used to treat void* correctly and run in through the cast system, while now it triggers the vanilla void specialization and that doesn't match void*, so it fails.

[rwgk]

Thanks, I'll try to reproduce that.

And this was opened about two weeks after releasing 2.10.0, it's been sitting in the issues for over two months.

Ouch. Maybe we need to encourage people more to send a PR with a reproducer, to lower the bar for someone to jump in debugging. I'm unable to look at all issues.

[henryiii]

This PR had a reproducer, which is one reason it took about 2 minutes to find the bad comment via git bisect. I would really like to encourage this more, though - and especially have some way to highlight things that are regressions.

[rwgk]

Oh, the link was up there. I missed it before.

[rwgk]

Confirmed (clang 14, although it looks like the compiler doesn't even matter).

clang++ -o pybind11/tests/test_virtual_functions.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_virtual_functions.cpp
/usr/local/google/home/rwgk/forked/pybind11/tests/test_virtual_functions.cpp:607:9: error: cannot initialize return object of type 'void *' with an rvalue of type 'enable_if_t<std::is_same<void, intrinsic_t<void *>>::value, void>' (aka 'void')
        PYBIND11_OVERRIDE_PURE(
        ^~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:2826:5: note: expanded from macro 'PYBIND11_OVERRIDE_PURE'
    PYBIND11_OVERRIDE_PURE_NAME(                                                                  \
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:2788:9: note: expanded from macro 'PYBIND11_OVERRIDE_PURE_NAME'
        PYBIND11_OVERRIDE_IMPL(PYBIND11_TYPE(ret_type), PYBIND11_TYPE(cname), name, __VA_ARGS__); \
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:2754:20: note: expanded from macro 'PYBIND11_OVERRIDE_IMPL'
            return pybind11::detail::cast_safe<ret_type>(std::move(o));                           \
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

[rwgk]

It can be fixed, diff below (I don't want to trigger the CI with what I have right now), but is it better than the rollback?

I need to make the test complete, and TBH understand how the overriding Python function can meaningfully return a void *.

diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
index 5699d4fb..8b8ef696 100644
--- a/include/pybind11/cast.h
+++ b/include/pybind11/cast.h
@@ -1178,16 +1178,33 @@ template <typename T>
 enable_if_t<cast_is_temporary_value_reference<T>::value, T> cast_safe(object &&) {
     pybind11_fail("Internal error: cast_safe fallback invoked");
 }
+
+#ifdef PR3861_ROLLBACK
+
 template <typename T>
-enable_if_t<std::is_same<void, intrinsic_t<T>>::value, void> cast_safe(object &&) {}
+enable_if_t<!cast_is_temporary_value_reference<T>::value, T> cast_safe(object &&o) {
+    return pybind11::cast<T>(std::move(o));
+}
+
+template <>
+inline void cast_safe<void>(object &&) {}
+
+#else
+
+template <typename T>
+enable_if_t<std::is_same<void, intrinsic_t<T>>::value && !std::is_pointer<T>::value, void>
+cast_safe(object &&) {}
+
 template <typename T>
-enable_if_t<detail::none_of<cast_is_temporary_value_reference<T>,
-                            std::is_same<void, intrinsic_t<T>>>::value,
+enable_if_t<!cast_is_temporary_value_reference<T>::value
+                && !(std::is_same<void, intrinsic_t<T>>::value && !std::is_pointer<T>::value),
             T>
 cast_safe(object &&o) {
     return pybind11::cast<T>(std::move(o));
 }

+#endif
+
 PYBIND11_NAMESPACE_END(detail)

 // The overloads could coexist, i.e. the #if is not strictly speaking needed,

[laramiel]

I think that the issue is the code used intrinsic_t<T> from detail/common.h:706

Try replacing this:

template <typename T>
enable_if_t<std::is_same<void, intrinsic_t<T>>::value, void> cast_safe(object &&) {}

with

template <typename T>
enable_if_t<std::is_same<void, intrinsic_t<T>>::value && !std::is_pointer<T>::value, void> cast_safe(object &&) {}

(The test below should be the same. Apparently we have similar comments crossing paths.).

[laramiel]

It looks like detail/common.h has a remove_cvref_t; use that instead of intrinsic_t. Like this:

template <typename T>
enable_if_t<std::is_same<void, remove_cvref_t<T>>::value, void> cast_safe(object &&) {}

[laramiel]

The new code should be:

template <typename T>
enable_if_t<cast_is_temporary_value_reference<T>::value, T> cast_safe(object &&) {
    pybind11_fail("Internal error: cast_safe fallback invoked");
}

template <typename T>
enable_if_t<std::is_same<void, remove_cvref_t<T>>::value, void> cast_safe(object &&) {}

template <typename T>
enable_if_t<detail::none_of<cast_is_temporary_value_reference<T>,
                            std::is_same<void, remove_cvref_t<T>>>::value,
            T>
cast_safe(object &&o) {
    return pybind11::cast<T>(std::move(o));
}

[laramiel]

Though arguably since you cannot form a reference to void, except through template shenanigans, it should be sufficient to use remove_cv_t<T>::value.

[rwgk]

Confirmed, @laramiel's fix works. Tested locally, all unit tests pass.

[henryiii]

We need to add the failing test and the fix for it - will one of you do it, or do you want me to do it?