-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] Support stub packages #17204
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
Conversation
|
76bf9f8 to
3e40e95
Compare
| let mut components = name.components(); | ||
| let module_name = components.next_back()?; | ||
|
|
||
| match resolve_package(search_path, components, &resolver_state) { | ||
| Ok(resolved_package) => { |
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 moved most of this block into resolve_module_in_search_path except the early-returning if package is a regular package (not a namespace package)
carljm
left a comment
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.
LGTM!
crates/red_knot_python_semantic/src/module_resolver/resolver.rs
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/src/module_resolver/resolver.rs
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/import/stub_packages.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/import/stub_packages.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/import/stub_packages.md
Outdated
Show resolved
Hide resolved
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.
This looks basically good! Two edge cases (which I'm not sure if you handle correctly or not here -- but would be good to test explicitly in any event) relate to the case of a single-file module in site-packages. E.g. if foo.py is a single-file module at the top-level in site-packages, I don't think it's valid to have a foo-stubs.pyi file in site-packages; foo-stubs.pyi should not be treated as the "stubs package" for the runtime foo module. (I'd be curious to see what mypy/pyright do there.) But it is definitely valid to have a foo-stubs/__init__.pyi file in site-packages, and that should be treated as the stub for the runtime foo package.
| The runtime package is a regular package but the stubs are namespace packages. Pyright skips the | ||
| stub package if the "regular" package isn't a namespace package. I'm not aware that the behavior | ||
| here is specificed, and using the stubs without probing the runtime package first requires slightly | ||
| fewer lookups. |
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.
this behaviour makes sense to me. I think in general, stubs packages should work exactly the same regardless of whether the runtime package is installed or not, and this behaviour seems consistent with that principle.
| It's recommended that stub packages use `__init__.pyi` files over `__init__.py` but it doesn't seem | ||
| to be an enforced convention. At least, Pyright is fine with the following. |
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.
lol, I have no opinion at all on this 😆 I'd be fine if we didn't support it, but I'm also fine if we do! It seems very unlikely to come up
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.
Yeah, not supporting it is harder, that's why I went with supporting it :)
df4817d to
9f62e3c
Compare
|
I added a test for There's already a test covering |
I confirmed that mypy also ignores a |
|
Thanks @MichaReiser! :-D |
* main: (42 commits) [playground] New default program (#17277) [red-knot] Add `--python-platform` CLI option (#17284) [red-knot] Allow ellipsis default params in stub functions (#17243) [red-knot] Fix stale syntax errors in playground (#17280) Update Rust crate clap to v4.5.35 (#17273) Fix RUF100 to detect unused file-level noqa directives with specific codes (#17042) (#17061) [ci] Fix pattern for code changes (#17275) [`airflow`] Update oudated `AIR301`, `AIR302` rules (#17123) [docs] fix formatting of "See Style Guide" link (#17272) [red-knot] Support stub packages (#17204) ruff_annotate_snippets: address unused code warnings [red-knot] Add a couple more tests for `*` imports (#17270) [red-knot] Add 'Format document' to playground (#17217) Update actions/setup-node action to v4.3.0 (#17259) Update actions/upload-artifact action to v4.6.2 (#17261) Update actions/download-artifact action to v4.2.1 (#17258) Update actions/setup-python action to v5.5.0 (#17260) Update actions/cache action to v4.2.3 (#17256) Update Swatinem/rust-cache action to v2.7.8 (#17255) Update actions/checkout action to v4.2.2 (#17257) ...
* main: (222 commits) [playground] New default program (#17277) [red-knot] Add `--python-platform` CLI option (#17284) [red-knot] Allow ellipsis default params in stub functions (#17243) [red-knot] Fix stale syntax errors in playground (#17280) Update Rust crate clap to v4.5.35 (#17273) Fix RUF100 to detect unused file-level noqa directives with specific codes (#17042) (#17061) [ci] Fix pattern for code changes (#17275) [`airflow`] Update oudated `AIR301`, `AIR302` rules (#17123) [docs] fix formatting of "See Style Guide" link (#17272) [red-knot] Support stub packages (#17204) ruff_annotate_snippets: address unused code warnings [red-knot] Add a couple more tests for `*` imports (#17270) [red-knot] Add 'Format document' to playground (#17217) Update actions/setup-node action to v4.3.0 (#17259) Update actions/upload-artifact action to v4.6.2 (#17261) Update actions/download-artifact action to v4.2.1 (#17258) Update actions/setup-python action to v5.5.0 (#17260) Update actions/cache action to v4.2.3 (#17256) Update Swatinem/rust-cache action to v2.7.8 (#17255) Update actions/checkout action to v4.2.2 (#17257) ...
Summary
This PR adds support for stub packages, except for partial stub packages (a stub package is always considered non-partial).
I read the specification at typing.python.org/en/latest/spec/distributing.html#stub-only-packages but I found it lacking some details, especially on how to handle namespace packages or when the regular and stub packages disagree on whether they're namespace packages. I tried to document my decisions in the mdtests where the specification isn't clear and compared the behavior to Pyright.
Mypy seems to only support stub packages in the venv folder. At least, it never picked up my stub packages otherwise. I decided not to spend too much time fighting mypyp, which is why I focused the comparison around Pyright
Closes #16612
Test plan
Added mdtests