From f0b9f755e465532f90ae6e696bdd7fa4e9d589ce Mon Sep 17 00:00:00 2001 From: Michael Voznesensky Date: Mon, 2 May 2022 12:30:19 -0700 Subject: [PATCH 1/2] Replace error printing code gated by NDEBUG with a new flag: PYBIND11_DETAILED_ERROR_MESSAGES (#3913) * Update cast.h * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Move definition to detail/common, change name, apply everywhere * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Rename debug_enabled in tests to detailed_error_messages_enabled --- include/pybind11/attr.h | 6 ++- include/pybind11/cast.h | 51 ++++++++++++---------- include/pybind11/detail/common.h | 7 +++ include/pybind11/detail/type_caster_base.h | 31 +++++++------ include/pybind11/gil.h | 6 +-- include/pybind11/pybind11.h | 9 ++-- tests/pybind11_tests.cpp | 6 +-- tests/test_methods_and_attributes.cpp | 6 +-- tests/test_methods_and_attributes.py | 20 ++++----- tests/test_pytypes.py | 6 +-- 10 files changed, 82 insertions(+), 66 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index 38139afa3..14600e9bb 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -10,6 +10,7 @@ #pragma once +#include "detail/common.h" #include "cast.h" #include @@ -477,7 +478,7 @@ struct process_attribute : process_attribute_default { } if (!a.value) { -#if !defined(NDEBUG) +#if defined(PYBIND11_DETAILED_ERROR_MESSAGES) std::string descr("'"); if (a.name) { descr += std::string(a.name) + ": "; @@ -498,7 +499,8 @@ struct process_attribute : process_attribute_default { #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); diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index e8128710e..22d2dba4c 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -777,8 +777,9 @@ protected: return true; } throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " -#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() + "''"); @@ -1001,9 +1002,9 @@ struct return_value_policy_override< template type_caster &load_type(type_caster &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 '" @@ -1068,10 +1069,10 @@ inline void handle::cast() const { template detail::enable_if_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() @@ -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, @@ -1197,7 +1198,7 @@ tuple make_tuple(Args &&...args_) { detail::make_caster::cast(std::forward(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 argtypes{{type_id()...}}; @@ -1249,7 +1250,7 @@ private: : arg(base), value(reinterpret_steal(detail::make_caster::cast( std::forward(x), return_value_policy::automatic, {}))), descr(descr) -#if !defined(NDEBUG) +#if defined(PYBIND11_DETAILED_ERROR_MESSAGES) , type(type_id()) #endif @@ -1289,7 +1290,7 @@ public: 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 @@ -1487,7 +1488,7 @@ private: auto o = reinterpret_steal( detail::make_caster::cast(std::forward(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()), @@ -1505,21 +1506,21 @@ private: 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); @@ -1534,7 +1535,7 @@ private: } for (auto k : reinterpret_borrow(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)); @@ -1545,9 +1546,10 @@ private: } [[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 @@ -1555,8 +1557,9 @@ private: "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) { @@ -1603,7 +1606,7 @@ unpacking_collector collect_arguments(Args &&...args) { template template object object_api::operator()(Args &&...args) const { -#ifndef NDEBUG +#if defined(PYBIND11_DETAILED_ERROR_MESSAGES) if (!PyGILState_Check()) { pybind11_fail("pybind11::object_api<>::operator() PyGILState_Check() failure."); } diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index a130ebea0..9355ecfda 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -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) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 5b2ec8e1e..a7b977132 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -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 } @@ -612,14 +613,15 @@ public: 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; @@ -631,15 +633,16 @@ public: } 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; diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index 446f4620b..a0b5de151 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -62,7 +62,7 @@ public: 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!"); } @@ -84,7 +84,7 @@ public: 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!"); } @@ -93,7 +93,7 @@ public: } # 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!"); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index e828f725d..cd86152e5 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -370,7 +370,7 @@ protected: 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()); @@ -518,8 +518,9 @@ protected: 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 " @@ -906,7 +907,7 @@ protected: // 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!"); diff --git a/tests/pybind11_tests.cpp b/tests/pybind11_tests.cpp index d9b88fcc2..3c0469915 100644 --- a/tests/pybind11_tests.cpp +++ b/tests/pybind11_tests.cpp @@ -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_(m, "UserType", "A `py::class_` type for testing") diff --git a/tests/test_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp index 10515e26f..815dd5e98 100644 --- a/tests/test_methods_and_attributes.cpp +++ b/tests/test_methods_and_attributes.cpp @@ -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"); diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 5a95303e1..54597fa78 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -216,15 +216,15 @@ def test_metaclass_override(): def test_no_mixed_overloads(): - from pybind11_tests import debug_enabled + from pybind11_tests import detailed_error_messages_enabled with pytest.raises(RuntimeError) as excinfo: m.ExampleMandA.add_mixed_overloads1() assert str( excinfo.value ) == "overloading a method with both static and instance methods is not supported; " + ( - "compile in debug mode for more details" - if not debug_enabled + "#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for more details" + if not detailed_error_messages_enabled else "error while attempting to bind static method ExampleMandA.overload_mixed1" "(arg0: float) -> str" ) @@ -234,8 +234,8 @@ def test_no_mixed_overloads(): assert str( excinfo.value ) == "overloading a method with both static and instance methods is not supported; " + ( - "compile in debug mode for more details" - if not debug_enabled + "#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for more details" + if not detailed_error_messages_enabled else "error while attempting to bind instance method ExampleMandA.overload_mixed2" "(self: pybind11_tests.methods_and_attributes.ExampleMandA, arg0: int, arg1: int)" " -> str" @@ -344,16 +344,16 @@ def test_cyclic_gc(): def test_bad_arg_default(msg): - from pybind11_tests import debug_enabled + from pybind11_tests import detailed_error_messages_enabled with pytest.raises(RuntimeError) as excinfo: m.bad_arg_def_named() assert msg(excinfo.value) == ( "arg(): could not convert default argument 'a: UnregisteredType' in function " "'should_fail' into a Python object (type not registered yet?)" - if debug_enabled + if detailed_error_messages_enabled else "arg(): could not convert default argument into a Python object (type not registered " - "yet?). Compile in debug mode for more information." + "yet?). #define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for more information." ) with pytest.raises(RuntimeError) as excinfo: @@ -361,9 +361,9 @@ def test_bad_arg_default(msg): assert msg(excinfo.value) == ( "arg(): could not convert default argument 'UnregisteredType' in function " "'should_fail' into a Python object (type not registered yet?)" - if debug_enabled + if detailed_error_messages_enabled else "arg(): could not convert default argument into a Python object (type not registered " - "yet?). Compile in debug mode for more information." + "yet?). #define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for more information." ) diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 5c715ada6..c740414ae 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -4,7 +4,7 @@ import sys import pytest import env -from pybind11_tests import debug_enabled +from pybind11_tests import detailed_error_messages_enabled from pybind11_tests import pytypes as m @@ -416,8 +416,8 @@ def test_print(capture): m.print_failure() assert str(excinfo.value) == "Unable to convert call argument " + ( "'1' of type 'UnregisteredType' to Python object" - if debug_enabled - else "to Python object (compile in debug mode for details)" + if detailed_error_messages_enabled + else "to Python object (#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)" ) From 287e4f233dcaea92398810b7ff95b7137a96f5a2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 2 May 2022 12:39:36 -0700 Subject: [PATCH 2/2] Test pickling a simple callable (does not work). (#3906) * Test pickling a simple callable (does not work). Currently only documents that it does not work. Starting point for future fix. * Use re.search to accommodate variations of the TypeError message. * PyPy: exercise full dumps/loads cycle. * Adding explicit "broken" comment. --- tests/test_pickling.cpp | 2 ++ tests/test_pickling.py | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/tests/test_pickling.cpp b/tests/test_pickling.cpp index 9787bd300..e154bc483 100644 --- a/tests/test_pickling.cpp +++ b/tests/test_pickling.cpp @@ -61,6 +61,8 @@ void wrap(py::module m) { } // namespace exercise_trampoline TEST_SUBMODULE(pickling, m) { + m.def("simple_callable", []() { return 20220426; }); + // test_roundtrip class Pickleable { public: diff --git a/tests/test_pickling.py b/tests/test_pickling.py index 52802ace8..12361a661 100644 --- a/tests/test_pickling.py +++ b/tests/test_pickling.py @@ -1,4 +1,5 @@ import pickle +import re import pytest @@ -6,6 +7,20 @@ import env from pybind11_tests import pickling as m +def test_pickle_simple_callable(): + assert m.simple_callable() == 20220426 + if env.PYPY: + serialized = pickle.dumps(m.simple_callable) + deserialized = pickle.loads(serialized) + assert deserialized() == 20220426 + else: + # To document broken behavior: currently it fails universally with + # all C Python versions. + with pytest.raises(TypeError) as excinfo: + pickle.dumps(m.simple_callable) + assert re.search("can.*t pickle .*PyCapsule.* object", str(excinfo.value)) + + @pytest.mark.parametrize("cls_name", ["Pickleable", "PickleableNew"]) def test_roundtrip(cls_name): cls = getattr(m, cls_name)