From f537093a2fc1b42e084479a07cfa01512837492e Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Mon, 5 Oct 2020 22:48:54 +0200 Subject: [PATCH] Fail on passing py::object with wrong Python type to py::object subclass using PYBIND11_OBJECT macro (#2349) * Fail on passing py::object with wrong Python type to py::object subclass using PYBIND11_OBJECT macro * Split off test_non_converting_constructors from test_constructors * Fix test_as_type, as py::type constructor now throws an error itself if the argument is not a type * Replace tp_name access by pybind11::detail::get_fully_qualified_tp_name * Move forward-declaration of get_fully_qualified_tp_name to detail/common.h * Don't add the builtins module name in get_fully_qualified_tp_name for PyPy * Add PYBIND11_BUILTINS_MODULE macro, and use it in get_fully_qualified_tp_name --- include/pybind11/cast.h | 3 --- include/pybind11/detail/class.h | 6 +++++- include/pybind11/detail/common.h | 8 +++++--- include/pybind11/pytypes.h | 11 +++++++++-- tests/test_class.cpp | 6 +----- tests/test_class.py | 4 ++-- tests/test_pytypes.cpp | 13 +++++++++++++ tests/test_pytypes.py | 14 ++++++++++++++ 8 files changed, 49 insertions(+), 16 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 469c42adf6..2dadfc92ff 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/class.h b/include/pybind11/detail/class.h index 9316865d23..54a857388d 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -28,7 +28,11 @@ 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(); + if (module_name == PYBIND11_BUILTINS_MODULE) + return type->tp_name; + 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 7e06287d1c..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) \ @@ -853,8 +855,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) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 48b5240836..e292b74fe6 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -812,11 +812,18 @@ 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 '" + \ + 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) \ /* 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_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 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..ab51b5515a 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -245,6 +245,20 @@ def test_constructors(): assert noconv2[k] is expected[k] +def test_non_converting_constructors(): + non_converting_test_cases = [ + ("bytes", range(10)), + ("none", 42), + ("ellipsis", 42), + ] + 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( + type(v).__name__, t) + assert str(excinfo.value) == expected_error + + def test_pybind11_str_raw_str(): # specifically to exercise pybind11::str::raw_str cvt = m.convert_to_pybind11_str