Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for nested C++11 exceptions #3608

Merged
merged 21 commits into from
Jan 14, 2022
Merged

Add support for nested C++11 exceptions #3608

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
ab9a1ec
Add support for nested C++11 exceptions
Skylion007 Jan 10, 2022
269da46
Remove wrong include
Skylion007 Jan 10, 2022
bf438d9
Fix if directive
Skylion007 Jan 10, 2022
42d5833
Fix missing skipif
Skylion007 Jan 10, 2022
a1ac841
Simplify code and try to work around MSVC bug
Skylion007 Jan 10, 2022
4cf2754
Clarify comment
Skylion007 Jan 10, 2022
eb13bef
Further simplify code
Skylion007 Jan 10, 2022
b89620a
Remove the last extra throw statement
Skylion007 Jan 11, 2022
09d1ad5
Qualify auto
Skylion007 Jan 11, 2022
3a9156c
Fix typo
Skylion007 Jan 11, 2022
5145500
Add missing return for consistency
Skylion007 Jan 11, 2022
d2b87f2
Fix clang-tidy complaint
Skylion007 Jan 11, 2022
c745ee3
Fix python2 stub
Skylion007 Jan 11, 2022
442d958
Make clang-tidy happy
Skylion007 Jan 11, 2022
d369c87
Fix compile error
Skylion007 Jan 11, 2022
c49df0d
Fix python2 function signature
Skylion007 Jan 12, 2022
2578c87
Extract C++20 utility and backport
Skylion007 Jan 13, 2022
1c526cf
Cleanup code a bit more
Skylion007 Jan 13, 2022
65a5ad8
Improve test case
Skylion007 Jan 13, 2022
e6cbe62
Consolidate code and fix signature
Skylion007 Jan 14, 2022
3fa58ee
Fix typo
Skylion007 Jan 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
# define PYBIND11_CPP14
# if __cplusplus >= 201703L
# define PYBIND11_CPP17
# if __cplusplus >= 202002L
# define PYBIND11_CPP20
# endif
# endif
# endif
#elif defined(_MSC_VER) && __cplusplus == 199711L
Expand All @@ -45,6 +48,9 @@
# define PYBIND11_CPP14
# if _MSVC_LANG > 201402L && _MSC_VER >= 1910
# define PYBIND11_CPP17
# if _MSVC_LANG > 202002L
# define PYBIND11_CPP20
# endif
# endif
# endif
#endif
Expand Down Expand Up @@ -612,6 +618,18 @@ template <typename T> using remove_cv_t = typename std::remove_cv<T>::type;
template <typename T> using remove_reference_t = typename std::remove_reference<T>::type;
#endif

#if defined(PYBIND11_CPP20)
using std::remove_cvref;
using std::remove_cvref_t;
#else
template <class T>
struct remove_cvref {
using type = remove_cv_t<remove_reference_t<T>>;
};
template <class T>
using remove_cvref_t = typename remove_cvref<T>::type;
#endif

/// Index sequences
#if defined(PYBIND11_CPP14)
using std::index_sequence;
Expand Down
110 changes: 98 additions & 12 deletions include/pybind11/detail/internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#pragma once

#include "../pytypes.h"
#include <exception>

/// Tracks the `internals` and `type_info` ABI version independent of the main library version.
///
Expand Down Expand Up @@ -280,21 +281,106 @@ inline internals **&get_internals_pp() {
return internals_pp;
}

#if PY_VERSION_HEX >= 0x03030000
inline bool raise_err(PyObject *exc_type, const char *msg) {
if (PyErr_Occurred()) {
raise_from(exc_type, msg);
return true;
}
PyErr_SetString(exc_type, msg);
return false;
};

// forward decl
inline void translate_exception(std::exception_ptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forward declarations should never have inline:

https://stackoverflow.com/questions/9317473/forward-declaration-of-inline-functions

(I find this highly counter-intuitive myself.)

Copy link
Collaborator

@henryiii henryiii Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C99 requires the inline on the definition; are you sure C++ cares? Looking at the spec, it seems clearly allowed unless it's nested inside another function https://en.cppreference.com/w/cpp/language/inline & https://en.cppreference.com/w/cpp/language/declarations#Specifiers seem to indicate it's allowed, and I didn't have any problems on any complier I quickly checked on godbolt.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C99 requires the inline on the definition;

Ouch.

My previous comment was based on what I remembered from working on
#3179.

From memory, in a Google-internal chat I was advised to not use inline in forward declarations.

Am I sure? No. I was asking for advice and then going with that. The work on PR #3179 left me believing that even compilers are unusually confused about what is correct.

To be pragmatic, we can reduce it to: do we want to be consistent within pybind11? PR #3179 removed some inline based on the advice I got, and to be consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwgk Other forward declare (line 40) still use inline.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally kept #3179 limited to functions involving PYBIND11_NOINLINE. I didn't look around too much, as I very often do, to not let a good project die the death of a thousand cuts.

For this PR, idk. If we're inconsistent already and the advice I got in Aug last year isn't universally accepted (as I was thinking until now) ... shrug :-)


template <class T,
enable_if_t<std::is_same<std::nested_exception, remove_cvref_t<T>>::value, int> = 0>
bool handle_nested_exception(const T &exc, const std::exception_ptr &p) {
std::exception_ptr nested = exc.nested_ptr();
if (nested != nullptr && nested != p) {
translate_exception(nested);
return true;
}
return false;
}

template <class T,
enable_if_t<!std::is_same<std::nested_exception, remove_cvref_t<T>>::value, int> = 0>
bool handle_nested_exception(const T &exc, const std::exception_ptr &p) {
if (auto *nep = dynamic_cast<const std::nested_exception *>(std::addressof(exc))) {
Skylion007 marked this conversation as resolved.
Show resolved Hide resolved
return handle_nested_exception(*nep, p);
}
return false;
}

#else
inline bool raise_err(PyObject *exc_type, const char *msg) {
PyErr_SetString(exc_type, msg);
return false;
}

template <class T>
bool handle_nested_exception(const T &, std::exception_ptr) {
return false;
}
#endif

inline void translate_exception(std::exception_ptr p) {
if (!p) {
return;
}
try {
if (p) std::rethrow_exception(p);
} catch (error_already_set &e) { e.restore(); return;
} catch (const builtin_exception &e) { e.set_error(); return;
} catch (const std::bad_alloc &e) { PyErr_SetString(PyExc_MemoryError, e.what()); return;
} catch (const std::domain_error &e) { PyErr_SetString(PyExc_ValueError, e.what()); return;
} catch (const std::invalid_argument &e) { PyErr_SetString(PyExc_ValueError, e.what()); return;
} catch (const std::length_error &e) { PyErr_SetString(PyExc_ValueError, e.what()); return;
} catch (const std::out_of_range &e) { PyErr_SetString(PyExc_IndexError, e.what()); return;
} catch (const std::range_error &e) { PyErr_SetString(PyExc_ValueError, e.what()); return;
} catch (const std::overflow_error &e) { PyErr_SetString(PyExc_OverflowError, e.what()); return;
} catch (const std::exception &e) { PyErr_SetString(PyExc_RuntimeError, e.what()); return;
std::rethrow_exception(p);
} catch (error_already_set &e) {
handle_nested_exception(e, p);
e.restore();
Skylion007 marked this conversation as resolved.
Show resolved Hide resolved
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the many return; needed (to make some compiler(s) happy)?
Would be nice to get rid of them. Or add a comment (maybe) right before the try, something like:

// The returns inside the catch blocks are needed to silence compiler warnings.
(If that's true.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why the return's are here. There were here before the diff.

} catch (const builtin_exception &e) {
// Could not use template since it's an abstract class.
if (auto *nep = dynamic_cast<const std::nested_exception *>(std::addressof(e))) {
handle_nested_exception(*nep, p);
}
e.set_error();
return;
} catch (const std::bad_alloc &e) {
handle_nested_exception(e, p);
raise_err(PyExc_MemoryError, e.what());
return;
} catch (const std::domain_error &e) {
handle_nested_exception(e, p);
raise_err(PyExc_ValueError, e.what());
return;
} catch (const std::invalid_argument &e) {
handle_nested_exception(e, p);
raise_err(PyExc_ValueError, e.what());
return;
} catch (const std::length_error &e) {
handle_nested_exception(e, p);
raise_err(PyExc_ValueError, e.what());
return;
} catch (const std::out_of_range &e) {
handle_nested_exception(e, p);
raise_err(PyExc_IndexError, e.what());
return;
} catch (const std::range_error &e) {
handle_nested_exception(e, p);
raise_err(PyExc_ValueError, e.what());
return;
} catch (const std::overflow_error &e) {
handle_nested_exception(e, p);
raise_err(PyExc_OverflowError, e.what());
return;
} catch (const std::exception &e) {
handle_nested_exception(e, p);
raise_err(PyExc_RuntimeError, e.what());
return;
} catch (const std::nested_exception &e) {
handle_nested_exception(e, p);
raise_err(PyExc_RuntimeError, "Caught an unknown nested exception!");
return;
} catch (...) {
PyErr_SetString(PyExc_RuntimeError, "Caught an unknown exception!");
raise_err(PyExc_RuntimeError, "Caught an unknown exception!");
return;
}
}
Expand Down
10 changes: 9 additions & 1 deletion tests/test_exceptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include "local_bindings.h"

#include "pybind11_tests.h"
#include <exception>
#include <stdexcept>
#include <utility>

// A type that should be raised as an exception in Python
Expand Down Expand Up @@ -105,7 +107,6 @@ struct PythonAlreadySetInDestructor {
py::str s;
};


TEST_SUBMODULE(exceptions, m) {
m.def("throw_std_exception", []() {
throw std::runtime_error("This exception was intentionally thrown.");
Expand Down Expand Up @@ -281,5 +282,12 @@ TEST_SUBMODULE(exceptions, m) {
}
});

m.def("throw_nested_exception", []() {
try {
throw std::runtime_error("Inner Exception");
} catch (const std::runtime_error &) {
std::throw_with_nested(std::runtime_error("Outer Exception"));
}
});
#endif
}
8 changes: 8 additions & 0 deletions tests/test_exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,14 @@ def pycatch(exctype, f, *args):
assert str(excinfo.value) == "this is a helper-defined translated exception"


@pytest.mark.skipif("env.PY2")
def test_throw_nested_exception():
with pytest.raises(RuntimeError) as excinfo:
m.throw_nested_exception()
assert str(excinfo.value) == "Outer Exception"
assert str(excinfo.value.__cause__) == "Inner Exception"


# This can often happen if you wrap a pybind11 class in a Python wrapper
def test_invalid_repr():
class MyRepr(object):
Expand Down