Operator methods include a type signature of the additional overload #2277
Conversation
| @@ -250,6 +250,7 @@ class cpp_function : public function { | |||
| /* Generate a proper function signature */ | |||
| std::string signature; | |||
| size_t type_index = 0, arg_index = 0; | |||
| std::string self_type("object"); | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
include/pybind11/pybind11.h
Outdated
| signatures += "\n"; | ||
| signatures += std::to_string(++index) + ". "; | ||
| signatures += rec->name; | ||
| signatures += "(self: " + self_type + ", other: object) -> NotImplemented\n"; |
There was a problem hiding this comment.
Although the type signature of an operator method is actually (*args: Any, **kwargs: Any) -> NotImplemented because pybind falls back to returning NotImplemented when all other type signatures fail, mypy is specifically looking for the signature implemented here. We could add an additional type signature (*args: Any, **kwargs: Any) -> NotImplemented for correctness as well but that feels like overkill to me?
|
Is there anything that I can do to help progress this pull request? |
|
Let's run such a thing past @wjakob, though. |
|
(More of a general rant, not specifically about this commit -- apologies @AWhetter) |
| if (rec->is_method) { | ||
| signatures += "self: " + self_type + ", "; | ||
| } | ||
| signatures += "*args, **kwargs) -> NotImplemented\n"; |
There was a problem hiding this comment.
Scrutinizing this again: NotImplemented is the value, and not the type, right. type(NotImplemented) prints <class 'NotImplementedType'> for me.
Also, is this the way it's done? Can you give me a reference of type annotations in pure Python code, perhaps?
There was a problem hiding this comment.
I think I had incorrectly assumed that being a singleton, that NotImplemented was treated like None and that you're allowed to supply it. In fact, mypy doesn't use or need it at all (python/mypy#6710 (comment)) and so it isn't often used. Given that comment, Any seems like the correct type to return?
Alternatively, we could combine the two signatures into one and simply say that the type of the second argument is always object, because it will always be accepted and mypy will assume that NotImplemented will be returned regardless. It's less useful for a user that way, but it resolves the original issue without much extra complexity in the implementation?
|
I will say that I'm not hugely invested in this change. While I made this for completeness and correctness, for the use case described in the original issue I don't think it's unreasonable to expect someone to have to go through the type stubs generated by stubgen after running it on a pybind module and having to manually curate the output. It does mean that the process goes from being automated to being more manual, but I think it's an acceptable workflow nonetheless. Overall I still think I would prefer the original issue to be addressed. I think it's worthwhile documenting that you can supply anything to operator methods for users' sake. But it's not the end of the world if we decide not to address it. @wjakob On the topic of bloat, do you mean bloat in terms of extra run time code, in code complexity, or in terms of final binary size? |
|
@AWhetter, as far as I understood the main concern is a steady increase of the binary size, by a series of small, insignificant increases such as this one. |
|
That makes sense. As mentioned in my previous comment, I can make this change smaller, but I get the impression that even that is too high a cost. I'm happy to close this and close the issue as wontfix if we think that's best? |
|
I wouldn't mind keeping this open for now, and collect it somewhere, in case we would later find a way to opt-in to this. |
|
For objective measurement, |
Closes #2269