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.

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

Replace error printing code gated by NDEBUG with a new flag: PYBIND11_DETAILED_ERROR_MESSAGES #3913

Merged
merged 7 commits into from
May 2, 2022
6 changes: 4 additions & 2 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#pragma once

#include "detail/common.h"
#include "cast.h"

#include <functional>
Expand Down Expand Up @@ -477,7 +478,7 @@ struct process_attribute<arg_v> : process_attribute_default<arg_v> {
}

if (!a.value) {
#if !defined(NDEBUG)
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
std::string descr("'");
if (a.name) {
descr += std::string(a.name) + ": ";
Expand All @@ -498,7 +499,8 @@ struct process_attribute<arg_v> : process_attribute_default<arg_v> {
#else
pybind11_fail("arg(): could not convert default argument "
"into a Python object (type not registered yet?). "
"Compile in debug mode for more information.");
"#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for "
"more information.");
#endif
}
r->args.emplace_back(a.name, a.descr, a.value.inc_ref(), !a.flag_noconvert, a.flag_none);
Expand Down
51 changes: 27 additions & 24 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -777,8 +777,9 @@ struct copyable_holder_caster : public type_caster_base<type> {
return true;
}
throw cast_error("Unable to cast from non-held to held instance (T& to Holder<T>) "
#if defined(NDEBUG)
"(compile in debug mode for type information)");
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
"(#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for "
"type information)");
#else
"of type '"
+ type_id<holder_type>() + "''");
Expand Down Expand Up @@ -1001,9 +1002,9 @@ struct return_value_policy_override<
template <typename T, typename SFINAE>
type_caster<T, SFINAE> &load_type(type_caster<T, SFINAE> &conv, const handle &handle) {
if (!conv.load(handle, true)) {
#if defined(NDEBUG)
throw cast_error(
"Unable to cast Python instance to C++ type (compile in debug mode for details)");
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
throw cast_error("Unable to cast Python instance to C++ type (#define "
"PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)");
#else
throw cast_error("Unable to cast Python instance of type "
+ (std::string) str(type::handle_of(handle)) + " to C++ type '"
Expand Down Expand Up @@ -1068,10 +1069,10 @@ inline void handle::cast() const {
template <typename T>
detail::enable_if_t<!detail::move_never<T>::value, T> move(object &&obj) {
if (obj.ref_count() > 1) {
#if defined(NDEBUG)
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
throw cast_error(
"Unable to cast Python instance to C++ rvalue: instance has multiple references"
" (compile in debug mode for details)");
" (#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)");
#else
throw cast_error("Unable to move from Python " + (std::string) str(type::handle_of(obj))
+ " instance to C++ " + type_id<T>()
Expand Down Expand Up @@ -1172,10 +1173,10 @@ PYBIND11_NAMESPACE_END(detail)

// The overloads could coexist, i.e. the #if is not strictly speaking needed,
// but it is an easy minor optimization.
#if defined(NDEBUG)
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
inline cast_error cast_error_unable_to_convert_call_arg() {
return cast_error(
"Unable to convert call argument to Python object (compile in debug mode for details)");
return cast_error("Unable to convert call argument to Python object (#define "
"PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)");
}
#else
inline cast_error cast_error_unable_to_convert_call_arg(const std::string &name,
Expand All @@ -1197,7 +1198,7 @@ tuple make_tuple(Args &&...args_) {
detail::make_caster<Args>::cast(std::forward<Args>(args_), policy, nullptr))...}};
for (size_t i = 0; i < args.size(); i++) {
if (!args[i]) {
#if defined(NDEBUG)
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
throw cast_error_unable_to_convert_call_arg();
#else
std::array<std::string, size> argtypes{{type_id<Args>()...}};
Expand Down Expand Up @@ -1249,7 +1250,7 @@ struct arg_v : arg {
: arg(base), value(reinterpret_steal<object>(detail::make_caster<T>::cast(
std::forward<T>(x), return_value_policy::automatic, {}))),
descr(descr)
#if !defined(NDEBUG)
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
,
type(type_id<T>())
#endif
Expand Down Expand Up @@ -1289,7 +1290,7 @@ struct arg_v : arg {
object value;
/// The (optional) description of the default value
const char *descr;
#if !defined(NDEBUG)
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
/// The C++ type name of the default value (only available when compiled in debug mode)
std::string type;
#endif
Expand Down Expand Up @@ -1487,7 +1488,7 @@ class unpacking_collector {
auto o = reinterpret_steal<object>(
detail::make_caster<T>::cast(std::forward<T>(x), policy, {}));
if (!o) {
#if defined(NDEBUG)
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
throw cast_error_unable_to_convert_call_arg();
#else
throw cast_error_unable_to_convert_call_arg(std::to_string(args_list.size()),
Expand All @@ -1505,21 +1506,21 @@ class unpacking_collector {

void process(list & /*args_list*/, arg_v a) {
if (!a.name) {
#if defined(NDEBUG)
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
nameless_argument_error();
#else
nameless_argument_error(a.type);
#endif
}
if (m_kwargs.contains(a.name)) {
#if defined(NDEBUG)
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
multiple_values_error();
#else
multiple_values_error(a.name);
#endif
}
if (!a.value) {
#if defined(NDEBUG)
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
throw cast_error_unable_to_convert_call_arg();
#else
throw cast_error_unable_to_convert_call_arg(a.name, a.type);
Expand All @@ -1534,7 +1535,7 @@ class unpacking_collector {
}
for (auto k : reinterpret_borrow<dict>(kp)) {
if (m_kwargs.contains(k.first)) {
#if defined(NDEBUG)
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
multiple_values_error();
#else
multiple_values_error(str(k.first));
Expand All @@ -1545,18 +1546,20 @@ class unpacking_collector {
}

[[noreturn]] static void nameless_argument_error() {
throw type_error("Got kwargs without a name; only named arguments "
"may be passed via py::arg() to a python function call. "
"(compile in debug mode for details)");
throw type_error(
"Got kwargs without a name; only named arguments "
"may be passed via py::arg() to a python function call. "
"(#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)");
}
[[noreturn]] static void nameless_argument_error(const std::string &type) {
throw type_error("Got kwargs without a name of type '" + type
+ "'; only named "
"arguments may be passed via py::arg() to a python function call. ");
}
[[noreturn]] static void multiple_values_error() {
throw type_error("Got multiple values for keyword argument "
"(compile in debug mode for details)");
throw type_error(
"Got multiple values for keyword argument "
"(#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)");
}

[[noreturn]] static void multiple_values_error(const std::string &name) {
Expand Down Expand Up @@ -1603,7 +1606,7 @@ unpacking_collector<policy> collect_arguments(Args &&...args) {
template <typename Derived>
template <return_value_policy policy, typename... Args>
object object_api<Derived>::operator()(Args &&...args) const {
#ifndef NDEBUG
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
if (!PyGILState_Check()) {
pybind11_fail("pybind11::object_api<>::operator() PyGILState_Check() failure.");
}
Expand Down
7 changes: 7 additions & 0 deletions include/pybind11/detail/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -1157,5 +1157,12 @@ constexpr inline bool silence_msvc_c4127(bool cond) { return cond; }
# define PYBIND11_SILENCE_MSVC_C4127(...) __VA_ARGS__
#endif

// Pybind offers detailed error messages by default for all builts that are debug (through the
// negation of ndebug). This can also be manually enabled by users, for any builds, through
// defining PYBIND11_DETAILED_ERROR_MESSAGES.
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES) && !defined(NDEBUG)
# define PYBIND11_DETAILED_ERROR_MESSAGES
#endif

PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
31 changes: 17 additions & 14 deletions include/pybind11/detail/type_caster_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -394,15 +394,16 @@ instance::get_value_and_holder(const type_info *find_type /*= nullptr default in
return value_and_holder();
}

#if defined(NDEBUG)
pybind11_fail("pybind11::detail::instance::get_value_and_holder: "
"type is not a pybind11 base of the given instance "
"(compile in debug mode for type details)");
#else
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
pybind11_fail("pybind11::detail::instance::get_value_and_holder: `"
+ get_fully_qualified_tp_name(find_type->type)
+ "' is not a pybind11 base of the given `"
+ get_fully_qualified_tp_name(Py_TYPE(this)) + "' instance");
#else
pybind11_fail(
"pybind11::detail::instance::get_value_and_holder: "
"type is not a pybind11 base of the given instance "
"(#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for type details)");
#endif
}

Expand Down Expand Up @@ -612,14 +613,15 @@ class type_caster_generic {
if (copy_constructor) {
valueptr = copy_constructor(src);
} else {
#if defined(NDEBUG)
throw cast_error("return_value_policy = copy, but type is "
"non-copyable! (compile in debug mode for details)");
#else
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
std::string type_name(tinfo->cpptype->name());
detail::clean_type_id(type_name);
throw cast_error("return_value_policy = copy, but type " + type_name
+ " is non-copyable!");
#else
throw cast_error("return_value_policy = copy, but type is "
"non-copyable! (#define PYBIND11_DETAILED_ERROR_MESSAGES or "
"compile in debug mode for details)");
#endif
}
wrapper->owned = true;
Expand All @@ -631,15 +633,16 @@ class type_caster_generic {
} else if (copy_constructor) {
valueptr = copy_constructor(src);
} else {
#if defined(NDEBUG)
throw cast_error("return_value_policy = move, but type is neither "
"movable nor copyable! "
"(compile in debug mode for details)");
#else
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
std::string type_name(tinfo->cpptype->name());
detail::clean_type_id(type_name);
throw cast_error("return_value_policy = move, but type " + type_name
+ " is neither movable nor copyable!");
#else
throw cast_error("return_value_policy = move, but type is neither "
"movable nor copyable! "
"(#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in "
"debug mode for details)");
#endif
}
wrapper->owned = true;
Expand Down
6 changes: 3 additions & 3 deletions include/pybind11/gil.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class gil_scoped_acquire {

if (!tstate) {
tstate = PyThreadState_New(internals.istate);
# if !defined(NDEBUG)
# if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
if (!tstate) {
pybind11_fail("scoped_acquire: could not create thread state!");
}
Expand All @@ -84,7 +84,7 @@ class gil_scoped_acquire {

PYBIND11_NOINLINE void dec_ref() {
--tstate->gilstate_counter;
# if !defined(NDEBUG)
# if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
if (detail::get_thread_state_unchecked() != tstate) {
pybind11_fail("scoped_acquire::dec_ref(): thread state must be current!");
}
Expand All @@ -93,7 +93,7 @@ class gil_scoped_acquire {
}
# endif
if (tstate->gilstate_counter == 0) {
# if !defined(NDEBUG)
# if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
if (!release) {
pybind11_fail("scoped_acquire::dec_ref(): internal error!");
}
Expand Down
9 changes: 5 additions & 4 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ class cpp_function : public function {
rec->is_constructor = (std::strcmp(rec->name, "__init__") == 0)
|| (std::strcmp(rec->name, "__setstate__") == 0);

#if !defined(NDEBUG) && !defined(PYBIND11_DISABLE_NEW_STYLE_INIT_WARNING)
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES) && !defined(PYBIND11_DISABLE_NEW_STYLE_INIT_WARNING)
if (rec->is_constructor && !rec->is_new_style_constructor) {
const auto class_name
= detail::get_fully_qualified_tp_name((PyTypeObject *) rec->scope.ptr());
Expand Down Expand Up @@ -518,8 +518,9 @@ class cpp_function : public function {
if (chain->is_method != rec->is_method) {
pybind11_fail(
"overloading a method with both static and instance methods is not supported; "
#if defined(NDEBUG)
"compile in debug mode for more details"
#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES)
"#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for more "
"details"
#else
"error while attempting to bind "
+ std::string(rec->is_method ? "instance" : "static") + " method "
Expand Down Expand Up @@ -906,7 +907,7 @@ class cpp_function : public function {

// 5. Put everything in a vector. Not technically step 5, we've been building it
// in `call.args` all along.
#if !defined(NDEBUG)
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
if (call.args.size() != func.nargs || call.args_convert.size() != func.nargs) {
pybind11_fail("Internal error: function call dispatcher inserted wrong number "
"of arguments!");
Expand Down
6 changes: 3 additions & 3 deletions tests/pybind11_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ PYBIND11_MODULE(pybind11_tests, m) {

bind_ConstructorStats(m);

#if !defined(NDEBUG)
m.attr("debug_enabled") = true;
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
m.attr("detailed_error_messages_enabled") = true;
#else
m.attr("debug_enabled") = false;
m.attr("detailed_error_messages_enabled") = false;
#endif

py::class_<UserType>(m, "UserType", "A `py::class_` type for testing")
Expand Down
6 changes: 3 additions & 3 deletions tests/test_methods_and_attributes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -361,10 +361,10 @@ TEST_SUBMODULE(methods_and_attributes, m) {

// test_bad_arg_default
// Issue/PR #648: bad arg default debugging output
#if !defined(NDEBUG)
m.attr("debug_enabled") = true;
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
m.attr("detailed_error_messages_enabled") = true;
#else
m.attr("debug_enabled") = false;
m.attr("detailed_error_messages_enabled") = false;
#endif
m.def("bad_arg_def_named", [] {
auto m = py::module_::import("pybind11_tests");
Expand Down
Loading