From 71aea49b8bce3019b1b5b19f1aebe2d2125b21c6 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Fri, 9 Oct 2020 01:09:56 +0200 Subject: [PATCH] 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 --- include/pybind11/pybind11.h | 4 ++-- tests/test_class.cpp | 40 +++++++++++++++++++++++++++++++++++++ tests/test_class.py | 35 ++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 1f1e310d5..76c53bff9 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1012,7 +1012,7 @@ public: 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 @@ public: std::string full_name = scope.attr("__name__").cast() + std::string(".") + name; m_ptr = PyErr_NewException(const_cast(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; diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 697e3f8bc..890fab736 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -406,6 +406,7 @@ TEST_SUBMODULE(class_, m) { struct IsNonFinalFinal {}; py::class_(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_>(m, "SamePointer") @@ -426,6 +428,44 @@ TEST_SUBMODULE(class_, m) { struct Empty {}; py::class_(m, "Empty") .def(py::init<>()); + + // test_base_and_derived_nested_scope + struct BaseWithNested { + struct Nested {}; + }; + + struct DerivedWithNested : BaseWithNested { + struct Nested {}; + }; + + py::class_ baseWithNested_class(m, "BaseWithNested"); + py::class_ derivedWithNested_class(m, "DerivedWithNested"); + py::class_(baseWithNested_class, "Nested") + .def_static("get_name", []() { return "BaseWithNested::Nested"; }); + py::class_(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_(m, "Duplicate"); + py::class_(m, "Duplicate"); + }); + m.def("register_duplicate_class_type", [](py::module_ m) { + py::class_(m, "OtherDuplicate"); + py::class_(m, "YetAnotherDuplicate"); + }); + m.def("register_duplicate_nested_class_name", [](py::object gt) { + py::class_(gt, "DuplicateNested"); + py::class_(gt, "DuplicateNested"); + }); + m.def("register_duplicate_nested_class_type", [](py::object gt) { + py::class_(gt, "OtherDuplicateNested"); + py::class_(gt, "YetAnotherDuplicateNested"); + }); } template class BreaksBase { public: diff --git a/tests/test_class.py b/tests/test_class.py index 87a612d90..10e4dd79b 100644 --- a/tests/test_class.py +++ b/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