Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Preview 3] Merge smart_holder into master (squash merge) #5339

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .codespell-ignore-lines
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@ template <op_id id, op_type ot, typename L, typename R>
template <detail::op_id id, detail::op_type ot, typename L, typename R, typename... Extra>
class_ &def(const detail::op_<id, ot, L, R> &op, const Extra &...extra) {
class_ &def_cast(const detail::op_<id, ot, L, R> &op, const Extra &...extra) {
int valu;
explicit movable_int(int v) : valu{v} {}
movable_int(movable_int &&other) noexcept : valu(other.valu) { other.valu = 91; }
explicit indestructible_int(int v) : valu{v} {}
REQUIRE(hld.as_raw_ptr_unowned<zombie>()->valu == 19);
REQUIRE(othr.valu == 19);
REQUIRE(orig.valu == 91);
(m.pass_valu, "Valu", "pass_valu:Valu(_MvCtor)*_CpCtor"),
atyp_valu rtrn_valu() { atyp_valu obj{"Valu"}; return obj; }
assert m.atyp_valu().get_mtxt() == "Valu"
// valu(e), ref(erence), ptr or p (pointer), r = rvalue, m = mutable, c = const,
@pytest.mark.parametrize("access", ["ro", "rw", "static_ro", "static_rw"])
struct IntStruct {
explicit IntStruct(int v) : value(v){};
Expand Down
32 changes: 30 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ on:
branches:
- master
- stable
- smart_holder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised to see the smart_holder branch listed here (and several other workflow definitions).
Isn't the goal of this PR to merge that branch into master, making the former branch superfluous?

- v*

permissions: read-all

concurrency:
group: test-${{ github.ref }}
group: test-sh-avl${{ github.ref }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that addition needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, thanks for catching this, that's an oversight. I forgot to remove it on the smart_holder branch. Will do that now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing it on the smart_holder branch didn't pan out:

(I couldn't explain why it didn't work. Another thing I don't want to spend my time on.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With addition is meant suffix. You must not remove the whole line.

Suggested change
group: test-sh-avl${{ github.ref }}
group: test-${{ github.ref }}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With addition is meant suffix. You must not remove the whole line.

Sorry I messed up this simple change so badly. I think I got it right now. (I had it in my mind wrongly that the line isn't on master, but it is.)

cancel-in-progress: true

env:
Expand Down Expand Up @@ -67,7 +68,34 @@ jobs:
# Extra ubuntu latest job
- runs-on: ubuntu-latest
python: '3.11'

# Exercise PYBIND11_USE_SMART_HOLDER_AS_DEFAULT
# with recent (or ideally latest) released Python version.
- runs-on: ubuntu-latest
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="-DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT"
- runs-on: macos-13
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="-DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT"
- runs-on: windows-2022
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="/DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT /GR /EHsc"
# Exercise PYBIND11_SMART_HOLDER_DISABLE
# with recent (or ideally latest) released Python version.
- runs-on: ubuntu-latest
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="-DPYBIND11_SMART_HOLDER_DISABLE"
- runs-on: macos-13
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="-DPYBIND11_SMART_HOLDER_DISABLE"
- runs-on: windows-2022
python: '3.12'
args: >
-DCMAKE_CXX_FLAGS="/DPYBIND11_SMART_HOLDER_DISABLE /GR /EHsc"

name: "🐍 ${{ matrix.python }} • ${{ matrix.runs-on }} • x64 ${{ matrix.args }}"
runs-on: ${{ matrix.runs-on }}
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/configure.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ on:
branches:
- master
- stable
- smart_holder
- v*

permissions:
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/emscripten.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ on:
branches:
- master
- stable
- smart_holder
- v*

concurrency:
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/format.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ on:
branches:
- master
- stable
- smart_holder
- "v*"

permissions:
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/pip.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ on:
branches:
- master
- stable
- smart_holder
- v*
release:
types:
Expand Down
2 changes: 2 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ repos:
- id: mixed-line-ending
- id: requirements-txt-fixer
- id: trailing-whitespace
exclude: \.patch?$

# Also code format the docs
- repo: https://github.com/adamchainz/blacken-docs
Expand All @@ -90,6 +91,7 @@ repos:
rev: "v1.5.5"
hooks:
- id: remove-tabs
exclude: (^docs/.*|\.patch)?$

# Avoid directional quotes
- repo: https://github.com/sirosen/texthooks
Expand Down
5 changes: 5 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,13 @@ set(PYBIND11_HEADERS
include/pybind11/detail/class.h
include/pybind11/detail/common.h
include/pybind11/detail/descr.h
include/pybind11/detail/dynamic_raw_ptr_cast_if_possible.h
include/pybind11/detail/init.h
include/pybind11/detail/internals.h
include/pybind11/detail/struct_smart_holder.h
include/pybind11/detail/type_caster_base.h
include/pybind11/detail/typeid.h
include/pybind11/detail/using_smart_holder.h
include/pybind11/detail/value_and_holder.h
include/pybind11/attr.h
include/pybind11/buffer_info.h
Expand All @@ -156,9 +159,11 @@ set(PYBIND11_HEADERS
include/pybind11/operators.h
include/pybind11/pybind11.h
include/pybind11/pytypes.h
include/pybind11/smart_holder.h
include/pybind11/stl.h
include/pybind11/stl_bind.h
include/pybind11/stl/filesystem.h
include/pybind11/trampoline_self_life_support.h
include/pybind11/type_caster_pyobject_ptr.h
include/pybind11/typing.h
include/pybind11/warnings.h)
Expand Down
2 changes: 2 additions & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
prune tests
prune ubench
include README_smart_holder.rst
recursive-include pybind11/include/pybind11 *.h
recursive-include pybind11 *.py
recursive-include pybind11 py.typed
Expand Down
4 changes: 4 additions & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@

.. start

.. Note::

This is the pybind11 **smart_holder** branch. Please refer to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't be true when merged, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, that's what I want to clean out last minute, "TODO this PR" in the PR description.

``README_smart_holder.rst`` for branch-specific information.

**pybind11** is a lightweight header-only library that exposes C++ types
in Python and vice versa, mainly to create Python bindings of existing
Expand Down
123 changes: 123 additions & 0 deletions README_smart_holder.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
==============================
pybind11 — smart_holder branch
==============================


Overview
========

- The smart_holder branch is a strict superset of the pybind11 master branch.
Everything that works with the master branch is expected to work exactly the
same with the smart_holder branch.

- Activating the smart_holder functionality for a given C++ type ``T`` is as
easy as changing ``py::class_<T>`` to ``py::classh<T>`` in client code.

- The ``py::classh<T>`` functionality includes

* support for **two-way** Python/C++ conversions for both
``std::unique_ptr<T>`` and ``std::shared_ptr<T>`` **simultaneously**.
— In contrast, ``py::class_<T>`` only supports one-way C++-to-Python
conversions for ``std::unique_ptr<T>``, or alternatively two-way
Python/C++ conversions for ``std::shared_ptr<T>``, which then excludes
the one-way C++-to-Python ``std::unique_ptr<T>`` conversions (this
manifests itself through undefined runtime behavior).

* passing a Python object back to C++ via ``std::unique_ptr<T>``, safely
**disowning** the Python object.

* safely passing `"trampoline"
<https://pybind11.readthedocs.io/en/stable/advanced/classes.html#overriding-virtual-functions-in-python>`_
objects (objects with C++ virtual function overrides implemented in
Python) via ``std::unique_ptr<T>`` or ``std::shared_ptr<T>`` back to C++:
associated Python objects are automatically kept alive for the lifetime
of the smart-pointer.

Note: As of `PR #5257 <https://github.com/pybind/pybind11/pull/5257>`_
the smart_holder functionality is fully baked into pybind11.
Prior to PR #5257 the smart_holder implementation was an "add-on", which made
it necessary to use a ``PYBIND11_SMART_HOLDER_TYPE_CASTERS`` macro. This macro
still exists for backward compatibility, but is now a no-op. The trade-off
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a severe limitation for me. I frequently need interoperability between different extension modules, compiled both with and without smart_holder support.
Will this limitation be lifted with #5296?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, #5296 would fix this, if it gets merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, #5296 would fix this, if it gets merged.

@wjakob rejected it. See there. (I didn't see any evidence that @wjakob actually reviewed the PR, only the description; please correct me if I'm wrong.)

But to give a more nuanced answer to your question:

  • If you use the smart_holder branch, and
  • you compile one extension withOUT defining PYBIND11_SMART_HOLDER_DISABLE, and
  • another extension WITH PYBIND11_SMART_HOLDER_DISABLE defined,

they will interoperate (because the PYBIND11_SMART_HOLDER_DISABLE define does not alter the PYBIND11_INTERNALS_ID).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any evidence that @wjakob actually reviewed the PR, only the description; please correct me if I'm wrong

This is not a good summary of my response, it is a bit more nuanced than that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't summarize anything!

for this convenience is that the ``PYBIND11_INTERNALS_VERSION`` needed to be
changed. Consequently, Python extension modules built with the smart_holder
branch no longer interoperate with extension modules built with the pybind11
master branch. If cross-extension-module interoperability is required, all
extension modules involved must be built with the smart_holder branch.
— Probably, most extension modules do not require cross-extension-module
interoperability, but exceptions to this are quite common.


What is fundamentally different?
--------------------------------

- Classic pybind11 has the concept of "smart-pointer is holder".
Interoperability between smart-pointers is completely missing. For example,
with ``py::class_<T, std::shared_ptr<T>>``, ``return``-ing a
``std::unique_ptr<T>`` leads to undefined runtime behavior
(`#1138 <https://github.com/pybind/pybind11/issues/1138>`_).
A `systematic analysis can be found here
<https://github.com/pybind/pybind11/pull/2672#issuecomment-748392993>`_.

- ``py::smart_holder`` has a richer concept in comparison, with well-defined
runtime behavior in all situations. ``py::smart_holder`` "knows" about both
``std::unique_ptr<T>`` and ``std::shared_ptr<T>``, and how they interoperate.


What motivated the development of the smart_holder code?
--------------------------------------------------------

- The original context was retooling of `PyCLIF
<https://github.com/google/clif/>`_, to use pybind11 underneath,
instead of directly targeting the Python C API. Essentially the smart_holder
branch is porting established PyCLIF functionality into pybind11. (However,
this work also led to bug fixes in PyCLIF.)


Installation
============

Currently ``git clone`` is the only option. We do not have released packages.

.. code-block:: bash

git clone --branch smart_holder https://github.com/pybind/pybind11.git

Everything else is exactly identical to using the default (master) branch.


Trampolines and std::unique_ptr
-------------------------------

A pybind11 `"trampoline"
<https://pybind11.readthedocs.io/en/stable/advanced/classes.html#overriding-virtual-functions-in-python>`_
is a C++ helper class with virtual function overrides that transparently
call back from C++ into Python. To enable safely passing a ``std::unique_ptr``
to a trampoline object between Python and C++, the trampoline class must
inherit from ``py::trampoline_self_life_support``, for example:

.. code-block:: cpp

class PyAnimal : public Animal, public py::trampoline_self_life_support {
...
};

This is the only difference compared to classic pybind11. A fairly
minimal but complete example is tests/test_class_sh_trampoline_unique_ptr.cpp.


Related links
=============

* The smart_holder branch addresses issue
`#1138 <https://github.com/pybind/pybind11/issues/1138>`_ and
the ten issues enumerated in the `description of PR 2839
<https://github.com/pybind/pybind11/pull/2839#issue-564808678>`_.

* `Description of PR #2672
<https://github.com/pybind/pybind11/pull/2672#issue-522688184>`_, from which
the smart_holder branch was created.

* Small `slide deck
<https://docs.google.com/presentation/d/1r7auDN0x-b6uf-XCvUnZz6z09raasRcCHBMVDh7PsnQ/>`_
presented in meeting with pybind11 maintainers on Feb 22, 2021. Slides 5
and 6 show performance comparisons. (These are outdated but probably not far off.)
7 changes: 7 additions & 0 deletions docs/advanced/smart_ptrs.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
Smart pointers
##############

.. Note::

This is the pybind11 **smart_holder** branch, but the information
below has NOT been updated accordingly yet. Please refer to
``README_smart_holder.rst`` under the top-level pybind11 directory
for updated information about smart pointers.

std::unique_ptr
===============

Expand Down
4 changes: 4 additions & 0 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,10 @@ struct type_record {
/// Is the class inheritable from python classes?
bool is_final : 1;

#ifdef PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
holder_enum_t holder_enum_v = holder_enum_t::undefined;
#endif

PYBIND11_NOINLINE void add_base(const std::type_info &base, void *(*caster)(void *) ) {
auto *base_info = detail::get_type_info(base, false);
if (!base_info) {
Expand Down
Loading