From 970ac197e3b7ea0f4798a191b0275f8691a65dfa Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 9 Jan 2024 14:36:44 -0800 Subject: [PATCH] Change pybind11 handling of callbacks with `Status`, `StatusOr` returns to capture C++ exceptions. This change builds on https://github.com/google/pywrapcc/pull/30022. PiperOrigin-RevId: 597047935 --- pybind11_abseil/BUILD | 2 + pybind11_abseil/CMakeLists.txt | 6 +- pybind11_abseil/status_caster.h | 45 ++++++ pybind11_abseil/statusor_caster.h | 42 +++++ pybind11_abseil/tests/BUILD | 27 +++- .../tests/status_testing_no_cpp_eh_lib.h | 50 ++++++ .../tests/status_testing_no_cpp_eh_pybind.cc | 29 ++++ .../tests/status_testing_no_cpp_eh_test.py | 28 ++++ .../status_testing_no_cpp_eh_test_lib.py | 145 ++++++++++++++++++ 9 files changed, 371 insertions(+), 3 deletions(-) create mode 100644 pybind11_abseil/tests/status_testing_no_cpp_eh_lib.h create mode 100644 pybind11_abseil/tests/status_testing_no_cpp_eh_pybind.cc create mode 100644 pybind11_abseil/tests/status_testing_no_cpp_eh_test.py create mode 100644 pybind11_abseil/tests/status_testing_no_cpp_eh_test_lib.py diff --git a/pybind11_abseil/BUILD b/pybind11_abseil/BUILD index 5e45fda..4cff8b7 100644 --- a/pybind11_abseil/BUILD +++ b/pybind11_abseil/BUILD @@ -84,6 +84,7 @@ pybind_library( ":no_throw_status", ":ok_status_singleton_lib", ":status_not_ok_exception", + "//pybind11_abseil/compat:status_from_py_exc", "//pybind11_abseil/cpp_capsule_tools:raw_ptr_from_capsule", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", @@ -97,6 +98,7 @@ pybind_library( ":check_status_module_imported", ":no_throw_status", ":status_caster", + "//pybind11_abseil/compat:status_from_py_exc", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", ], diff --git a/pybind11_abseil/CMakeLists.txt b/pybind11_abseil/CMakeLists.txt index 9c20ed3..d1b7483 100644 --- a/pybind11_abseil/CMakeLists.txt +++ b/pybind11_abseil/CMakeLists.txt @@ -91,6 +91,7 @@ target_link_libraries( INTERFACE check_status_module_imported no_throw_status ok_status_singleton_lib + status_from_py_exc status_not_ok_exception raw_ptr_from_capsule absl::status @@ -105,8 +106,9 @@ target_include_directories(statusor_caster INTERFACE $) target_link_libraries( - statusor_caster INTERFACE check_status_module_imported no_throw_status - status_caster absl::status absl::statusor) + statusor_caster + INTERFACE check_status_module_imported no_throw_status status_caster + status_from_py_exc absl::status absl::statusor) # init_from_tag ================================================================ diff --git a/pybind11_abseil/status_caster.h b/pybind11_abseil/status_caster.h index 128ef81..eef6aba 100644 --- a/pybind11_abseil/status_caster.h +++ b/pybind11_abseil/status_caster.h @@ -5,6 +5,7 @@ #ifndef PYBIND11_ABSEIL_STATUS_CASTER_H_ #define PYBIND11_ABSEIL_STATUS_CASTER_H_ +#include #include #include @@ -13,6 +14,7 @@ #include "absl/status/status.h" #include "pybind11_abseil/check_status_module_imported.h" +#include "pybind11_abseil/compat/status_from_py_exc.h" #include "pybind11_abseil/cpp_capsule_tools/raw_ptr_from_capsule.h" #include "pybind11_abseil/no_throw_status.h" #include "pybind11_abseil/ok_status_singleton_lib.h" @@ -118,6 +120,49 @@ struct type_caster : public type_caster_base { } }; +#if defined(PYBIND11_HAS_RETURN_VALUE_POLICY_PACK) + +// This code requires https://github.com/google/pywrapcc +// IMPORTANT: +// KEEP +// type_caster +// func_wrapper +// IN THE SAME HEADER FILE +// to avoid surprising behavior differences and ODR violations. + +namespace type_caster_std_function_specializations { + +template +struct func_wrapper : func_wrapper_base { + using func_wrapper_base::func_wrapper_base; + // NOTE: `noexcept` to guarantee that no C++ exception will ever escape. + absl::Status operator()(Args... args) const noexcept { + gil_scoped_acquire acq; + try { + object py_result = + hfunc.f.call_with_policies(rvpp, std::forward(args)...); + try { + return py_result.template cast(); + } catch (cast_error& e) { + return absl::Status(absl::StatusCode::kInvalidArgument, e.what()); + } + } + // All exceptions derived from std::exception are handled here: + // https://github.com/pybind/pybind11/blob/aec6cc5406edb076f5a489c2d7f84bb07052c4a3/include/pybind11/detail/internals.h#L363-L420 + // Design choice for safety: Intentionally no `catch (...)`: + // Occurrence of such exceptions in this context is considered a bug in + // user code. The `noexcept` above will lead to process termination. + catch (error_already_set& e) { + e.restore(); + return pybind11_abseil::compat::StatusFromPyExcGivenErrOccurred(); + } + } +}; + +} // namespace type_caster_std_function_specializations + +#endif + } // namespace detail } // namespace pybind11 diff --git a/pybind11_abseil/statusor_caster.h b/pybind11_abseil/statusor_caster.h index d6573e4..d345715 100644 --- a/pybind11_abseil/statusor_caster.h +++ b/pybind11_abseil/statusor_caster.h @@ -5,7 +5,9 @@ #ifndef PYBIND11_ABSEIL_STATUSOR_CASTER_H_ #define PYBIND11_ABSEIL_STATUSOR_CASTER_H_ +#include #include +#include #include #include @@ -14,6 +16,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "pybind11_abseil/check_status_module_imported.h" +#include "pybind11_abseil/compat/status_from_py_exc.h" #include "pybind11_abseil/no_throw_status.h" #include "pybind11_abseil/status_caster.h" @@ -121,6 +124,45 @@ struct type_caster> { } }; +#if defined(PYBIND11_HAS_RETURN_VALUE_POLICY_PACK) + +// This code requires https://github.com/google/pywrapcc +// IMPORTANT: +// KEEP +// type_caster> +// func_wrapper, Args...> +// IN THE SAME HEADER FILE +// to avoid surprising behavior differences and ODR violations. + +namespace type_caster_std_function_specializations { + +template +struct func_wrapper, Args...> : func_wrapper_base { + using func_wrapper_base::func_wrapper_base; + // NOTE: `noexcept` to guarantee that no C++ exception will ever escape. + absl::StatusOr operator()(Args... args) const noexcept { + gil_scoped_acquire acq; + try { + object py_result = + hfunc.f.call_with_policies(rvpp, std::forward(args)...); + try { + return py_result.template cast>(); + } catch (cast_error& e) { + return absl::Status(absl::StatusCode::kInvalidArgument, e.what()); + } + } + // See comment for the corresponding `catch` in status_caster.h. + catch (error_already_set& e) { + e.restore(); + return pybind11_abseil::compat::StatusFromPyExcGivenErrOccurred(); + } + } +}; + +} // namespace type_caster_std_function_specializations + +#endif + } // namespace detail } // namespace pybind11 diff --git a/pybind11_abseil/tests/BUILD b/pybind11_abseil/tests/BUILD index 4f69775..2f25adf 100644 --- a/pybind11_abseil/tests/BUILD +++ b/pybind11_abseil/tests/BUILD @@ -1,6 +1,5 @@ # Tests and examples for pybind11_abseil. -# LOAD(pytype_strict_contrib_test) load("@pybind11_bazel//:build_defs.bzl", "pybind_extension") licenses(["notice"]) @@ -24,6 +23,32 @@ py_test( srcs_version = "PY3", ) +cc_library( + name = "status_testing_no_cpp_eh_lib", + hdrs = ["status_testing_no_cpp_eh_lib.h"], + deps = [ + "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", + "@local_config_python//:python_headers", # buildcleaner: keep + ], +) + +pybind_extension( + name = "status_testing_no_cpp_eh_pybind", + srcs = ["status_testing_no_cpp_eh_pybind.cc"], + deps = [ + ":status_testing_no_cpp_eh_lib", + "//pybind11_abseil:import_status_module", + "//pybind11_abseil:status_caster", + "//pybind11_abseil:statusor_caster", + ], +) + +py_library( + name = "status_testing_no_cpp_eh_test_lib", + srcs = ["status_testing_no_cpp_eh_test_lib.py"], +) + pybind_extension( name = "absl_example", srcs = ["absl_example.cc"], diff --git a/pybind11_abseil/tests/status_testing_no_cpp_eh_lib.h b/pybind11_abseil/tests/status_testing_no_cpp_eh_lib.h new file mode 100644 index 0000000..486f083 --- /dev/null +++ b/pybind11_abseil/tests/status_testing_no_cpp_eh_lib.h @@ -0,0 +1,50 @@ +// This code is meant to be built with C++ exception handling disabled: +// the whole point of absl::Status, absl::StatusOr is to provide an alternative +// to C++ exception handling. + +#ifndef PYBIND11_ABSEIL_TESTS_STATUS_TESTING_NO_CPP_EH_LIB_H_ +#define PYBIND11_ABSEIL_TESTS_STATUS_TESTING_NO_CPP_EH_LIB_H_ + +#include + +#include +#include + +#include "absl/status/status.h" +#include "absl/status/statusor.h" + +namespace pybind11_abseil_tests { +namespace status_testing_no_cpp_eh { + +inline std::string CallCallbackWithStatusReturn( + const std::function &cb) { + absl::Status cb_return_value = cb(); + return cb_return_value.ToString(); +} + +inline std::string CallCallbackWithStatusOrIntReturn( + const std::function()> &cb) { + absl::StatusOr cb_return_value = cb(); + if (cb_return_value.ok()) { + return std::to_string(cb_return_value.value()); + } + return cb_return_value.status().ToString(); +} + +inline PyObject *CallCallbackWithStatusOrObjectReturn( + const std::function()> &cb) { + absl::StatusOr cb_return_value = cb(); + if (cb_return_value.ok()) { + return cb_return_value.value(); + } + return PyUnicode_FromString(cb_return_value.status().ToString().c_str()); +} + +inline absl::Status GenerateErrorStatusNotOk() { + return absl::AlreadyExistsError("Something went wrong, again."); +} + +} // namespace status_testing_no_cpp_eh +} // namespace pybind11_abseil_tests + +#endif // PYBIND11_ABSEIL_TESTS_STATUS_TESTING_NO_CPP_EH_LIB_H_ diff --git a/pybind11_abseil/tests/status_testing_no_cpp_eh_pybind.cc b/pybind11_abseil/tests/status_testing_no_cpp_eh_pybind.cc new file mode 100644 index 0000000..3fd5b9f --- /dev/null +++ b/pybind11_abseil/tests/status_testing_no_cpp_eh_pybind.cc @@ -0,0 +1,29 @@ +#if true // go/pybind11_include_order +#include +#endif + +#include +#include + +#include "pybind11_abseil/import_status_module.h" +#include "pybind11_abseil/status_caster.h" +#include "pybind11_abseil/statusor_caster.h" +#include "pybind11_abseil/tests/status_testing_no_cpp_eh_lib.h" + +namespace pybind11_abseil_tests { +namespace status_testing_no_cpp_eh { + +PYBIND11_MODULE(status_testing_no_cpp_eh_pybind, m) { + pybind11::google::ImportStatusModule(); + + m.def("CallCallbackWithStatusReturn", &CallCallbackWithStatusReturn); + m.def("CallCallbackWithStatusOrIntReturn", + &CallCallbackWithStatusOrIntReturn); + m.def("CallCallbackWithStatusOrObjectReturn", + &CallCallbackWithStatusOrObjectReturn, + pybind11::return_value_policy::take_ownership); + m.def("GenerateErrorStatusNotOk", &GenerateErrorStatusNotOk); +} + +} // namespace status_testing_no_cpp_eh +} // namespace pybind11_abseil_tests diff --git a/pybind11_abseil/tests/status_testing_no_cpp_eh_test.py b/pybind11_abseil/tests/status_testing_no_cpp_eh_test.py new file mode 100644 index 0000000..fd78a5c --- /dev/null +++ b/pybind11_abseil/tests/status_testing_no_cpp_eh_test.py @@ -0,0 +1,28 @@ +from absl.testing import absltest + +from pybind11_abseil.tests import status_testing_no_cpp_eh_pybind +from pybind11_abseil.tests import status_testing_no_cpp_eh_test_lib as test_lib + + +class _TestModuleMixin: + + def getTestModule(self): # pylint: disable=invalid-name + return status_testing_no_cpp_eh_pybind + + +class StatusReturnTest(test_lib.StatusReturnTest, _TestModuleMixin): + pass + + +class StatusOrReturnTest(test_lib.StatusOrReturnTest, _TestModuleMixin): + pass + + +class StatusOrPyObjectPtrTest( + test_lib.StatusOrPyObjectPtrTest, _TestModuleMixin +): + pass + + +if __name__ == '__main__': + absltest.main() diff --git a/pybind11_abseil/tests/status_testing_no_cpp_eh_test_lib.py b/pybind11_abseil/tests/status_testing_no_cpp_eh_test_lib.py new file mode 100644 index 0000000..84ea302 --- /dev/null +++ b/pybind11_abseil/tests/status_testing_no_cpp_eh_test_lib.py @@ -0,0 +1,145 @@ +"""This is a py_library to enable testing with pybind11 & PyCLIF.""" + +# This is the default for py_test code: +# pylint: disable=missing-class-docstring +# pylint: disable=missing-function-docstring + +from absl.testing import absltest +from absl.testing import parameterized + +# Exercises status_from_core_py_exc.cc:StatusFromFetchedExc() +TAB_StatusFromFetchedExc = ( + (MemoryError, 'RESOURCE_EXHAUSTED: MemoryError'), + (NotImplementedError, 'UNIMPLEMENTED: NotImplementedError'), + (KeyboardInterrupt, 'ABORTED: KeyboardInterrupt'), + (SystemError, 'INTERNAL: SystemError'), + (SyntaxError, 'INTERNAL: SyntaxError'), + (TypeError, 'INVALID_ARGUMENT: TypeError'), + (ValueError, 'OUT_OF_RANGE: ValueError'), + (LookupError, 'NOT_FOUND: LookupError'), + (RuntimeError, 'UNKNOWN: RuntimeError'), +) + + +class StatusReturnTest(parameterized.TestCase): + + def setUp(self): + super().setUp() + self.tm = self.getTestModule() # pytype: disable=attribute-error + + def testStatusOk(self): # pylint: disable=invalid-name + def cb(): + pass + + self.assertEqual(self.tm.CallCallbackWithStatusReturn(cb), 'OK') + + @parameterized.parameters(*TAB_StatusFromFetchedExc) + def testStatusFromFetchedExc(self, etype, expected): # pylint: disable=invalid-name + + def cb(): + raise etype('Msg.') + + self.assertEqual( + self.tm.CallCallbackWithStatusReturn(cb), expected + ': Msg.' + ) + + def testStatusWrongReturnType(self): # pylint: disable=invalid-name + def cb(): + return ['something'] + + if getattr(self.tm, '__pyclif_codegen_mode__', None) == 'c_api': + expected = 'OK' + else: + expected = ( + 'INVALID_ARGUMENT: Unable to cast Python instance of type to C++ type 'absl::Status'" + ) + self.assertEqual(self.tm.CallCallbackWithStatusReturn(cb), expected) + + def testAssertionErrorBare(self): # pylint: disable=invalid-name + + def cb(): + assert False + + self.assertEqual( + self.tm.CallCallbackWithStatusReturn(cb), 'UNKNOWN: AssertionError: ' + ) + + def testAssertionErrorWithValue(self): # pylint: disable=invalid-name + + def cb(): + assert False, 'Unexpected' + + self.assertEqual( + self.tm.CallCallbackWithStatusReturn(cb), + 'UNKNOWN: AssertionError: Unexpected', + ) + + def testErrorStatusNotOkRoundTrip(self): # pylint: disable=invalid-name + + def cb(): + self.tm.GenerateErrorStatusNotOk() + + self.assertEqual( + self.tm.CallCallbackWithStatusReturn(cb), + 'ALREADY_EXISTS: Something went wrong, again.', + ) + + +class StatusOrReturnTest(parameterized.TestCase): + + def setUp(self): + super().setUp() + self.tm = self.getTestModule() # pytype: disable=attribute-error + + def testStatusOrIntOk(self): # pylint: disable=invalid-name + def cb(): + return 5 + + self.assertEqual(self.tm.CallCallbackWithStatusOrIntReturn(cb), '5') + + @parameterized.parameters(*TAB_StatusFromFetchedExc) + def testStatusOrIntFromFetchedExc(self, etype, expected): # pylint: disable=invalid-name + def cb(): + raise etype('Msg.') + + self.assertEqual( + self.tm.CallCallbackWithStatusOrIntReturn(cb), expected + ': Msg.' + ) + + def testStatusOrIntWrongReturnType(self): # pylint: disable=invalid-name + def cb(): + return '5' + + if getattr(self.tm, '__pyclif_codegen_mode__', None) == 'c_api': + expected = 'INVALID_ARGUMENT: TypeError: expecting int' + else: + expected = ( + 'INVALID_ARGUMENT: Unable to cast Python instance of type to C++ type 'absl::StatusOr'" + ) + self.assertEqual(self.tm.CallCallbackWithStatusOrIntReturn(cb), expected) + + +class StatusOrPyObjectPtrTest(absltest.TestCase): + + def setUp(self): + super().setUp() + self.tm = self.getTestModule() # pytype: disable=attribute-error + + def testStatusOrObject(self): # pylint: disable=invalid-name + if getattr(self.tm, '__pyclif_codegen_mode__', None) != 'c_api': + self.skipTest('TODO(cl/578064081)') + # No leak (manually verified under cl/485274434). + while True: + lst = [1, 2, 3, 4] + + def cb(): + return lst + + # call many times to be sure that object reference is not being removed + for _ in range(10): + res = self.tm.CallCallbackWithStatusOrObjectReturn(cb) + self.assertListEqual(res, lst) + self.assertIs(res, lst) + return # Comment out for manual leak checking (use `top` command).