pybind11 icon indicating copy to clipboard operation
pybind11 copied to clipboard

Add `pybind11_select_caster`

Open laramiel opened this issue 3 years ago • 4 comments

Description

Instead of only using type_caster template specialization, it is possible to register the conversions with a function declaration, which will be found with ADL.

namespace foo {

class BarCaster;

class Bar {
  friend BarCaster pybind11_select_caster(Bar*);
};

The parameter is a pointer to the C++ (intrinsic) type. The returned type is the caster class. This function should have no implementation!

  • The declarations need to be defined in the original namespace of the converted C++ type, ideally declared as a friend function in the type itself.
  • The default behavior did not change: type_caster<T> will be used as a fallback.

Because type_caster<T> not always used, load_caster has to accept a generic Caster argument.

This is manually adapted from #3643 and #3674

NOTE

https://en.cppreference.com/w/cpp/language/definition#One_Definition_Rule

ODR violations can be easy to introduce and hard to diagnose in pybind11. Essentially if there exist two types (perhaps specializations of pybind11::type_caster<T>), or two functions (perhaps pybind11::cast<T>) which use definitions that have "different sequences of tokens", then the code is in violation of the ODR.

While this doesn't solve that problem, it allows a classes to more easily declare custom casters which can help ensure that the correct caster is always used.

For more on ODR violations, see https://www.youtube.com/watch?v=IY8tHh2LSX4

Suggested changelog entry:

Allow type_caster<> selection via ADL.

laramiel avatar Apr 11 '22 20:04 laramiel

@laramiel @amauryfa @henryiii @Skylion007

I revised custom.rst top to bottom. Could you please review?

I only changed custom.rst, the rest is as before.

I kept example code changes to a minimum, but did address @Skylion007's concern (PyErr_Clear()).

rwgk avatar May 06 '22 02:05 rwgk

@laramiel @amauryfa @henryiii @Skylion007

Thanks Amaury and Laramie for the feedback! And Henry for the [[maybe_unused]] hint from 3 weeks ago. I think I addressed everything now, and beyond, by porting the rst code blocks to the new tests/test_docs_advanced_cast_custom.cpp, making a pass revising the source code comments, clang-formatting, then copying back from the test to the rst file.

I also renamed test_make_caster_adl to test_select_caster, to make it more clear what is tested. (I tried to use test_pybind11_select_caster but that leads to a name clash, because test_ is stripped off for the submodule name.)

Regrading test_docs_advanced_cast_custom.cpp, the three alternatives shown in the documentation are handled with a #define:

// 0: Use the older pybind11::detail::type_caster<T> specialization mechanism.
// 1: Use pybind11_select_caster declaration in user_space.
// 2: Use pybind11_select_caster friend function declaration.
#ifndef PYBIND11_TEST_DOCS_ADVANCED_CAST_CUSTOM_ALTERNATIVE
#    define PYBIND11_TEST_DOCS_ADVANCED_CAST_CUSTOM_ALTERNATIVE 1
#endif

It would be ideal to exercise all 3 alternatives, but I'm not sure how to best do that. I'm not even sure if it's worth the trouble, the extra coverage is just a few lines of C++. I'm thinking of leaving that for "later, maybe". What's in this PR is already clearly beyond the status quo (rst code blocks not tested at all; outdated formatting).

rwgk avatar May 07 '22 09:05 rwgk

Putting this PR into Draft mode. The rationale:

  • The primary motivation behind the original PR #3643 was to add support for translation-unit-specific casters.
  • But the idea that pybind11_select_caster could be a stepping stone towards TU-specific casters was conclusively laid to rest by what we learned from the experiments under #3931.
  • We are still looking into another idea, which is not very concrete at the moment (it involves leveraging the typename... Extra mechanism).
  • To not make the starting point for that work more complicated, we think it is best to hold off merging pybind11_select_caster for now.

Let's wait and see where the new idea leads, and reevaluate adding pybind11_select_caster when there is more certainty.

rwgk avatar May 13 '22 19:05 rwgk