Replace "Unknown internal error occurred" with a more helpful message. (#3982)

Author: rwgk

include/pybind11/detail/type_caster_base.h

@@ -470,14 +470,16 @@ PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp)
     return isinstance(obj, type);
 }
 
-PYBIND11_NOINLINE std::string error_string() {
-    if (!PyErr_Occurred()) {
-        PyErr_SetString(PyExc_RuntimeError, "Unknown internal error occurred");
-        return "Unknown internal error occurred";
+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.");
     }
 
-    error_scope scope; // Preserve error state
-
     PyErr_NormalizeException(&scope.type, &scope.value, &scope.trace);
     if (scope.trace != nullptr) {
         PyException_SetTraceback(scope.value, scope.trace);

include/pybind11/pytypes.h

@@ -360,7 +360,7 @@ T reinterpret_steal(handle h) {
 }
 
 PYBIND11_NAMESPACE_BEGIN(detail)
-std::string error_string();
+std::string error_string(const char *called = nullptr);
 PYBIND11_NAMESPACE_END(detail)
 
 #if defined(_MSC_VER)
@@ -375,20 +375,27 @@ PYBIND11_NAMESPACE_END(detail)
 /// python).
 class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::runtime_error {
 public:
-    /// Constructs a new exception from the current Python error indicator, if any.  The current
+    /// 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()) {
+    error_already_set() : std::runtime_error(detail::error_string("pybind11::error_already_set")) {
         PyErr_Fetch(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr());
     }
 
+    /// 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
+    ///          crashes (undefined behavior) if the Python interpreter is finalizing.
     inline ~error_already_set() override;
 
-    /// Give the currently-held error back to Python, if any.  If there is currently a Python error
-    /// already set it is cleared first.  After this call, the current object no longer stores the
-    /// error variables (but the `.what()` string is still available).
+    /// 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.
+    /// 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());
     }
@@ -405,6 +412,7 @@ class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::runtime_error {
     }
     /// An alternate version of `discard_as_unraisable()`, where a string provides information on
     /// the location of the error. For example, `__func__` could be helpful.
+    /// WARNING: The GIL must be held when this member function is called!
     void discard_as_unraisable(const char *err_context) {
         discard_as_unraisable(reinterpret_steal<object>(PYBIND11_FROM_STRING(err_context)));
     }

tests/test_exceptions.cpp

@@ -228,7 +228,10 @@ TEST_SUBMODULE(exceptions, m) {
             throw py::error_already_set();
         } catch (const std::runtime_error &e) {
             if ((err && e.what() != std::string("ValueError: foo"))
-                || (!err && e.what() != std::string("Unknown internal error occurred"))) {
+                || (!err
+                    && e.what()
+                           != std::string("Internal error: pybind11::error_already_set called "
+                                          "while Python error indicator not set."))) {
                 PyErr_Clear();
                 throw std::runtime_error("error message mismatch");
             }

tests/test_exceptions.py

@@ -16,7 +16,10 @@ def test_std_exception(msg):
 def test_error_already_set(msg):
     with pytest.raises(RuntimeError) as excinfo:
         m.throw_already_set(False)
-    assert msg(excinfo.value) == "Unknown internal error occurred"
+    assert (
+        msg(excinfo.value)
+        == "Internal error: pybind11::error_already_set called while Python error indicator not set."
+    )
 
     with pytest.raises(ValueError) as excinfo:
         m.throw_already_set(True)