Open bluenote10 opened 1 year ago
Thanks for identifying the problem. I agree that ...
is a poor choice for representing invalid expressions.
I think it's better to introduce a wrapper similar to the one proposed in https://github.com/pybind/pybind11/pull/4888
Annotated[Any, pybind11.CppType("cpp_namespace::UserType")]
# pybind11_stubgen.typing_ext
def InvalidExpression(args: str) -> Any:
...
I made it a capitalized function instead of a class to reuse it in the context where I need an expression of arbitrary type. For example, in representing default values.
The resulting stubs would look like
from pybind11_stubgen.typing_ext import InvalidExpression
class SomeStruct:
a: Annotated[Incomplete, InvalidExpression("std::filesystem::path")]
def foo(x: : Annotated[Incomplete, InvalidExpression("std::filesystem::path")] = InvalidExpression("/dev/null")): ...
You are proposing to introduce a family of replacement strategies. Do you think all of them make sense? Can we stick to one?
I think Any
is a good choice. That is the default for untyped parameters for most type checkers.
def foo(x: int, y) -> None:
...
reveal_type(foo) # pyright reveals type as "(x: int, y: Any) -> None"
I do like the idea of _typeshed.Incompete
though to communicate that something went wrong.
The resulting stubs would look like
Yes, that would be great already!
You are proposing to introduce a family of replacement strategies. Do you think all of them make sense? Can we stick to one?
I think the only real choice is whether the underlying type should behave like object
or Any
/Incomplete
. This makes a rather big difference, because Any
/Incomplete
would basically turn off type checking in these places. This will mainly make a difference on the return side, because if you have a function/property that is typed -> Any
you can accidentally use it in completely wrong contexts. In contrast, if it would have -> object
semantics, the user cannot do much with it, i.e., the value cannot be accidentally passed to a place that needs, say an actual/specific type T
. In a sense it would restrict the usage, and require manual isinstance
or # type: ignore
to do anything with such bindings. This can prevent bugs, and gives an incentive to actually fix the incomplete binding signature.
I don't really care about having all combinations, i.e., always wrapping into Annotated
is fine, and the decision between Any
and Incomplete
probably also doesn't need to be configurable. This would bring it down to a boolean switch, for the object
vs Any
/Incomplete
decision, so two possible types:
Annotated[object, InvalidExpression("std::filesystem::path")]
Annotated[Incomplete, InvalidExpression("std::filesystem::path")]
Regarding the latter, I don't have a strong opinion whether to use Any
or Incomplete
. One could argue that the second field in the annotation already communicates "invalid expression", so that using Incomplete
doesn't add much more information, and Any
suffices as well.
I don't think object
is an option at all.
Let's say we have a library with problematic bindings. It has an author and an end user.
The user should not be punished for the sins of the author. If we go with the object
option, mypy would create a ton of false positives for user code that would be hard to fix without altering the library/stubs.
The pybind11-stubgen
already prompts errors for invalid expressions. It's incentive enough to fix the bindings.
So I would say we go with one and only option:
Annotated[Incomplete, InvalidExpression("std::filesystem::path")]
The user should not be punished for the sins of the author. If we go with the object option, mypy would create a ton of false positives for user code that would be hard to fix without altering the library/stubs.
Here is a use case: Think of a mono-repo which houses everything, C++ libraries, their stubs, and all user code. The stubs are not meant to be used by someone else. We really would like to "punish ourselves" for having bad stubs, because it will force us to work on getting the stubs right. The mere fact that working with broken stubs is a pain helps to make them better, so to say.
This is definitely not the standard use case, and using Any
/ Incomplete
as the default is fine. But it would be nice to offer it as an option, for people who want to stick to strict type checking, and reduce the potential from bugs introduced by Any
leaking around.
Side note: I find it interesting that the mentality of typed Python and TypeScript is quite different regarding that choice. The equivalent of Any
vs object
is the any
vs unknown
choice in TypeScript. It is often summarized as:
unknown
is I don't knowany
is I don't careand the community repeats "favor unknown
over any
" like a mantra, despite or because it forces one to write cleaner code, but with some degree of punishment.
From that perspective: If the stub generator fails to infer a type it falls into the "I don't know" category, and not the "I don't care". So unknown
/ object
is valid choice, and semantically it is "more correct" than using Any
. In practice the right decision depends on whether the use case favors strictness or practicality.
The pybind11-stubgen already prompts errors for invalid expressions. It's incentive enough to fix the bindings.
It should be noted that there are a few technical limitations in pybind11 itself that prevent being 100% invalid expression free (need to investigate them further, but so far I'm afraid they may not even be easily fixable on pybind11 side). Therefore, we are kind of forced to turn off this check in pybind11-stubgen anyway to use it.
Do you have an example?
Do you have an example?
Unfortunately not yet something small reproducible. I think there may actually be several root causes in pybind11. Trying not to go too much off-topic: One seems to be related to instantiation of templated classes / functions, which under certain conditions seem to mess up the type annotations. Another seems to be related to complex inter package/module dependencies that cannot simply be resolved via typical py::module::import
due to possible circular dependencies (which are actually technically valid at runtime, but just not statically :confused:). But I'll have to investigate all that independently.
Our observation is simply that even despite putting a lot of effort into getting the stubs perfect, there may be edge cases (possibly bugs in pybind11) that mean the output is only ~99.9% valid, and not 100%. Unfortunately such a scenario then requires to disable the "error on invalid expressions" feature, and it would become valuable to see/feel these broken 0.1% as being strictly typed in the "I don't know" rather than the "I don't care" sense.
IMO, the 0.1% of use cases definitely falls into "use stubgen as library and tweak it for your use case". It would be unfair to bother everyone else with extra options in CLI.
Although, I don't mind having some specialized mixins in the repo (for being used as a library, not a CLI tool) if they could be reused by someone else in the 0.1%
Unfortunately not yet something small reproducible. I think there may actually be several root causes in pybind11. Trying not to go too much off-topic: One seems to be related to instantiation of templated classes / functions, which under certain conditions seem to mess up the type annotations. Another seems to be related to complex inter package/module dependencies that cannot simply be resolved via typical py::module::import due to possible circular dependencies (which are actually technically valid at runtime, but just not statically 😕). But I'll have to investigate all that independently.
I've found that a 2-pass solution usually fixes any type related issues as you're describing here, even with complex templates and weird dependency loops in types: first define all your classes / enums first, and then in a second pass add all the functions/attributes to the classes.
I'm currently experimenting with migrating from mypy's stubgen to pybind11-stubgen and noticed a bigger difference in the handling of invalid types (unfortunately not 100% preventable due to certain limitations in pybind11 itself). In the following example I have not added the
#include <pybind11/stl/filesystem.h>
on purpose to emulate the situation of a missing binding:pybind11-stubgen produces the following stub output:
We run mypy on our Python code to ensure proper usage of type stubs (including the stubs as an extra consistency check). Unfortunately, mypy does not like the generated stubs. First it complains about the last signature, and this error even prevents further checking of the file:
After removing this last function, mypy still complains about all other usages of the ellipsis as a type with
error: Unexpected "..." [misc]
. Pyright seems to be bit more permissive, but also complains about the usages inb
,c
, andd
with"..." is not allowed in this context
.For comparison, mypy's stubgen produces the following, which isn't fully consistent either, but at least does not mess up the type checking:
This made me wonder why pybind11-stubgen is actually using
...
and what the typing specs say about using...
as a type. I've found a few related issues, but I'm still not quite sure if this is intended to work:My understanding so far was that
...
actually has a special role different from meaningAny
or so. For instance intuple[int, ...]
it expresses that the tuple type is heterogenous with an undefined arity, whereas the typetuple[int, Any]
would rather mean an arity of 2, with the second tuple field arbitrarily typed. If I remember correctly it also means something special inCallable[[...], object]
(also in the sense of "arbitrary arity"). Note that this problematic with using...
in the sense of an incomplete binding: If there is anstd::pair<int, SomeIncompleteType>
the it makes a difference if the stub generator outputstuple[int, ...]
(homogenous tuple of ints, arbitrary length), ortuple[int, Incomplete]
(length-2 tuple, with second element untyped).That's why I'm surprised that the stub generator uses
...
for incomplete types. Is there any reason not to use typeshed'sIncomplete
, which was meant for that purpose? From a practical perspective it would be nice if the generated stub would be understood by mypy (+ pyright etc.).As a bonus feature, it would be even nicer if the behavior how the incomplete type mapping works could be controlled a bit more explicitly. A random idea would be to have a an argument
--invalid-expression-handling-strategy
allowing forany
: the type will get replaced byAny
. This should always work, but is the least explicit / type-safe.incomplete
: the type will get replaced by_typeshed.Incompete
. This is technically equivalent to usingAny
, but communicates more clearly the fact that the type stub in completely defined.object
: the type will get replaced byobject
. This adds extra type-safety because the usages will typically require explicit type checking.These three could be accompanied with
any-annotated
,incomplete-annotated
, andobject-annotated
which does the same, but wraps the type into e.g.Annotated[Incomplete, "<raw type annotation>"]
. This would basically be the equivalent to the current--print-invalid-expressions-as-is
but by moving the raw annotation into a string wrapped inAnnotated
the resulting stub would be syntactically valid (at least in combination with pybind11, printing the raw type annotations directly typically results just in invalid syntax).