Skip to content

smart_holder: open issues #2901

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

Closed
rhaschke opened this issue Mar 14, 2021 · 13 comments
Closed

smart_holder: open issues #2901

rhaschke opened this issue Mar 14, 2021 · 13 comments

Comments

@rhaschke
Copy link
Contributor

When using @rwgk's smart_holder branch and providing a custom holder type like this:

py::class_<MyClass, std::shared_ptr<MyClass>, MyBaseClass>(m, "MyClass")
		.def(py::init<>());

I've got a misleading static assert:

pybind11/include/pybind11/pybind11.h:1297:13: error: static assertion failed: py::class_ holder vs type_caster mismatch: missing PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(T, std::shared_ptr<T>)?
 1297 |             !(detail::is_instantiation<std::shared_ptr, holder_type>::value
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1298 |               && wrapped_type_uses_smart_holder_type_caster),
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I would assume:

  1. That it is possible to define custom holders
  2. If multiple holder types are defined, a more informative assert would be thrown
@rwgk
Copy link
Collaborator

rwgk commented Mar 14, 2021

Hi Robert, are you using -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT?
Or are you using the PYBIND11_SMART_HOLDER_TYPE_CASTERS(MyClass)?
If that's not it, could you please share the code that fails building?

@rhaschke
Copy link
Contributor Author

I'm using -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT. I successfully migrated my code to the smart_holder branch - at least everything compiles. See here for required changes:

  • explicitly specifying a custom holder type doesn't work anymore
  • (reference) arguments of std::shared_ptr<T>& don't work anymore
  • make_iterator doesn't work for a container (std::list) of std::unique_ptr<T>

@rwgk
Copy link
Collaborator

rwgk commented Mar 14, 2021

  • explicitly specifying a custom holder type doesn't work anymore

It does :-) I tried to be really careful about. What you need is actually in the static_assert message (just with T instead of your typename):

PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(MyClass, std::shared_ptr<MyClass>)

Here are many examples:

PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(MyObject2, std::shared_ptr<MyObject2>)

Maybe I should rename the macro, because it's deceptively similar to PYBIND11_SMART_HOLDER_TYPE_CASTERS?
Maybe it's just too precise for its own good? Does PYBIND11_CUSTOM_HOLDER_TYPE_CASTERS look better?

@rwgk
Copy link
Collaborator

rwgk commented Mar 14, 2021

  • (reference) arguments of std::shared_ptr& don't work anymore

Yes, that's on purpose: it could be used to undermine ownership correctness (you could call .reset(some_other_ptr)). But const std::shared_ptr<T>& should work:

git diff smart_holder master tests/test_methods_and_attributes.cpp
  • make_iterator doesn't work for a container (std::list) of std::unique_ptr

Do you have a reproducer for this?

@rhaschke
Copy link
Contributor Author

You need a PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(T, std::shared_ptr<T>)

Defining PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(T, std::shared_ptr<T>) for all concrete types T works indeed.

However, this is somewhat laborious and I didn't expect this would be needed.
So far, there wasn't any need for such a declaration in old pybind11. Only if you wanted to use a custom holder, you had to use PYBIND11_DECLARE_HOLDER_TYPE(T, custom_holder_type<T>) once to declare your holder in a templated fashion for any type T.

  • make_iterator doesn't work for a container (std::list) of std::unique_ptr

Do you have a reproducer for this?

See here. I can prepare a unit test in pybind11 if you like. It's essentially providing an iterator for std::list<std::unique_ptr<T>>.

I have some more test issues. Need to inspect them in more detail...

@rwgk
Copy link
Collaborator

rwgk commented Mar 14, 2021

However, this is somewhat laborious and I didn't expect this would be needed.

Do you have actual custom holders?
The idea is that you can simply remove most or ideally all std::shared_ptr<T> holders, because the smart_holder also covers the shared_ptr handling.
I didn't have an idea for avoiding the need for the macro that would fit into the existing framework. I think it's possible only with intrusive changes to pybind11, maybe, I haven't thought a lot about that direction.

I'll look into the list unique_ptr issue later. (I want to cleanup #2902 first.)

@rhaschke
Copy link
Contributor Author

True, using the smart_holder instead of std::shared_ptr<T> avoids the need for the macro. However, requiring the macro breaks existing code and we should find a better solution, I think.
Also, I noticed, using std::shared_ptr holders (instead of smart holders), breaks function calls passing the same (shared) argument twice: f(arg, arg).

@rwgk
Copy link
Collaborator

rwgk commented Mar 15, 2021

However, requiring the macro breaks existing code and we should find a better solution, I think.

Sorry I need to explain this better before announcing the smart_holder for wider use: you're basically jumping ahead by using -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT (see comment for the macro). I put being compatible with existing code over ease of using smart_holder, given the constraint that either the smart_holder (without that define) or unique_ptr/shared_ptr (with that define) require a macro — unless, maybe, pybind11 is redesigned in a fundamental way, but almost certainly incompatible with existing code.

Also, I noticed, using std::shared_ptr holders (instead of smart holders), breaks function calls passing the same (shared) argument twice: f(arg, arg).

That's unexpected. If you have py::class<T, std::shared_ptr<T>> with the PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS macro (is that what you have?) I don't expect a behavior change at all, compared to vanilla pybind11 master.

@rhaschke
Copy link
Contributor Author

rhaschke commented Mar 15, 2021

I tried to reproduce this issue in pybind11 with a minimal test case - but I couldn't. I don't yet understand what happens...
EDIT: After hours of trial and error, I finally succeeded to reproduce the issue. It was related to multiple compilation units!
The second issue (using make_iterator on a container of std::unique_ptrs) is reproducable as well.
See here for the new failing tests.

@rwgk
Copy link
Collaborator

rwgk commented Mar 15, 2021

This issue only shows up when

  • using PYBIND11_USE_SMART_HOLDER_AS_DEFAULT
  • defining a class_ with std::shared_ptr holder in an external compilation unit

You need PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS(External, std::shared_ptr<External>) also in test_class.cpp.

I know this isn't great, but see the rationale from before (compatibility/fitting into exisiting more important than ease of use).

Note that the PyCLIF pybind11 code generator we're working on does NOT use smart_holder as the default. It automatically creates a header file for each extension with the SMART_HOLDER_TYPE_CASTERS macros, and the existing PyCLIF architecture already enforces that these are included when needed. I.e. it's a non-issue for us.

Moving everybody to smart_holder as the default will come with the advice to remove all std::shared_ptr holders. The only situation where macros or maybe alternatively/better #includes are needed are for custom smart pointers used as holders. What exactly is still wide open. I'm thinking ideally we'll bake support for custom smart pointers into the smart_holder and then remove type_caster_base entirely. This will mean though that people with custom smart pointers will have to make changes to switch (changes more involved then adding a one-line macro in the right places). [wires crossing: I wrote this before you just edited your message]

@rhaschke
Copy link
Contributor Author

Thanks for the feedback. I knew migrating away from std::shared_ptr holders would resolve the first issue. But, it was driving me crazy that I wasn't able to reproduce the issue in pybind11 unittest. Now, we know that it is related to external compilation units.

@rwgk
Copy link
Collaborator

rwgk commented Mar 15, 2021

Is this the error message for the IntTypeList case?

pybind11_fail("Missing value for wrapped C++ type:"

I need to debug that. If you could create a PR just for that case, with the "Allow edits and access to secrets by maintainers" box checked, that would be a great start for me. test_class_sh_make_iterator might be a good name; maybe we want to keep it permanently.

@rhaschke rhaschke changed the title smart_holder: smart_holder: open issues Mar 16, 2021
@rhaschke
Copy link
Contributor Author

I'm closing this in favor of #2904. I tracked the issue down to a missing cast implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants