From 56f717756b8dab54c825f455ffd2495009046225 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Wed, 7 Sep 2016 13:32:49 -0400 Subject: [PATCH 1/2] Fix type caster for heap reference types Need to use the intrinsic type, not the raw type. Fixes #392. --- include/pybind11/cast.h | 11 ++++++----- tests/test_issues.cpp | 29 +++++++++++++++++++++++++++++ tests/test_issues.py | 14 ++++++++++++++ 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index b4698d816..c72ba166b 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -259,24 +259,25 @@ using cast_op_type = typename std::conditional class type_caster_base : public type_caster_generic { + using itype = intrinsic_t; public: static PYBIND11_DESCR name() { return type_descr(_()); } type_caster_base() : type_caster_generic(typeid(type)) { } - static handle cast(const type &src, return_value_policy policy, handle parent) { + static handle cast(const itype &src, return_value_policy policy, handle parent) { if (policy == return_value_policy::automatic || policy == return_value_policy::automatic_reference) policy = return_value_policy::copy; return cast(&src, policy, parent); } - static handle cast(type &&src, return_value_policy policy, handle parent) { + static handle cast(itype &&src, return_value_policy policy, handle parent) { if (policy == return_value_policy::automatic || policy == return_value_policy::automatic_reference) policy = return_value_policy::move; return cast(&src, policy, parent); } - static handle cast(const type *src, return_value_policy policy, handle parent) { + static handle cast(const itype *src, return_value_policy policy, handle parent) { return type_caster_generic::cast( src, policy, parent, src ? &typeid(*src) : nullptr, &typeid(type), make_copy_constructor(src), make_move_constructor(src)); @@ -284,8 +285,8 @@ public: template using cast_op_type = pybind11::detail::cast_op_type; - operator type*() { return (type *) value; } - operator type&() { if (!value) throw reference_cast_error(); return *((type *) value); } + operator itype*() { return (type *) value; } + operator itype&() { if (!value) throw reference_cast_error(); return *((itype *) value); } protected: typedef void *(*Constructor)(const void *stream); diff --git a/tests/test_issues.cpp b/tests/test_issues.cpp index 7c106434b..66ff79dd7 100644 --- a/tests/test_issues.cpp +++ b/tests/test_issues.cpp @@ -191,6 +191,35 @@ void init_issues(py::module &m) { py::class_(m2, "MoveIssue2").def(py::init()).def_readwrite("value", &MoveIssue2::v); m2.def("get_moveissue1", [](int i) -> MoveIssue1 * { return new MoveIssue1(i); }, py::return_value_policy::move); m2.def("get_moveissue2", [](int i) { return MoveIssue2(i); }, py::return_value_policy::move); + + // Issue 392: overridding reference-returning functions + class OverrideTest { + public: + struct A { int value = 99; }; + int v; + A a; + explicit OverrideTest(int v) : v{v} {} + virtual int int_value() { return v; } + virtual int &int_ref() { return v; } + virtual A A_value() { return a; } + virtual A &A_ref() { return a; } + }; + class PyOverrideTest : public OverrideTest { + public: + using OverrideTest::OverrideTest; + int int_value() override { PYBIND11_OVERLOAD(int, OverrideTest, int_value); } + int &int_ref() override { PYBIND11_OVERLOAD(int &, OverrideTest, int_ref); } + A A_value() override { PYBIND11_OVERLOAD(A, OverrideTest, A_value); } + A &A_ref() override { PYBIND11_OVERLOAD(A &, OverrideTest, A_ref); } + }; + py::class_(m2, "OverrideTest_A") + .def_readwrite("value", &OverrideTest::A::value); + py::class_(m2, "OverrideTest") + .def(py::init()) + .def("int_value", &OverrideTest::int_value) + .def("int_ref", &OverrideTest::int_ref) + .def("A_value", &OverrideTest::A_value) + .def("A_ref", &OverrideTest::A_ref); } // MSVC workaround: trying to use a lambda here crashes MSCV diff --git a/tests/test_issues.py b/tests/test_issues.py index c1e0756fd..056737a1a 100644 --- a/tests/test_issues.py +++ b/tests/test_issues.py @@ -166,3 +166,17 @@ def test_move_fallback(): assert m2.value == 2 m1 = get_moveissue1(1) assert m1.value == 1 + +def test_override_ref(): + from pybind11_tests.issues import OverrideTest + o = OverrideTest(42) + + i = o.int_ref() + assert o.int_value() == 42 + assert o.int_ref() == 42 + + assert o.A_value().value == 99 + a = o.A_ref() + assert a.value == 99 + a.value = 7 + assert a.value == 7 From c03db9bad9690d780d7fd1fafd8c7ef3009aa967 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Wed, 7 Sep 2016 13:38:32 -0400 Subject: [PATCH 2/2] Fail static_assert when trying to reference non-referencable types The previous commit to address #392 triggers a compiler warning about returning a reference to a local variable, which is *not* a false alarm: the following: py::cast(o) (which happens internally in an overload declaration) really is returning a reference to a local, because the cast operators for the type_caster for numeric types returns a reference to its own member. This commit adds a static_assert to make that a compilation failure rather than returning a reference into about-to-be-freed memory. Incidentally, this is also a fix for #219, which is exactly the same issue: we can't reference numeric primitives that are cast from wrappers around python numeric types. --- include/pybind11/cast.h | 12 ++++++++++++ tests/test_issues.cpp | 7 +++++-- tests/test_issues.py | 5 +++-- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c72ba166b..8ec0aa492 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -855,10 +855,22 @@ template struct move_if_unreferenced::type> : std::true_type {}; template using move_never = std::integral_constant::value && !move_if_unreferenced::value>; +// Detect whether returning a `type` from a cast on type's type_caster is going to result in a +// reference or pointer to a local variable of the type_caster. Basically, only +// non-reference/pointer `type`s and reference/pointers from a type_caster_generic are safe; +// everything else returns a reference/pointer to a local variable. +template using cast_is_temporary_value_reference = bool_constant< + (std::is_reference::value || std::is_pointer::value) && + !std::is_base_of>::value +>; + + NAMESPACE_END(detail) template T cast(const handle &handle) { using type_caster = detail::make_caster; + static_assert(!detail::cast_is_temporary_value_reference::value, + "Unable to cast type to reference: value is local to type caster"); type_caster conv; if (!conv.load(handle, true)) { #if defined(NDEBUG) diff --git a/tests/test_issues.cpp b/tests/test_issues.cpp index 66ff79dd7..c4d89c104 100644 --- a/tests/test_issues.cpp +++ b/tests/test_issues.cpp @@ -208,7 +208,9 @@ void init_issues(py::module &m) { public: using OverrideTest::OverrideTest; int int_value() override { PYBIND11_OVERLOAD(int, OverrideTest, int_value); } - int &int_ref() override { PYBIND11_OVERLOAD(int &, OverrideTest, int_ref); } + // Not allowed (uncommenting should hit a static_assert failure): we can't get a reference + // to a python numeric value, since we only copy values in the numeric type caster: +// int &int_ref() override { PYBIND11_OVERLOAD(int &, OverrideTest, int_ref); } A A_value() override { PYBIND11_OVERLOAD(A, OverrideTest, A_value); } A &A_ref() override { PYBIND11_OVERLOAD(A &, OverrideTest, A_ref); } }; @@ -217,9 +219,10 @@ void init_issues(py::module &m) { py::class_(m2, "OverrideTest") .def(py::init()) .def("int_value", &OverrideTest::int_value) - .def("int_ref", &OverrideTest::int_ref) +// .def("int_ref", &OverrideTest::int_ref) .def("A_value", &OverrideTest::A_value) .def("A_ref", &OverrideTest::A_ref); + } // MSVC workaround: trying to use a lambda here crashes MSCV diff --git a/tests/test_issues.py b/tests/test_issues.py index 056737a1a..86bd6a513 100644 --- a/tests/test_issues.py +++ b/tests/test_issues.py @@ -171,9 +171,10 @@ def test_override_ref(): from pybind11_tests.issues import OverrideTest o = OverrideTest(42) - i = o.int_ref() + # Not allowed (see associated .cpp comment) + #i = o.int_ref() + #assert o.int_ref() == 42 assert o.int_value() == 42 - assert o.int_ref() == 42 assert o.A_value().value == 99 a = o.A_ref()