From e564142bb5e2c324e581a7bac8371f0c0ff5dc36 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 24 May 2022 22:50:40 -0700 Subject: [PATCH 1/5] Add error_already_set_what what tests, asserting the status quo. --- tests/test_exceptions.cpp | 8 ++++++ tests/test_exceptions.py | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 3e9a3d771e..577cdea88e 100644 --- a/tests/test_exceptions.cpp +++ b/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(what, py_err_set_after_what); + }); } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 0be61804a6..2dcd80b21d 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -270,3 +270,54 @@ 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: ('happy',)"), + ), +) +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 + + +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] == ["FlakyException: ('failure_point_init',)", "", "At:"] + # Checking the first two lines of the traceback as formatted in error_string(), + # which is actually for a different exception (ValueError)! + 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(): + what, py_err_set_after_what = m.error_already_set_what( + FlakyException, ("failure_point_str",) + ) + assert not py_err_set_after_what + assert what == "FlakyException: ('failure_point_str',)" From 254bfeb2a0f906b523c62825999d1ab8a2a4da7c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 24 May 2022 23:09:00 -0700 Subject: [PATCH 2/5] Move PyErr_NormalizeException() up a few lines. --- include/pybind11/detail/type_caster_base.h | 5 ++--- tests/test_exceptions.py | 18 ++++++++---------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index a7b9771326..6dea2733a4 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -478,6 +478,8 @@ PYBIND11_NOINLINE std::string error_string() { error_scope scope; // Preserve error state + PyErr_NormalizeException(&scope.type, &scope.value, &scope.trace); + std::string errorString; if (scope.type) { errorString += handle(scope.type).attr("__name__").cast(); @@ -486,9 +488,6 @@ PYBIND11_NOINLINE std::string error_string() { if (scope.value) { errorString += (std::string) str(scope.value); } - - PyErr_NormalizeException(&scope.type, &scope.value, &scope.trace); - if (scope.trace != nullptr) { PyException_SetTraceback(scope.value, scope.trace); } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 2dcd80b21d..ca19caf3e7 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -288,8 +288,8 @@ def __str__(self): "exc_type, exc_value, expected_what", ( (ValueError, "plain_str", "ValueError: plain_str"), - (ValueError, ("tuple_elem",), "ValueError: ('tuple_elem',)"), - (FlakyException, ("happy",), "FlakyException: ('happy',)"), + (ValueError, ("tuple_elem",), "ValueError: tuple_elem"), + (FlakyException, ("happy",), "FlakyException: FlakyException.__str__"), ), ) def test_error_already_set_what_with_happy_exceptions( @@ -307,17 +307,15 @@ def test_flaky_exception_failure_point_init(): assert not py_err_set_after_what lines = what.splitlines() # PyErr_NormalizeException replaces the original FlakyException with ValueError: - assert lines[:3] == ["FlakyException: ('failure_point_init',)", "", "At:"] - # Checking the first two lines of the traceback as formatted in error_string(), - # which is actually for a different exception (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(): - what, py_err_set_after_what = m.error_already_set_what( - FlakyException, ("failure_point_str",) - ) - assert not py_err_set_after_what - assert what == "FlakyException: ('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" From 98571b6df6b76291dcab22dd58c14adbbf7327ce Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 24 May 2022 23:56:18 -0700 Subject: [PATCH 3/5] @pytest.mark.skipif("env.PYPY") from PR #1895 is required even for this much simpler PR --- tests/test_exceptions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index ca19caf3e7..dc75941134 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -300,6 +300,7 @@ def test_error_already_set_what_with_happy_exceptions( 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",) From 4f080471bc077bce7658fc3dbee2dd5ef0d8a2e1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 25 May 2022 12:12:03 -0700 Subject: [PATCH 4/5] Move PyException_SetTraceback() with PyErr_NormalizeException() as suggested by @skylion007 --- include/pybind11/detail/type_caster_base.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 6dea2733a4..14561759b6 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -479,6 +479,9 @@ 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) { @@ -488,9 +491,6 @@ PYBIND11_NOINLINE std::string error_string() { if (scope.value) { errorString += (std::string) str(scope.value); } - if (scope.trace != nullptr) { - PyException_SetTraceback(scope.value, scope.trace); - } #if !defined(PYPY_VERSION) if (scope.trace) { From b816c325b0008820b53baea8e61f2b452560d434 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 25 May 2022 12:37:27 -0700 Subject: [PATCH 5/5] Insert a std::move() as suggested by @skylion007 --- tests/test_exceptions.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 577cdea88e..d7b31cf744 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -305,6 +305,6 @@ TEST_SUBMODULE(exceptions, m) { std::string what = py::error_already_set().what(); bool py_err_set_after_what = (PyErr_Occurred() != nullptr); PyErr_Clear(); - return py::make_tuple(what, py_err_set_after_what); + return py::make_tuple(std::move(what), py_err_set_after_what); }); }