Fix stubgen regressions with pybind11 and mypy 1.7 #16504
Conversation
|
@bluenote10 Have a look. |
This comment has been minimized.
This comment has been minimized.
chadrik
force-pushed
the
stubgen/fix-regressions
branch
from
ecc6b2e
to
1e1e8da
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| .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.
This Point example is getting a bit out of hand, or rather, it makes it not so obvious which aspect we are actually trying to test (it distracts with actual "point" semantics).
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 def_readwrite is handled (there was this rejected PEP that suggested to allow adding docstring to fields -- i.e., I was wondering if the --include-docstring feature tries to do that or not).
There was a problem hiding this comment.
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.
No worries, I can also take care of that in a follow-up.
chadrik
force-pushed
the
stubgen/fix-regressions
branch
from
73b7c47
to
86931c1
Compare
This comment has been minimized.
This comment has been minimized.
Fix pybind11 stubgen tests so that they report failures
chadrik
force-pushed
the
stubgen/fix-regressions
branch
from
86931c1
to
d54189c
Compare
|
Diff from mypy_primer, showing the effect of this PR on open source code: discord.py (https://github.com/Rapptz/discord.py): typechecking got 1.08x slower (180.0s -> 193.5s)
(Performance measurements are based on a single noisy sample)
|
| @@ -237,6 +239,26 @@ def __init__( | |||
| self.resort_members = self.is_c_module | |||
| super().__init__(_all_, include_private, export_less, include_docstrings) | |||
| self.module_name = module_name | |||
| if self.is_c_module: | |||
| # Add additional implicit imports. | |||
| # C-extensions are given more lattitude since they do not import the typing module. | |||
There was a problem hiding this comment.
Any reason for this particular choice of names? Obvious omissions are Sequence and Set.
There was a problem hiding this comment.
I restored what was there before. I was thinking of inspecting the contents of typing and typing_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.
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 to stubgen but it can wait for another PR.
| self.known_imports = { | ||
| "_typeshed": ["Incomplete"], | ||
| "typing": ["Any", "TypeVar", "NamedTuple"], | ||
| "collections.abc": ["Generator"], |
There was a problem hiding this comment.
maybe also AsyncGenerator?
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-docstringfeature (#13284, not my PR, ftr),./misc/test-stubgenc.shbegan always reporting success. That has been fixed.It was also pointed out that
--include-docstringdoes 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
typingobjects that generate implicit imports for the inspection-based stub generator. I originally removed these because I encountered an issue generating stubs forPySide2(and another internal library) where there was an object with the same name as one of thetypingobjects and the auto-import created broken stubs. I felt somewhat justified in this decision as there was a straightforward solution -- e.g. uselistortyping.Listinstead ofList. 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.