Rewrite and simplify error_already_set

`error_already_set` is more complicated than it needs to be, partly
because it manages reference counts itself rather than using
`py::object`, and partly because it tries to do more exception clearing
than is needed.  This commit greatly simplifies it, and fixes pybind#927.

Using `py::object` instead of `PyObject *` means we can rely on
implicit copy/move constructors.

The current logic did both a `PyErr_Clear` on deletion *and* a
`PyErr_Fetch` on creation.  I can't see how the `PyErr_Clear` on
deletion is ever useful: the `Fetch` on creation itself clears the
error, so the only way doing a `PyErr_Clear` on deletion could do
anything if is some *other* exception was raised while the
`error_already_set` object was alive--but in that case, clearing some
other exception seems wrong.  (Code that is worried about an exception
handler raising another exception would already catch a second
`error_already_set` from exception code).

The destructor itself called `clear()`, but `clear()` was a little bit
convoluted: it called `restore()` to restore the currently captured
error, but then immediately cleared it.  The only point seems to be
because `PyErr_Restore` decrements the reference counts on the held
objects, but we can simply do that manually.  Or, actually, just *not*
decrement them at all and let them be unreferenced (via the `py::object`
destructor) when the `error_already_set` is itself destroyed.

`clear()`, however, also had the side effect of clearing the current
error, even if the current `error_already_set` didn't have a current
error (e.g. because of a previous `restore()` or `clear()` call).  I
don't really see how clearing the error here can ever actually be
useful: the only way the current error could be set is if you called
`restore()` (in which case the current stored error-related members have
already been released), or if some *other* code raised the error, in
which case `clear()` on *this* object is clearing an error for which it
shouldn't be responsible.

Neither of those seem like intentional or desirable features, and
manually requesting deletion of the stored references similarly seems
pointless, so I've just made `clear()` an empty method and marked it
deprecated.

Author: jagerman

include/pybind11/common.h

@@ -226,7 +226,6 @@ extern "C" {
         try {                                                                  \
             return pybind11_init();                                            \
         } catch (pybind11::error_already_set &e) {                             \
-            e.clear();                                                         \
             PyErr_SetString(PyExc_ImportError, e.what());                      \
             return nullptr;                                                    \
         } catch (const std::exception &e) {                                    \
@@ -273,7 +272,6 @@ extern "C" {
             pybind11_init_##name(m);                                           \
             return m.ptr();                                                    \
         } catch (pybind11::error_already_set &e) {                             \
-            e.clear();                                                         \
             PyErr_SetString(PyExc_ImportError, e.what());                      \
             return nullptr;                                                    \
         } catch (const std::exception &e) {                                    \
@@ -348,8 +346,6 @@ inline static constexpr int log2(size_t n, int k = 0) { return (n <= 1) ? k : lo
 // Returns the size as a multiple of sizeof(void *), rounded up.
 inline static constexpr size_t size_in_ptrs(size_t s) { return 1 + ((s - 1) >> log2(sizeof(void *))); }
 
-inline std::string error_string();
-
 /**
  * The space to allocate for simple layout instance holders (see below) in multiple of the size of
  * a pointer (e.g.  2 means 16 bytes on 64-bit architectures).  The default is the minimum required
@@ -698,36 +694,6 @@ template<typename T> T& get_or_create_shared_data(const std::string& name) {
     return *ptr;
 }
 
-/// 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(const error_already_set &) = delete;
-
-    error_already_set(error_already_set &&e)
-        : std::runtime_error(e.what()), type(e.type), value(e.value),
-          trace(e.trace) { e.type = e.value = e.trace = nullptr; }
-
-    inline ~error_already_set(); // implementation in pybind11.h
-
-    error_already_set& operator=(const error_already_set &) = delete;
-
-    /// Give the error back to Python
-    void restore() { PyErr_Restore(type, value, trace); type = value = trace = nullptr; }
-
-    /// Clear the held Python error state (the C++ `what()` message remains intact)
-    void clear() { restore(); PyErr_Clear(); }
-
-    /// Check if the trapped exception matches a given Python exception class
-    bool matches(PyObject *ex) const { return PyErr_GivenExceptionMatches(ex, type); }
-
-private:
-    PyObject *type, *value, *trace;
-};
-
 /// C++ bindings of builtin Python exceptions
 class builtin_exception : public std::runtime_error {
 public:

include/pybind11/embed.h

@@ -51,7 +51,6 @@
             pybind11_init_##name(m);                                          \
             return m.ptr();                                                   \
         } catch (pybind11::error_already_set &e) {                            \
-            e.clear();                                                        \
             PyErr_SetString(PyExc_ImportError, e.what());                     \
             return nullptr;                                                   \
         } catch (const std::exception &e) {                                   \

include/pybind11/pybind11.h

@@ -1745,13 +1745,6 @@ class gil_scoped_acquire { };
 class gil_scoped_release { };
 #endif
 
-error_already_set::~error_already_set() {
-    if (value) {
-        gil_scoped_acquire gil;
-        clear();
-    }
-}
-
 inline function get_type_overload(const void *this_ptr, const detail::type_info *this_type, const char *name)  {
     handle self = detail::get_object_handle(this_ptr, this_type);
     if (!self)

include/pybind11/pytypes.h

@@ -279,6 +279,31 @@ template <typename T> T reinterpret_borrow(handle h) { return {h, object::borrow
 \endrst */
 template <typename T> T reinterpret_steal(handle h) { return {h, object::stolen_t{}}; }
 
+NAMESPACE_BEGIN(detail)
+inline std::string error_string();
+NAMESPACE_END(detail)
+
+/// 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.ptr(), &value.ptr(), &trace.ptr());
+    }
+
+    /// Give the error back to Python
+    void restore() { PyErr_Restore(type.release().ptr(), value.release().ptr(), trace.release().ptr()); }
+
+    /// Does nothing; provided for backwards compatibility.
+    PYBIND11_DEPRECATED("Use of error_already_set.clear() is deprecated")
+    void clear() {}
+
+    /// Check if the trapped exception matches a given Python exception class
+    bool matches(handle ex) const { return PyErr_GivenExceptionMatches(ex.ptr(), type.ptr()); }
+
+private:
+    object type, value, trace;
+};
+
 /** \defgroup python_builtins _
     Unless stated otherwise, the following C++ functions behave the same
     as their Python counterparts.

tests/test_exceptions.cpp

@@ -93,9 +93,7 @@ void exception_matches() {
         foo["bar"];
     }
     catch (py::error_already_set& ex) {
-        if (ex.matches(PyExc_KeyError))
-            ex.clear();
-        else
+        if (!ex.matches(PyExc_KeyError))
             throw;
     }
 }