Disable implicit conversion of 0 to pybind11::handle. (#4008)

* Disable implicit conversion from `0` to `pybind11::handle`.

* Reverse or-ed condition in an attempt to resolve GCC 8.3.0 errors (i386/debian:buster).

* Trying the simpler `std::is_same<T, PyObject *>`

* Add implicit_conversion_from_pytorch_THPObjectPtr_to_handle test.

* Accommodate types with implicit conversions to `PyObject *`, other than `handle` & `handle` subclasses, or integral types.

* Fix copy-paste mishap (picked wrong name).

* Revamp SFINAE construct to actually fix the pytorch issue (already validated against pytorch proper).

The first version of the reduced pytorch code was critically missing the move ctor. The first version of the accompanying test was meaningless.

Note: It turns out the `!std::is_arithmetic<T>` condition is not needed: `int` is not in general implicitly convertible to `PyObject *`, only the literal `0` is.

* Use `NOLINT(performance-noexcept-move-constructor)` for reduced code from the wild (rather than changing the code).

* Use any_of, all_of, negation. It turns out to clang-format nicer.

* Clean up comments for changed code.

* Reduce pytorch situation further, add test for operator ... const.

* Use `none_of` as suggested by @Skylion007

* Add `pure_compile_tests_for_handle_from_PyObject_pointers()`

* Fix inconsequential oversight (retested).

* Factor our `is_pyobj_ptr_or_nullptr_t` to make the SFINAE conditions more readable.

* Remove stray line (oversight).

* Make the `pure_compile_tests_for_handle_from_PyObject_pointers()` "rhs-const-complete", too.

* Remove the temporary PYBIND11_UNDO_PR4008 `#ifdef`.

Author: rwgk

include/pybind11/pytypes.h

@@ -190,6 +190,11 @@ class object_api : public pyobject_tag {
     bool rich_compare(object_api const &other, int value) const;
 };
 
+template <typename T>
+using is_pyobj_ptr_or_nullptr_t = detail::any_of<std::is_same<T, PyObject *>,
+                                                 std::is_same<T, PyObject *const>,
+                                                 std::is_same<T, std::nullptr_t>>;
+
 PYBIND11_NAMESPACE_END(detail)
 
 #if !defined(PYBIND11_HANDLE_REF_DEBUG) && !defined(NDEBUG)
@@ -211,9 +216,23 @@ class handle : public detail::object_api<handle> {
 public:
     /// The default constructor creates a handle with a ``nullptr``-valued pointer
     handle() = default;
-    /// Creates a ``handle`` from the given raw Python object pointer
+
+    /// Enable implicit conversion from ``PyObject *`` and ``nullptr``.
+    /// Not using ``handle(PyObject *ptr)`` to avoid implicit conversion from ``0``.
+    template <typename T,
+              detail::enable_if_t<detail::is_pyobj_ptr_or_nullptr_t<T>::value, int> = 0>
+    // NOLINTNEXTLINE(google-explicit-constructor)
+    handle(T ptr) : m_ptr(ptr) {}
+
+    /// Enable implicit conversion through ``T::operator PyObject *()``.
+    template <
+        typename T,
+        detail::enable_if_t<detail::all_of<detail::none_of<std::is_base_of<handle, T>,
+                                                           detail::is_pyobj_ptr_or_nullptr_t<T>>,
+                                           std::is_convertible<T, PyObject *>>::value,
+                            int> = 0>
     // NOLINTNEXTLINE(google-explicit-constructor)
-    handle(PyObject *ptr) : m_ptr(ptr) {} // Allow implicit conversion from PyObject*
+    handle(T &obj) : m_ptr(obj) {}
 
     /// Return the underlying ``PyObject *`` pointer
     PyObject *ptr() const { return m_ptr; }

tests/test_pytypes.cpp

@@ -39,7 +39,68 @@ class float_ : public py::object {
 };
 } // namespace external
 
+namespace implicit_conversion_from_0_to_handle {
+// Uncomment to trigger compiler error. Note: Before PR #4008 this used to compile successfully.
+// void expected_to_trigger_compiler_error() { py::handle(0); }
+} // namespace implicit_conversion_from_0_to_handle
+
+// Used to validate systematically that PR #4008 does/did NOT change the behavior.
+void pure_compile_tests_for_handle_from_PyObject_pointers() {
+    {
+        PyObject *ptr = Py_None;
+        py::handle{ptr};
+    }
+    {
+        PyObject *const ptr = Py_None;
+        py::handle{ptr};
+    }
+    // Uncomment to trigger compiler errors.
+    // PyObject const *               ptr = Py_None; py::handle{ptr};
+    // PyObject const *const          ptr = Py_None; py::handle{ptr};
+    // PyObject volatile *            ptr = Py_None; py::handle{ptr};
+    // PyObject volatile *const       ptr = Py_None; py::handle{ptr};
+    // PyObject const volatile *      ptr = Py_None; py::handle{ptr};
+    // PyObject const volatile *const ptr = Py_None; py::handle{ptr};
+}
+
+namespace handle_from_move_only_type_with_operator_PyObject {
+
+// Reduced from
+// https://github.com/pytorch/pytorch/blob/279634f384662b7c3a9f8bf7ccc3a6afd2f05657/torch/csrc/utils/object_ptr.h
+struct operator_ncnst {
+    operator_ncnst() = default;
+    operator_ncnst(operator_ncnst &&) = default;
+    operator PyObject *() /* */ { return Py_None; } // NOLINT(google-explicit-constructor)
+};
+
+struct operator_const {
+    operator_const() = default;
+    operator_const(operator_const &&) = default;
+    operator PyObject *() const { return Py_None; } // NOLINT(google-explicit-constructor)
+};
+
+bool from_ncnst() {
+    operator_ncnst obj;
+    auto h = py::handle(obj);  // Critical part of test: does this compile?
+    return h.ptr() == Py_None; // Just something.
+}
+
+bool from_const() {
+    operator_const obj;
+    auto h = py::handle(obj);  // Critical part of test: does this compile?
+    return h.ptr() == Py_None; // Just something.
+}
+
+void m_defs(py::module_ &m) {
+    m.def("handle_from_move_only_type_with_operator_PyObject_ncnst", from_ncnst);
+    m.def("handle_from_move_only_type_with_operator_PyObject_const", from_const);
+}
+
+} // namespace handle_from_move_only_type_with_operator_PyObject
+
 TEST_SUBMODULE(pytypes, m) {
+    handle_from_move_only_type_with_operator_PyObject::m_defs(m);
+
     // test_bool
     m.def("get_bool", [] { return py::bool_(false); });
     // test_int

tests/test_pytypes.py

@@ -9,6 +9,11 @@
 from pybind11_tests import pytypes as m
 
 
+def test_handle_from_move_only_type_with_operator_PyObject():  # noqa: N802
+    assert m.handle_from_move_only_type_with_operator_PyObject_ncnst()
+    assert m.handle_from_move_only_type_with_operator_PyObject_const()
+
+
 def test_bool(doc):
     assert doc(m.get_bool) == "get_bool() -> bool"