Move PyErr_NormalizeException() up a few lines (#3971)

* Add error_already_set_what what tests, asserting the status quo.

* Move PyErr_NormalizeException() up a few lines.

* @pytest.mark.skipif("env.PYPY") from PR #1895 is required even for this much simpler PR

* Move PyException_SetTraceback() with PyErr_NormalizeException() as suggested by @Skylion007

* Insert a std::move() as suggested by @Skylion007

Author: rwgk

include/pybind11/detail/type_caster_base.h

@@ -478,6 +478,11 @@ PYBIND11_NOINLINE std::string error_string() {
 
     error_scope scope; // Preserve error state
 
+    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>();
@@ -487,12 +492,6 @@ PYBIND11_NOINLINE std::string error_string() {
         errorString += (std::string) str(scope.value);
     }
 
-    PyErr_NormalizeException(&scope.type, &scope.value, &scope.trace);
-
-    if (scope.trace != nullptr) {
-        PyException_SetTraceback(scope.value, scope.trace);
-    }
-
 #if !defined(PYPY_VERSION)
     if (scope.trace) {
         auto *trace = (PyTracebackObject *) scope.trace;

tests/test_exceptions.cpp

@@ -299,4 +299,12 @@ TEST_SUBMODULE(exceptions, m) {
             std::throw_with_nested(std::runtime_error("Outer Exception"));
         }
     });
+
+    m.def("error_already_set_what", [](const py::object &exc_type, const py::object &exc_value) {
+        PyErr_SetObject(exc_type.ptr(), exc_value.ptr());
+        std::string what = py::error_already_set().what();
+        bool py_err_set_after_what = (PyErr_Occurred() != nullptr);
+        PyErr_Clear();
+        return py::make_tuple(std::move(what), py_err_set_after_what);
+    });
 }

tests/test_exceptions.py

@@ -270,3 +270,53 @@ def test_local_translator(msg):
         m.throws_local_simple_error()
     assert not isinstance(excinfo.value, cm.LocalSimpleException)
     assert msg(excinfo.value) == "this mod"
+
+
+class FlakyException(Exception):
+    def __init__(self, failure_point):
+        if failure_point == "failure_point_init":
+            raise ValueError("triggered_failure_point_init")
+        self.failure_point = failure_point
+
+    def __str__(self):
+        if self.failure_point == "failure_point_str":
+            raise ValueError("triggered_failure_point_str")
+        return "FlakyException.__str__"
+
+
+@pytest.mark.parametrize(
+    "exc_type, exc_value, expected_what",
+    (
+        (ValueError, "plain_str", "ValueError: plain_str"),
+        (ValueError, ("tuple_elem",), "ValueError: tuple_elem"),
+        (FlakyException, ("happy",), "FlakyException: FlakyException.__str__"),
+    ),
+)
+def test_error_already_set_what_with_happy_exceptions(
+    exc_type, exc_value, expected_what
+):
+    what, py_err_set_after_what = m.error_already_set_what(exc_type, exc_value)
+    assert not py_err_set_after_what
+    assert what == expected_what
+
+
+@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()
+    # PyErr_NormalizeException replaces the original FlakyException with ValueError:
+    assert lines[:3] == ["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__")
+    assert lines[4].endswith("): 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"