Skip to content
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

Make ModuleFinder try identifying non-PEP 561 packages #8238

Conversation

Michael0x2a
Copy link
Collaborator

@Michael0x2a Michael0x2a commented Jan 4, 2020

This pull request is an attempt at mitigating #4542 by making mypy report a custom error when it detects installed packages that are not PEP 561 compliant.

(I don't think it resolves it though -- I've come to the conclusion that import handling is just inherently complex/spooky. So if you were in a cynical mood, you could maybe argue the issue is just fundamentally unresolvable...)

But anyways, this PR:

  1. Removes the hard-coded list of "popular third party libraries" from moduleinfo.py and replaces it with a heuristic that tries to find when an import "plausibly matches" some directory or Python file while we search for packages containing py.typed.

    If we do find a plausible match, we generate an error that looks something like this:

    test.py:1: error: Skipping analyzing 'scipy': found module but no type hints or library stubs
    test.py:1: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
    

    The heuristic I'm using obviously isn't foolproof since we don't have any obvious signifiers like the py.typed file we can look for, but it seemed to work well enough when I tried testing in on some of the libraries in the old list.

    Hopefully this will result in less confusion when users use a mix of "popular" and "unpopular" libraries.

  2. Gives us a way to add more fine-grained "module not found" error messages and heuristics in the future: we can add more entries to the ModuleNotFoundReason enum.

  3. Updates the docs about missing imports to use these new errors. I added a new subsection per each error type to try and make things a little less unwieldy.

  4. Adds clarifications on what I think are common points of confusion to the doc -- e.g. that missing imports are inferred to be of type Any, what exactly it means to add a # type: ignore, and the whole virtualenv thing.

  5. Modifies the docs to more strongly discourage the use of MYPYPATH. Unless I'm wrong, it's not a feature most people will find useful.

One limitation of this PR is that I added tests for just ModuleFinder. I didn't want to dive into modifying our testcases framework to support adding custom site-packages/some moral equivalent -- and my PR only changes the behavior of ModuleFinder when it would have originally reported something was not found, anyways.

This pull request is an attempt at mitigating
python#4542 by making
mypy report a custom error when it detects installed
packages that are not PEP 561 compliant.

(I don't think it resolves it though -- I've come to
the conclusion that import handling is just inherently
complex/spooky. So if you were in a cynical mode, you
could perhaps argue the issue is just fundamentally
unresolvable...)

But anyways, this PR:

1.  Removes the hard-coded list of "popular third party
    libraries" from `moduleinfo.py` and replaces it with
    a heuristic that tries to find when an import "plausibly
    matches" some directory or Python file while we search
    for packages containing ``py.typed``.

    If we do find a plausible match, we generate an error
    that looks something like this:

    ```
    test.py:1: error: Skipping analyzing 'scipy': found module but no type hints or library stubs
    test.py:1: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
    ```

    The heuristic I'm using obviously isn't foolproof since
    we don't have any obvious signifiers like the ``py.typed``
    file we can look for, but it seemed to work well enough
    when I tried testing in on some of the libraries in the old list.

    Hopefully this will result in less confusion when users
    use a mix of "popular" and "unpopular" libraries.

2.  Gives us a way to add more fine-grained "module not found"
    error messages and heuristics in the future: we can add
    more entries to the ModuleNotFoundReason enum.

3.  Updates the docs about missing imports to use these new
    errors. I added a new subsection per each error type to
    try and make things a little less unwieldy.

4.  Adds what I think are common points of confusion to the
    doc -- e.g. that missing imports are inferred to be of
    type Any, what exactly it means to add a `# type: ignore`,
    and the whole virtualenv confusion thing.

5.  Modifies the docs to more strongly discourage the use
    of MYPYPATH. Unless I'm wrong, it's not a feature most
    people will find useful.

One limitation of this PR is that I added tests for just ModuleFinder.
I didn't want to dive into modifying our testcases framework to support
adding custom site-packages/some moral equivalent -- and my PR
only changes the behavior of ModuleFinder when it would have originally
reported something was not found, anyways.
@msullivan msullivan self-requested a review January 9, 2020 18:38
@msullivan
Copy link
Collaborator

I will review this but I need to work up some courage first

@Michael0x2a
Copy link
Collaborator Author

Michael0x2a commented Jan 9, 2020

lol -- if it's any consolation, the changes that actually matter boil down to about 10-15 lines of code in modulefinder.py that make some methods return an enum instead of None when it couldn't find a module.

Everything else is just updating error messages/docs/tests/deleting dead code.

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

OK yeah this looks great. Thanks.

@Michael0x2a Michael0x2a merged commit 7af3191 into python:master Feb 23, 2020
@Michael0x2a Michael0x2a deleted the make-module-finder-identify-non-pep-561-packages branch February 23, 2020 23:38
sthagen added a commit to sthagen/python-mypy that referenced this pull request Feb 24, 2020
Make ModuleFinder try identifying non-PEP 561 packages (python#8238)
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.

2 participants