Merge pull request #400 from jagerman/add-ref-virtual-macros

Add a way to deal with copied value references

docs/advanced.rst

@@ -298,13 +298,11 @@ helper class that is defined as follows:
 
 The macro :func:`PYBIND11_OVERLOAD_PURE` should be used for pure virtual
 functions, and :func:`PYBIND11_OVERLOAD` should be used for functions which have
-a default implementation.
-
-There are also two alternate macros :func:`PYBIND11_OVERLOAD_PURE_NAME` and
-:func:`PYBIND11_OVERLOAD_NAME` which take a string-valued name argument between
-the *Parent class* and *Name of the function* slots. This is useful when the
-C++ and Python versions of the function have different names, e.g.
-``operator()`` vs ``__call__``.
+a default implementation.  There are also two alternate macros
+:func:`PYBIND11_OVERLOAD_PURE_NAME` and :func:`PYBIND11_OVERLOAD_NAME` which
+take a string-valued name argument between the *Parent class* and *Name of the
+function* slots. This is useful when the C++ and Python versions of the
+function have different names, e.g.  ``operator()`` vs ``__call__``.
 
 The binding code also needs a few minor adaptations (highlighted):
 
@@ -327,10 +325,10 @@ The binding code also needs a few minor adaptations (highlighted):
         return m.ptr();
     }
 
-Importantly, pybind11 is made aware of the trampoline trampoline helper class
-by specifying it as an extra template argument to :class:`class_`.  (This can
-also be combined with other template arguments such as a custom holder type;
-the order of template types does not matter).  Following this, we are able to
+Importantly, pybind11 is made aware of the trampoline helper class by
+specifying it as an extra template argument to :class:`class_`.  (This can also
+be combined with other template arguments such as a custom holder type; the
+order of template types does not matter).  Following this, we are able to
 define a constructor as usual.
 
 Note, however, that the above is sufficient for allowing python classes to
@@ -357,6 +355,25 @@ a virtual method call.
 
 Please take a look at the :ref:`macro_notes` before using this feature.
 
+.. note::
+
+    When the overridden type returns a reference or pointer to a type that
+    pybind11 converts from Python (for example, numeric values, std::string,
+    and other built-in value-converting types), there are some limitations to
+    be aware of:
+
+    - because in these cases there is no C++ variable to reference (the value
+      is stored in the referenced Python variable), pybind11 provides one in
+      the PYBIND11_OVERLOAD macros (when needed) with static storage duration.
+      Note that this means that invoking the overloaded method on *any*
+      instance will change the referenced value stored in *all* instances of
+      that type.
+
+    - Attempts to modify a non-const reference will not have the desired
+      effect: it will change only the static cache variable, but this change
+      will not propagate to underlying Python instance, and the change will be
+      replaced the next time the overload is invoked.
+
 .. seealso::
 
     The file :file:`tests/test_virtual_functions.cpp` contains a complete

include/pybind11/cast.h

@@ -863,14 +863,8 @@ template <typename type> using cast_is_temporary_value_reference = bool_constant
     !std::is_base_of<type_caster_generic, make_caster<type>>::value
 >;
 
-
-NAMESPACE_END(detail)
-
-template <typename T> T cast(const handle &handle) {
-    using type_caster = detail::make_caster<T>;
-    static_assert(!detail::cast_is_temporary_value_reference<T>::value,
-            "Unable to cast type to reference: value is local to type caster");
-    type_caster conv;
+// Basic python -> C++ casting; throws if casting fails
+template <typename TypeCaster> TypeCaster &load_type(TypeCaster &conv, const handle &handle) {
     if (!conv.load(handle, true)) {
 #if defined(NDEBUG)
         throw cast_error("Unable to cast Python instance to C++ type (compile in debug mode for details)");
@@ -879,7 +873,22 @@ template <typename T> T cast(const handle &handle) {
             (std::string) handle.get_type().str() + " to C++ type '" + type_id<T>() + "''");
 #endif
     }
-    return conv.operator typename type_caster::template cast_op_type<T>();
+    return conv;
+}
+// Wrapper around the above that also constructs and returns a type_caster
+template <typename T> make_caster<T> load_type(const handle &handle) {
+    make_caster<T> conv;
+    load_type(conv, handle);
+    return conv;
+}
+
+NAMESPACE_END(detail)
+
+template <typename T> T cast(const handle &handle) {
+    static_assert(!detail::cast_is_temporary_value_reference<T>::value,
+            "Unable to cast type to reference: value is local to type caster");
+    using type_caster = detail::make_caster<T>;
+    return detail::load_type<T>(handle).operator typename type_caster::template cast_op_type<T>();
 }
 
 template <typename T> object cast(const T &value,
@@ -896,7 +905,7 @@ template <typename T> T handle::cast() const { return pybind11::cast<T>(*this);
 template <> inline void handle::cast() const { return; }
 
 template <typename T>
-typename std::enable_if<detail::move_always<T>::value || detail::move_if_unreferenced<T>::value, T>::type move(object &&obj) {
+detail::enable_if_t<detail::move_always<T>::value || detail::move_if_unreferenced<T>::value, T> move(object &&obj) {
     if (obj.ref_count() > 1)
 #if defined(NDEBUG)
         throw cast_error("Unable to cast Python instance to C++ rvalue: instance has multiple references"
@@ -906,18 +915,8 @@ typename std::enable_if<detail::move_always<T>::value || detail::move_if_unrefer
                 " instance to C++ " + type_id<T>() + " instance: instance has multiple references");
 #endif
 
-    typedef detail::type_caster<T> type_caster;
-    type_caster conv;
-    if (!conv.load(obj, true))
-#if defined(NDEBUG)
-        throw cast_error("Unable to cast Python instance to C++ type (compile in debug mode for details)");
-#else
-        throw cast_error("Unable to cast Python instance of type " +
-            (std::string) obj.get_type().str() + " to C++ type '" + type_id<T>() + "''");
-#endif
-
     // Move into a temporary and return that, because the reference may be a local value of `conv`
-    T ret = std::move(conv.operator T&());
+    T ret = std::move(detail::load_type<T>(obj).operator T&());
     return ret;
 }
 
@@ -926,16 +925,16 @@ typename std::enable_if<detail::move_always<T>::value || detail::move_if_unrefer
 //   object has multiple references, but trying to copy will fail to compile.
 // - If both movable and copyable, check ref count: if 1, move; otherwise copy
 // - Otherwise (not movable), copy.
-template <typename T> typename std::enable_if<detail::move_always<T>::value, T>::type cast(object &&object) {
+template <typename T> detail::enable_if_t<detail::move_always<T>::value, T> cast(object &&object) {
     return move<T>(std::move(object));
 }
-template <typename T> typename std::enable_if<detail::move_if_unreferenced<T>::value, T>::type cast(object &&object) {
+template <typename T> detail::enable_if_t<detail::move_if_unreferenced<T>::value, T> cast(object &&object) {
     if (object.ref_count() > 1)
         return cast<T>(object);
     else
         return move<T>(std::move(object));
 }
-template <typename T> typename std::enable_if<detail::move_never<T>::value, T>::type cast(object &&object) {
+template <typename T> detail::enable_if_t<detail::move_never<T>::value, T> cast(object &&object) {
     return cast<T>(object);
 }
 
@@ -944,6 +943,30 @@ template <typename T> T object::cast() && { return pybind11::cast<T>(std::move(*
 template <> inline void object::cast() const & { return; }
 template <> inline void object::cast() && { return; }
 
+NAMESPACE_BEGIN(detail)
+
+struct overload_unused {}; // Placeholder type for the unneeded (and dead code) static variable in the OVERLOAD_INT macro
+template <typename ret_type> using overload_caster_t = conditional_t<
+    cast_is_temporary_value_reference<ret_type>::value, make_caster<ret_type>, overload_unused>;
+
+// Trampoline use: for reference/pointer types to value-converted values, we do a value cast, then
+// store the result in the given variable.  For other types, this is a no-op.
+template <typename T> enable_if_t<cast_is_temporary_value_reference<T>::value, T> cast_ref(object &&o, make_caster<T> &caster) {
+    return load_type(caster, o).operator typename make_caster<T>::template cast_op_type<T>();
+}
+template <typename T> enable_if_t<!cast_is_temporary_value_reference<T>::value, T> cast_ref(object &&, overload_unused &) {
+    pybind11_fail("Internal error: cast_ref fallback invoked"); }
+
+// Trampoline use: Having a pybind11::cast with an invalid reference type is going to static_assert, even
+// though if it's in dead code, so we provide a "trampoline" to pybind11::cast that only does anything in
+// cases where pybind11::cast is valid.
+template <typename T> enable_if_t<!cast_is_temporary_value_reference<T>::value, T> cast_safe(object &&o) {
+    return pybind11::cast<T>(std::move(o)); }
+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 <> inline void cast_safe<void>(object &&) {}
+
+NAMESPACE_END(detail)
 
 
 template <return_value_policy policy = return_value_policy::automatic_reference,

include/pybind11/pybind11.h

@@ -1489,8 +1489,15 @@ template <class T> function get_overload(const T *this_ptr, const char *name) {
 #define PYBIND11_OVERLOAD_INT(ret_type, cname, name, ...) { \
         pybind11::gil_scoped_acquire gil; \
         pybind11::function overload = pybind11::get_overload(static_cast<const cname *>(this), name); \
-        if (overload) \
-            return overload(__VA_ARGS__).template cast<ret_type>();  }
+        if (overload) { \
+            auto o = overload(__VA_ARGS__); \
+            if (pybind11::detail::cast_is_temporary_value_reference<ret_type>::value) { \
+                static pybind11::detail::overload_caster_t<ret_type> caster; \
+                return pybind11::detail::cast_ref<ret_type>(std::move(o), caster); \
+            } \
+            else return pybind11::detail::cast_safe<ret_type>(std::move(o)); \
+        } \
+    }
 
 #define PYBIND11_OVERLOAD_NAME(ret_type, cname, name, fn, ...) \
     PYBIND11_OVERLOAD_INT(ret_type, cname, name, __VA_ARGS__) \

tests/test_virtual_functions.cpp

@@ -21,14 +21,22 @@ public:
 
     virtual int run(int value) {
         py::print("Original implementation of "
-                  "ExampleVirt::run(state={}, value={})"_s.format(state, value));
+                  "ExampleVirt::run(state={}, value={}, str1={}, str2={})"_s.format(state, value, get_string1(), *get_string2()));
         return state + value;
     }
 
     virtual bool run_bool() = 0;
     virtual void pure_virtual() = 0;
+
+    // Returning a reference/pointer to a type converted from python (numbers, strings, etc.) is a
+    // bit trickier, because the actual int& or std::string& or whatever only exists temporarily, so
+    // we have to handle it specially in the trampoline class (see below).
+    virtual const std::string &get_string1() { return str1; }
+    virtual const std::string *get_string2() { return &str2; }
+
 private:
     int state;
+    const std::string str1{"default1"}, str2{"default2"};
 };
 
 /* This is a wrapper class that must be generated */
@@ -36,7 +44,7 @@ class PyExampleVirt : public ExampleVirt {
 public:
     using ExampleVirt::ExampleVirt; /* Inherit constructors */
 
-    virtual int run(int value) {
+    int run(int value) override {
         /* Generate wrapping code that enables native function overloading */
         PYBIND11_OVERLOAD(
             int,         /* Return type */
@@ -46,7 +54,7 @@ public:
         );
     }
 
-    virtual bool run_bool() {
+    bool run_bool() override {
         PYBIND11_OVERLOAD_PURE(
             bool,         /* Return type */
             ExampleVirt,  /* Parent class */
@@ -56,7 +64,7 @@ public:
         );
     }
 
-    virtual void pure_virtual() {
+    void pure_virtual() override {
         PYBIND11_OVERLOAD_PURE(
             void,         /* Return type */
             ExampleVirt,  /* Parent class */
@@ -65,6 +73,27 @@ public:
                              in the previous line is needed for some compilers */
         );
     }
+
+    // We can return reference types for compatibility with C++ virtual interfaces that do so, but
+    // note they have some significant limitations (see the documentation).
+    const std::string &get_string1() override {
+        PYBIND11_OVERLOAD(
+            const std::string &, /* Return type */
+            ExampleVirt,         /* Parent class */
+            get_string1,         /* Name of function */
+                                 /* (no arguments) */
+        );
+    }
+
+    const std::string *get_string2() override {
+        PYBIND11_OVERLOAD(
+            const std::string *, /* Return type */
+            ExampleVirt,         /* Parent class */
+            get_string2,         /* Name of function */
+                                 /* (no arguments) */
+        );
+    }
+
 };
 
 class NonCopyable {
@@ -107,11 +136,11 @@ public:
 };
 class NCVirtTrampoline : public NCVirt {
 #if !defined(__INTEL_COMPILER)
-    virtual NonCopyable get_noncopyable(int a, int b) {
+    NonCopyable get_noncopyable(int a, int b) override {
         PYBIND11_OVERLOAD(NonCopyable, NCVirt, get_noncopyable, a, b);
     }
 #endif
-    virtual Movable get_movable(int a, int b) {
+    Movable get_movable(int a, int b) override {
         PYBIND11_OVERLOAD_PURE(Movable, NCVirt, get_movable, a, b);
     }
 };

tests/test_virtual_functions.py

@@ -20,13 +20,23 @@ def test_override(capture, msg):
             print('ExtendedExampleVirt::run_bool()')
             return False
 
+        def get_string1(self):
+            return "override1"
+
         def pure_virtual(self):
             print('ExtendedExampleVirt::pure_virtual(): %s' % self.data)
 
+    class ExtendedExampleVirt2(ExtendedExampleVirt):
+        def __init__(self, state):
+            super(ExtendedExampleVirt2, self).__init__(state + 1)
+
+        def get_string2(self):
+            return "override2"
+
     ex12 = ExampleVirt(10)
     with capture:
         assert runExampleVirt(ex12, 20) == 30
-    assert capture == "Original implementation of ExampleVirt::run(state=10, value=20)"
+    assert capture == "Original implementation of ExampleVirt::run(state=10, value=20, str1=default1, str2=default2)"
 
     with pytest.raises(RuntimeError) as excinfo:
         runExampleVirtVirtual(ex12)
@@ -37,7 +47,7 @@ def test_override(capture, msg):
         assert runExampleVirt(ex12p, 20) == 32
     assert capture == """
         ExtendedExampleVirt::run(20), calling parent..
-        Original implementation of ExampleVirt::run(state=11, value=21)
+        Original implementation of ExampleVirt::run(state=11, value=21, str1=override1, str2=default2)
     """
     with capture:
         assert runExampleVirtBool(ex12p) is False
@@ -46,11 +56,19 @@ def test_override(capture, msg):
         runExampleVirtVirtual(ex12p)
     assert capture == "ExtendedExampleVirt::pure_virtual(): Hello world"
 
+    ex12p2 = ExtendedExampleVirt2(15)
+    with capture:
+        assert runExampleVirt(ex12p2, 50) == 68
+    assert capture == """
+        ExtendedExampleVirt::run(50), calling parent..
+        Original implementation of ExampleVirt::run(state=17, value=51, str1=override1, str2=override2)
+    """
+
     cstats = ConstructorStats.get(ExampleVirt)
-    assert cstats.alive() == 2
-    del ex12, ex12p
+    assert cstats.alive() == 3
+    del ex12, ex12p, ex12p2
     assert cstats.alive() == 0
-    assert cstats.values() == ['10', '11']
+    assert cstats.values() == ['10', '11', '17']
     assert cstats.copy_constructions == 0
     assert cstats.move_constructions >= 0