Merge pull request #408 from dean0x7d/exc-destructors

Fix Python C API calls in desctuctors triggered by error_already_set

include/pybind11/cast.h

@@ -53,12 +53,8 @@ PYBIND11_NOINLINE inline internals &get_internals() {
             [](std::exception_ptr p) -> void {
             [](std::exception_ptr p) -> void {
                 try {
                 try {
                     if (p) std::rethrow_exception(p);
                     if (p) std::rethrow_exception(p);
-                } catch (const error_already_set &)      {                                                 return;
-                } catch (const index_error &e)           { PyErr_SetString(PyExc_IndexError,    e.what()); return;
-                } catch (const key_error &e)             { PyErr_SetString(PyExc_KeyError,      e.what()); return;
-                } catch (const value_error &e)           { PyErr_SetString(PyExc_ValueError,    e.what()); return;
-                } catch (const type_error &e)            { PyErr_SetString(PyExc_TypeError,     e.what()); return;
-                } catch (const stop_iteration &e)        { PyErr_SetString(PyExc_StopIteration, e.what()); return;
+                } catch (error_already_set &e)           { e.restore();                                    return;
+                } catch (const builtin_exception &e)     { e.set_error();                                  return;
                 } catch (const std::bad_alloc &e)        { PyErr_SetString(PyExc_MemoryError,   e.what()); return;
                 } catch (const std::bad_alloc &e)        { PyErr_SetString(PyExc_MemoryError,   e.what()); return;
                 } catch (const std::domain_error &e)     { PyErr_SetString(PyExc_ValueError,    e.what()); return;
                 } catch (const std::domain_error &e)     { PyErr_SetString(PyExc_ValueError,    e.what()); return;
                 } catch (const std::invalid_argument &e) { PyErr_SetString(PyExc_ValueError,    e.what()); return;
                 } catch (const std::invalid_argument &e) { PyErr_SetString(PyExc_ValueError,    e.what()); return;

include/pybind11/common.h

@@ -384,22 +384,42 @@ inline void ignore_unused(const int *) { }
 
 
 NAMESPACE_END(detail)
 NAMESPACE_END(detail)
 
 
-#define PYBIND11_RUNTIME_EXCEPTION(name) \
-    class name : public std::runtime_error { public: \
-        name(const std::string &w) : std::runtime_error(w) { }; \
-        name(const char *s) : std::runtime_error(s) { }; \
-        name() : std::runtime_error("") { } \
+/// Fetch and hold an error which was already set in Python
+class error_already_set : public std::runtime_error {
+public:
+    error_already_set() : std::runtime_error(detail::error_string()) {
+        PyErr_Fetch(&type, &value, &trace);
+    }
+    ~error_already_set() { Py_XDECREF(type); Py_XDECREF(value); Py_XDECREF(trace); }
+
+    /// Give the error back to Python
+    void restore() { PyErr_Restore(type, value, trace); type = value = trace = nullptr; }
+
+private:
+    PyObject *type, *value, *trace;
+};
+
+/// C++ bindings of builtin Python exceptions
+class builtin_exception : public std::runtime_error {
+public:
+    using std::runtime_error::runtime_error;
+    virtual void set_error() const = 0; /// Set the error using the Python C API
+};
+
+#define PYBIND11_RUNTIME_EXCEPTION(name, type) \
+    class name : public builtin_exception { public: \
+        using builtin_exception::builtin_exception; \
+        name() : name("") { } \
+        void set_error() const override { PyErr_SetString(type, what()); } \
     };
     };
 
 
-// C++ bindings of core Python exceptions
-class error_already_set : public std::runtime_error { public: error_already_set() : std::runtime_error(detail::error_string())  {} };
-PYBIND11_RUNTIME_EXCEPTION(stop_iteration)
-PYBIND11_RUNTIME_EXCEPTION(index_error)
-PYBIND11_RUNTIME_EXCEPTION(key_error)
-PYBIND11_RUNTIME_EXCEPTION(value_error)
-PYBIND11_RUNTIME_EXCEPTION(type_error)
-PYBIND11_RUNTIME_EXCEPTION(cast_error) /// Thrown when pybind11::cast or handle::call fail due to a type casting error
-PYBIND11_RUNTIME_EXCEPTION(reference_cast_error) /// Used internally
+PYBIND11_RUNTIME_EXCEPTION(stop_iteration, PyExc_StopIteration)
+PYBIND11_RUNTIME_EXCEPTION(index_error, PyExc_IndexError)
+PYBIND11_RUNTIME_EXCEPTION(key_error, PyExc_KeyError)
+PYBIND11_RUNTIME_EXCEPTION(value_error, PyExc_ValueError)
+PYBIND11_RUNTIME_EXCEPTION(type_error, PyExc_TypeError)
+PYBIND11_RUNTIME_EXCEPTION(cast_error, PyExc_RuntimeError) /// Thrown when pybind11::cast or handle::call fail due to a type casting error
+PYBIND11_RUNTIME_EXCEPTION(reference_cast_error, PyExc_RuntimeError) /// Used internally
 
 
 [[noreturn]] PYBIND11_NOINLINE inline void pybind11_fail(const char *reason) { throw std::runtime_error(reason); }
 [[noreturn]] PYBIND11_NOINLINE inline void pybind11_fail(const char *reason) { throw std::runtime_error(reason); }
 [[noreturn]] PYBIND11_NOINLINE inline void pybind11_fail(const std::string &reason) { throw std::runtime_error(reason); }
 [[noreturn]] PYBIND11_NOINLINE inline void pybind11_fail(const std::string &reason) { throw std::runtime_error(reason); }

include/pybind11/eigen.h

@@ -217,7 +217,6 @@ struct type_caster<Type, typename std::enable_if<is_eigen_sparse<Type>::value>::
             try {
             try {
                 obj = matrix_type(obj);
                 obj = matrix_type(obj);
             } catch (const error_already_set &) {
             } catch (const error_already_set &) {
-                PyErr_Clear();
                 return false;
                 return false;
             }
             }
         }
         }

include/pybind11/pybind11.h

@@ -426,7 +426,8 @@ protected:
                 if (result.ptr() != PYBIND11_TRY_NEXT_OVERLOAD)
                 if (result.ptr() != PYBIND11_TRY_NEXT_OVERLOAD)
                     break;
                     break;
             }
             }
-        } catch (const error_already_set &) {
+        } catch (error_already_set &e) {
+            e.restore();
             return nullptr;
             return nullptr;
         } catch (...) {
         } catch (...) {
             /* When an exception is caught, give each registered exception
             /* When an exception is caught, give each registered exception
@@ -1313,7 +1314,6 @@ NAMESPACE_END(detail)
 
 
 template <return_value_policy policy = return_value_policy::automatic_reference, typename... Args>
 template <return_value_policy policy = return_value_policy::automatic_reference, typename... Args>
 void print(Args &&...args) {
 void print(Args &&...args) {
-    error_scope scope; // Preserve error state
     auto c = detail::collect_arguments<policy>(std::forward<Args>(args)...);
     auto c = detail::collect_arguments<policy>(std::forward<Args>(args)...);
     detail::print(c.args(), c.kwargs());
     detail::print(c.args(), c.kwargs());
 }
 }

tests/test_eval.cpp

@@ -63,7 +63,6 @@ test_initializer eval([](py::module &m) {
         try {
         try {
             py::eval("nonsense code ...");
             py::eval("nonsense code ...");
         } catch (py::error_already_set &) {
         } catch (py::error_already_set &) {
-            PyErr_Clear();
             return true;
             return true;
         }
         }
         return false;
         return false;

tests/test_exceptions.cpp

@@ -66,6 +66,13 @@ void throws_logic_error() {
     throw std::logic_error("this error should fall through to the standard handler");
     throw std::logic_error("this error should fall through to the standard handler");
 }
 }
 
 
+struct PythonCallInDestructor {
+    PythonCallInDestructor(const py::dict &d) : d(d) {}
+    ~PythonCallInDestructor() { d["good"] = py::cast(true); }
+
+    py::dict d;
+};
+
 test_initializer custom_exceptions([](py::module &m) {
 test_initializer custom_exceptions([](py::module &m) {
     // make a new custom exception and use it as a translation target
     // make a new custom exception and use it as a translation target
     static py::exception<MyException> ex(m, "MyException");
     static py::exception<MyException> ex(m, "MyException");
@@ -123,4 +130,15 @@ test_initializer custom_exceptions([](py::module &m) {
             PyErr_SetString(PyExc_ValueError, "foo");
             PyErr_SetString(PyExc_ValueError, "foo");
         throw py::error_already_set();
         throw py::error_already_set();
     });
     });
+
+    m.def("python_call_in_destructor", [](py::dict d) {
+        try {
+            PythonCallInDestructor set_dict_in_destructor(d);
+            PyErr_SetString(PyExc_ValueError, "foo");
+            throw py::error_already_set();
+        } catch (const py::error_already_set&) {
+            return true;
+        }
+        return false;
+    });
 });
 });

tests/test_exceptions.py

@@ -13,6 +13,14 @@ def test_error_already_set(msg):
     assert msg(excinfo.value) == "foo"
     assert msg(excinfo.value) == "foo"
 
 
 
 
+def test_python_call_in_catch():
+    from pybind11_tests import python_call_in_destructor
+
+    d = {}
+    assert python_call_in_destructor(d) is True
+    assert d["good"] is True
+
+
 def test_custom(msg):
 def test_custom(msg):
     from pybind11_tests import (MyException, throws1, throws2, throws3, throws4,
     from pybind11_tests import (MyException, throws1, throws2, throws3, throws4,
                                 throws_logic_error)
                                 throws_logic_error)