Implement PEP 561 searching #4403

ethanhs · 2017-12-22T20:53:46Z

This is an implementation of PEP 561.

Test functionality
Check PEP 561 conformance to resolution order
ignore errors in these files
support running with alternate Python executable
document PEP 561 feature
document --python-executable flag
test --python-executable flag
--python-version flag sets --python-executable if possible.

This branch should work as intended and be feature complete, but it is possible that I've overlooked something/made a mistake.
(Picked up from #4278 for simplicity).

Fixes #2625, #1190, #965.

The tests install a typed package and verify that the modules can be found correctly. It then does the same for a stub package.

ethanhs · 2017-12-22T22:49:38Z

Hm, that self test failure is strange. It seems it is picking up the pytest package? I will have to investigate that.

JelleZijlstra · 2017-12-23T17:34:34Z

docs/source/installed_packages.rst

+Making PEP 561 compatible packages
+**********************************
+
+Packages that supply type information should put a ``py.typed``.


... in their package directory. Maybe we should add an example of how to do this with setup.py?

Yes an example in the docs seems like a good idea, I will add that.

JelleZijlstra · 2017-12-23T17:36:31Z

mypy/build.py

+
+def call_python(python: str, command: str) -> str:
+    return check_output(python + ' -c ' + command,
+                        stderr=STDOUT).decode('UTF-8')


Why do you need to redirect stderr? And is utf-8 safe? Maybe we should pass PYTHONIOENCODING (https://docs.python.org/3/using/cmdline.html#envvar-PYTHONIOENCODING).

I originally wanted to handle error text, but I think it may not be needed after all.

I'm not the most familiar with best practices for encoding/decoding, but wouldn't the encoding of sys.stdout be most proper?

JelleZijlstra · 2017-12-23T17:37:34Z

mypy/build.py

+        if not check.startswith('Python'):
+            return package_dirs
+        # If we have a working python executable, query information from it
+        output = call_python(python, SITE_PACKAGE_COMMANDS[0])


Seems clearer to have two named constants instead of indexing with 0 and 1.

JelleZijlstra · 2017-12-23T17:40:03Z

mypy/build.py

+                for pkg_dir in package_dirs:
+                    stub_name = components[0] + '_stubs'
+                    typed_file = os.path.join(pkg_dir, components[0], 'py.typed')
+                    stub_typed_file = os.path.join(pkg_dir, stub_name, 'py.typed')


It wasn't clear to me from reading PEP 561 that stub packages also need a py.typed file.

Hm, I intended the section saying

Package maintainers who wish to support type checking of their code MUST add a marker file named py.typed to their package supporting typing.

to mean all packages should add it (in my mind including stub only packages). But I suppose having the _stubs suffix is rather indicative that it supports typing. I don't entirely have a preference here, but it seems more consistent to have the file in stub packages.

JelleZijlstra · 2017-12-23T17:42:02Z

mypy/main.py

@@ -245,6 +245,7 @@ def add_invertible_flag(flag: str,
                        version='%(prog)s ' + __version__)
    parser.add_argument('--python-version', type=parse_version, metavar='x.y',
                        help='use Python x.y')
+    parser.add_argument('--python', action='store', help="Point to a Python executable.")


This is rather laconic. Maybe "Python executable whose installed packages will be used in typechecking".

JelleZijlstra · 2017-12-23T17:44:22Z

test-data/packages/typedpkg/setup.py

+    package_data={'typedpkg': ['py.typed']},
+    packages=['typedpkg'],
+    include_package_data=True,
+)


add a newline

JelleZijlstra · 2017-12-23T17:44:46Z

test-data/packages/typedpkg/typedpkg/sample.py

+
+def ex(a: Iterable[str]) -> Tuple[str, ...]:
+    """Example typed package. This intentionally has an error."""
+    return a + ('Hello')


another missing newline (also in some files further down)

JelleZijlstra · 2017-12-23T17:47:21Z

mypy/build.py

            if path:
+                if any((path.startswith(d) for d in package_dirs_cache)):


Couldn't this break if the package_dirs_cache contains /a/b and the package is in a/bc?

Good point, I will fix this.

I'm actually less concerned about this. Since the directory will always be of the form eg /path/to/site-packages, any string concatenated to that will either be wrong (/path/to/site-packagesfoo?), or unsafe (as in it's unsafe to add site-packages to your path).

os.path.commonpath([d, path]) == d would be correct, as would not os.path.relpath(path, d).startswith('..')

JelleZijlstra · 2017-12-23T20:16:36Z

pytest is picked up because your implementation seems to always accept .py files in site-packages; pytest is implemented as a single file pytest.py. I added some print statements and got found pytest at .../lib/python3.6/site-packages/pytest.py. I'm not sure where in your code it's going wrong.

ethanhs · 2017-12-23T20:22:40Z

Yes I found that out earlier. It is due to my adding of site-packages to the candidate directories if pytest is not found otherwise. I have fixed that and will add some tests for scenarios where modules shouldn't be found.

Also added newlines and refactored call_python in build and get_package_dirs.

ethanhs · 2018-01-03T22:24:03Z

Bah testing installed packages is maddening. It seems I need to refactor find_module a bit more seriously as it isn't the most malleable to fit to finding typed packages.

eric-wieser · 2018-01-16T07:57:14Z

mypy/build.py

+
+
+def call_python(python: str, command: str) -> str:
+    return check_output(python + ' -c ' + command).decode(sys.stdout.encoding)


Use check_output([python, '-c', command]) to save a level of quoting

eric-wieser · 2018-01-16T07:59:21Z

mypy/test/helpers.py

+    return [
+        s.rstrip('\n\r')
+        for stream in streams
+        for s in str(stream, 'utf8').splitlines()


stream.decode('utf8') is a much more common spelling

eric-wieser · 2018-01-16T08:02:57Z

mypy/build.py

+                if os.path.isfile(stub_typed_file):
+                    components[0] = stub_name
+                    rest = components[:-1]
+                    path = os.path.join(pkg_dir, *rest)


Should this update dir_chain?

It doesn't necessarily have to, as dir_chain isn't used after this, unless I am overlooking something.

eric-wieser · 2018-01-19T01:52:15Z

mypy/build.py

+                    path = os.path.join(pkg_dir, dir_chain)
+                    dirs.append(path)
+
+            find_module_dir_cache[dir_chain] = dirs


It's used right here, isn't it?

Ah, good point, I will fix this.

Actually looking again we don't want to change this as it looks up the unmutated dir_cache.

Would be good if you would somehow make that clearer, perhaps ideal_dir vs real_dir or something.

carljm · 2018-03-05T20:14:00Z

There's also #4623 working in the same area.

eric-wieser · 2018-03-05T20:17:21Z

@carljm: The problem is that a lot of the caching just about worked when there were no extra arguments, but is now broken because it's not also keyed on those new arguments.

The refactor of find_module might have gone a little too far

eric-wieser · 2018-03-05T20:21:11Z

mypy/build.py

+            path = os.path.join(pkg_dir, dir_chain)
+            dirs.append(path)
+
+    find_module_dir_cache[dir_chain, lib_path] = dirs


This causes the list to grown on every invocation - should the third party section be inside the cache-checking if?

ethanhs · 2018-03-05T21:06:04Z

mypy/build.py

+                find_module_isdir_cache[pathitem, dir_chain] = isdir
+            if isdir:
+                dirs.append(dir)
+        find_module_dir_cache[dir_chain, lib_path] = dirs


@eric-wieser I realized I was essentially mutating the dir cache to add site packages then stripping that out. So I refactored things to not do that, which is much nicer.

Much better!

Everything from here to the top of this function would be better as a separate helper function, because it would then be obvious that the caching is correct.

Left a TODO.

eric-wieser · 2018-03-05T22:19:22Z

mypy/build.py

+            third_party_dirs.append(path)
+
+    return tuple(third_party_dirs +
+                 find_module_dir_cache[dir_chain, lib_path]), components


Isn't this the same as the components that was passed in? Why return it?

Ah, I meant to remove this. Doing that now.

eric-wieser · 2018-03-05T22:21:42Z

mypy/build.py

+            path = os.path.join(pkg_dir, *rest)
+            if os.path.isdir(path):
+                third_party_dirs.append(path)
+            components[0] = prefix


Why do you modify components? This would be clearer as:

stub_components = [stub_name] + components[1:] path = os.path.join(pkg_dir, *stub_components[:-1])

Rather than mutating the components list

eric-wieser · 2018-03-05T22:37:55Z

mypy/build.py

+
+
+def find_module_in_base_dirs(id: str, candidate_base_dirs: Iterable[str],
+                             last_component: str) -> Optional[str]:


~~This isn't really correct on python 2 - you need to walk up the tree looking for init.py files at each level, rather than assuming that only the last stage matters~~

Edit: I see that happens in verify_module

Note: this is one of the correctness issues for mypy's import implementation (that should be addressed in a separate PR, not here). It's not even enough to verify __init__.py at each level, you really have to go level-by-level from the top down like the real implementation does, otherwise if you are importing a.b.c and you have two different a/__init__.py on the path, you might "find" a/b/c.py under the wrong a/ directory and think everything is fine, when really the code you found is not importable at runtime.

eric-wieser · 2018-03-05T23:24:37Z

mypy/build.py

-        # many elements of lib_path don't even have a subdirectory 'foo/bar'.  Discover
-        # that only once and cache it for when we look for modules like 'foo.bar.blah'
-        # that will require the same subdirectory.
+    if (id, python_executable, lib_path) not in find_module_cache:


Could do key = (id, python_executable, tuple(lib_path)) to avoid writing this three times.

eric-wieser · 2018-03-05T23:26:58Z

mypy/build.py

-        # many elements of lib_path don't even have a subdirectory 'foo/bar'.  Discover
-        # that only once and cache it for when we look for modules like 'foo.bar.blah'
-        # that will require the same subdirectory.
+    if (id, python_executable, lib_path) not in find_module_cache:
        components = id.split('.')
        dir_chain = os.sep.join(components[:-1])  # e.g., 'foo/bar'


nit: may as well do this line within find_base_dirs, since you don't use the result anywhere else

gvanrossum · 2018-03-06T05:08:00Z

FWIW after I merged master into this, TestPEP561.test_typed_pkg started to fail, like this:

>       assert out == expected_out, err
E       AssertionError: 
E       assert "simple.py:4:...ltins.str]'\n" == "simple.py:4: ...ltins.str]'\n"
E         Skipping 37 identical leading characters in diff, use -v to show
E         - 'builtins.list[builtins.str]'
E         ?            ^^^
E         + 'builtins.tuple[builtins.str]'
E         ?           +++ ^

gvanrossum

I've not dared to review the core of the code in build.py yet, but I trust that Eric has looked at that thoroughly, so here are a few quick comments about the rest.

In the sake of progress I would really like to see a few smaller PRs that are easier to review and merge (example: splitting run() and split_lines() out of testpythoneval.py), perhaps leaving the big fireworks for last. Also please try to have less repetition in the test files.

Also -- the meaning of mypy.defaults.PYTHON3_VERSION is getting dubious. Maybe add a comment explaining what it's still used for?

gvanrossum · 2018-03-06T05:00:32Z

mypy/main.py

@@ -482,6 +528,33 @@ def add_invertible_flag(flag: str,
        print("Warning: --no-fast-parser no longer has any effect.  The fast parser "
              "is now mypy's default and only parser.")

+    try:


This is a pretty complex block of logic -- can you at least split it out into a helper function?

gvanrossum · 2018-03-06T05:18:20Z

mypy/test/testdiff.py

@@ -53,6 +53,8 @@ def build(self, source: str) -> Tuple[List[str], Optional[Dict[str, MypyFile]]]:
        options.use_builtins_fixtures = True
        options.show_traceback = True
        options.cache_dir = os.devnull
+        options.python_version = (3, 6)


That seems a bit arbitrary. Why not mypy.defaults.PYTHON3_VERSION? Or sys.version_info[:2]? (Then again I realize this is very far from your code -- the feature that this tests isn't even announced yet. :-)

Ditto for testmerge.py, and half-ditto for testsemanal.py.

gvanrossum · 2018-03-06T05:22:27Z

runtests.py

@@ -256,7 +261,7 @@ def add_stubs(driver: Driver) -> None:
                module = file_to_module(f[len(stubdir) + 1:])
                modules.add(module)

-    driver.add_mypy_modules('stubs', sorted(modules))
+    driver.add_mypy_modules('stubs', sorted(modules), extra_args=['--python-version=3.5'])


Why 3.5 here?

gvanrossum · 2018-03-06T05:24:34Z

test-data/unit/check-async-await.test

@@ -184,7 +184,7 @@ async def f() -> None:
 [typing fixtures/typing-full.pyi]

 [case testAsyncForComprehension]
-# flags: --fast-parser --python-version 3.6
+# flags: --fast-parser --python-version 3.6 --no-site-packages


It's unfortunate you had to add --no-site-packages to so many test files. Maybe it's better to set some more conservative default options for all data-driven testcases in one place?

I think I can add it somewhere in testcheck.py, I'll take a look.

gvanrossum · 2018-03-06T05:26:25Z

test-data/unit/cmdline.test

@@ -581,7 +581,7 @@ m.py:6: error: Explicit "Any" is not allowed
 m.py:9: error: Explicit "Any" is not allowed

 [case testDisallowAnyExplicitVarDeclaration]
-# cmd: mypy m.py
+# cmd: mypy --python-version=3.6 m.py


Similar for the Python version here. Maybe all these tests should be run with 3.6 unless they override it?

ethanhs · 2018-03-06T06:03:11Z

I've not dared to review the core of the code in build.py yet, but I trust that Eric has looked at that thoroughly, so here are a few quick comments about the rest.

Ok, thanks for the review!

In the sake of progress I would really like to see a few smaller PRs that are easier to review and merge (example: splitting run() and split_lines() out of testpythoneval.py), perhaps leaving the big fireworks for last. Also please try to have less repetition in the test files.

That makes a lot of sense, I will try to split things out into smaller PRs.

With "less repetition in the test files" I presume you mean that the sprinkling of flags around test cases is less than ideal? I somewhat agree, however this is a reflection of the change of mypy defaulting to sys.executable. Quite a few tests depend on syntax or libraries that may not be available in the running Python. If we pin the few tests that have minimum version dependencies, my thinking was that we would get the best coverage of testing across supported Python versions (so that tests without hard dependencies will be checked on all Python versions). Since modifying the default off of 3.6 did find some real bugs in mypy and typeshed, I think it would be valuable to have this.

Also -- the meaning of mypy.defaults.PYTHON3_VERSION is getting dubious. Maybe add a comment explaining what it's still used for?

I will add a note about this.

eric-wieser · 2018-03-06T07:06:26Z

I would really like to see a few smaller PR

Changing the default python version would be another easy one to split out, and would remove many of the tests from this diff

Split out of #4403, these are helpers, and will eventually be used by other tests.

This was discovered in #4403. I thought I'd add it while I am splitting changes out of that PR. It was originally introduced in #4526 but not added to the docs it seems. (the rest of the diff is due to my using an actual output of the help command from master)

This sets the default Python version used for type checking to `sys.version_info`. Fixes #4620. The design of this is such that we set tests to default to the running Python whenever possible, but modify tests that use new syntax and libraries to run on Python 3.5 or 3.6. Example output of failing tests on 3.4 before test changes https://gist.github.com/ethanhs/f782bec70eab0678d9e869465b40a571#file-output-log-L512. This was split out of #4403.

ethanhs · 2018-03-07T09:28:18Z

Okay, so with #4692, all but the core searching should be split out of this PR.

eric-wieser · 2018-03-07T09:47:40Z

I think I'd be inclined to leave the executable-finding in the same PR, as the feature doesn't make a huge amount of sense without PEP561

ethanhs · 2018-03-07T11:37:21Z

Closing in favor of #4693 because this has a lot of noise not needed, and I'd rather not rebase over 100 commits.

Split out of python#4403, these are helpers, and will eventually be used by other tests.

This was discovered in python#4403. I thought I'd add it while I am splitting changes out of that PR. It was originally introduced in python#4526 but not added to the docs it seems. (the rest of the diff is due to my using an actual output of the help command from master)

This sets the default Python version used for type checking to `sys.version_info`. Fixes python#4620. The design of this is such that we set tests to default to the running Python whenever possible, but modify tests that use new syntax and libraries to run on Python 3.5 or 3.6. Example output of failing tests on 3.4 before test changes https://gist.github.com/ethanhs/f782bec70eab0678d9e869465b40a571#file-output-log-L512. This was split out of python#4403.

ethanhs added 14 commits December 13, 2017 21:49

Simplify find_module cache and document PEP 561

e4b5274

More work on PEP 561 impl

6b6a97d

Scaffold testing and fix bugs.

2c936e1

Add python arg for find_module

8f64f81

Get packages from Python executables

ddc526f

Add support for non-running Python, fix bug in impl

329dc68

Merge branch 'master' of https://github.com/python/mypy into pep561-impl

eba98d4

Fix mypy self check

5576629

Fix weird subprocess bug that works in pydevd, but not otherwise

2397763

Add docs for PEP561 impl

fda2a2c

Add initial tests for PEP 561 checking

59f0c49

The tests install a typed package and verify that the modules can be found correctly. It then does the same for a stub package.

Clean up tests a bit

c976008

Get tests passing

8df8b8d

Add note about tests

4ca6939

This was referenced Dec 22, 2017

[WIP] Begin implementing PEP 561 checking #4278

Closed

How to make packages compatible with mypy #2625

Closed

JelleZijlstra reviewed Dec 23, 2017

View reviewed changes

ethanhs added 4 commits January 3, 2018 13:22

Clear site-packages from cache.

a96601c

Show sample package layout, change help text

336fb6b

Also added newlines and refactored call_python in build and get_package_dirs.

Merge branch 'master' into pep561-impl

8492d8b

Merge branch 'master' into pep561-impl

c41eef2

eric-wieser reviewed Jan 16, 2018

View reviewed changes

eric-wieser reviewed Jan 19, 2018

View reviewed changes

Fix deletion of site-packages from cache

1f005d7

eric-wieser reviewed Mar 5, 2018

View reviewed changes

Don't mutate dir cache with site packages

24b6742

ethanhs commented Mar 5, 2018

View reviewed changes

eric-wieser reviewed Mar 5, 2018

View reviewed changes

Refactor stub package searching

afb80ea

eric-wieser reviewed Mar 5, 2018

View reviewed changes

gvanrossum reviewed Mar 6, 2018

View reviewed changes

This was referenced Mar 6, 2018

Move run_command into test.helpers #4683

Merged

Add --cache-fine-grained to docs #4685

Merged

Set python_version to default to sys.version_info #4686

Merged

gvanrossum pushed a commit that referenced this pull request Mar 6, 2018

Move run_command into test.helpers (#4683)

34e63a1

Split out of #4403, these are helpers, and will eventually be used by other tests.

ethanhs mentioned this pull request Mar 7, 2018

Add --python-executable and --no-infer-executable flags #4692

Closed

ethanhs mentioned this pull request Mar 7, 2018

Implement PEP561 #4693

Merged

9 tasks

ethanhs closed this Mar 7, 2018

yedpodtrzitko pushed a commit to kiwicom/mypy that referenced this pull request Mar 15, 2018

Move run_command into test.helpers (python#4683)

eeb114d

Split out of python#4403, these are helpers, and will eventually be used by other tests.

		if path:
		if any((path.startswith(d) for d in package_dirs_cache)):



		def call_python(python: str, command: str) -> str:
		return check_output(python + ' -c ' + command).decode(sys.stdout.encoding)



		def find_module_in_base_dirs(id: str, candidate_base_dirs: Iterable[str],
		last_component: str) -> Optional[str]:

Implement PEP 561 searching #4403

Implement PEP 561 searching #4403

Conversation

ethanhs commented Dec 22, 2017 • edited Loading

ethanhs commented Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanhs Dec 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

JelleZijlstra commented Dec 23, 2017

ethanhs commented Dec 23, 2017

ethanhs commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm commented Mar 5, 2018

eric-wieser commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

eric-wieser Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

carljm Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

eric-wieser Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Mar 6, 2018

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanhs commented Mar 6, 2018

eric-wieser commented Mar 6, 2018

ethanhs commented Mar 7, 2018

eric-wieser commented Mar 7, 2018 • edited Loading

ethanhs commented Mar 7, 2018

ethanhs commented Dec 22, 2017 •

edited

Loading

ethanhs commented Dec 22, 2017 •

edited

Loading

ethanhs Dec 23, 2017 •

edited

Loading

eric-wieser Jan 16, 2018 •

edited

Loading

eric-wieser Mar 5, 2018 •

edited

Loading

eric-wieser Mar 5, 2018 •

edited

Loading

carljm Mar 5, 2018 •

edited

Loading

eric-wieser Mar 5, 2018 •

edited

Loading

eric-wieser commented Mar 7, 2018 •

edited

Loading