Check scope's __dict__ instead of using hasattr when registering classes and exceptions (#2335)

* Check scope's __dict__ instead of using hasattr when registering classes and exceptions, to allow registering the same name in a derived class scope

* Extend test_base_and_derived_nested_scope test

* Add tests on error being thrown registering duplicate classes

* Circumvent bug with combination of test_class.py::test_register_duplicate_class and test_factory_constructors.py::test_init_factory_alias

Author: YannickJadoul

include/pybind11/pybind11.h

@@ -1012,7 +1012,7 @@ class generic_type : public object {
     PYBIND11_OBJECT_DEFAULT(generic_type, object, PyType_Check)
 protected:
     void initialize(const type_record &rec) {
-        if (rec.scope && hasattr(rec.scope, rec.name))
+        if (rec.scope && hasattr(rec.scope, "__dict__") && rec.scope.attr("__dict__").contains(rec.name))
             pybind11_fail("generic_type: cannot initialize type \"" + std::string(rec.name) +
                           "\": an object with that name is already defined");
 
@@ -1930,7 +1930,7 @@ class exception : public object {
         std::string full_name = scope.attr("__name__").cast<std::string>() +
                                 std::string(".") + name;
         m_ptr = PyErr_NewException(const_cast<char *>(full_name.c_str()), base.ptr(), NULL);
-        if (hasattr(scope, name))
+        if (hasattr(scope, "__dict__") && scope.attr("__dict__").contains(name))
             pybind11_fail("Error during initialization: multiple incompatible "
                           "definitions with name \"" + std::string(name) + "\"");
         scope.attr(name) = *this;

tests/test_class.cpp

@@ -406,6 +406,7 @@ TEST_SUBMODULE(class_, m) {
     struct IsNonFinalFinal {};
     py::class_<IsNonFinalFinal>(m, "IsNonFinalFinal", py::is_final());
 
+    // test_exception_rvalue_abort
     struct PyPrintDestructor {
         PyPrintDestructor() = default;
         ~PyPrintDestructor() {
@@ -417,6 +418,7 @@ TEST_SUBMODULE(class_, m) {
         .def(py::init<>())
         .def("throw_something", &PyPrintDestructor::throw_something);
 
+    // test_multiple_instances_with_same_pointer
     struct SamePointer {};
     static SamePointer samePointer;
     py::class_<SamePointer, std::unique_ptr<SamePointer, py::nodelete>>(m, "SamePointer")
@@ -426,6 +428,44 @@ TEST_SUBMODULE(class_, m) {
     struct Empty {};
     py::class_<Empty>(m, "Empty")
         .def(py::init<>());
+
+    // test_base_and_derived_nested_scope
+    struct BaseWithNested {
+        struct Nested {};
+    };
+
+    struct DerivedWithNested : BaseWithNested {
+        struct Nested {};
+    };
+
+    py::class_<BaseWithNested> baseWithNested_class(m, "BaseWithNested");
+    py::class_<DerivedWithNested, BaseWithNested> derivedWithNested_class(m, "DerivedWithNested");
+    py::class_<BaseWithNested::Nested>(baseWithNested_class, "Nested")
+        .def_static("get_name", []() { return "BaseWithNested::Nested"; });
+    py::class_<DerivedWithNested::Nested>(derivedWithNested_class, "Nested")
+        .def_static("get_name", []() { return "DerivedWithNested::Nested"; });
+
+    // test_register_duplicate_class
+    struct Duplicate {};
+    struct OtherDuplicate {};
+    struct DuplicateNested {};
+    struct OtherDuplicateNested {};
+    m.def("register_duplicate_class_name", [](py::module_ m) {
+        py::class_<Duplicate>(m, "Duplicate");
+        py::class_<OtherDuplicate>(m, "Duplicate");
+    });
+    m.def("register_duplicate_class_type", [](py::module_ m) {
+        py::class_<OtherDuplicate>(m, "OtherDuplicate");
+        py::class_<OtherDuplicate>(m, "YetAnotherDuplicate");
+    });
+    m.def("register_duplicate_nested_class_name", [](py::object gt) {
+        py::class_<DuplicateNested>(gt, "DuplicateNested");
+        py::class_<OtherDuplicateNested>(gt, "DuplicateNested");
+    });
+    m.def("register_duplicate_nested_class_type", [](py::object gt) {
+        py::class_<OtherDuplicateNested>(gt, "OtherDuplicateNested");
+        py::class_<OtherDuplicateNested>(gt, "YetAnotherDuplicateNested");
+    });
 }
 
 template <int N> class BreaksBase { public:

tests/test_class.py

@@ -383,3 +383,38 @@ def test_multiple_instances_with_same_pointer(capture):
     # No assert: if this does not trigger the error
     #   pybind11_fail("pybind11_object_dealloc(): Tried to deallocate unregistered instance!");
     # and just completes without crashing, we're good.
+
+
+# https://github.com/pybind/pybind11/issues/1624
+def test_base_and_derived_nested_scope():
+    assert issubclass(m.DerivedWithNested, m.BaseWithNested)
+    assert m.BaseWithNested.Nested != m.DerivedWithNested.Nested
+    assert m.BaseWithNested.Nested.get_name() == "BaseWithNested::Nested"
+    assert m.DerivedWithNested.Nested.get_name() == "DerivedWithNested::Nested"
+
+
+@pytest.mark.skip("See https://github.com/pybind/pybind11/pull/2564")
+def test_register_duplicate_class():
+    import types
+    module_scope = types.ModuleType("module_scope")
+    with pytest.raises(RuntimeError) as exc_info:
+        m.register_duplicate_class_name(module_scope)
+    expected = ('generic_type: cannot initialize type "Duplicate": '
+                'an object with that name is already defined')
+    assert str(exc_info.value) == expected
+    with pytest.raises(RuntimeError) as exc_info:
+        m.register_duplicate_class_type(module_scope)
+    expected = 'generic_type: type "YetAnotherDuplicate" is already registered!'
+    assert str(exc_info.value) == expected
+
+    class ClassScope:
+        pass
+    with pytest.raises(RuntimeError) as exc_info:
+        m.register_duplicate_nested_class_name(ClassScope)
+    expected = ('generic_type: cannot initialize type "DuplicateNested": '
+                'an object with that name is already defined')
+    assert str(exc_info.value) == expected
+    with pytest.raises(RuntimeError) as exc_info:
+        m.register_duplicate_nested_class_type(ClassScope)
+    expected = 'generic_type: type "YetAnotherDuplicateNested" is already registered!'
+    assert str(exc_info.value) == expected