Implement classmethod pytype

[Skylion007]

Description

I've been mulling over how to implement this for a while now and finally got it working without segfaults.

Closes #1693 and implements classmethod support in pybind11. This allows defining classmethods which have the class of the caller automatically prepended to the list of args. This is nice because it allows adding/modifying class variables through the python bindings directly without a helper C++ class or struct.

NB: One thing I have noticed during my testing is that while __new__ has a classmethod signature, it is actually implemented as a static method. Therefore trying to add a classmethod onto __new__ will cause errors.

@rwgk I would like some thoughts on how to think about designing some proper tests for this functionality as there are probably a lot of edge cases having to do with potential mem leaks and other odd behavior. We also do play fast and loose with the terminology of classmethod with our add_classmethod function in pybind11 so I might need to refactor this.

Tests that may need to be added:

1. memleaks? (I am not if the added attributes are properly cleaned up).
2. docstring
3. overloading precedence / siblings

Additional things needed: documentation.

Suggested changelog entry:

* Implement classmethod support

[Skylion007]

Seems pypy doesn't expose the PyClassMethod_Type, is there a stable API way to implement the check_ method? Or even a good way to test it to verify it works. @rwgk @henryiii thoughts ?

[rwgk]

Seems pypy doesn't expose the PyClassMethod_Type, is there a stable API way to implement the check_ method? Or even a good way to test it to verify it works. @rwgk @henryiii thoughts ?

@mattip for the PyPy PyClassMethod_Type question

(I can look at this PR only later.)

[Skylion007]

Interestingly noticed it's not part of the CPython Stable-API, which is really odd for a pytype. It appears that CPython wants projects to move to the PyMethodDef API, but that only supports 3.9 and higher: https://docs.python.org/3/c-api/structures.html#c.PyMethodDef Still that doesn't give us a way of checking if a pytype is a classmethod, wonder what API we are suppose to use.

Then again, PyStaticMethod_Type isn't part of the Stable API either, but at least its supported by PyPy.

[mattip]

at least its supported by PyPy

At this point PyPy implements APIs on the basis of them being used by popular projects and can be easily implemented, irregardless of whether they are part of a formal standard.

[mattip]

It turned out to be only a few lines, so done in pypy c6e0a52f0. This will be part of the next PyPy release. For now you can put a #if !defined(PYPY_VERSION_NUM) || PYPY_VERSION_NUM >= 0x07030a00 to skip the failing code which is equivalent to sys.implementation.name != 'pypy' or sys.implementation.version >= (7, 3, 10)

[rwgk]

Tests that may need to be added:
memleaks? (I am not if the added attributes are properly cleaned up).

GHA valgrind is happy, ASAN & MSAN in the Google environment, too. That's a great start.

The only other thing I'd do, because it's super easy and conclusive beyond a doubt: locally modify the new test(s) by inserting while True:, run the test, in another shell (on the same machine) run the top command for a couple minutes and watch RES to see if it is stable for the test.

overloading precedence / siblings

I'd add a test similar to what you have, but with one argument, as a way to ensure that extra... is plumbed through.

TBH I need to learn what siblings are.

I wouldn't worry about overload precedence, that logic is somewhere completely different and I don't see how the new .def_classmethod could break anything there.

[rwgk]

Maybe serial or seq or similar? (It doesn't look like a universally unique identifier.)

[rwgk]

Maybe (untested):

    assert no hasattr(m.NoConstructor, "uuid")
    for i in range(num_instances):
        m.NoConstructor.new_instance_uuid()
        assert m.NoConstructor.uuid == i + 1

[rwgk]

Oh ...
Commit 2e3d29d from two months ago escaped my attention.
If you reply with Done or similar here it's more likely that that doesn't happen.

[virtuald]

Seems useful. Just add some docs and skip the old versions of pypy and we're done here?

[rwgk]

Seems useful. Just add some docs and skip the old versions of pypy and we're done here?

Agreed.

[virtuald]

FWIW, using this locally, docstring has 'self' as first parameter instead of 'cls'. The type is correct however. pybind11-stubgen will also need a fix to generate the classmethod correctly.

[virtuald]

Here's a format-patch for the docstring:

From d82875412b5e75ab23df4492d2f4b8624b9c69ef Mon Sep 17 00:00:00 2001
From: Dustin Spicuzza <[email protected]>
Date: Sat, 24 Dec 2022 14:39:47 -0500
Subject: [PATCH] Fix classmethod docstring

---
 include/pybind11/attr.h     | 21 +++++++++++++++++++--
 include/pybind11/pybind11.h |  2 +-
 tests/test_class.cpp        | 10 ++++++++++
 tests/test_class.py         |  3 +++
 4 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h
index db7cd8ef..cdd1423a 100644
--- a/include/pybind11/attr.h
+++ b/include/pybind11/attr.h
@@ -26,6 +26,12 @@ struct is_method {
     explicit is_method(const handle &c) : class_(c) {}
 };
 
+/// Annotation for classmethods
+struct is_classmethod {
+    handle class_;
+    explicit is_classmethod(const handle &c) : class_(c) {}
+};
+
 /// Annotation for operators
 struct is_operator {};
 
@@ -426,6 +432,16 @@ struct process_attribute<is_method> : process_attribute_default<is_method> {
     }
 };
 
+/// Process an attribute which indicates that this function is a classmethod
+template <>
+struct process_attribute<is_classmethod> : process_attribute_default<is_classmethod> {
+    static void init(const is_classmethod &s, function_record *r) {
+        r->is_method = true;
+        r->scope = s.class_;
+        r->args.emplace_back("cls", nullptr, handle(), /*convert=*/true, /*none=*/false);
+    }
+};
+
 /// Process an attribute which indicates the parent scope of a method
 template <>
 struct process_attribute<scope> : process_attribute_default<scope> {
@@ -668,10 +684,11 @@ using extract_guard_t = typename exactly_one_t<is_call_guard, call_guard<>, Extr
 /// Check the number of named arguments at compile time
 template <typename... Extra,
           size_t named = constexpr_sum(std::is_base_of<arg, Extra>::value...),
-          size_t self = constexpr_sum(std::is_same<is_method, Extra>::value...)>
+          size_t self = constexpr_sum(std::is_same<is_method, Extra>::value...),
+          size_t cls = constexpr_sum(std::is_same<is_classmethod, Extra>::value...)>
 constexpr bool expected_num_args(size_t nargs, bool has_args, bool has_kwargs) {
     PYBIND11_WORKAROUND_INCORRECT_MSVC_C4100(nargs, has_args, has_kwargs);
-    return named == 0 || (self + named + size_t(has_args) + size_t(has_kwargs)) == nargs;
+    return named == 0 || (cls + self + named + size_t(has_args) + size_t(has_kwargs)) == nargs;
 }
 
 PYBIND11_NAMESPACE_END(detail)
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 9fd4e214..bd2fd548 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -1593,7 +1593,7 @@ public:
     class_ &def_classmethod(const char *name_, Func &&f, const Extra &...extra) {
         cpp_function cf(std::forward<Func>(f),
                         name(name_),
-                        is_method(*this),
+                        is_classmethod(*this),
                         sibling(getattr(*this, name_, none())),
                         extra...);
         auto cf_name = cf.name();
diff --git a/tests/test_class.cpp b/tests/test_class.cpp
index 8dfd2630..a8e68efa 100644
--- a/tests/test_class.cpp
+++ b/tests/test_class.cpp
@@ -90,6 +90,16 @@ TEST_SUBMODULE(class_, m) {
                 cls.attr("seq_id") = seq_id + py::int_(1);
                 return NoConstructorNew::new_instance();
             },
+            "Returns a new instance and then increment the seq_id")
+        .def_classmethod(
+            "new_instance_seq_id_arg",
+            [](py::type &cls, int unused) {
+                py::int_ seq_id = getattr(cls, "seq_id", py::int_(0));
+                cls.attr("seq_id") = seq_id + py::int_(1);
+                cls.attr("unused") = unused;
+                return NoConstructorNew::new_instance();
+            },
+            py::arg("unused"),
             "Returns a new instance and then increment the seq_id");
 
     py::class_<NoConstructorNew>(m, "NoConstructorNew")
diff --git a/tests/test_class.py b/tests/test_class.py
index dca01b9d..38fc7ea4 100644
--- a/tests/test_class.py
+++ b/tests/test_class.py
@@ -37,6 +37,9 @@ def test_classmethod(num_instances=10):
         m.NoConstructor.new_instance_seq_id()
         assert m.NoConstructor.seq_id == i + 1
 
+    assert m.NoConstructor.new_instance_seq_id.__doc__.startswith("new_instance_seq_id(cls: type) ->")
+    assert m.NoConstructor.new_instance_seq_id_arg.__doc__.startswith("new_instance_seq_id_arg(cls: type, unused: int) ->")
+
 
 def test_type():
     assert m.check_type(1) == m.DerivedClass1
-- 
2.36.1

[virtuald]

Just kidding, that doesn't work if you specify arguments. Updated comment. It seems like the two options are to modify the function record (triggers an internals API bump?) or go this route. This seems less invasive, but it's also less consistent with how args are done elsewhere.