From 2c62f5951cf21e10a57345e7df3acb4ceea3048b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 25 Jan 2024 14:18:32 -0800 Subject: [PATCH 1/7] Use `py::metaclass((PyObject *) &PyType_Type)` and test compatibility with `metaclass=abc.ABCMeta` --- tests/CMakeLists.txt | 3 ++- tests/test_wip.cpp | 17 +++++++++++++++++ tests/test_wip.py | 12 ++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 tests/test_wip.cpp create mode 100644 tests/test_wip.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 91cd230b..73550f3c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -186,7 +186,8 @@ set(PYBIND11_TEST_FILES test_unnamed_namespace_a test_unnamed_namespace_b test_vector_unique_ptr_member - test_virtual_functions) + test_virtual_functions + test_wip) # Invoking cmake with something like: # cmake -DPYBIND11_TEST_OVERRIDE="test_callbacks.cpp;test_pickling.cpp" .. diff --git a/tests/test_wip.cpp b/tests/test_wip.cpp new file mode 100644 index 00000000..02b2cb6e --- /dev/null +++ b/tests/test_wip.cpp @@ -0,0 +1,17 @@ +#include + +#include "pybind11_tests.h" + +namespace pybind11_tests { +namespace wip { + +struct Vanilla {}; + +} // namespace wip +} // namespace pybind11_tests + +TEST_SUBMODULE(wip, m) { + using namespace pybind11_tests::wip; + + py::class_(m, "Vanilla", py::metaclass((PyObject *) &PyType_Type)).def(py::init<>()); +} diff --git a/tests/test_wip.py b/tests/test_wip.py new file mode 100644 index 00000000..219c2759 --- /dev/null +++ b/tests/test_wip.py @@ -0,0 +1,12 @@ +import abc + +from pybind11_tests import wip as m + + +class VanillaABC(m.Vanilla, metaclass=abc.ABCMeta): + pass + + +def test_vanilla(): + assert m.Vanilla().__class__.__name__ == "Vanilla" + assert type(m.Vanilla).__name__ == "type" From 20a41392066592d93db1ac5280173b4657ce4629 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 25 Jan 2024 14:16:55 -0800 Subject: [PATCH 2/7] `@__setstate__` support (first stab) --- include/pybind11/pybind11.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 004baea2..48438a1d 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -403,7 +403,13 @@ class cpp_function : public function { strdup_guard guarded_strdup; /* Create copies of all referenced C-style strings */ - rec->name = guarded_strdup(rec->name ? rec->name : ""); + bool setstate_is_ctor = true; + if (rec->name && std::strcmp(rec->name, "@__setstate__") == 0) { + rec->name = guarded_strdup(rec->name + 1); + setstate_is_ctor = false; + } else { + rec->name = guarded_strdup(rec->name ? rec->name : ""); + } if (rec->doc) { rec->doc = guarded_strdup(rec->doc); } @@ -419,7 +425,7 @@ class cpp_function : public function { } rec->is_constructor = (std::strcmp(rec->name, "__init__") == 0) - || (std::strcmp(rec->name, "__setstate__") == 0); + || (setstate_is_ctor && std::strcmp(rec->name, "__setstate__") == 0); #if defined(PYBIND11_DETAILED_ERROR_MESSAGES) && !defined(PYBIND11_DISABLE_NEW_STYLE_INIT_WARNING) if (rec->is_constructor && !rec->is_new_style_constructor) { From 53e816004265d738617a3f92e16d7b6491b4f474 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 25 Jan 2024 15:50:49 -0800 Subject: [PATCH 3/7] Add `test_abc_meta_incompatibility()`, `test_abc_meta_compatibility()` to test_methods_and_attributes.py --- tests/test_methods_and_attributes.py | 17 +++++++++++++++++ tests/test_wip.py | 7 ------- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 8af4afb5..fa72bb10 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -1,3 +1,4 @@ +import abc import sys import pytest @@ -227,6 +228,22 @@ def test_metaclass_override(): assert isinstance(m.MetaclassOverride.__dict__["readonly"], int) +def test_abc_meta_incompatibility(): # Mostly to clearly expose the behavior. + with pytest.raises(TypeError) as exc_info: + + class ExampleMandAABC(m.ExampleMandA, metaclass=abc.ABCMeta): + pass + + assert "metaclass conflict" in str(exc_info.value) + + +def test_abc_meta_compatibility(): + class MetaclassOverrideABC(m.MetaclassOverride, metaclass=abc.ABCMeta): + pass + + assert type(MetaclassOverrideABC).__name__ == "ABCMeta" + + def test_no_mixed_overloads(): from pybind11_tests import detailed_error_messages_enabled diff --git a/tests/test_wip.py b/tests/test_wip.py index 219c2759..d07b008f 100644 --- a/tests/test_wip.py +++ b/tests/test_wip.py @@ -1,12 +1,5 @@ -import abc - from pybind11_tests import wip as m -class VanillaABC(m.Vanilla, metaclass=abc.ABCMeta): - pass - - def test_vanilla(): assert m.Vanilla().__class__.__name__ == "Vanilla" - assert type(m.Vanilla).__name__ == "type" From 10b0796c7057c8885403395f346d24c35feb3be3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 25 Jan 2024 17:14:54 -0800 Subject: [PATCH 4/7] Add `exercise_getinitargs_getstate_setstate` to test_pickling --- tests/CMakeLists.txt | 3 +-- tests/test_pickling.cpp | 45 +++++++++++++++++++++++++++++++++++++++++ tests/test_pickling.py | 20 ++++++++++++++++++ tests/test_wip.cpp | 17 ---------------- tests/test_wip.py | 5 ----- 5 files changed, 66 insertions(+), 24 deletions(-) delete mode 100644 tests/test_wip.cpp delete mode 100644 tests/test_wip.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 73550f3c..91cd230b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -186,8 +186,7 @@ set(PYBIND11_TEST_FILES test_unnamed_namespace_a test_unnamed_namespace_b test_vector_unique_ptr_member - test_virtual_functions - test_wip) + test_virtual_functions) # Invoking cmake with something like: # cmake -DPYBIND11_TEST_OVERRIDE="test_callbacks.cpp;test_pickling.cpp" .. diff --git a/tests/test_pickling.cpp b/tests/test_pickling.cpp index e154bc48..a3671793 100644 --- a/tests/test_pickling.cpp +++ b/tests/test_pickling.cpp @@ -8,11 +8,14 @@ BSD-style license that can be found in the LICENSE file. */ +#include + #include "pybind11_tests.h" #include #include #include +#include namespace exercise_trampoline { @@ -60,6 +63,47 @@ void wrap(py::module m) { } // namespace exercise_trampoline +namespace exercise_getinitargs_getstate_setstate { + +// Mechanism similar to what was established with Boost.Python: +// https://www.boost.org/doc/libs/1_58_0/libs/python/doc/v2/pickle.html +// This mechanism was also adopted for PyCLIF: +// https://github.com/google/clif/blob/7d388e1de7db5beeb3d7429c18a2776d8188f44f/clif/testing/python/pickle_compatibility.clif + +class StoreTwoWithState { +public: + StoreTwoWithState(int v0, int v1) : values{v0, v1}, state{"blank"} {} + const std::vector &GetInitArgs() const { return values; } + const std::string &GetState() const { return state; } + void SetState(const std::string &state) { this->state = state; } + +private: + std::vector values; + std::string state; +}; + +void wrap(py::module m) { + py::class_(std::move(m), "StoreTwoWithState") + .def(py::init()) + .def("__getinitargs__", + [](const StoreTwoWithState &self) { return py::tuple(py::cast(self.GetInitArgs())); }) + .def("__getstate__", &StoreTwoWithState::GetState) + .def( + "__reduce_ex__", + [](py::handle self, int /*protocol*/) { + return py::make_tuple(self.attr("__class__"), + self.attr("__getinitargs__")(), + self.attr("__getstate__")()); + }, + py::arg("protocol") = -1) + .def( + "@__setstate__", // See google/pywrapcc#30094 for background. + [](StoreTwoWithState *self, const std::string &state) { self->SetState(state); }, + py::arg("state")); +} + +} // namespace exercise_getinitargs_getstate_setstate + TEST_SUBMODULE(pickling, m) { m.def("simple_callable", []() { return 20220426; }); @@ -191,4 +235,5 @@ TEST_SUBMODULE(pickling, m) { #endif exercise_trampoline::wrap(m); + exercise_getinitargs_getstate_setstate::wrap(m); } diff --git a/tests/test_pickling.py b/tests/test_pickling.py index 12361a66..859ac087 100644 --- a/tests/test_pickling.py +++ b/tests/test_pickling.py @@ -1,3 +1,4 @@ +import copy import pickle import re @@ -91,3 +92,22 @@ def test_roundtrip_simple_cpp_derived(): # Issue #3062: pickleable base C++ classes can incur object slicing # if derived typeid is not registered with pybind11 assert not m.check_dynamic_cast_SimpleCppDerived(p2) + + +# +# exercise_getinitargs_getstate_setstate +# +def test_StoreTwoWithState(): + obj = m.StoreTwoWithState(-38, 27) + assert obj.__getinitargs__() == (-38, 27) + assert obj.__getstate__() == "blank" + obj.__setstate__("blue") + reduced = obj.__reduce_ex__() + assert reduced == (m.StoreTwoWithState, (-38, 27), "blue") + cpy = copy.deepcopy(obj) + assert cpy.__reduce_ex__() == reduced + assert pickle.HIGHEST_PROTOCOL > 0 # To be sure the loop below makes sense. + for protocol in range(-1, pickle.HIGHEST_PROTOCOL + 1): + serialized = pickle.dumps(obj, protocol) + restored = pickle.loads(serialized) + assert restored.__reduce_ex__() == reduced diff --git a/tests/test_wip.cpp b/tests/test_wip.cpp deleted file mode 100644 index 02b2cb6e..00000000 --- a/tests/test_wip.cpp +++ /dev/null @@ -1,17 +0,0 @@ -#include - -#include "pybind11_tests.h" - -namespace pybind11_tests { -namespace wip { - -struct Vanilla {}; - -} // namespace wip -} // namespace pybind11_tests - -TEST_SUBMODULE(wip, m) { - using namespace pybind11_tests::wip; - - py::class_(m, "Vanilla", py::metaclass((PyObject *) &PyType_Type)).def(py::init<>()); -} diff --git a/tests/test_wip.py b/tests/test_wip.py deleted file mode 100644 index d07b008f..00000000 --- a/tests/test_wip.py +++ /dev/null @@ -1,5 +0,0 @@ -from pybind11_tests import wip as m - - -def test_vanilla(): - assert m.Vanilla().__class__.__name__ == "Vanilla" From 1d7ac2f794e410b6e9ebdc74c62412e667efcf83 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 25 Jan 2024 17:47:10 -0800 Subject: [PATCH 5/7] Improve `"@__setstate__"` related code in pybind11.h for readability. --- include/pybind11/pybind11.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 48438a1d..6655a213 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -394,6 +394,11 @@ class cpp_function : public function { // it alive. auto *rec = unique_rec.get(); + // Note the "@__setstate__" manipulation below. + rec->is_constructor = rec->name != nullptr + && (std::strcmp(rec->name, "__init__") == 0 + || std::strcmp(rec->name, "__setstate__") == 0); + // Keep track of strdup'ed strings, and clean them up as long as the function's capsule // has not taken ownership yet (when `unique_rec.release()` is called). // Note: This cannot easily be fixed by a `unique_ptr` with custom deleter, because the @@ -403,12 +408,13 @@ class cpp_function : public function { strdup_guard guarded_strdup; /* Create copies of all referenced C-style strings */ - bool setstate_is_ctor = true; - if (rec->name && std::strcmp(rec->name, "@__setstate__") == 0) { + if (rec->name == nullptr) { + rec->name = guarded_strdup(""); + } else if (std::strcmp(rec->name, "@__setstate__") == 0) { + // See google/pywrapcc#30094 for background. rec->name = guarded_strdup(rec->name + 1); - setstate_is_ctor = false; } else { - rec->name = guarded_strdup(rec->name ? rec->name : ""); + rec->name = guarded_strdup(rec->name); } if (rec->doc) { rec->doc = guarded_strdup(rec->doc); @@ -424,9 +430,6 @@ class cpp_function : public function { } } - rec->is_constructor = (std::strcmp(rec->name, "__init__") == 0) - || (setstate_is_ctor && std::strcmp(rec->name, "__setstate__") == 0); - #if defined(PYBIND11_DETAILED_ERROR_MESSAGES) && !defined(PYBIND11_DISABLE_NEW_STYLE_INIT_WARNING) if (rec->is_constructor && !rec->is_new_style_constructor) { const auto class_name From 6bc10a7c38c5469940c502685d11f324fe6873ed Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 25 Jan 2024 18:08:00 -0800 Subject: [PATCH 6/7] Resolve MSVC errors: declaration of 'state' hides class member --- tests/test_pickling.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/test_pickling.cpp b/tests/test_pickling.cpp index a3671793..5b7a7a2b 100644 --- a/tests/test_pickling.cpp +++ b/tests/test_pickling.cpp @@ -72,14 +72,14 @@ namespace exercise_getinitargs_getstate_setstate { class StoreTwoWithState { public: - StoreTwoWithState(int v0, int v1) : values{v0, v1}, state{"blank"} {} - const std::vector &GetInitArgs() const { return values; } - const std::string &GetState() const { return state; } - void SetState(const std::string &state) { this->state = state; } + StoreTwoWithState(int v0, int v1) : values_{v0, v1}, state_{"blank"} {} + const std::vector &GetInitArgs() const { return values_; } + const std::string &GetState() const { return state_; } + void SetState(const std::string &state) { state_ = state; } private: - std::vector values; - std::string state; + std::vector values_; + std::string state_; }; void wrap(py::module m) { From 846bfbdc8b5e102ffc3513f357dd1bdd17b2bc1b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 29 Jan 2024 10:27:14 -0800 Subject: [PATCH 7/7] Change `@__setstate__` to `__setstate__[non-constructor]` based on feedback by @rainwoodman. Also revise comments. --- include/pybind11/pybind11.h | 6 +++--- tests/test_pickling.cpp | 12 +++++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 6655a213..f2036903 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -394,7 +394,7 @@ class cpp_function : public function { // it alive. auto *rec = unique_rec.get(); - // Note the "@__setstate__" manipulation below. + // Note the "__setstate__" manipulation below. rec->is_constructor = rec->name != nullptr && (std::strcmp(rec->name, "__init__") == 0 || std::strcmp(rec->name, "__setstate__") == 0); @@ -410,9 +410,9 @@ class cpp_function : public function { /* Create copies of all referenced C-style strings */ if (rec->name == nullptr) { rec->name = guarded_strdup(""); - } else if (std::strcmp(rec->name, "@__setstate__") == 0) { + } else if (std::strcmp(rec->name, "__setstate__[non-constructor]") == 0) { // See google/pywrapcc#30094 for background. - rec->name = guarded_strdup(rec->name + 1); + rec->name = guarded_strdup("__setstate__"); } else { rec->name = guarded_strdup(rec->name); } diff --git a/tests/test_pickling.cpp b/tests/test_pickling.cpp index 5b7a7a2b..fae2c115 100644 --- a/tests/test_pickling.cpp +++ b/tests/test_pickling.cpp @@ -65,10 +65,16 @@ void wrap(py::module m) { namespace exercise_getinitargs_getstate_setstate { -// Mechanism similar to what was established with Boost.Python: -// https://www.boost.org/doc/libs/1_58_0/libs/python/doc/v2/pickle.html +// Exercise `__setstate__[non-constructor]` (see google/pywrapcc#30094), which +// was added to support a pickle protocol as established with Boost.Python +// (in 2002): +// https://www.boost.org/doc/libs/1_31_0/libs/python/doc/v2/pickle.html // This mechanism was also adopted for PyCLIF: // https://github.com/google/clif/blob/7d388e1de7db5beeb3d7429c18a2776d8188f44f/clif/testing/python/pickle_compatibility.clif +// The code below exercises the pickle protocol intentionally exactly as used +// in PyCLIF, to ensure that pybind11 stays fully compatible with existing +// client code relying on the long-established protocol. (It is impractical to +// change all such client code.) class StoreTwoWithState { public: @@ -97,7 +103,7 @@ void wrap(py::module m) { }, py::arg("protocol") = -1) .def( - "@__setstate__", // See google/pywrapcc#30094 for background. + "__setstate__[non-constructor]", // See google/pywrapcc#30094 for background. [](StoreTwoWithState *self, const std::string &state) { self->SetState(state); }, py::arg("state")); }