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

Various stubgen regressions in mypy 1.7.0 #16486

Closed
bluenote10 opened this issue Nov 14, 2023 · 3 comments
Closed

Various stubgen regressions in mypy 1.7.0 #16486

bluenote10 opened this issue Nov 14, 2023 · 3 comments
Labels
bug mypy got something wrong topic-stubgen

Comments

@bluenote10
Copy link
Contributor

bluenote10 commented Nov 14, 2023

Bug Report

Trying to update mypy from 1.6.1 to 1.7.0 has revealed a few regressions in the stub generator output related to pybind11 modules.

To Reproduce

Consider this pybind11 module:

#include <filesystem>
#include <optional>
#include <utility>
#include <vector>

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

namespace py = pybind11;

std::vector<float> funcReturningVector()
{
  return std::vector<float>{1.0, 2.0, 3.0, 4.0};
}

std::pair<int, float> funcReturningPair()
{
  return std::pair{42, 1.0};
}

std::optional<int> funcReturningOptional()
{
  return std::nullopt;
}

std::filesystem::path funcIncompleteSignature()
{
  // This example does not include <pybind11/stl/filesystem.h> on purpose
  // to demonstrate the signature of an incomplete binding.
  return std::filesystem::path{"foobar"};
}

struct MyStruct
{
  int a;
  int b;
  int c;
};

PYBIND11_MODULE(my_native_module, m)
{
  m.def("func_returning_vector", &funcReturningVector);
  m.def("func_returning_pair", &funcReturningPair);
  m.def("func_returning_optional", &funcReturningOptional);

  m.def("func_incomplete_signature", &funcIncompleteSignature);

  py::class_<MyStruct>(m, "MyStruct")
      .def_readwrite("a", &MyStruct::a)
      .def_readwrite("b", &MyStruct::b, "some docstring")
      .def_property_readonly(
          "c",
          [](const MyStruct& x) {
            return x.c;
          },
          "some docstring");
}

To reproduce, store the above snippet as my_native_module.cpp and then:

# Install latest pybind11:
pip install pybind11==2.11.1

# Compile pybind11 module:
c++ -O3 -Wall -shared -std=c++17 -fPIC $(python3 -m pybind11 --includes) my_native_module.cpp -o my_native_module.so

# Run the stub generator on it:
stubgen -p my_native_module

Expected Behavior

mypy 1.6.1 outputs the following, which is pretty reasonable:

from typing import Any, List, Optional, Tuple

class MyStruct:
    a: int
    b: int
    def __init__(self, *args, **kwargs) -> None: ...
    @property
    def c(self) -> int: ...

def func_incomplete_signature(*args, **kwargs) -> Any: ...
def func_returning_optional() -> Optional[int]: ...
def func_returning_pair() -> Tuple[int,float]: ...
def func_returning_vector() -> List[float]: ...

Actual Behavior

mypy 1.7.0 outputs the following:

from _typeshed import Incomplete

class MyStruct:
    a: int
    b: Incomplete
    def __init__(self, *args, **kwargs) -> None: ...
    @property
    def c(self): ...

def func_incomplete_signature(*args, **kwargs): ...
def func_returning_optional() -> Optional[int]: ...
def func_returning_pair() -> Tuple[int, float]: ...
def func_returning_vector() -> List[float]: ...

The regressions are:

  • The most obvious thing is that the imports for List, Tuple, and Optional are missing. This makes the resulting .pyi invalid, because it uses symbols that have not been imported. The stub output does not type check. This is not limited to these three types, but applies to other types like Iterable, Iterator, Dict, Set, Union, etc.
  • The type annotation of field b has regressed from int to Incomplete, apparently due to the explicit docstring.
  • The type annotation of field c has regressed from int to not annotated at all, probably also because of the explicit docstring.
  • The signature of the func_incomplete_signature has also slightly worsened: Before, it was at least annotated as -> Any but now it misses any return type annotation at all. This is problematic when users want to validate their generated stubs for completeness, i.e., run mypy --disallow-untyped-defs on the output. Of course semantically it is the same, but still it would be nicer to always produce some return type (arguably a broken signature may be rather annotated as -> object instead of -> Incomplete to avoid running into bugs from incomplete stub output).

Your Environment

  • Mypy version used: 1.7.0
  • Mypy command-line flags: irrelevant for stubgen
  • Mypy configuration options from mypy.ini (and other config files): irrelevant for stubgen
  • Python version used: 3.10
@bluenote10
Copy link
Contributor Author

It also looks to me like --include-docstrings has no effect in general:

If I copy the contents of test-data/pybind11_mypy_demo/src/main.cpp locally, compile it, and run the stub generator with --include-docstrings, the output is exactly the same as if I omit it, i.e., no docstrings in the result. I've verified that the __doc__ are properly set at runtime.

I've also checked out the mypy repo and ran ./misc/test-stubgenc.sh locally. This also results in all docstrings getting removed from the test-data/pybind11_mypy_demo/stubgen-include-docs outputs. Surprisingly the bash scripts exits with return code 0 in this case, which it shouldn't, because the output does not match to the expected output. It looks like the CI got broken in #13284. I guess the issues is in line 27:

EXIT=$?

This $? is always 0 so it swallows the test failure. It should probably be just EXIT=1.

@chadrik
Copy link
Contributor

chadrik commented Nov 15, 2023

The most obvious thing is that the imports for List, Tuple, and Optional are missing. This makes the resulting .pyi invalid, because it uses symbols that have not been imported. The stub output does not type check. This is not limited to these three types, but applies to other types like Iterable, Iterator, Dict, Set, Union, etc.

The MR that was merged prior to 1.7 unified the code and many of the behaviors of the C-extension and pure-python stubgen processes. In the pure-python code path, imports are only added for modules that have been imported: there is limited specialized behavior to detect and import types whose names match those in the typing or typing_extensions modules and these are shared by the C and pure-python code paths:

        known_imports = {
            "_typeshed": ["Incomplete"],
            "typing": ["Any", "TypeVar", "NamedTuple"],
            "collections.abc": ["Generator"],
            "typing_extensions": ["TypedDict", "ParamSpec", "TypeVarTuple"],
        }

There are real-world projects that I use stubgen on (PySide2) which have classes that match the names of those in the typing module, and the automatic generation of imports broke the stubs. The simple solution to the problem is for docstrings to use native types (list, dict, set, etc), or specify the module (typing.List, typing.Dict), but I do think that it makes sense to add a new option to stubgen to automatically add typing imports for known types.

The signature of the func_incomplete_signature has also slightly worsened: Before, it was at least annotated as -> Any but now it misses any return type annotation at all. This is problematic when users want to validate their generated stubs for completeness, i.e., run mypy --disallow-untyped-defs on the output. Of course semantically it is the same, but still it would be nicer to always produce some return type (arguably a broken signature may be rather annotated as -> object instead of -> Incomplete to avoid running into bugs from incomplete stub output).

This is another example of the C-extension behavior being adjusted to match the pure-python behavior (by sharing code paths). The C-extension behavior now correctly distinguishes types that are known to be Any (via annotations or docstrings), from those which are not known. This is a feature that ensures that stub authors can find and correct unknown types. In the case of unknown types, stubgen now consistently omits the type from the signature, or it uses the placeholder type _typeshed.Incomplete if the type can only be partially inferred, for example Iterator[_typeshed.Incomplete] in the case of a function that has a yield statement. Iterator[Any] would be over-specific in scenarios where the contained type cannot be inferred.

It would be reasonable to add a flag to use Any instead of Incomplete and generate them in cases that would otherwise be omitted, and with the new unified behavior, this flag would work for both C and pure-python generators. Such a hypothetical flag would likely produce output like this:

def func_returning_array(*args: Any, **kwargs: Any) -> Any: ...

The type annotation of field b has regressed from int to Incomplete, apparently due to the explicit docstring.
The type annotation of field c has regressed from int to not annotated at all, probably also because of the explicit docstring.

I'll add these to the pybind11 demo and fix them in the generator.

JelleZijlstra pushed a commit that referenced this issue Nov 29, 2023
This addresses several regressions identified in
#16486

The primary regression from #15770 is
that pybind11 properties with docstrings were erroneously assigned
`typeshed. Incomplete`.

The reason for the regression is that as of the introduction of the
`--include-docstring` feature
(#13284, not my PR, ftr),
`./misc/test-stubgenc.sh` began always reporting success. That has been
fixed.

It was also pointed out that `--include-docstring` does not work for
C-extensions. This was not actually a regression as it turns out this
feature was never implemented for C-extensions (though the tests
suggested it had been), but luckily my efforts to unify the pure-python
and C-extension code-paths made fixing this super easy (barely an
inconvenience)! So that is working now.

I added back the extended list of `typing` objects that generate
implicit imports for the inspection-based stub generator. I originally
removed these because I encountered an issue generating stubs for
`PySide2` (and another internal library) where there was an object with
the same name as one of the `typing` objects and the auto-import created
broken stubs. I felt somewhat justified in this decision as there was a
straightforward solution -- e.g. use `list` or `typing.List` instead of
`List`. That said, I recognize that the problem that I encountered is
more niche than the general desire to add import statements for typing
objects, so I've changed the behavior back for now, with the intention
to eventually add a flag to control this behavior.
@bluenote10
Copy link
Contributor Author

I just had time to re-check the behavior with mypy 1.8 that includes the fix by @chadrik. The example now produces the follow, which looks pretty:

from typing import List, Optional, Tuple

class FieldAnnotationTest:
    a: int
    b: int
    def __init__(self, *args, **kwargs) -> None: ...
    @property
    def c(self) -> int: ...

def answer() -> int: ...
def func_incomplete_signature(*args, **kwargs): ...
def func_returning_optional() -> Optional[int]: ...
def func_returning_pair() -> Tuple[int, float]: ...
def func_returning_vector() -> List[float]: ...

There is only the very minor regression left that the signature of the func_incomplete_signature lacks a return type, which only affects people who try to make their stubs conform to --disallow-untyped-defs in a post-processing (we apply a post-processing to *args and **kwargs to be typed as Incomplete -- now we also have to include the return type in that post-processing logic). I guess this is good enough to close this issue, and perhaps I will also find the time to contribute a small fix for that remaining aspect.

@chadrik Thanks again for the fix!

JelleZijlstra pushed a commit that referenced this issue Jan 13, 2024
Improve test cases around #16486

This PR does not change any actual mypy behavior, only hardens the
stubgen tests. The specific changes are:

- **dedicated test cases**: The existing pybind11 test cases originate
from a pybind11 demo. They cover a specific topic involving geometry
types and semi-implemented logic related to it. This somehow distracts
from the aspects we are trying to test here from the mypy stubgen
perspective, because it hides the actual intent of the bindings. I've
simply started adding new test cases that clearly express via their name
what the test case is addressing. I've kept the original demo stuff for
now, so that the new cases are just an addition (overhead is
negligible).
- **running mypy on the generated stubs**: In general it is crucial that
the output produced by the stubgen can actually be type checked by mypy
(this was the regression in #18486). This wasn't covered by the CI check
so far. I've added check now, which would have avoided the regression.
My goal for follow up PRs would be that we can use `mypy
--disallow-untyped-defs` or even `mypy --strict` on the output.
- **minor directory restructuring**: So far the expected stub outputs
were stored in folder names `stubgen` and `stubgen-include-docs`. This
was a bit confusing, because the folder `stubgen` suggested it contains
_the_ stubgen (implementation). I went for `expected_stubs_no_docs` and
`expected_stubs_with_docs` to make the role of the two folders more
explicit.
- **minor script bugfix**: Fix a bug in `test-stubgen.sh`: The
pre-delete functionality was broken, because the `*` was quoted and
therefore did not match.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-stubgen
Projects
None yet
Development

No branches or pull requests

3 participants