From 93e69191c1ad81ccde1b3dfd46bb69dccd8e45ce Mon Sep 17 00:00:00 2001 From: Cris Luengo Date: Fri, 25 Jun 2021 18:56:17 -0600 Subject: [PATCH 1/2] fix: enable py::implicitly_convertible for py::class_-wrapped types (#3059) * Allow casting from None to a custom object, closes #2778 * ci.yml patch from the smart_holder branch for full CI coverage. --- .github/workflows/ci.yml | 26 +++++++++++----------- include/pybind11/detail/type_caster_base.h | 20 +++++++++++------ tests/test_methods_and_attributes.cpp | 18 +++++++++++++++ tests/test_methods_and_attributes.py | 11 +++++++++ 4 files changed, 55 insertions(+), 20 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e00745df7..df7441b9e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -100,7 +100,7 @@ jobs: run: > cmake -S . -B . -DPYBIND11_WERROR=ON - -DDOWNLOAD_CATCH=ON + -DDOWNLOAD_CATCH=OFF -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=11 ${{ matrix.args }} @@ -111,10 +111,10 @@ jobs: - name: Python tests C++11 run: cmake --build . --target pytest -j 2 - - name: C++11 tests - # TODO: Figure out how to load the DLL on Python 3.8+ - if: "!(runner.os == 'Windows' && (matrix.python == 3.8 || matrix.python == 3.9 || matrix.python == '3.10-dev'))" - run: cmake --build . --target cpptest -j 2 + #- name: C++11 tests + # # TODO: Figure out how to load the DLL on Python 3.8+ + # if: "!(runner.os == 'Windows' && (matrix.python == 3.8 || matrix.python == 3.9 || matrix.python == '3.10-dev'))" + # run: cmake --build . --target cpptest -j 2 - name: Interface test C++11 run: cmake --build . --target test_cmake_build @@ -127,7 +127,7 @@ jobs: run: > cmake -S . -B build2 -DPYBIND11_WERROR=ON - -DDOWNLOAD_CATCH=ON + -DDOWNLOAD_CATCH=OFF -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=17 ${{ matrix.args }} @@ -139,10 +139,10 @@ jobs: - name: Python tests run: cmake --build build2 --target pytest - - name: C++ tests - # TODO: Figure out how to load the DLL on Python 3.8+ - if: "!(runner.os == 'Windows' && (matrix.python == 3.8 || matrix.python == 3.9 || matrix.python == '3.10-dev'))" - run: cmake --build build2 --target cpptest + #- name: C++ tests + # # TODO: Figure out how to load the DLL on Python 3.8+ + # if: "!(runner.os == 'Windows' && (matrix.python == 3.8 || matrix.python == 3.9 || matrix.python == '3.10-dev'))" + # run: cmake --build build2 --target cpptest - name: Interface test run: cmake --build build2 --target test_cmake_build @@ -754,7 +754,7 @@ jobs: cmake -S . -B build -G "Visual Studio 16 2019" -A Win32 -DPYBIND11_WERROR=ON - -DDOWNLOAD_CATCH=ON + -DDOWNLOAD_CATCH=OFF -DDOWNLOAD_EIGEN=ON ${{ matrix.args }} - name: Build C++11 @@ -800,7 +800,7 @@ jobs: cmake -S . -B build -G "Visual Studio 14 2015" -A x64 -DPYBIND11_WERROR=ON - -DDOWNLOAD_CATCH=ON + -DDOWNLOAD_CATCH=OFF -DDOWNLOAD_EIGEN=ON - name: Build C++14 @@ -849,7 +849,7 @@ jobs: cmake -S . -B build -G "Visual Studio 15 2017" -A x64 -DPYBIND11_WERROR=ON - -DDOWNLOAD_CATCH=ON + -DDOWNLOAD_CATCH=OFF -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=${{ matrix.std }} ${{ matrix.args }} diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 0dd9d48e5..a8d3938c7 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -657,12 +657,6 @@ public: PYBIND11_NOINLINE bool load_impl(handle src, bool convert) { if (!src) return false; if (!typeinfo) return try_load_foreign_module_local(src); - if (src.is_none()) { - // Defer accepting None to other overloads (if we aren't in convert mode): - if (!convert) return false; - value = nullptr; - return true; - } auto &this_ = static_cast(*this); this_.check_holder_compat(); @@ -731,7 +725,19 @@ public: } // Global typeinfo has precedence over foreign module_local - return try_load_foreign_module_local(src); + if (try_load_foreign_module_local(src)) { + return true; + } + + // Custom converters didn't take None, now we convert None to nullptr. + if (src.is_none()) { + // Defer accepting None to other overloads (if we aren't in convert mode): + if (!convert) return false; + value = nullptr; + return true; + } + + return false; } diff --git a/tests/test_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp index 3eb22b4a3..67ee117c6 100644 --- a/tests/test_methods_and_attributes.cpp +++ b/tests/test_methods_and_attributes.cpp @@ -118,6 +118,14 @@ int none3(std::shared_ptr &obj) { return obj ? obj->answer : -1; } int none4(std::shared_ptr *obj) { return obj && *obj ? (*obj)->answer : -1; } int none5(const std::shared_ptr &obj) { return obj ? obj->answer : -1; } +// Issue #2778: implicit casting from None to object (not pointer) +class NoneCastTester { +public: + int answer = -1; + NoneCastTester() = default; + NoneCastTester(int v) : answer(v) {}; +}; + struct StrIssue { int val = -1; @@ -354,6 +362,16 @@ TEST_SUBMODULE(methods_and_attributes, m) { m.def("no_none_kwarg", &none2, "a"_a.none(false)); m.def("no_none_kwarg_kw_only", &none2, py::kw_only(), "a"_a.none(false)); + // test_casts_none + // Issue #2778: implicit casting from None to object (not pointer) + py::class_(m, "NoneCastTester") + .def(py::init<>()) + .def(py::init()) + .def(py::init([](py::none const&) { return NoneCastTester{}; })); + py::implicitly_convertible(); + m.def("ok_obj_or_none", [](NoneCastTester const& foo) { return foo.answer; }); + + // test_str_issue // Issue #283: __str__ called on uninitialized instance when constructor arguments invalid py::class_(m, "StrIssue") diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 2aaf9331f..89b7225a9 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -431,6 +431,17 @@ def test_accepts_none(msg): assert "incompatible function arguments" in str(excinfo.value) +def test_casts_none(msg): + """#2778: implicit casting from None to object (not pointer)""" + a = m.NoneCastTester() + assert m.ok_obj_or_none(a) == -1 + a = m.NoneCastTester(4) + assert m.ok_obj_or_none(a) == 4 + a = m.NoneCastTester(None) + assert m.ok_obj_or_none(a) == -1 + assert m.ok_obj_or_none(None) == -1 + + def test_str_issue(msg): """#283: __str__ called on uninitialized instance when constructor arguments invalid""" From fbae8f313be887e0bf4b702fe551966efdc8cbc2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 30 Jun 2021 12:34:32 -0700 Subject: [PATCH 2/2] pickle setstate: setattr __dict__ only if not empty (#2972) * pickle setstate: setattr __dict__ only if not empty, to not force use of py::dynamic_attr() unnecessarily. * Adding unit test. * Clang 3.6 & 3.7 compatibility. * PyPy compatibility. * Minor iwyu fix, additional comment. * Addressing reviewer requests. * Applying clang-tidy suggested fixes. * Adding check_dynamic_cast_SimpleCppDerived, related to issue #3062. --- include/pybind11/detail/init.h | 8 ++++- tests/test_pickling.cpp | 56 ++++++++++++++++++++++++++++++++++ tests/test_pickling.py | 36 ++++++++++++++++++++++ 3 files changed, 99 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h index 3ef78c117..3269e0425 100644 --- a/include/pybind11/detail/init.h +++ b/include/pybind11/detail/init.h @@ -293,7 +293,13 @@ template ::value, int> = 0> void setstate(value_and_holder &v_h, std::pair &&result, bool need_alias) { construct(v_h, std::move(result.first), need_alias); - setattr((PyObject *) v_h.inst, "__dict__", result.second); + auto d = handle(result.second); + if (PyDict_Check(d.ptr()) && PyDict_Size(d.ptr()) == 0) { + // Skipping setattr below, to not force use of py::dynamic_attr() for Class unnecessarily. + // See PR #2972 for details. + return; + } + setattr((PyObject *) v_h.inst, "__dict__", d); } /// Implementation for py::pickle(GetState, SetState) diff --git a/tests/test_pickling.cpp b/tests/test_pickling.cpp index c2cee6c3c..0d5827315 100644 --- a/tests/test_pickling.cpp +++ b/tests/test_pickling.cpp @@ -1,7 +1,9 @@ +// clang-format off /* tests/test_pickling.cpp -- pickle support Copyright (c) 2016 Wenzel Jakob + Copyright (c) 2021 The Pybind Development Team. All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. @@ -9,6 +11,58 @@ #include "pybind11_tests.h" +// clang-format on + +#include +#include +#include + +namespace exercise_trampoline { + +struct SimpleBase { + int num = 0; + virtual ~SimpleBase() = default; + + // For compatibility with old clang versions: + SimpleBase() = default; + SimpleBase(const SimpleBase &) = default; +}; + +struct SimpleBaseTrampoline : SimpleBase {}; + +struct SimpleCppDerived : SimpleBase {}; + +void wrap(py::module m) { + py::class_(m, "SimpleBase") + .def(py::init<>()) + .def_readwrite("num", &SimpleBase::num) + .def(py::pickle( + [](const py::object &self) { + py::dict d; + if (py::hasattr(self, "__dict__")) + d = self.attr("__dict__"); + return py::make_tuple(self.attr("num"), d); + }, + [](const py::tuple &t) { + if (t.size() != 2) + throw std::runtime_error("Invalid state!"); + auto cpp_state = std::unique_ptr(new SimpleBaseTrampoline); + cpp_state->num = t[0].cast(); + auto py_state = t[1].cast(); + return std::make_pair(std::move(cpp_state), py_state); + })); + + m.def("make_SimpleCppDerivedAsBase", + []() { return std::unique_ptr(new SimpleCppDerived); }); + m.def("check_dynamic_cast_SimpleCppDerived", [](const SimpleBase *base_ptr) { + return dynamic_cast(base_ptr) != nullptr; + }); +} + +} // namespace exercise_trampoline + +// clang-format off + TEST_SUBMODULE(pickling, m) { // test_roundtrip class Pickleable { @@ -130,4 +184,6 @@ TEST_SUBMODULE(pickling, m) { return std::make_pair(cpp_state, py_state); })); #endif + + exercise_trampoline::wrap(m); } diff --git a/tests/test_pickling.py b/tests/test_pickling.py index 6b27a73a5..303213068 100644 --- a/tests/test_pickling.py +++ b/tests/test_pickling.py @@ -45,3 +45,39 @@ def test_enum_pickle(): data = pickle.dumps(e.EOne, 2) assert e.EOne == pickle.loads(data) + + +# +# exercise_trampoline +# +class SimplePyDerived(m.SimpleBase): + pass + + +def test_roundtrip_simple_py_derived(): + p = SimplePyDerived() + p.num = 202 + p.stored_in_dict = 303 + data = pickle.dumps(p, pickle.HIGHEST_PROTOCOL) + p2 = pickle.loads(data) + assert isinstance(p2, SimplePyDerived) + assert p2.num == 202 + assert p2.stored_in_dict == 303 + + +def test_roundtrip_simple_cpp_derived(): + p = m.make_SimpleCppDerivedAsBase() + assert m.check_dynamic_cast_SimpleCppDerived(p) + p.num = 404 + if not env.PYPY: + # To ensure that this unit test is not accidentally invalidated. + with pytest.raises(AttributeError): + # Mimics the `setstate` C++ implementation. + setattr(p, "__dict__", {}) # noqa: B010 + data = pickle.dumps(p, pickle.HIGHEST_PROTOCOL) + p2 = pickle.loads(data) + assert isinstance(p2, m.SimpleBase) + assert p2.num == 404 + # 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)