Skip to content

Add support for partial stub packages #5227

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

Merged
merged 2 commits into from
Jun 21, 2018
Merged

Conversation

emmatyping
Copy link
Member

This should conform to PEP 561 as laid out in
https://www.python.org/dev/peps/pep-0561/#partial-stub-packages.

mypy/build.py Outdated
# 'partial\n' to make the package partial
# Partial here means that mypy should look at the runtime
# package if installed.
with open(stub_typed_file) as f:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the fscache not show the content of the file? Should we handle the case where the fscache thinks it exists but it actually doesn't?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I should probably use fscache.read() here

mypy/build.py Outdated
# package if installed.
with open(stub_typed_file) as f:
src = f.read()
if src == 'partial\n':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the PEP does explicitly say partial\n, would it be better to just check src.strip() == 'partial'? It doesn't seem worth worrying about the exact newline representation.

Copy link
Member Author

@emmatyping emmatyping Jun 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are probably right.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I like the idea. This marker allows fine grained customisation of whether to use fallbacks (inline, typeshed, etc) if a module stub is not found. While my PR #5231 allows fine grained customisation whether to report an error if a module stub is not found (with or without a fallback depending on previous marker).

# Partial here means that mypy should look at the runtime
# package if installed.
if fscache.read(stub_typed_file).decode().strip() == 'partial':
runtime_path = os.path.join(pkg_dir, dir_chain)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP 561 is not 100% clear about this, but I think mypy should also use typeshed if a module is not found in a partial stub package. Unless I am missing something, the current implementation only captures one use case when a stub package is used to override an inline/runtime package. But there is another use case allowed by PEP 561 -- override typeshed with a stub package, what to do if the latter is declared partial and a module is not found? I think mypy should look in typeshed.

I mean you have this list 1...5 in the PEP, I think a simple rule is that if a module is not found in (3) but the package is partial we should just continue in normal order to (4) inline/runtime packages, and then (5) typeshed.

Copy link
Member Author

@emmatyping emmatyping Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the PEP text should be amended to be this way, fortunately for mypy, we almost already do what you describe. I think if I just re-order the elements of candidate_base_dirs to be in the right order as described by the PEP (which it sadly currently is not), it will "just work".

E: I also think it would cleaner and make more sense to refactor calculating the search path order in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it would cleaner and make more sense to refactor calculating the search path order in a separate PR.

OK, just don't forget to make this PR before the next release, so it will not contain partial implementation of partial packages :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is #5256

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will merge this one now, and will review #5256 later (it looks independent).

@ilevkivskyi ilevkivskyi merged commit 94fc51f into python:master Jun 21, 2018
ilevkivskyi added a commit that referenced this pull request Jun 25, 2018
There is a problem with annotating large frameworks -- they are large. Therefore it is hard to produce good stubs in a single pass. A possible better workflow would be to allow indicating that a given (sub-)package is incomplete.

I propose to use `__getattr__` for the role of such indicator. A motivation is that currently adding a `__getattr__` to `package/__init__.pyi` already makes `from package import mod` work, but `import package.mod` still fails (plus simplicity of implementation). Here are the rules that I propose:
* One can declare a (sub-)package as incomplete by adding a `__getattr__` to its `__init__.pyi`
* If the return type of this function is `types.ModuleType` or `Any`, we assume that all imports from this (sub-)package succeed.
* Incomplete package can contain a complete subpackage:
```
# file a/__init__.pyi
from types import ModuleType
def __getattr__(attr: str) -> ModuleType: ...

# file a/b/__init__.pyi
# empty (i.e. complete package)

# file main.py
import a.d  # OK
import a.b.c  # Error module not found
```

Note: these rules apply only to stubs (i.e. `.pyi` files). I add several tests to illustrate this behaviour.
This PR shouldn't give any significant performance penalty because the added parsing/loading only happens when an error would be reported (for our internal workflow the penalty will be zero because of the flags we use).

This PR will allow gradually adding stub modules to a large framework package, without generating loads of false positives for user code.

Note: PEP 561 introduces the notion of a partial stub package, implemented in #5227. I think however this is a bit different use case that I don't want to mix with this one for two reasons:
* Partial packages in PEP 561 are mainly focused on interaction between stubs and inline/runtime packages.
* The proposed feature may be also used in typeshed, not only for installed stub packages.
ilevkivskyi pushed a commit that referenced this pull request Jun 25, 2018
As promised in #5227, here is an implementation for refactoring and making the search path compliant with PEP 561. The order is specified in https://www.python.org/dev/peps/pep-0561/#type-checker-module-resolution-order.
@emmatyping emmatyping deleted the partialpkg branch December 4, 2018 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants