Merge branch 'pybind:master' into master

include/pybind11/detail/class.h

@@ -455,6 +455,8 @@ extern "C" inline void pybind11_object_dealloc(PyObject *self) {
 #endif
 }
 
+std::string error_string();
+
 /** Create the type which can be used as a common base for all classes.  This is
     needed in order to satisfy Python's requirements for multiple inheritance.
     Return value: New reference. */
@@ -490,7 +492,7 @@ inline PyObject *make_object_base_type(PyTypeObject *metaclass) {
     type->tp_weaklistoffset = offsetof(instance, weakrefs);
 
     if (PyType_Ready(type) < 0) {
-        pybind11_fail("PyType_Ready failed in make_object_base_type():" + error_string());
+        pybind11_fail("PyType_Ready failed in make_object_base_type(): " + error_string());
     }
 
     setattr((PyObject *) type, "__module__", str("pybind11_builtins"));
@@ -707,7 +709,7 @@ inline PyObject *make_new_python_type(const type_record &rec) {
     }
 
     if (PyType_Ready(type) < 0) {
-        pybind11_fail(std::string(rec.name) + ": PyType_Ready failed (" + error_string() + ")!");
+        pybind11_fail(std::string(rec.name) + ": PyType_Ready failed: " + error_string());
     }
 
     assert(!rec.dynamic_attr || PyType_HasFeature(type, Py_TPFLAGS_HAVE_GC));

include/pybind11/detail/type_caster_base.h

@@ -470,73 +470,6 @@ PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp)
     return isinstance(obj, type);
 }
 
-PYBIND11_NOINLINE std::string error_string(const char *called) {
-    error_scope scope; // Fetch error state (will be restored when this function returns).
-    if (scope.type == nullptr) {
-        if (called == nullptr) {
-            called = "pybind11::detail::error_string()";
-        }
-        pybind11_fail("Internal error: " + std::string(called)
-                      + " called while Python error indicator not set.");
-    }
-
-    PyErr_NormalizeException(&scope.type, &scope.value, &scope.trace);
-    if (scope.trace != nullptr) {
-        PyException_SetTraceback(scope.value, scope.trace);
-    }
-
-    std::string errorString;
-    if (scope.type) {
-        errorString += handle(scope.type).attr("__name__").cast<std::string>();
-        errorString += ": ";
-    }
-    if (scope.value) {
-        errorString += (std::string) str(scope.value);
-    }
-
-#if !defined(PYPY_VERSION)
-    if (scope.trace) {
-        auto *trace = (PyTracebackObject *) scope.trace;
-
-        /* Get the deepest trace possible */
-        while (trace->tb_next) {
-            trace = trace->tb_next;
-        }
-
-        PyFrameObject *frame = trace->tb_frame;
-        Py_XINCREF(frame);
-        errorString += "\n\nAt:\n";
-        while (frame) {
-#    if PY_VERSION_HEX >= 0x030900B1
-            PyCodeObject *f_code = PyFrame_GetCode(frame);
-#    else
-            PyCodeObject *f_code = frame->f_code;
-            Py_INCREF(f_code);
-#    endif
-            int lineno = PyFrame_GetLineNumber(frame);
-            errorString += "  ";
-            errorString += handle(f_code->co_filename).cast<std::string>();
-            errorString += '(';
-            errorString += std::to_string(lineno);
-            errorString += "): ";
-            errorString += handle(f_code->co_name).cast<std::string>();
-            errorString += '\n';
-            Py_DECREF(f_code);
-#    if PY_VERSION_HEX >= 0x030900B1
-            auto *b_frame = PyFrame_GetBack(frame);
-#    else
-            auto *b_frame = frame->f_back;
-            Py_XINCREF(b_frame);
-#    endif
-            Py_DECREF(frame);
-            frame = b_frame;
-        }
-    }
-#endif
-
-    return errorString;
-}
-
 PYBIND11_NOINLINE handle get_object_handle(const void *ptr, const detail::type_info *type) {
     auto &instances = get_internals().registered_instances;
     auto range = instances.equal_range(ptr);

include/pybind11/pybind11.h

@@ -2625,17 +2625,21 @@ void print(Args &&...args) {
     detail::print(c.args(), c.kwargs());
 }
 
-error_already_set::~error_already_set() {
-    if (m_type) {
+inline void
+error_already_set::m_fetched_error_deleter(detail::error_fetch_and_normalize *raw_ptr) {
     gil_scoped_acquire gil;
     error_scope scope;
-        m_type.release().dec_ref();
-        m_value.release().dec_ref();
-        m_trace.release().dec_ref();
-    }
+    delete raw_ptr;
+}
+
+inline const char *error_already_set::what() const noexcept {
+    gil_scoped_acquire gil;
+    error_scope scope;
+    return m_fetched_error->error_string().c_str();
 }
 
 PYBIND11_NAMESPACE_BEGIN(detail)
+
 inline function
 get_type_override(const void *this_ptr, const type_info *this_type, const char *name) {
     handle self = get_object_handle(this_ptr, this_type);

include/pybind11/pytypes.h

@@ -12,7 +12,15 @@
 #include "detail/common.h"
 #include "buffer_info.h"
 
+#include <assert.h>
+#include <cstddef>
+#include <exception>
+#include <frameobject.h>
+#include <iterator>
+#include <memory>
+#include <string>
 #include <type_traits>
+#include <typeinfo>
 #include <utility>
 
 #if defined(PYBIND11_HAS_OPTIONAL)
@@ -383,7 +391,175 @@ T reinterpret_steal(handle h) {
 }
 
 PYBIND11_NAMESPACE_BEGIN(detail)
-std::string error_string(const char *called = nullptr);
+
+// Equivalent to obj.__class__.__name__ (or obj.__name__ if obj is a class).
+inline const char *obj_class_name(PyObject *obj) {
+    if (Py_TYPE(obj) == &PyType_Type) {
+        return reinterpret_cast<PyTypeObject *>(obj)->tp_name;
+    }
+    return Py_TYPE(obj)->tp_name;
+}
+
+std::string error_string();
+
+struct error_fetch_and_normalize {
+    // Immediate normalization is long-established behavior (starting with
+    // https://github.com/pybind/pybind11/commit/135ba8deafb8bf64a15b24d1513899eb600e2011
+    // from Sep 2016) and safest. Normalization could be deferred, but this could mask
+    // errors elsewhere, the performance gain is very minor in typical situations
+    // (usually the dominant bottleneck is EH unwinding), and the implementation here
+    // would be more complex.
+    explicit error_fetch_and_normalize(const char *called) {
+        PyErr_Fetch(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr());
+        if (!m_type) {
+            pybind11_fail("Internal error: " + std::string(called)
+                          + " called while "
+                            "Python error indicator not set.");
+        }
+        const char *exc_type_name_orig = detail::obj_class_name(m_type.ptr());
+        if (exc_type_name_orig == nullptr) {
+            pybind11_fail("Internal error: " + std::string(called)
+                          + " failed to obtain the name "
+                            "of the original active exception type.");
+        }
+        m_lazy_error_string = exc_type_name_orig;
+        // PyErr_NormalizeException() may change the exception type if there are cascading
+        // failures. This can potentially be extremely confusing.
+        PyErr_NormalizeException(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr());
+        if (m_type.ptr() == nullptr) {
+            pybind11_fail("Internal error: " + std::string(called)
+                          + " failed to normalize the "
+                            "active exception.");
+        }
+        const char *exc_type_name_norm = detail::obj_class_name(m_type.ptr());
+        if (exc_type_name_orig == nullptr) {
+            pybind11_fail("Internal error: " + std::string(called)
+                          + " failed to obtain the name "
+                            "of the normalized active exception type.");
+        }
+        if (exc_type_name_norm != m_lazy_error_string) {
+            std::string msg = std::string(called)
+                              + ": MISMATCH of original and normalized "
+                                "active exception types: ";
+            msg += "ORIGINAL ";
+            msg += m_lazy_error_string;
+            msg += " REPLACED BY ";
+            msg += exc_type_name_norm;
+            msg += ": " + format_value_and_trace();
+            pybind11_fail(msg);
+        }
+    }
+
+    error_fetch_and_normalize(const error_fetch_and_normalize &) = delete;
+    error_fetch_and_normalize(error_fetch_and_normalize &&) = delete;
+
+    std::string format_value_and_trace() const {
+        std::string result;
+        std::string message_error_string;
+        if (m_value) {
+            auto value_str = reinterpret_steal<object>(PyObject_Str(m_value.ptr()));
+            if (!value_str) {
+                message_error_string = detail::error_string();
+                result = "<MESSAGE UNAVAILABLE DUE TO ANOTHER EXCEPTION>";
+            } else {
+                result = value_str.cast<std::string>();
+            }
+        } else {
+            result = "<MESSAGE UNAVAILABLE>";
+        }
+        if (result.empty()) {
+            result = "<EMPTY MESSAGE>";
+        }
+
+        bool have_trace = false;
+        if (m_trace) {
+#if !defined(PYPY_VERSION)
+            auto *tb = reinterpret_cast<PyTracebackObject *>(m_trace.ptr());
+
+            // Get the deepest trace possible.
+            while (tb->tb_next) {
+                tb = tb->tb_next;
+            }
+
+            PyFrameObject *frame = tb->tb_frame;
+            Py_XINCREF(frame);
+            result += "\n\nAt:\n";
+            while (frame) {
+#    if PY_VERSION_HEX >= 0x030900B1
+                PyCodeObject *f_code = PyFrame_GetCode(frame);
+#    else
+                PyCodeObject *f_code = frame->f_code;
+                Py_INCREF(f_code);
+#    endif
+                int lineno = PyFrame_GetLineNumber(frame);
+                result += "  ";
+                result += handle(f_code->co_filename).cast<std::string>();
+                result += '(';
+                result += std::to_string(lineno);
+                result += "): ";
+                result += handle(f_code->co_name).cast<std::string>();
+                result += '\n';
+                Py_DECREF(f_code);
+#    if PY_VERSION_HEX >= 0x030900B1
+                auto *b_frame = PyFrame_GetBack(frame);
+#    else
+                auto *b_frame = frame->f_back;
+                Py_XINCREF(b_frame);
+#    endif
+                Py_DECREF(frame);
+                frame = b_frame;
+            }
+
+            have_trace = true;
+#endif //! defined(PYPY_VERSION)
+        }
+
+        if (!message_error_string.empty()) {
+            if (!have_trace) {
+                result += '\n';
+            }
+            result += "\nMESSAGE UNAVAILABLE DUE TO EXCEPTION: " + message_error_string;
+        }
+
+        return result;
+    }
+
+    std::string const &error_string() const {
+        if (!m_lazy_error_string_completed) {
+            m_lazy_error_string += ": " + format_value_and_trace();
+            m_lazy_error_string_completed = true;
+        }
+        return m_lazy_error_string;
+    }
+
+    void restore() {
+        if (m_restore_called) {
+            pybind11_fail("Internal error: pybind11::detail::error_fetch_and_normalize::restore() "
+                          "called a second time. ORIGINAL ERROR: "
+                          + error_string());
+        }
+        PyErr_Restore(m_type.inc_ref().ptr(), m_value.inc_ref().ptr(), m_trace.inc_ref().ptr());
+        m_restore_called = true;
+    }
+
+    bool matches(handle exc) const {
+        return (PyErr_GivenExceptionMatches(m_type.ptr(), exc.ptr()) != 0);
+    }
+
+    // Not protecting these for simplicity.
+    object m_type, m_value, m_trace;
+
+private:
+    // Only protecting invariants.
+    mutable std::string m_lazy_error_string;
+    mutable bool m_lazy_error_string_completed = false;
+    mutable bool m_restore_called = false;
+};
+
+inline std::string error_string() {
+    return error_fetch_and_normalize("pybind11::detail::error_string").error_string();
+}
+
 PYBIND11_NAMESPACE_END(detail)
 
 #if defined(_MSC_VER)
@@ -396,39 +572,30 @@ PYBIND11_NAMESPACE_END(detail)
 /// thrown to propagate python-side errors back through C++ which can either be caught manually or
 /// else falls back to the function dispatcher (which then raises the captured error back to
 /// python).
-class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::runtime_error {
+class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::exception {
 public:
-    /// Constructs a new exception from the current Python error indicator.  The current
-    /// Python error indicator will be cleared.
-    error_already_set() : std::runtime_error(detail::error_string("pybind11::error_already_set")) {
-        PyErr_Fetch(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr());
-    }
+    /// Fetches the current Python exception (using PyErr_Fetch()), which will clear the
+    /// current Python error indicator.
+    error_already_set()
+        : m_fetched_error{new detail::error_fetch_and_normalize("pybind11::error_already_set"),
+                          m_fetched_error_deleter} {}
 
-    /// WARNING: The GIL must be held when this copy constructor is invoked!
-    error_already_set(const error_already_set &) = default;
-    error_already_set(error_already_set &&) = default;
-
-    /// WARNING: This destructor needs to acquire the Python GIL. This can lead to
+    /// The what() result is built lazily on demand.
+    /// WARNING: This member function needs to acquire the Python GIL. This can lead to
     ///          crashes (undefined behavior) if the Python interpreter is finalizing.
-    inline ~error_already_set() override;
+    const char *what() const noexcept override;
 
     /// Restores the currently-held Python error (which will clear the Python error indicator first
-    /// if already set). After this call, the current object no longer stores the error variables.
-    /// NOTE: Any copies of this object may still store the error variables. Currently there is no
-    //        protection against calling restore() from multiple copies.
+    /// if already set).
     /// NOTE: This member function will always restore the normalized exception, which may or may
     ///       not be the original Python exception.
     /// WARNING: The GIL must be held when this member function is called!
-    void restore() {
-        PyErr_Restore(m_type.release().ptr(), m_value.release().ptr(), m_trace.release().ptr());
-    }
+    void restore() { m_fetched_error->restore(); }
 
     /// If it is impossible to raise the currently-held error, such as in a destructor, we can
     /// write it out using Python's unraisable hook (`sys.unraisablehook`). The error context
     /// should be some object whose `repr()` helps identify the location of the error. Python
-    /// already knows the type and value of the error, so there is no need to repeat that. After
-    /// this call, the current object no longer stores the error variables, and neither does
-    /// Python.
+    /// already knows the type and value of the error, so there is no need to repeat that.
     void discard_as_unraisable(object err_context) {
         restore();
         PyErr_WriteUnraisable(err_context.ptr());
@@ -447,16 +614,18 @@ public:
     /// Check if the currently trapped error type matches the given Python exception class (or a
     /// subclass thereof).  May also be passed a tuple to search for any exception class matches in
     /// the given tuple.
-    bool matches(handle exc) const {
-        return (PyErr_GivenExceptionMatches(m_type.ptr(), exc.ptr()) != 0);
-    }
+    bool matches(handle exc) const { return m_fetched_error->matches(exc); }
 
-    const object &type() const { return m_type; }
-    const object &value() const { return m_value; }
-    const object &trace() const { return m_trace; }
+    const object &type() const { return m_fetched_error->m_type; }
+    const object &value() const { return m_fetched_error->m_value; }
+    const object &trace() const { return m_fetched_error->m_trace; }
 
 private:
-    object m_type, m_value, m_trace;
+    std::shared_ptr<detail::error_fetch_and_normalize> m_fetched_error;
+
+    /// WARNING: This custom deleter needs to acquire the Python GIL. This can lead to
+    ///          crashes (undefined behavior) if the Python interpreter is finalizing.
+    static void m_fetched_error_deleter(detail::error_fetch_and_normalize *raw_ptr);
 };
 #if defined(_MSC_VER)
 #    pragma warning(pop)
@@ -492,8 +661,7 @@ inline void raise_from(PyObject *type, const char *message) {
 
 /// Sets the current Python error indicator with the chosen error, performing a 'raise from'
 /// from the error contained in error_already_set to indicate that the chosen error was
-/// caused by the original error. After this function is called error_already_set will
-/// no longer contain an error.
+/// caused by the original error.
 inline void raise_from(error_already_set &err, PyObject *type, const char *message) {
     err.restore();
     raise_from(type, message);

tests/test_exceptions.cpp

@@ -105,6 +105,11 @@ struct PythonAlreadySetInDestructor {
     py::str s;
 };
 
+std::string error_already_set_what(const py::object &exc_type, const py::object &exc_value) {
+    PyErr_SetObject(exc_type.ptr(), exc_value.ptr());
+    return py::error_already_set().what();
+}
+
 TEST_SUBMODULE(exceptions, m) {
     m.def("throw_std_exception",
           []() { throw std::runtime_error("This exception was intentionally thrown."); });
@@ -269,7 +274,9 @@ TEST_SUBMODULE(exceptions, m) {
                   if (ex.matches(exc_type)) {
                       py::print(ex.what());
                   } else {
-                      throw;
+                      // Simply `throw;` also works and is better, but using `throw ex;`
+                      // here to cover that situation (as observed in the wild).
+                      throw ex; // Invokes the copy ctor.
                   }
               }
           });
@@ -317,4 +324,14 @@ TEST_SUBMODULE(exceptions, m) {
             = reinterpret_cast<void (*)()>(PyLong_AsVoidPtr(cm.attr("funcaddr").ptr()));
         interleaved_error_already_set();
     });
+
+    m.def("test_error_already_set_double_restore", [](bool dry_run) {
+        PyErr_SetString(PyExc_ValueError, "Random error.");
+        py::error_already_set e;
+        e.restore();
+        PyErr_Clear();
+        if (!dry_run) {
+            e.restore();
+        }
+    });
 }

tests/test_exceptions.py

@@ -305,13 +305,16 @@ def test_error_already_set_what_with_happy_exceptions(
 
 @pytest.mark.skipif("env.PYPY", reason="PyErr_NormalizeException Segmentation fault")
 def test_flaky_exception_failure_point_init():
-    what, py_err_set_after_what = m.error_already_set_what(
-        FlakyException, ("failure_point_init",)
-    )
-    assert not py_err_set_after_what
-    lines = what.splitlines()
+    with pytest.raises(RuntimeError) as excinfo:
+        m.error_already_set_what(FlakyException, ("failure_point_init",))
+    lines = str(excinfo.value).splitlines()
     # PyErr_NormalizeException replaces the original FlakyException with ValueError:
-    assert lines[:3] == ["ValueError: triggered_failure_point_init", "", "At:"]
+    assert lines[:3] == [
+        "pybind11::error_already_set: MISMATCH of original and normalized active exception types:"
+        " ORIGINAL FlakyException REPLACED BY ValueError: triggered_failure_point_init",
+        "",
+        "At:",
+    ]
     # Checking the first two lines of the traceback as formatted in error_string():
     assert "test_exceptions.py(" in lines[3]
     assert lines[3].endswith("): __init__")
@@ -319,10 +322,25 @@ def test_flaky_exception_failure_point_init():
 
 
 def test_flaky_exception_failure_point_str():
-    # The error_already_set ctor fails due to a ValueError in error_string():
-    with pytest.raises(ValueError) as excinfo:
-        m.error_already_set_what(FlakyException, ("failure_point_str",))
-    assert str(excinfo.value) == "triggered_failure_point_str"
+    what, py_err_set_after_what = m.error_already_set_what(
+        FlakyException, ("failure_point_str",)
+    )
+    assert not py_err_set_after_what
+    lines = what.splitlines()
+    if env.PYPY and len(lines) == 3:
+        n = 3  # Traceback is missing.
+    else:
+        n = 5
+    assert (
+        lines[:n]
+        == [
+            "FlakyException: <MESSAGE UNAVAILABLE DUE TO ANOTHER EXCEPTION>",
+            "",
+            "MESSAGE UNAVAILABLE DUE TO EXCEPTION: ValueError: triggered_failure_point_str",
+            "",
+            "At:",
+        ][:n]
+    )
 
 
 def test_cross_module_interleaved_error_already_set():
@@ -332,3 +350,13 @@ def test_cross_module_interleaved_error_already_set():
         "2nd error.",  # Almost all platforms.
         "RuntimeError: 2nd error.",  # Some PyPy builds (seen under macOS).
     )
+
+
+def test_error_already_set_double_restore():
+    m.test_error_already_set_double_restore(True)  # dry_run
+    with pytest.raises(RuntimeError) as excinfo:
+        m.test_error_already_set_double_restore(False)
+    assert str(excinfo.value) == (
+        "Internal error: pybind11::detail::error_fetch_and_normalize::restore()"
+        " called a second time. ORIGINAL ERROR: ValueError: Random error."
+    )