Ignore generic type signature in pybind11 functions and methods with overloads

[AWhetter]

I don't know if this is the correct place to address this issue, but I know that there's the expertise here to be able to discuss the correct approach to the problem. I've written this as a feature request, but there may be better approaches to the problem.

Use Case

I have a C++ module that's exposed to Python with pybind11. In this module I have a class with a method that has multiple types. To use an example from the pybind11 documentation:

py::class_<Pet>(m, "Pet")
 .def(py::init<const std::string &, int>())
 .def("set", (void (Pet::*)(int)) &Pet::set, "Set the pet's age")
 .def("set", (void (Pet::*)(const std::string &)) &Pet::set, "Set the pet's name");

This produces docstrings that look like the following:

>>> help(example.Pet)

class Pet(__builtin__.object)
 | Methods defined here:
 |
 | __init__(...)
 | __init__(self: Pet, arg0: str, arg1: int) -> None
 |
 | set(...)
 | set(*args, **kwargs) -> Any
 | Overloaded function.
 |
 | 1. set(self: Pet, arg0: int) -> None
 |
 | Set the pet's age
 |
 | 2. set(self: Pet, arg0: str) -> None
 |
 | Set the pet's name

Current Behaviour

When running stubgen against this module, the generated type stubs look like the following:

from typing import overload

class Pet:
 def __init__(self: Pet, arg0: str, arg1: int) -> None: ...
 @overload
 def set(self: Pet, arg0: int) -> None: ...
 @overload
 def set(self: Pet, arg0: str) -> None: ...
 @overload
 def set(*args, **kwargs) -> Any: ...

The inclusion of the last overload essentially makes the type definition of the method useless because it'll accept anything.
The reason that this feels to me like an issue for stubgen to address is because I think that pybind11 is doing the right thing. The inclusion of set(*args, **kwargs) -> Any makes sense because tools like Sphinx need to be given a valid type signature and those tools expect that signature on the first line of the docstring.

Expected Behaviour

When running stubgen against this module, the generated type stubs should look like the following:

from typing import overload

class Pet:
 def __init__(self: Pet, arg0: str, arg1: int) -> None: ...
 @overload
 def set(self: Pet, arg0: int) -> None: ...
 @overload
 def set(self: Pet, arg0: str) -> None: ...

Note that the set(*args, **kwargs) -> Any type signature has been ignored.

Implementation

If we decide that getting stubgen to ignore this specific type signature is the right thing to do, how should we do it? Ignoring *args, *kwargs here is not correct because having *args and **kwargs could be part of a valid signature:

mypy/mypy/stubdoc.py

Lines 193 to 202 in 007b7af

	def get_signatures(self) -> List[FunctionSig]:
	"""Return sorted copy of the list of signatures found so far."""
	def has_arg(name: str, signature: FunctionSig) -> bool:
	return any(x.name == name for x in signature.args)
	def args_kwargs(signature: FunctionSig) -> bool:
	return has_arg('*args', signature) and has_arg('**kwargs', signature)
	# Move functions with (*args, **kwargs) in their signature to last place.
	return list(sorted(self.signatures, key=lambda x: 1 if args_kwargs(x) else 0))

We could look for the "Overloaded signature." line and ignore the type signature in only that case. It feels brittle, but I don't know how else we can determine whether this situation exists.

Do we need to worry about the case where someone might decide to write the following:

"""func(*args, **kwargs) -> Any
Do a thing.

func() -> None
Do something else.

Versions

mypy 0.780
Python 3.7.7

[JukkaL]

Perhaps somebody who has experience with pybind can comment on this issue?

[AWhetter]

I did have another idea for a possible fix which is that when stubgen finds a mix of numbered type signatures and type signatures without numbering, it will include only those type signatures that are numbered. It would be way less brittle than looking for Overloaded function. and even allow usage of function calls and signatures in the docstring descriptions without confusing stubgen?

[YannickJadoul]

I believe the idea from the pybind11-side is to honor the "first line of the docs is the signature" rule for C-functions that can't have __annotations__ set. The function as a whole accepts set(*args, **kwargs) -> Any, and pybind11 internally does overload resolution between the overloads listed further down in the docstring.

But from this, it seems like mypy scans the whole string docstring for signatures and extracts everything that looks like a docstring?

[AWhetter]

@JukkaL Are you looking for feedback from mypy contributors who know pybind or from pybind contributors?

I'm not sure if the issue is specific to pybind. I've tried to reason that what pybind is doing is the correct thing, and I think it could apply to any C extension.

[AWhetter]

I've done some more digging and it appears that not only is putting a single signature at the start of your docstring a convention but that you can also put multiple signatures at the start of the docstring. I can't find any concrete source for this (such as a PEP) but it's what the CPython source code uses (see https://github.com/python/cpython/blob/c0f22fb8b3006936757cebb959cee94e285bc503/Objects/rangeobject.c#L156) and Sphinx also recently started supporting this convention: sphinx-doc/sphinx#7748

So I'm now thinking that pybind is not doing the right thing.
The convention is to have a single docstring that describes every signature, but pybind lets you define the docstring of each signature separately and combines them afterwards. I think that the minimum change that pybind can make to support the convention is to not output the (*args, **kwargs) -> Any signature and to instead put all signatures at the start of the docstring:

>>> help(example.Pet)

class Pet(__builtin__.object)
 | Methods defined here:
 |
 | __init__(...)
 | __init__(self: Pet, arg0: str, arg1: int) -> None
 |
 | set(...)
 | set(self: Pet, arg0: int) -> None
 | set(self: Pet, arg0: str) -> None
 | Overloaded function.
 |
 | 1. set(self: Pet, arg0: int) -> None
 |
 | Set the pet's age
 |
 | 2. set(self: Pet, arg0: str) -> None
 |
 | Set the pet's name

On the stubgen side we could get away with changing nothing, but it would be an improvement to support signature deduplication if it does not do that already.

As an alternative and I think more ideal solution, I think users could use pybind like this:

py::options options;
options.disable_signature_sections(); // This new option would control this new behaviour

py::class_<Pet>(m, "Pet")
 .def(py::init<const std::string &, int>())
 .def("set", (void (Pet::*)(int)) &Pet::set, "Set an attribute on the pet. When arg0 is a string the pet's name is set.\nWhen arg0 is an integer the pet's age is set.")
 .def("set", (void (Pet::*)(const std::string &)) &Pet::set);

With that code, pybind would make docstrings like this:

>>> help(example.Pet)

class Pet(__builtin__.object)
 | Methods defined here:
 |
 | __init__(...)
 | __init__(self: Pet, arg0: str, arg1: int) -> None
 |
 | set(...)
 | set(self: Pet, arg0: int) -> None
 | set(self: Pet, arg0: str) -> None
 | Set an attribute on the pet. When arg0 is a string the pet's name is set.
 | When arg0 is an integer the pet's age is set.

This solution would not require a change on the stubgen side.

@YannickJadoul I've made suggestions about changes to pybind here. How would you feel about moving this discussion to a pybind11 issue?

[YannickJadoul]

@AWhetter This does seem interesting to me, yes :-) Of course, that doesn't per se mean a lot of pybind11, but as far as I'm concerned, this is definitely something worth discussing in an issue or PR. Somehow, such a solution seems to make more sense to me than mypy/stubgen adapting.

I you don't mind, please open an issue (or draft PR; not sure what you've already tried), and we can discuss in the context of pybind11! :-)