option to merge stubs into source trees

[gaborbernat]

Resolves #5028.

Need to clean up, both logic mostly there (tested with https://github.com/gaborbernat/tox/tree/type-hint-stubs/tox).

[JelleZijlstra]

I left a couple of stylistic comments.

[gvanrossum]

I think this would need a flag to enable it; I expect that not everybody will want this functionality.

This PR probably ought to be merged quickly to avoid running into merge conflicts; but it requires very thorough review (probably by Jukka or Ivan).

[gaborbernat]

@gvanrossum fair enough, I'll hide it behind a flag.

[JukkaL]

I can review this later this week. Agreed that we want to merge this quickly -- even if this is not 100% finished, if it's behind a flag and doesn't risk breaking other functionality we can hopefully merge it, and it can be iterated on afterwards.

[gaborbernat]

Running works fine. Debugging though generates this extra attached output which breaks the tests. Probably should reformulate to debugging instead running.

…

On Thu, Jun 7, 2018, 19:47 Ethan Smith ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mypy/test/testcmdline.py <#5139 (comment)>: > @@ -58,6 +58,14 @@ def test_python_cmdline(testcase: DataDrivenTestCase) -> None: outb = process.stdout.read() # Split output into lines. out = [s.rstrip('\n\r') for s in str(outb, 'utf8').splitlines()] + + is_running_in_py_charm = "PYCHARM_HOSTED" in os.environ How are you running things from PyCharm? I've been able to run the test suite for months without issue. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAqIPkEtNylTSnLOQsaib9nJuPltkn7qks5t6XUmgaJpZM4UYCqM> .

[JukkaL]

I did a quick overview review pass. The general approach looks reasonable. Thanks for working on this -- this has been requested several times.

I suspect that there are various remaining edge cases, some of which may be a bit tricky (conditional definitions, for example). This just means that this feature needs some real-world use and iteration before it will be generally useful, most of which can happen in separate PRs.

Here's my suggestion for the next steps:

1. Get tests to pass and do some minimal clean-up such as separating unrelated changes from this PR.
2. A core team member (probably me) will try this out and do a more detailed review. The review can be pretty lenient since this is a big feature and we can't expect this to be finished in a single PR.
3. Merge this PR. At this point the feature may be undocumented/hidden, depending on how mature we feel this is.
4. Create follow-up issues for additional work. [Are you willing to continue working on this after a basic implementation gets merged, by the way? If yes, it's easier to justify merging an incomplete feature.]
5. After some iteration make this public as an experimental feature and invite other mypy users to try this out and send feedback.

Finally, the PR doesn't quite follow the style used elsewhere in the codebase. Are you okay with changing the style? At least some of this can happen after this has been merged.

[gaborbernat]

Hello @JukkaL,

1. Many of the changes unrelated to this pr I've done to help me ease the development/setup. Once we want to merge it I can easily remove it. It already sits on its own individual comment exactly for this reason. So sure👌
2. If core members can think of edge cases not covered now I'm open to handle/add them even part of this pr.
3. I believe hidden behind a feature flag we can make it experimental, meaning we documentat it but don't mark it mature yet.
4. I'm committed to finishing and maintaining this. I want to use this feature extensively in my projects, so might as well own it throughout.
5. Sounds good.

As far as style goes whatever you guys want. For all new files I've used the excellent fully automated black formatter with isort import ordering mostly because I prefer spending my mental power on the implementation difficulties rather than how to indent/space my code. That being said I'm not married or attached to any style on my side. Ideally we would have this change automated but if not possible I can alter manually here and there.

[gvanrossum]

A suggestion: if changes unrelated to this topic would benefit others developing mypy, why not submit then as separate PRs? Once accepted you can merge and they won't be part of this PR yet. (Of course these need to be things that don't get in the way of using mypy or other development workflows -- checking for environment variables might be useful here.)

[gaborbernat]

@gvanrossum that would be great and I plan to do that. I don't completely understand what's the current development workflow used by core devs, so my changes don't get in way. Those can be probably discussed inside that pr though.

[gvanrossum]

Mostly we don't use PyCharm much. :-)

[gaborbernat]

#5189 now contains the changes unrelated to this PR.

[gaborbernat]

So this is now in a review ready state I would say (all tests now pass). Before v0.1 I think I just need to add proper cache handling, some documentation on it. And we're good to go I would say. However, the underlying merge logic is now the proposed one so ready for review.

[JukkaL]

I can't get this to work on the command line. I created a stub file t.pyi and the corresponding source file t.py, and another module t2 that imports t. All of the files are in the same directory. No matter what I do, only either the stub or the source file of t seems to be processed. For example, I added a syntax error to t.py and type checked t2 and t and there was no error:

$ mypy --merge-stub-into-src t2.py
<no syntax error>
$ mypy --merge-stub-into-src -m t
<no syntax error>

Similarly, if I add a syntax error to t.pyi and type check t.py no merging is happening:

$ mypy --merge-stub-into-src t.py
<no syntax error>

[gaborbernat]

@JukkaL it does not work like that; only support now specifying the directory. The source finder logic does the collection of files, and then matching stub files with source files; and that's only used if you do mypy --merge-stub-into-src ..

That being said I believe, the way could work:

mypy --merge-stub-into-src t2.py # only check the py file
mypy --merge-stub-into-src t2.py t2.pyi # be able to merge - this is not supported yet, but should be
mypy --merge-stub-into-src -m t # this should also pick up both file

[JukkaL]

Thanks for the hint, now I got this to merge some files.

That being said I believe, the way could work:

These all look reasonable. In addition, if I don't include a module explicitly on the command line and the module is found by following imports, we should perform a merge (this corresponds to the mypy --merge-stub-into-src t2.py case above).

I've found a couple of additional issues. First, if I define function f in the .py file but the .pyi file is empty, I can't import f from the module. I'd expect the function to be available (with a dynamically typed signature). I guess it might be reasonable to generate an error about a missing definition in the stub, but this might be impractical, since stubs generally don't include functions not included in the public interface of a module.

Second, the type of an imported name from a merged module is wrong, and it can cause crashes. Here is an example:

# p/t.py
def f(x): pass

# p/t.pyi
def f(x: int) -> None: ...

# p/__init__.py

# t2.py
from p.t import f
reveal_type(f)

If I type check the above example I get a crash:

$ mypy --show-traceback --merge-stub-into-src t2.py p
t2.py:2: error: Revealed type is 'def (x: int?) -> None?'
t2.py: error: INTERNAL ERROR -- please report a bug at https://github.com/python/mypy/issues version: 0.620+dev-2d887dd02ce234e579a2087fb4b84a45ef43c0f2
Traceback (most recent call last):
  File "/Users/jukka/venv/mypy/bin/mypy", line 11, in <module>
    load_entry_point('mypy==0.620+dev.2d887dd02ce234e579a2087fb4b84a45ef43c0f2', 'console_scripts', 'mypy')()
  File "/Users/jukka/src/mypy/mypy/__main__.py", line 7, in console_entry
    main(None)
  File "/Users/jukka/src/mypy/mypy/main.py", line 91, in main
    res = type_check_only(sources, bin_dir, options, flush_errors, fscache)  # noqa
  File "/Users/jukka/src/mypy/mypy/main.py", line 148, in type_check_only
    fscache=fscache)
  File "/Users/jukka/src/mypy/mypy/build.py", line 188, in build
    flush_errors, fscache)
  File "/Users/jukka/src/mypy/mypy/build.py", line 293, in _build
    graph = dispatch(sources, manager)
  File "/Users/jukka/src/mypy/mypy/build.py", line 2468, in dispatch
    process_graph(graph, manager)
  File "/Users/jukka/src/mypy/mypy/build.py", line 2767, in process_graph
    process_stale_scc(graph, scc, manager)
  File "/Users/jukka/src/mypy/mypy/build.py", line 2896, in process_stale_scc
    graph[id].finish_passes()
  File "/Users/jukka/src/mypy/mypy/build.py", line 2116, in finish_passes
    manager.report_file(self.tree, self.type_map(), self.options)
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/contextlib.py", line 99, in __exit__
    self.gen.throw(type, value, traceback)
  File "/Users/jukka/src/mypy/mypy/build.py", line 1867, in wrap_context
    yield
  File "/Users/jukka/src/mypy/mypy/build.py", line 2111, in finish_passes
    self._patch_indirect_dependencies(self.type_checker().module_refs, self.type_map())
  File "/Users/jukka/src/mypy/mypy/build.py", line 2121, in _patch_indirect_dependencies
    types = set(type_map.values())
  File "/Users/jukka/src/mypy/mypy/types.py", line 906, in __hash__
    return hash((self.ret_type, self.is_type_obj(),
  File "/Users/jukka/src/mypy/mypy/types.py", line 781, in is_type_obj
    return self.fallback.type.is_metaclass()
AttributeError: 'NoneType' object has no attribute 'type'
t2.py: : note: use --pdb to drop into pdb

The likely reason for the crash is on the first line of output (def (x: int?) -> None?) -- the question marks mean that the type of f is not semantically analyzed, which should not happen. Likely the merged type is not updated correctly during merge, and we are leaking the pre-merge type of f in p/t.py during import.

[gaborbernat]

Will try to tackle the raised issues.

[ilevkivskyi]

x = y = 1

Btw, what is the status of this PR? We will have a release next week, is this ready for a review?

[gaborbernat]

I don't think I'll have enough time to get it into a proper state, as I'm caught up with some tox maintenance. Will probably pick it up next week again, so let's aim for the next release cycle. Would not like to rush things and mess something somewhere up.

[gvanrossum]

This seems stalled...?

[gaborbernat]

Actually I'm just planning to pick it up again. @JukkaL change requests are non trivial so this got put back on my schedule a bit.

[gvanrossum]

OK, both Jukka and I are on vacation, so be patient...

(I tried to send this before but somehow the mail bounced.)

[gaborbernat]

No worries I still need to work a bit on this before review/merge time. By the way how do we handle the lack of Type on Python 3.5 and less. Needed to type hint the code of this PR.

[emmatyping]

@gaborbernat you can do:

MYPY=False
if MYPY:
    from typing import Type

And then string escape Type when you use it.

[gaborbernat]

@JukkaL picking this up again, so what blockers we need to get this merged? 👍

[JukkaL]

Based on a quick review pass, these may be sufficient to get this merged:

• Fix remaining failing tests.
• Make style consistent with project style in previously existing files at least, and preferably everywhere (in particular, multi-line function signatures).
• Add at least one command line test that does an end-to-end source/stub merge (in pythoneval.test, probably).
• Get module discovery reviewed in detail, since it has changes to existing functionality. @msullivan may have the most context on this.
• Maybe refactor the new pytest-style tests to use unittest-style test classes, for consistency.
• One of the core team members should do some manual testing and a final round of review. Fix any big enough issues found.

It would also be good to have some commitment for additional work, since this is a pretty big feature. As discussed above, there will likely be various features and edge cases that would still need more work after this PR.

[JukkaL]

We don't use this style for multi-line signatures. The original signature conforms to our style.

[JukkaL]

Another signature with inconsistent style.

[JukkaL]

Can you put these tests inside a test class derived from unittest.TestCase, for consistency?

[gaborbernat]

yes, will do 👍

[JukkaL]

Sorry, I've been busy with other things to finish the review. If you are still interested in finishing this up, I can dedicate some time to reviewing this in May. (Or maybe during PyCon sprints, in case you are coming there.)

[gaborbernat]

@JukkaL yeah I still plan to finish it, will try to make it in some merge-able state by the sprint is my current plan (and yes I'll be there 😄 - I actually have a talk accepted about mypy 🤔 )