Allow subclasses of py::args and py::kwargs

[gentlegiantJGC]

Description

The current implementation does not allow subclasses of args or kwargs.
This change allows subclasses to be used.

This was originally submitted as part of #5357 but more discussion is required to solve it.
This is a very minor change which would allow me to implement my own workaround until it is solved.

Suggested changelog entry:

Allow subclasses of `py::args` and `py::kwargs`.

[rwgk]

That seems fine to me.

Could you please add a test? (That's the only way to ensure this feature change doesn't get lost.)

[gentlegiantJGC]

@rwgk is this test sufficient?

[rwgk]

Looks good to me. Not too sure myself, but I'd make the change below, although it's tangential. WDYT?

(Please post a comment to respond. I don't get notifications for git pushes.)

diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp
index afa9956c..09036ccd 100644
--- a/tests/test_kwargs_and_defaults.cpp
+++ b/tests/test_kwargs_and_defaults.cpp
@@ -25,11 +25,11 @@ namespace pybind11 {
 namespace detail {
 template <>
 struct handle_type_name<ArgsSubclass> {
-    static constexpr auto name = const_name("*args");
+    static constexpr auto name = const_name("*Args");
 };
 template <>
 struct handle_type_name<KWArgsSubclass> {
-    static constexpr auto name = const_name("**kwargs");
+    static constexpr auto name = const_name("**KWArgs");
 };
 } // namespace detail
 } // namespace pybind11
diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py
index ac1f71a9..d7dfcb4e 100644
--- a/tests/test_kwargs_and_defaults.py
+++ b/tests/test_kwargs_and_defaults.py
@@ -431,3 +431,7 @@ def test_args_refcount():
         (7, 8, myval),
         {"a": 1, "b": myval},
     )
+    assert (
+        m.args_kwargs_subclass_function.__doc__
+        == "args_kwargs_subclass_function(*Args, **KWArgs) -> tuple\n"
+    )

[gentlegiantJGC]

I have made the changes you suggested and added some more tests.

[rwgk]

Nice! Thanks a lot for the great work on the tests.

[francesco-ballarin]

Hi @rwgk @gentlegiantJGC,
the change in 14cde21 has disallowed wrapping of pointer objects defined through

typedef struct _p_Vec *Vec;

where Vec is the user-facing type, and _p_Vec is a private type that must not be exposed to the user.

See wjakob/nanobind#605 for more info when we recently had a similar issue within nanobind.

Is there any way you can implement this new functionality, but still retaining the old behavior?
e.g., something like an or between std::is_same<intrinsic_t<Arg>, args> and std::is_base_of<args, intrinsic_t<Arg>>?

Snippet of the compilation error

      /usr/local/include/c++/12/type_traits: In instantiation of ‘struct std::is_base_of<pybind11::kwargs, _p_Vec>’:
      /usr/local/include/pybind11/detail/common.h:879:55:   required from ‘constexpr int pybind11::detail::constexpr_last() [with Predicate = argument_loader<value_and_holder&, _p_Vec*>::argument_is_kwargs; Ts = {value_and_holder&, _p_Vec*}]’
      /usr/local/include/pybind11/cast.h:1577:83:   required from ‘constexpr const int pybind11::detail::argument_loader<pybind11::detail::value_and_holder&, _p_Vec*>::kwargs_pos’
      /usr/local/include/pybind11/cast.h:1577:27:   required from ‘class pybind11::detail::argument_loader<pybind11::detail::value_and_holder&, _p_Vec*>’
      /usr/local/include/pybind11/pybind11.h:245:43:   required from ‘void pybind11::cpp_function::initialize(Func&&, Return (*)(Args ...), const Extra& ...) [with Func = pybind11::detail::initimpl::constructor<_p_Vec*>::execute<pybind11::class_<dolfin::PETScVector, std::shared_ptr<dolfin::PETScVector>, dolfin::GenericVector, dolfin::PETScObject> >(pybind11::class_<dolfin::PETScVector, std::shared_ptr<dolfin::PETScVector>, dolfin::GenericVector, dolfin::PETScObject>&)::<lambda(pybind11::detail::value_and_holder&, _p_Vec*)>; Return = void; Args = {pybind11::detail::value_and_holder&, _p_Vec*}; Extra = {pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::detail::is_new_style_constructor}]’
      /usr/local/include/pybind11/pybind11.h:127:19:   required from ‘pybind11::cpp_function::cpp_function(Func&&, const Extra& ...) [with Func = pybind11::detail::initimpl::constructor<_p_Vec*>::execute<pybind11::class_<dolfin::PETScVector, std::shared_ptr<dolfin::PETScVector>, dolfin::GenericVector, dolfin::PETScObject> >(pybind11::class_<dolfin::PETScVector, std::shared_ptr<dolfin::PETScVector>, dolfin::GenericVector, dolfin::PETScObject>&)::<lambda(pybind11::detail::value_and_holder&, _p_Vec*)>; Extra = {pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::detail::is_new_style_constructor}; <template-parameter-1-3> = void]’
      /usr/local/include/pybind11/pybind11.h:1631:22:   required from ‘pybind11::class_<type_, options>& pybind11::class_<type_, options>::def(const char*, Func&&, const Extra& ...) [with Func = pybind11::detail::initimpl::constructor<_p_Vec*>::execute<pybind11::class_<dolfin::PETScVector, std::shared_ptr<dolfin::PETScVector>, dolfin::GenericVector, dolfin::PETScObject> >(pybind11::class_<dolfin::PETScVector, std::shared_ptr<dolfin::PETScVector>, dolfin::GenericVector, dolfin::PETScObject>&)::<lambda(pybind11::detail::value_and_holder&, _p_Vec*)>; Extra = {pybind11::detail::is_new_style_constructor}; type_ = dolfin::PETScVector; options = {std::shared_ptr<dolfin::PETScVector>, dolfin::GenericVector, dolfin::PETScObject}]’
      /usr/local/include/pybind11/detail/init.h:205:15:   required from ‘static void pybind11::detail::initimpl::constructor<Args>::execute(Class&, const Extra& ...) [with Class = pybind11::class_<dolfin::PETScVector, std::shared_ptr<dolfin::PETScVector>, dolfin::GenericVector, dolfin::PETScObject>; Extra = {}; typename std::enable_if<(! Class::has_alias), int>::type <anonymous> = 0; Args = {_p_Vec*}]’
      /usr/local/include/pybind11/pybind11.h:1669:21:   required from ‘pybind11::class_<type_, options>& pybind11::class_<type_, options>::def(const pybind11::detail::initimpl::constructor<Args ...>&, const Extra& ...) [with Args = {_p_Vec*}; Extra = {}; type_ = dolfin::PETScVector; options = {std::shared_ptr<dolfin::PETScVector>, dolfin::GenericVector, dolfin::PETScObject}]’
      /tmp/dolfin-src/python/src/la.cpp:831:11:   required from here
      /usr/local/include/c++/12/type_traits:1447:38: error: invalid use of incomplete type ‘struct _p_Vec’
       1447 |     : public integral_constant<bool, __is_base_of(_Base, _Derived)>
            |                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      In file included from /usr/local/include/petscsf.h:8,
                       from /usr/local/include/petsc.h:13:
      /usr/local/include/petscvec.h:26:16: note: forward declaration of ‘struct _p_Vec’
         26 | typedef struct _p_Vec *Vec;
            |                ^~~~~~

Thanks,
Francesco

ps: in case you need to have a look at the custom casters, do not follow the link in the above discussion, since it will lead you to a nanobind wrapped library. The relevant file in the corresponding pybind11-wrapped library is https://bitbucket.org/fenics-project/dolfin/src/master/python/src/petsc_casters.h .

[gentlegiantJGC]

@francesco-ballarin I am a beginner C++ user so I don't fully understand the issue.

The fastest solution is probably to submit a pull request with those changes and some test cases to make sure they don't get broken in the future.

I thought is_base_of would be a superset of is_same

[rwgk]

Standard procedure would be to roll back #5381, and then to roll forward with fix plus additional test.

Sorry I don't have spare time to help here. Please try to forward-fix in the next couple days. If there is no fix, I'll roll back so that you have more time.

[rwgk]

I thought is_base_of would be a superset of is_same.

That's what I was thinking, too, tbh.

Without understanding the issue:

• I'd first set myself up with a test that works without Allow subclasses of py::args and py::kwargs #5381, but breaks with it. (I'd not even start thinking about a fix before I have that.)
• Then try using things like std::remove_pointer, std::remove_cv to fix.

[francesco-ballarin]

@rwgk I gave it a try in #5396 including trying to sketch out a test that mimicks this situation, but I'll definitely need help from either of the two of you in there.

[rwgk]

@rwgk I gave it a try in #5396 including trying to sketch out a test that mimicks this situation, but I'll definitely need help from either of the two of you in there.

Technically, you're actually in a best position to work on this, because you are setup up already to test with the "use case in the wild".

For you, it could work like this:

• Try a fix to convince ourselves that it's pretty easy (almost certainly).
• Reduce your use case to a minimal test (that's some work; although guessing can often get you there quickly).
• Then update your PR with the fix and minimal test.

For background:

• I took a brief look at your test, I believe it's much more complex than it needs to be. I'd definitely try to reduce it, to not compromise the quality of our unit tests.

• The other problem is that it isn't obvious (to me; not having looked closely) that your test works when undoing (locally) Allow subclasses of py::args and py::kwargs #5381.

Trying to be helpful, but untested:

I'd maybe start with any_of<std::is_same<...>, std::is_base_of<...>>. That really should work.

But I wouldn't stop there without thinking, at least a little bit. I'd want to understand why std::is_base_of<> doesn't work by itself. Maybe that'll lead to a better fix?

[francesco-ballarin]

I'd want to understand why std::is_base_of<> doesn't work by itself.

I think it boils down to the following, but that's just my intution, not a test:

1. The error message in my use case comes from the fact that one of the two arguments is a pointer to a forward declared class. I guess that std::is_same<> is smart enough to say that if a class is a pointer typedef (to whatever class, fully declared or not) and the other isn't, then they are definitely not the same. I was relying on this intuition in the other PR.
2. Instead, std::is_base_of<> needs to determine if inheritance is taking place, and that information obviously won't be available for a forward declared class.

Coming up with a test case that isn't

much more complex than it needs to be

and does

not compromise the quality of our unit tests.

it's not going to be obvious to me.

Simplest solution would be to update the other PR to simply use any_of<std::is_same<...>, std::is_base_of<...>> as you suggest.

[rwgk]

and that information obviously won't be available for a forward declared class.

Oh. Yes, I can believe that explanation.

Simplest solution would be to update the other PR to simply use any_of<std::is_same<...>, std::is_base_of<...>> as you suggest.

Sigh.

But OK, this isn't ideal, but the fix is so simple, I don't want to make this artificially difficult.

Could you please update your PR, to implement just the simple fix? Please add

// See PR #5396.

somewhere near your fix. Then in the description of 5396, add links to your comment above, and this comment.

This is so that people working on the code in the future can easily find out why both is_same and is_base_of are needed.

[francesco-ballarin]

Sure, will do that tomorrow.