Repository: OpenAssetIO/OpenAssetIO (language: python)

Issue:
Hybrid exception cannot be caught as `std::exception`
## What
This was noticed in the usdResolver, but I think it's an OpenAssetIO problem so logging here.
We have code in the usd resolver tests that triggers a python exception, and attempts to verify the error message, catching by `std::exception&`. https://github.com/OpenAssetIO/usdOpenAssetIOResolver/blob/de7b465d0af08712bc34002d45746484c40505d1/src/resolver.cpp#L168

However, what happens is that we end up catching in the Wildcard exception handler, and logging "unknown non-exception type caught"
(CI logs here, although it dosen't show the full output : https://github.com/elliotcmorris/usdOpenAssetIOResolver/actions/runs/7062456693/job/19226520598)

## Why
Unknown, very likely an RTTI issue across exception boundrys., although I wouldn't rule out a diamond inheritance problem either.  (See here : https://godbolt.org/z/e6PE5b9n4, it isn't exactly what we're seeing in the usd example, but it does show our exception heirarchy demonstrating catch behaviour strangeness.)

Another thought is a standard library incompatibility between clang 14.3 (what our CI builds+runs the usd resolver on) and clang 10.5, (what our CI builds openassetio on,) however, I did test, albeit hastily, running on a 10.5 docker image and it still didn't seem to function.

Information from my experiments:
- The error, which is a MalformedEntityReference error from bal, thrown as a BatchElementException in the openassetio middleware, is catchable as an `OpenAssetIOException` or a `BatchElementException` (or even a HybridException<BatchElementException>, i went to the effort of making that type public to check)
- When caught as one of these exceptions, typeid reports the type correctly as a HybridException<BatchElementException>
- The standard exception types show up in exported symbols, (checked with `nm`) but the `HybridException` template instantiations dont.
- Attempting to explicitly instantiate the HybridException Types in both OpenAssetIO and the calling code itself (using the same hacked version making it public,) had no effect. The idea was to force the template type information to be exported, although I could never manage it.

I'm at a bit of a loss. This isn't a super emergency, as it will only occur from a python manager implementation, using the exceptional conveniences into a C++ host, attempting to catch as `std::exception` or `std::runtime_error`. That being said it's a bit of a footgun. My instinct says this is to do with being a template exception across a module boundry, and we may need to rethink.

What I havn't done but really _should_ have done is removed USD from this whole equation and attempted to catch a hybrid exception in-process in openassetio itself, that would tell us for sure if there's RTTI export nonsense involved or not, and that's probably where i'd pick this investigation up.



Discussion / hints:
The godbolt link in the description (minus the `virtual` inheritance) highlights the issue.

It seems the ambiguous base class of `std::exception`, shared by both `OpenAssetIOException` (via its `std::runtime_error` base) and by `pybind11::error_already_set`, means that the `HybridException` cannot be caught as a `std::exception`.

For this to work, we would need both `pybind11::error_already_set` and `std::runtime_error` to have a `virtual` base of `std::exception`, but that's out of our control.

As an aside: in our conanfile we use pybind11 2.10.1, where `error_already_set` inherits from `std::exception`  The ci-base:2022.x Docker images bundle pybind11 2.8.1, where `error_already_set` inherits from `std::runtime_error`.  This means that when using the Docker container, `catch`ing a `HybridException` as a `std::runtime_error` fails, whereas locally it succeeds.



Fix patch:
diff --git a/src/openassetio-python/cmodule/src/errors/exceptionsBinding.cpp b/src/openassetio-python/cmodule/src/errors/exceptionsBinding.cpp
index 75bfc93b3..ce899e4b7 100644
--- a/src/openassetio-python/cmodule/src/errors/exceptionsBinding.cpp
+++ b/src/openassetio-python/cmodule/src/errors/exceptionsBinding.cpp
@@ -20,6 +20,7 @@ using openassetio::errors::NotImplementedException;
 using openassetio::errors::OpenAssetIOException;
 using openassetio::errors::UnhandledException;
 using openassetio::python::exceptions::CppExceptionsAndPyClassNames;
+using openassetio::python::exceptions::HybridException;
 
 /**
  * @section topy Conversion from C++ exception to Python exception.
@@ -63,6 +64,12 @@ void setPyException(const Exception &exception, const py::module_ &pyModule,
  * given `exception_ptr` is rethrown and propagates up the nested
  * try-catch blocks until it is caught.
  *
+ * If the exception is a `HybridException` type, i.e. was originally a
+ * Python exception that has propagated through C++ and is now
+ * propagating back out to Python, then rethrow the original Python
+ * error. Otherwise, construct a new Python exception from the C++
+ * exception.
+ *
  * @tparam I Index of exception type to catch in
  * CppExceptionsAndPyClassNames list.
  * @param pyModule Python module containing Python exception classes.
@@ -76,6 +83,8 @@ void tryCatch(const py::module_ &pyModule, std::exception_ptr pexc) {
     } else {
       tryCatch<I - 1>(pyModule, std::move(pexc));
     }
+  } catch (const HybridException<CppExceptionsAndPyClassNames::Exceptions<I>> &cppExc) {
+    throw cppExc.originalPyExc;
   } catch (const CppExceptionsAndPyClassNames::Exceptions<I> &cppExc) {
     setPyException(cppExc, pyModule, CppExceptionsAndPyClassNames::kClassNames[I]);
   }
diff --git a/src/openassetio-python/private/include/openassetio/private/python/exceptions.hpp b/src/openassetio-python/private/include/openassetio/private/python/exceptions.hpp
index bbfc15f89..f0f753a45 100644
--- a/src/openassetio-python/private/include/openassetio/private/python/exceptions.hpp
+++ b/src/openassetio-python/private/include/openassetio/private/python/exceptions.hpp
@@ -93,19 +93,13 @@ struct CppExceptionsAndPyClassNames {
 /**
  * Hybrid of OpenAssetIO and pybind11 exception class.
  *
- * Multiple inheritance means that in C++ we can `catch` this exception
- * as either `error_already_set` or the given OpenAssetIO exception. If
- * (re)thrown, it can be caught further up the stack (again) as either
- * of the two exceptions. In this way we can satisfy the two use-cases:
- * - Catching the exception in C++ as an OpenAssetIO C++ exception.
- * - Allowing the exception to propagate back to Python via pybind11,
- *   which will translate it back into a Python exception (see
- *   registerExceptions).
- *
- * Note that calling `.what()` on this exception is ambiguous and will
- * cause a compiler error. However, this is a good thing, since this
- * exception should never appear in a `catch` (should only be caught as
- * one of the base classes).
+ * This template class inherits from its template argument, allowing it
+ * to be caught transparently as the wrapped C++ exception type.
+ *
+ * The corresponding Python exception (as reflected by a pybind11
+ * `error_already_set` object), is stored for potential recall if this
+ * C++ exception propagates back out to Python - see
+ * `registerExceptions`.
  *
  * Generalisation assuming that the given exception type is a simple
  * exception that takes a string message as its only constructor
@@ -113,11 +107,20 @@ struct CppExceptionsAndPyClassNames {
  *
  * @tparam CppException C++ exception type corresponding to given
  * Python exception.
+ *
+ * @todo Remove PYBIND11_EXPORT once tests that require it are
+ *   redesigned. I.e. _openassetio_test throws HybridException which is
+ *   then caught in _openassetio, requiring global RTTI info for libc++.
+ *   This is not representative of real-world use, since HybridException
+ *   should be private to _openassetio. See
+ *   https://github.com/OpenAssetIO/OpenAssetIO/issues/1237
  */
 template <class CppException>
-struct HybridException : py::error_already_set, CppException {
+struct PYBIND11_EXPORT HybridException : CppException {
   explicit HybridException(const py::error_already_set &pyExc)
-      : py::error_already_set{pyExc}, CppException{pyExc.what()} {}
+      : CppException{pyExc.what()}, originalPyExc{pyExc} {}
+
+  py::error_already_set originalPyExc;
 };
 
 /**
@@ -127,13 +130,15 @@ struct HybridException : py::error_already_set, CppException {
  * generalisation, above, for more details.
  */
 template <>
-struct HybridException<BatchElementException> : py::error_already_set, BatchElementException {
+struct HybridException<BatchElementException> : BatchElementException {
   explicit HybridException(const py::error_already_set &pyExc)
-      : py::error_already_set{pyExc},
-        BatchElementException{
-            py::cast<std::size_t>(pyExc.value().attr("index")),
-            py::cast<openassetio::errors::BatchElementError>(pyExc.value().attr("error")),
-            pyExc.what()} {}
+      : BatchElementException{py::cast<std::size_t>(pyExc.value().attr("index")),
+                              py::cast<openassetio::errors::BatchElementError>(
+                                  pyExc.value().attr("error")),
+                              pyExc.what()},
+        originalPyExc{pyExc} {}
+
+  py::error_already_set originalPyExc;
 };
 
 /**


Test patch:
diff --git a/src/openassetio-python/tests/cmodule/resources/_openassetio_test/errorsTest.cpp b/src/openassetio-python/tests/cmodule/resources/_openassetio_test/errorsTest.cpp
index 38313a8ba..6a95a79b5 100644
--- a/src/openassetio-python/tests/cmodule/resources/_openassetio_test/errorsTest.cpp
+++ b/src/openassetio-python/tests/cmodule/resources/_openassetio_test/errorsTest.cpp
@@ -361,6 +361,13 @@ struct PyExceptionThrower : ExceptionThrower {
  * some additional confidence. Note that the GIL is released
  * deliberately in order to better simulate the situation in OpenAssetIO
  * Python bindings.
+ *
+ * `throwPythonExceptionAndCatchAsStdException` instructs Python to
+ * throw an exception and attempts to catch it as a `std::exception` and
+ * a `std::runtime_error`. This is a regression test against RTTI issues
+ * between hybrid pybind11 and OpenAssetIO exceptions, which have a
+ * common STL base class so must be composed with care (i.e. avoid
+ * multiple inheritance).
  */
 void registerExceptionThrower(py::module_& mod) {
   mod.def("throwException", [](const std::string& exceptionName, const std::string& msgData) {
@@ -401,6 +408,23 @@ void registerExceptionThrower(py::module_& mod) {
       py::arg("exceptionThrower"), py::arg("catchExceptionName"),
       py::call_guard<py::gil_scoped_release>{});
 
+  mod.def(
+      "throwPythonExceptionAndCatchAsStdException",
+      [](ExceptionThrower& exceptionThrower) {
+        // If we fail to catch in either of the following try-catch
+        // blocks, the exception will propagate, implicitly failing the
+        // pytest test in Python.
+        try {
+          exceptionThrower.throwFromOverride();
+        } catch (const std::exception&) {  // NOLINT(*-empty-catch)
+        }
+        try {
+          exceptionThrower.throwFromOverride();
+        } catch (const std::runtime_error&) {  // NOLINT(*-empty-catch)
+        }
+      },
+      py::arg("exceptionThrower"), py::call_guard<py::gil_scoped_release>{});
+
   py::class_<ExceptionThrower, PyExceptionThrower>{mod, "ExceptionThrower"}
       .def(py::init<>())
       .def("throwFromOverride", &ExceptionThrower::throwFromOverride)
diff --git a/src/openassetio-python/tests/cmodule/test_errors.py b/src/openassetio-python/tests/cmodule/test_errors.py
index 78b3a548e..d4f6db054 100644
--- a/src/openassetio-python/tests/cmodule/test_errors.py
+++ b/src/openassetio-python/tests/cmodule/test_errors.py
@@ -83,6 +83,16 @@ def test_when_python_exception_thrown_then_can_be_caught_in_cpp(
             exception_thrower, exception_type.__name__
         )
 
+    @pytest.mark.parametrize("exception_type", all_exceptions)
+    def test_when_python_exception_thrown_then_can_be_caught_in_cpp_as_stl_base_type(
+        self, exception_type, exception_thrower
+    ):
+        exception = make_exception(exception_type)
+
+        exception_thrower.callee.side_effect = exception
+
+        _openassetio_test.throwPythonExceptionAndCatchAsStdException(exception_thrower)
+
     @pytest.mark.parametrize("exception_type", all_exceptions)
     def test_when_python_exception_thrown_then_can_be_caught_in_cpp_rethrown_and_caught_in_python(
         self, exception_type, exception_thrower