Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow replacing ... by object / Any / Incomplete? #191

Open
bluenote10 opened this issue Nov 21, 2023 · 10 comments · May be fixed by #199
Open

Allow replacing ... by object / Any / Incomplete? #191

bluenote10 opened this issue Nov 21, 2023 · 10 comments · May be fixed by #199
Labels
bug Something isn't working

Comments

@bluenote10
Copy link

bluenote10 commented Nov 21, 2023

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:

#include <filesystem>
#include <unordered_map>
#include <vector>

#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

struct SomeStruct
{
  std::filesystem::path a;
  std::vector<std::filesystem::path> b;
  std::unordered_map<std::filesystem::path, int> c;
  std::unordered_map<int, std::filesystem::path> d;
};

std::filesystem::path funcA()
{
  return std::filesystem::path{"foobar"};
}

void funcB(std::filesystem::path, std::filesystem::path)
{
}

PYBIND11_MODULE(my_native_module, m)
{
  pybind11::class_<SomeStruct>(m, "SomeStruct")
      .def_readwrite("a", &SomeStruct::a)
      .def_readwrite("b", &SomeStruct::b)
      .def_readwrite("c", &SomeStruct::c)
      .def_readwrite("d", &SomeStruct::d);
  m.def("func_a", &funcA);
  m.def("func_b", &funcB);
}

pybind11-stubgen produces the following stub output:

from __future__ import annotations
__all__ = ['SomeStruct', 'func_a', 'func_b']
class SomeStruct:
    a: ...
    b: list[...]
    c: dict[..., int]
    d: dict[int, ...]
def func_a() -> ...:
    ...
def func_b(arg0: ..., arg1: ...) -> None:
    ...

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:

out/my_native_module.pyi:10: error: Ellipses cannot accompany other argument types in function type signature  [syntax]
Found 1 error in 1 file (errors prevented further checking)

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 in b, c, and d 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:

from _typeshed import Incomplete

class SomeStruct:
    a: Incomplete
    b: Incomplete
    c: Incomplete
    d: Incomplete
    def __init__(self, *args, **kwargs) -> None: ...

def func_a(*args, **kwargs): ...
def func_b(arg0, arg1) -> None: ...

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 meaning Any or so. For instance in tuple[int, ...] it expresses that the tuple type is heterogenous with an undefined arity, whereas the type tuple[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 in Callable[[...], object] (also in the sense of "arbitrary arity"). Note that this problematic with using ... in the sense of an incomplete binding: If there is an std::pair<int, SomeIncompleteType> the it makes a difference if the stub generator outputs tuple[int, ...] (homogenous tuple of ints, arbitrary length), or tuple[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's Incomplete, 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 for

  • any: the type will get replaced by Any. 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 using Any, but communicates more clearly the fact that the type stub in completely defined.
  • object: the type will get replaced by object. This adds extra type-safety because the usages will typically require explicit type checking.

These three could be accompanied with any-annotated, incomplete-annotated, and object-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 in Annotated 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).

@sizmailov sizmailov added the bug Something isn't working label Nov 21, 2023
@sizmailov
Copy link
Owner

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 pybind/pybind11#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?

@ringohoffman
Copy link
Contributor

ringohoffman commented Nov 22, 2023

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.

@bluenote10
Copy link
Author

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.

@sizmailov
Copy link
Owner

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")]

@bluenote10
Copy link
Author

bluenote10 commented Nov 23, 2023

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 know
  • any is I don't care

and 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.

@bluenote10
Copy link
Author

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.

@sizmailov
Copy link
Owner

Do you have an example?

@bluenote10
Copy link
Author

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 😕). 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.

@sizmailov
Copy link
Owner

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%

@sizmailov sizmailov linked a pull request Nov 25, 2023 that will close this issue
@virtuald
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants