From 6f873cf53ff75278ea76424886be1aa7a2278c56 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Fri, 31 Jul 2020 16:44:05 +0200 Subject: [PATCH 1/7] Fail on passing py::object with wrong Python type to py::object subclass using PYBIND11_OBJECT macro --- include/pybind11/pytypes.h | 9 +++++++-- tests/test_pytypes.cpp | 13 +++++++++++++ tests/test_pytypes.py | 12 ++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 48b5240836..4095fe5298 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -812,11 +812,16 @@ PYBIND11_NAMESPACE_END(detail) : Parent(check_(o) ? o.release().ptr() : ConvertFun(o.ptr()), stolen_t{}) \ { if (!m_ptr) throw error_already_set(); } +#define PYBIND11_OBJECT_CHECK_FAILED(Name, o) \ + type_error("Object of type '" + std::string(Py_TYPE(o.ptr())->tp_name) + "' is not an instance of '" #Name "'") + #define PYBIND11_OBJECT(Name, Parent, CheckFun) \ PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \ /* This is deliberately not 'explicit' to allow implicit conversion from object: */ \ - Name(const object &o) : Parent(o) { } \ - Name(object &&o) : Parent(std::move(o)) { } + Name(const object &o) : Parent(o) \ + { if (o && !check_(o)) throw PYBIND11_OBJECT_CHECK_FAILED(Name, o); } \ + Name(object &&o) : Parent(std::move(o)) \ + { if (o && !check_(o)) throw PYBIND11_OBJECT_CHECK_FAILED(Name, o); } #define PYBIND11_OBJECT_DEFAULT(Name, Parent, CheckFun) \ PYBIND11_OBJECT(Name, Parent, CheckFun) \ diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index d38ea197d4..8717f601eb 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -243,6 +243,19 @@ TEST_SUBMODULE(pytypes, m) { m.def("convert_to_pybind11_str", [](py::object o) { return py::str(o); }); + m.def("nonconverting_constructor", [](std::string type, py::object value) -> py::object { + if (type == "bytes") { + return py::bytes(value); + } + else if (type == "none") { + return py::none(value); + } + else if (type == "ellipsis") { + return py::ellipsis(value); + } + throw std::runtime_error("Invalid type"); + }); + m.def("get_implicit_casting", []() { py::dict d; d["char*_i1"] = "abc"; diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 0618cd54c9..1608f9339b 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -244,6 +244,18 @@ def test_constructors(): for k in noconv2: assert noconv2[k] is expected[k] + type_error_tests = [ + ("bytes", range(10)), + ("none", 42), + ("ellipsis", 42), + ] + for t, v in type_error_tests: + with pytest.raises(TypeError) as excinfo: + m.nonconverting_constructor(t, v) + expected_error = "Object of type '{}' is not an instance of '{}'".format( + type(v).__name__, t) + assert str(excinfo.value) == expected_error + def test_pybind11_str_raw_str(): # specifically to exercise pybind11::str::raw_str From 1e99e9cf66481af4967dc065ec2e8ff22a138c39 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Mon, 17 Aug 2020 01:34:55 +0200 Subject: [PATCH 2/7] Split off test_non_converting_constructors from test_constructors --- tests/test_pytypes.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 1608f9339b..ab51b5515a 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -244,12 +244,14 @@ def test_constructors(): for k in noconv2: assert noconv2[k] is expected[k] - type_error_tests = [ + +def test_non_converting_constructors(): + non_converting_test_cases = [ ("bytes", range(10)), ("none", 42), ("ellipsis", 42), ] - for t, v in type_error_tests: + for t, v in non_converting_test_cases: with pytest.raises(TypeError) as excinfo: m.nonconverting_constructor(t, v) expected_error = "Object of type '{}' is not an instance of '{}'".format( From 22f9557661a002e3e0beb97332ca99ee32d752c3 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Mon, 21 Sep 2020 14:33:59 +0200 Subject: [PATCH 3/7] Fix test_as_type, as py::type constructor now throws an error itself if the argument is not a type --- tests/test_class.cpp | 6 +----- tests/test_class.py | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 2d4aef7aff..697e3f8bc5 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -157,11 +157,7 @@ TEST_SUBMODULE(class_, m) { }); m.def("as_type", [](py::object ob) { - auto tp = py::type(ob); - if (py::isinstance(ob)) - return tp; - else - throw std::runtime_error("Invalid type"); + return py::type(ob); }); // test_mismatched_holder diff --git a/tests/test_class.py b/tests/test_class.py index 38eb55ff54..87a612d90a 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -59,10 +59,10 @@ def test_type_of_py_nodelete(): def test_as_type_py(): assert m.as_type(int) == int - with pytest.raises(RuntimeError): + with pytest.raises(TypeError): assert m.as_type(1) == int - with pytest.raises(RuntimeError): + with pytest.raises(TypeError): assert m.as_type(m.DerivedClass1()) == m.DerivedClass1 From 4ac0bd43d7991b64fd78a80a19d79c521c4ea63c Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Fri, 2 Oct 2020 22:41:59 +0200 Subject: [PATCH 4/7] Replace tp_name access by pybind11::detail::get_fully_qualified_tp_name --- include/pybind11/pytypes.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 4095fe5298..e292b74fe6 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -813,7 +813,9 @@ PYBIND11_NAMESPACE_END(detail) { if (!m_ptr) throw error_already_set(); } #define PYBIND11_OBJECT_CHECK_FAILED(Name, o) \ - type_error("Object of type '" + std::string(Py_TYPE(o.ptr())->tp_name) + "' is not an instance of '" #Name "'") + type_error("Object of type '" + \ + pybind11::detail::get_fully_qualified_tp_name(Py_TYPE(o.ptr())) + \ + "' is not an instance of '" #Name "'") #define PYBIND11_OBJECT(Name, Parent, CheckFun) \ PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \ From d8664ad94325b0090f313858f5ba70f18e0a9fe7 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Fri, 2 Oct 2020 22:51:32 +0200 Subject: [PATCH 5/7] Move forward-declaration of get_fully_qualified_tp_name to detail/common.h --- include/pybind11/cast.h | 3 --- include/pybind11/detail/common.h | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 835406e874..c540871b2c 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -39,9 +39,6 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) PYBIND11_NAMESPACE_BEGIN(detail) -// Forward-declaration; see detail/class.h -std::string get_fully_qualified_tp_name(PyTypeObject*); - /// A life support system for temporary objects created by `type_caster::load()`. /// Adding a patient will keep it alive up until the enclosing function returns. class loader_life_support { diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 7e06287d1c..b4a48cb21c 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -853,8 +853,8 @@ class any_container { const std::vector *operator->() const { return &v; } }; -PYBIND11_NAMESPACE_END(detail) - - +// Forward-declaration; see detail/class.h +std::string get_fully_qualified_tp_name(PyTypeObject*); +PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) From f2424c5082d18fe6a141db02580fa0d05ebb6c5e Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Mon, 5 Oct 2020 00:10:33 +0200 Subject: [PATCH 6/7] Don't add the builtins module name in get_fully_qualified_tp_name for PyPy --- include/pybind11/detail/class.h | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 8c3c3a0ea0..d912336f72 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -28,7 +28,20 @@ inline std::string get_fully_qualified_tp_name(PyTypeObject *type) { #if !defined(PYPY_VERSION) return type->tp_name; #else - return handle((PyObject *) type).attr("__module__").cast() + "." + type->tp_name; + auto module_name = handle((PyObject *) type).attr("__module__").cast(); + bool is_builtin_module = module_name == +# if PY_MAJOR_VERSION >= 3 + "builtins" +# else + "__builtin__" +# endif + ; + if (is_builtin_module) { + return type->tp_name; + } + else { + return std::move(module_name) + "." + type->tp_name; + } #endif } From 6b1cfdfae6a9e77ee5117be0c4954cafc1ed60b8 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Mon, 5 Oct 2020 22:18:00 +0200 Subject: [PATCH 7/7] Add PYBIND11_BUILTINS_MODULE macro, and use it in get_fully_qualified_tp_name --- include/pybind11/detail/class.h | 13 ++----------- include/pybind11/detail/common.h | 2 ++ 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index d912336f72..bb4c145aeb 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -29,19 +29,10 @@ inline std::string get_fully_qualified_tp_name(PyTypeObject *type) { return type->tp_name; #else auto module_name = handle((PyObject *) type).attr("__module__").cast(); - bool is_builtin_module = module_name == -# if PY_MAJOR_VERSION >= 3 - "builtins" -# else - "__builtin__" -# endif - ; - if (is_builtin_module) { + if (module_name == PYBIND11_BUILTINS_MODULE) return type->tp_name; - } - else { + else return std::move(module_name) + "." + type->tp_name; - } #endif } diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index b4a48cb21c..a48de04399 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -182,6 +182,7 @@ #define PYBIND11_STR_TYPE ::pybind11::str #define PYBIND11_BOOL_ATTR "__bool__" #define PYBIND11_NB_BOOL(ptr) ((ptr)->nb_bool) +#define PYBIND11_BUILTINS_MODULE "builtins" // Providing a separate declaration to make Clang's -Wmissing-prototypes happy. // See comment for PYBIND11_MODULE below for why this is marked "maybe unused". #define PYBIND11_PLUGIN_IMPL(name) \ @@ -209,6 +210,7 @@ #define PYBIND11_STR_TYPE ::pybind11::bytes #define PYBIND11_BOOL_ATTR "__nonzero__" #define PYBIND11_NB_BOOL(ptr) ((ptr)->nb_nonzero) +#define PYBIND11_BUILTINS_MODULE "__builtin__" // Providing a separate PyInit decl to make Clang's -Wmissing-prototypes happy. // See comment for PYBIND11_MODULE below for why this is marked "maybe unused". #define PYBIND11_PLUGIN_IMPL(name) \