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

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

Merged
merged 19 commits into from
Jul 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ca8d9fd
Disable implicit conversion from `0` to `pybind11::handle`.
Jun 14, 2022
21aa772
Reverse or-ed condition in an attempt to resolve GCC 8.3.0 errors (i3…
Jun 15, 2022
4ff51fd
Trying the simpler `std::is_same<T, PyObject *>`
Jun 15, 2022
e322611
Add implicit_conversion_from_pytorch_THPObjectPtr_to_handle test.
Jun 15, 2022
02fdc20
Accommodate types with implicit conversions to `PyObject *`, other th…
Jun 15, 2022
75317d8
Fix copy-paste mishap (picked wrong name).
Jun 15, 2022
05283e2
Revamp SFINAE construct to actually fix the pytorch issue (already va…
Jun 16, 2022
5410da7
Use `NOLINT(performance-noexcept-move-constructor)` for reduced code …
Jun 16, 2022
ded9c66
Use any_of, all_of, negation. It turns out to clang-format nicer.
Jun 16, 2022
6aa92e1
Clean up comments for changed code.
Jun 16, 2022
468addb
Reduce pytorch situation further, add test for operator ... const.
Jun 17, 2022
bc22bc8
Use `none_of` as suggested by @skylion007
Jun 17, 2022
5340256
Add `pure_compile_tests_for_handle_from_PyObject_pointers()`
Jun 17, 2022
a2781c6
Fix inconsequential oversight (retested).
Jun 18, 2022
8e6ad64
Factor our `is_pyobj_ptr_or_nullptr_t` to make the SFINAE conditions …
Jun 18, 2022
0bb8284
Remove stray line (oversight).
Jun 18, 2022
dbab18a
Make the `pure_compile_tests_for_handle_from_PyObject_pointers()` "rh…
Jun 18, 2022
5d42ec7
Remove the temporary PYBIND11_UNDO_PR4008 `#ifdef`.
Jun 18, 2022
ac94635
Merge branch 'master' into implicit_conversion_from_0_to_handle
Jul 14, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions include/pybind11/pytypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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; }
Expand Down
61 changes: 61 additions & 0 deletions tests/test_pytypes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions tests/test_pytypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down