experimental-inspect: emit __all__ in the generated stubs - #6242
experimental-inspect: emit __all__ in the generated stubs#6242jonasdedden wants to merge 2 commits into
experimental-inspect: emit __all__ in the generated stubs#6242Conversation
|
https://peps.python.org/pep-0008/#module-level-dunder-names Maybe we should make sure that |
Thanks for linking the PEP8 on this topic! I had a deeper look, since the Let's look at I also checked What I implemented already conformed with |
|
Regarding the sorting I introduced in da59625. @chirizxc , would you care right now for I'm asking because there are also two other stub files on One could therefore argue that compliance to RUF022 (sorting of the |
Tpt
left a comment
There was a problem hiding this comment.
Thank you!
+1 for the approach taken on incomplete modules
| Other, | ||
| } | ||
|
|
||
| fn kind_of(name: &str) -> MemberKind { |
There was a problem hiding this comment.
instead of guessing the kind, what about sorting constants, classes and other separately then concatenate the outputs? This way you can avoid this guess
There was a problem hiding this comment.
Going further, is it correct to be sorting here at all? Wouldn't that potentially lead to the type stub having a different value for the constant compared to the runtime behaviour? Can that cause problems?
If the suggestion in https://github.com/PyO3/pyo3/pull/6242/changes#r3652677133 is to make the macros emit __all__ anyway, should we skip adding any code to pyo3-introspection and just add emission in the macros, where we can be confident of the runtime value?
There was a problem hiding this comment.
I removed the entire sorting again, mainly because I think that convert_members (introspection.rs:205-229) already sorts each of modules/classes/functions/attributes by name, and there also doesn't seem to be too much focus on strict ruff compliance elsewhere right now.
Regarding emitting __all__ in macros: I believe this would be a larger change and has some kinks here and there (small investigation by the side running). For now I'd propose to do it in the way this PR does it, and maybe we can investigate later?
| } | ||
|
|
||
| #[test] | ||
| fn test_dunder_all() { |
There was a problem hiding this comment.
nit: I would not bother writing unit tests for things that are already well covered by the integration tests. It's more code to maintain and not much added coverage
There was a problem hiding this comment.
I deleted test_isort_style_cmp and test_dunder_all_of_incomplete_module (the latter is covered by pytests/stubs/__init__.pyi); I kept one trimmed test_dunder_all for the two cases pytests doesn't cover (submodule in __all__, empty module). Feel free to signal if you want to have this moved into a separate integration-style test.
| /// | ||
| /// [`PyModuleMethods::add`]: https://docs.rs/pyo3/latest/pyo3/types/trait.PyModuleMethods.html#tymethod.add | ||
| fn dunder_all_stubs(module: &Module, imports: &Imports) -> Option<String> { | ||
| if module.incomplete { |
There was a problem hiding this comment.
pedantic nit: I would also check here that the module does not already include a __all__ constant. This way we can move the __all__ generation into the macros without breaking the old pyo3-introspection versions (the generating code here would just stop to run)
There was a problem hiding this comment.
Done, now bailing out if module.attributes already contains an __all__.
e6e8bbb to
cf721d0
Compare
|
@davidhewitt I explored generating Pros of macro-approach:
Contras:
Given that |
`PyModuleMethods::add` appends every name it adds to the module `__all__`, so every `#[pymodule]` with at least one member has an `__all__` at runtime. The generated stubs never declared one, so the stub and the runtime disagreed and `mypy.stubtest` reported one error per module. `__all__` is only emitted for complete modules: for a module tagged incomplete we don't know the full list of members, and a partial `__all__` would hide from type checkers names that do exist at runtime. Fixes PyO3#6241 It is also followed by an empty line now, so that it never sticks to the first declaration. Both match what `typeshed` does: of the 231 stdlib stubs declaring `__all__`, 229 have it before any class or function and 204 have an empty line after it. `dunder_all_stubs` now sizes the name vector upfront and returns before allocating it at all when the module has no member, as suggested in review. Review comments
…time one Nothing tied the `__all__` in the checked-in stubs to the value the modules actually build at import time, which is what let the `#[pyfunction(name = "...")]` mismatch go unnoticed. The test compares sets rather than lists: the stubs list the members in the order they are declared in, which is not the order `PyModuleMethods::add` appends them in. `__all__` is only ever consumed as a set of names, so that difference is not observable — writing the assertion this way is also the clearest place to record that decision. It needs no `experimental-inspect` build, only the checked-in stubs and the extension, so it runs as part of the normal `pytest` session. Run the `__all__` stub test where the build matches the stubs `ruff`'s `B009` rejected the `getattr` call, and the test itself only holds for a `pyo3_pytests` built with `experimental-async,experimental-inspect`, which is what the checked-in stubs are generated from. `pytests`' own session builds without them, so `annotations` and `with_async` are missing there; run the test from the `test-introspection` session instead.
3da1fee to
58691aa
Compare
Fixes #6241.
Problem
PyModuleMethods::addappends every name it adds to the module's__all__(
src/types/module.rs:500), soevery
#[pymodule]with at least one member has an__all__at runtime.pyo3-introspectionneveremitted one, so the stub and the runtime disagreed and
mypy.stubtestreported one error per module:Beyond
stubtest, a stub without__all__makes type checkers fall back to the implicit re-exportrules, which is not what the runtime module actually exports.
Change
module_stubsnow emits an__all__listing the module's submodules, classes, functions andattributes — exactly the names
addputs there at runtime. The names are sorted, and the list isserialized through the existing
Exprmachinery so string escaping is shared with the rest of thegenerator.
The checked-in
pytests/stubs/*.pyiare regenerated accordingly.The open question in the issue: incomplete modules
Modules with a
#[pymodule]/#[pymodule_init]function are tagged incomplete and get adef __getattr__(name: str) -> Incomplete: ...marker. This PR emits no__all__for those.The alternatives don't hold up:
__all__ = [...]asserts a completeness the introspector cannot verify. It would hidefrom type checkers names that do exist at runtime, and it does not silence
stubtestanyway — itjust swaps
__all__ is not present in stubfornames exported from the stub do not correspond to the names exported at runtime.__all__: list[str]does not help either:stubtestexplicitly handles that case andstill compares the exported names (
mypy/stubtest.py,_verify_exported_names), so it reports thesame mismatch.
Leaving
__all__out keeps the stub accurate. It is easy to revisit if maintainers prefer otherwise— it's the one
if module.incompleteguard indunder_all_stubs.Effect on
pyo3_pytestsMeasured with
stubtest2.3.0 on CPython 3.13, stubs copied next to the extension:__all__errorsstubtesterrorsThe one remaining
__all__error is the rootpyo3_pytestsmodule, which is incomplete because ofits
#[pymodule_init]. Its runtime__all__consists entirely of submodules, which the stubs do notdeclare yet — that is the separate submodule-declaration issue, and the two fixes compose.
Tests
pyo3-introspection/src/stubs.rs: the normal case, an incomplete module (no__all__), and an empty module (no__all__, since nothing is everadded and the attributedoes not exist at runtime either).
nox -s test-introspectionpasses against the regeneratedpytests/stubs.guide/src/type-stub.md) example output and the incomplete-modules note updated.