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

Author: YannickJadoul

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 {

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<std::string>() + "." + type->tp_name;
+    auto module_name = handle((PyObject *) type).attr("__module__").cast<std::string>();
+    if (module_name == PYBIND11_BUILTINS_MODULE)
+        return type->tp_name;
+    else
+        return std::move(module_name) + "." + type->tp_name;
 #endif
 }
 

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<T> *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)

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) \

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<py::type>(ob))
-            return tp;
-        else
-            throw std::runtime_error("Invalid type");
+        return py::type(ob);
     });
 
     // test_mismatched_holder

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
 
 

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";

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