-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix stubgen regressions with pybind11 and mypy 1.7 #16504
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -576,6 +576,14 @@ def __init__( | |
self.sig_generators = self.get_sig_generators() | ||
# populated by visit_mypy_file | ||
self.module_name: str = "" | ||
# These are "soft" imports for objects which might appear in annotations but not have | ||
# a corresponding import statement. | ||
self.known_imports = { | ||
"_typeshed": ["Incomplete"], | ||
"typing": ["Any", "TypeVar", "NamedTuple"], | ||
"collections.abc": ["Generator"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe also AsyncGenerator? |
||
"typing_extensions": ["TypedDict", "ParamSpec", "TypeVarTuple"], | ||
} | ||
|
||
def get_sig_generators(self) -> list[SignatureGenerator]: | ||
return [] | ||
|
@@ -667,15 +675,7 @@ def set_defined_names(self, defined_names: set[str]) -> None: | |
for name in self._all_ or (): | ||
self.import_tracker.reexport(name) | ||
|
||
# These are "soft" imports for objects which might appear in annotations but not have | ||
# a corresponding import statement. | ||
known_imports = { | ||
"_typeshed": ["Incomplete"], | ||
"typing": ["Any", "TypeVar", "NamedTuple"], | ||
"collections.abc": ["Generator"], | ||
"typing_extensions": ["TypedDict", "ParamSpec", "TypeVarTuple"], | ||
} | ||
for pkg, imports in known_imports.items(): | ||
for pkg, imports in self.known_imports.items(): | ||
for t in imports: | ||
# require=False means that the import won't be added unless require_name() is called | ||
# for the object during generation. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ | |
|
||
#include <cmath> | ||
#include <pybind11/pybind11.h> | ||
#include <pybind11/stl.h> | ||
|
||
namespace py = pybind11; | ||
|
||
|
@@ -102,6 +103,11 @@ struct Point { | |
return distance_to(other.x, other.y); | ||
} | ||
|
||
std::vector<double> as_vector() | ||
{ | ||
return std::vector<double>{x, y}; | ||
} | ||
|
||
double x, y; | ||
}; | ||
|
||
|
@@ -134,14 +140,15 @@ void bind_basics(py::module& basics) { | |
.def(py::init<double, double>(), py::arg("x"), py::arg("y")) | ||
.def("distance_to", py::overload_cast<double, double>(&Point::distance_to, py::const_), py::arg("x"), py::arg("y")) | ||
.def("distance_to", py::overload_cast<const Point&>(&Point::distance_to, py::const_), py::arg("other")) | ||
.def_readwrite("x", &Point::x) | ||
.def("as_list", &Point::as_vector) | ||
.def_readwrite("x", &Point::x, "some docstring") | ||
.def_property("y", | ||
[](Point& self){ return self.y; }, | ||
[](Point& self, double value){ self.y = value; } | ||
) | ||
.def_property_readonly("length", &Point::length) | ||
.def_property_readonly_static("x_axis", [](py::object cls){return Point::x_axis;}) | ||
.def_property_readonly_static("y_axis", [](py::object cls){return Point::y_axis;}) | ||
.def_property_readonly_static("y_axis", [](py::object cls){return Point::y_axis;}, "another docstring") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This In my opinion it would be nicer if we could add new test cases that more directly express what we want to test for, which is what I tried to prepare in the issue description #16486. In particular it would be nice to have e.g. struct FieldAnnotationTest
{
int a;
int b;
int c;
};
py::class_<FieldAnnotationTest>(m, "FieldAnnotationTest")
.def_readwrite("a", &FieldAnnotationTest::a)
.def_readwrite("b", &FieldAnnotationTest::b, "some docstring")
.def_property_readonly(
"c",
[](const FieldAnnotationTest& x) {
return x.c;
},
"some docstring"); because it is also an interesting question how a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what you're asking for is a good idea, but I'm not sure if I have the time to commit to doing it, since this is something I'm working on outside of work at the moment. If all the tests can be placed into main.cpp and we can easily piggy-back what's already there, then it might be straight-forward. Is that what you were thinking? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, I can also take care of that in a follow-up. |
||
.def_readwrite_static("length_unit", &Point::length_unit) | ||
.def_property_static("angle_unit", | ||
[](py::object& /*cls*/){ return Point::angle_unit; }, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from . import basics as basics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for this particular choice of names? Obvious omissions are Sequence and Set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restored what was there before. I was thinking of inspecting the contents of
typing
andtyping_extensions
modules to get the full list. Adding more names increases the likelihood of a name clash with first party code (the reason I removed these in the first place), so first I'd like to add a new flag,--no-implicit-typing-import
, to disable this functionality for users who don't want it. Give me a few days to find some time to do that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm going to make the call and say that I don't have additional time to invest in this right now, I'm sorry.
In its current state this PR restores the previous behavior and fixes all known regressions, so I think this I think we should merge this soon as-is, and consider cherry-picking it to a 1.7.x release, if there are any plans for that. I'd like to get these problems fixed before more people are affected by them.
I really want to add a
--no-implicit-typing-import
flag tostubgen
but it can wait for another PR.