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

Add mypy as a flycheck checker #1354

Merged
merged 2 commits into from Jun 9, 2018

Conversation

Projects
None yet
4 participants
@Wilfred
Contributor

Wilfred commented Oct 31, 2017

This is based on https://github.com/lbolla/emacs-flycheck-mypy and
adds support for warnings too.

This was originally proposed by Lunaryorn: lbolla/emacs-flycheck-mypy#1 and the original author is happy with us doing this.

Mypy is becoming more common in the Python world, it doesn't overlap with pylint or flake8, so I think it's a great fit for flycheck.

Let me know if I've missed anything or you'd like any changes.

@cpitclaudel

cpitclaudel requested changes Oct 31, 2017 edited

Thanks; I've had this on the back burner for a while. Here's what I have in my .emacs:

(flycheck-def-option-var flycheck-python-mypy-use-python-2 nil (python-mypy)
  "Whether to pass --py2 to mypy."
  :type 'boolean
  :safe #'booleanp
  :package-version '("flycheck" . "30"))

(flycheck-def-option-var flycheck-python-mypy-silent-imports nil (python-mypy)
  "Whether to disable type-checking of imported modules."
  :type 'boolean
  :safe #'booleanp
  :package-version '("flycheck" . "30"))

(flycheck-define-checker python-mypy
  "A Python type checker using mypy.

See URL `http://www.mypy-lang.org/'."
  :command ("mypy"
            "--shadow-file" source-original source
            (option-flag "--py2" flycheck-python-mypy-use-python-2)
            (option-flag "--silent-imports" flycheck-python-mypy-silent-imports)
            "--fast-parser"
            "--check-untyped-defs"
            "--warn-redundant-casts"
            "--warn-unused-ignores"
            "--suppress-error-context"
            source-original)
  :error-patterns
  ((error line-start (file-name) ":" line ":" (optional column ":")
          " error:" (message) line-end))
  :next-checkers (python-pylint)
  :modes python-mode)

Would you like to merge this into your definition? I'd love to include mypy by default.

One thing that my definition is missing is a check that the current file exists on disk; without that check, mypy gets confused by the --shadow-file option.

flycheck.el Outdated
@@ -8788,6 +8789,26 @@ See URL `https://docs.python.org/3.4/library/py_compile.html'."
line ", " column ", " (one-or-more not-newline) line-end))
:modes python-mode)
(flycheck-def-args-var flycheck-python-mypy-args python-mypy)

This comment has been minimized.

@cpitclaudel

cpitclaudel Oct 31, 2017

Member

We usually avoid generic -args variables, in favor of specific variables for each relevant flag; I think it's be better to do this here too.

This comment has been minimized.

@Wilfred

Wilfred Oct 31, 2017

Contributor

Which do you think are the most relevant?

$ mypy --help
usage: mypy [-h] [-v] [-V] [--python-version x.y] [--platform PLATFORM] [-2]
            [--ignore-missing-imports]
            [--follow-imports {normal,silent,skip,error}]
            [--disallow-any {unimported, expr, unannotated, decorated, explicit, generics}]
            [--disallow-untyped-calls] [--disallow-untyped-defs]
            [--disallow-incomplete-defs] [--check-untyped-defs]
            [--disallow-subclassing-any] [--warn-incomplete-stub]
            [--disallow-untyped-decorators] [--warn-redundant-casts]
            [--no-warn-no-return] [--warn-return-any] [--warn-unused-ignores]
            [--warn-unused-configs] [--show-error-context]
            [--no-implicit-optional] [-i] [--quick-and-dirty]
            [--cache-dir DIR] [--skip-version-check] [--strict-optional]
            [--strict-optional-whitelist [GLOB [GLOB ...]]]
            [--junit-xml JUNIT_XML] [--pdb] [--show-traceback] [--stats]
            [--inferstats] [--custom-typing MODULE]
            [--custom-typeshed-dir DIR] [--scripts-are-modules]
            [--config-file CONFIG_FILE] [--show-column-numbers]
            [--find-occurrences CLASS.MEMBER] [--strict]
            [--shadow-file SOURCE_FILE SHADOW_FILE] [--any-exprs-report DIR]
            [--cobertura-xml-report DIR] [--html-report DIR]
            [--linecount-report DIR] [--linecoverage-report DIR]
            [--memory-xml-report DIR] [--old-html-report DIR]
            [--txt-report DIR] [--xml-report DIR] [--xslt-html-report DIR]
            [--xslt-txt-report DIR] [-m MODULE] [-c PROGRAM_TEXT] [-p PACKAGE]
            [files [files ...]]

I like to run with --py2 --strict --disallow-any=generics for new scripts, or --py2 --check-untyped-defs when I'm gradually porting a codebase (to check calls to annotated functions even when the caller isn't yet annotated).

--strict includes quite a few arguments -- do we offer the individual arguments too?

This comment has been minimized.

@cpitclaudel

cpitclaudel Nov 5, 2017

Member

My usual approach is to add whatever the original writer of the checker finds useful, and accept pull requests for more as needed :)

flycheck.el Outdated
Customize `flycheck-python-mypy-args` to add specific args to default
executable.
E.g. when processing Python 2 files, add \"--py2\".

This comment has been minimized.

@cpitclaudel

cpitclaudel Oct 31, 2017

Member

For example, this could controled by a flycheck-mypy-python-version variable.

flycheck.el Outdated
:command ("mypy"
(eval flycheck-python-mypy-args)
source-original)

This comment has been minimized.

@cpitclaudel

cpitclaudel Oct 31, 2017

Member

mypy supports stdin, so we should use that.

This comment has been minimized.

@Wilfred

Wilfred Oct 31, 2017

Contributor

How do you feel about using source-inplace here instead? That way mypy can follow imports too.

This comment has been minimized.

@cpitclaudel

cpitclaudel Nov 5, 2017

Member

mypy can follow imports if you give it --shadow-file, and this way we don't need to pollute the current folder with flycheck_-prefixed files

@Wilfred

This comment has been minimized.

Contributor

Wilfred commented Oct 31, 2017

@cpitclaudel thanks for sharing your version!

I tend to use mypy with flake8 afterwards, so what should I put as :next-checkers? I see you've used python-pylint, but that doesn't seem to have :next-checkers configured.

@dakra

This comment has been minimized.

dakra commented Nov 5, 2017

I use #1080 since 10 month now. Like in the comment there, I had to rename some arguments that got changed in newer mypy versions but otherwise works like a charm.

So I'm +1 for merging this here or #1080 or any mypy checker really ;)

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Nov 5, 2017

I tend to use mypy with flake8 afterwards, so what should I put as :next-checkers? I see you've used python-pylint, but that doesn't seem to have :next-checkers configured.

It's an annoying issue, indeed, which is what stalled my previous PR for a mypy checker.
I'm OK with leaving flake8 in there.

@dakra

This comment has been minimized.

dakra commented Jan 29, 2018

@Wilfred bump
I would like a mypy checker :)

@Wilfred Wilfred force-pushed the Wilfred:add_mypy branch 2 times, most recently from 17f3e05 to 97f0c3b Jan 30, 2018

@Wilfred

This comment has been minimized.

Contributor

Wilfred commented Jan 30, 2018

@cpitclaudel could you review this again? I think this is in much better shape now.

We now have just one configurable option, the path to a mypy.ini configuration file. This allows you to configure mypy in any way, which I find really convenient. If you have different mypy.ini files you can use .dir-locals.el.

We're also now using --shadow-file with source so we're not cluttering the current directory.

This checker works pretty well for buffers that are backed by files, but I don't know how to handle other buffers. I want to do something like this:

(if source-original
    ;; If this file is backed by a buffer, tell mypy where the
    ;; original file was but shadow it with a temporary file of the
    ;; current buffer contents.
    (concat "--shadow-file" source-original source source-original)
  ;; Just run mypy on a temporary file containing the buffer contents.
  source)

but I'm not sure how to express this with eval or the :command DSL.

@Wilfred Wilfred force-pushed the Wilfred:add_mypy branch from 97f0c3b to de9c2b6 Jan 30, 2018

@Wilfred

This comment has been minimized.

Contributor

Wilfred commented Jan 30, 2018

Added docs and changed the checker to use (config-file ...) which I hadn't spotted before.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jan 30, 2018

This checker works pretty well for buffers that are backed by files, but I don't know how to handle other buffers.

If --shadow-file is indeed checking the temporary created by source, then it should work seemlessly for buffers not backed by files. Is this not the case?

@Wilfred

This comment has been minimized.

Contributor

Wilfred commented Jan 30, 2018

@fmdkdd --shadow-file allows you to say "use this temporary file as if it was this other file on disk". There is no 'other file on disk' for buffers that aren't backed by files.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jan 30, 2018

There is no 'other file on disk' for buffers that aren't backed by files.

Indeed, but why does mypy care about the actual file, since we already gave it the content it should check? My question is, if we pass foo.py as a phony file path, with a real temporary file given to --shadow-file, does mypy complain?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Mar 16, 2018

@fmdkdd I think it's a mypy bug:

$ cd /tmp/
$ mkdir python
$ cd python/
$ echo '"""test"""' > a.py
$ mypy --shadow-file b.py a.py a.py
mypy: can't read file 'a.py': No such file or directory
@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Mar 16, 2018

I opened python/mypy#4746

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Mar 16, 2018

Great! Keep us posted.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented May 24, 2018

Edit: acknowledged as an issue, but I don't have time to fix it myself, so let's just add a check to ensure that the buffer is saved.

We should also pass a flag to prevent all these .mypy cache directories from being created.

Then I'd be in favor of merging. I think it shouldn't be enabled for all files by default, though, since it's pretty slow. Or maybe it should just run last?

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented May 25, 2018

Edit: acknowledged as an issue, but I don't have time to fix it myself, so let's just add a check to ensure that the buffer is saved.

Agreed. We should add a comment stating that checking on saved buffers is a workaround for the mypy bug, and add a link to it.

We should also pass a flag to prevent all these .mypy cache directories from being created.
Then I'd be in favor of merging. I think it shouldn't be enabled for all files by default, though, since it's pretty slow. Or maybe it should just run last?

Aren't these two related? If you leave the cache, then potentially it can be reused for later calls?

In any case, if we put python-mypy after python-pycompile, then we force users to opt-in by selecting mypy in their own config (since pycompile should always be enabled if python is available). I'd rather have that than a checker disabled by default.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented May 25, 2018

Aren't these two related? If you leave the cache, then potentially it can be reused for later calls?

Yeah, but with the checker on by default that means that you get .mypy-cache folders all over the file system. We could add an option.

In any case, if we put python-mypy after python-pycompile, then we force users to opt-in by selecting mypy in their own config (since pycompile should always be enabled if python is available). I'd rather have that than a checker disabled by default.

Sounds good

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented May 25, 2018

We could add an option.

Sure.

@Wilfred Are you still free to work on this PR? The following changes need to be made:

  1. Ensure that the buffer is saved in :predicate to workaround python/mypy#4746 (and add a comment to that effect with a link to the bug)
  2. Move python-mypy in flycheck-checkers after python-pycompile
  3. Add a user option to disable creating .mypy-cache folders

@Wilfred Wilfred force-pushed the Wilfred:add_mypy branch from de9c2b6 to d517ae8 May 29, 2018

@Wilfred

This comment has been minimized.

Contributor

Wilfred commented May 29, 2018

@fmdkdd thanks, I had some time today and your explicit steps were very helpful :)

I've made the changes you requested, and rebased on latest master.

@fmdkdd

LGTM, with a minor change needed.

flycheck.el Outdated
.mypy_cache files."
:safe #'booleanp
:type 'boolean
:package-version '(flycheck . "0.16"))

This comment has been minimized.

@fmdkdd

fmdkdd May 30, 2018

Member

The version number should be 32, not 0.16.

This comment has been minimized.

@Wilfred

Wilfred May 30, 2018

Contributor

Fixed.

@cpitclaudel

Thanks, this is looking good. A few documentation comments from me.

.. defcustom:: flycheck-python-mypy-disable-incremental
Switches off incremental type checking. This is slower, but
prevents mypy from writing ``.mypy_cache`` files.

This comment has been minimized.

@cpitclaudel

cpitclaudel May 30, 2018

Member

I'd suggest to say creating .mypy_cache folder instead

This comment has been minimized.

@cpitclaudel

cpitclaudel May 30, 2018

Member

I also wonder if the setting should be called enable, and be off by default. These folders are really annoying. Opinions?

This comment has been minimized.

@cpitclaudel

cpitclaudel May 30, 2018

Member

Or maybe there's a way to make mypy put all these folders in one place on the file system, and we should let users customize that?

This comment has been minimized.

@Wilfred

Wilfred May 30, 2018

Contributor

I've updated the doc as you suggested.

I did think about having an enable flag, but this is consistent with the mypy behaviour, which enables caching by default. Personally I don't mind the .mypy_cache directory being created, and I find mypy rather slow otherwise.

This comment has been minimized.

@cpitclaudel

cpitclaudel May 30, 2018

Member

OK, I'm sold :)

@Wilfred Wilfred force-pushed the Wilfred:add_mypy branch from cdf2cf8 to 52f680c May 30, 2018

@fmdkdd

fmdkdd approved these changes May 31, 2018

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented May 31, 2018

Thanks! One last thing: please add a line to CHANGES.rst under New syntax checkers.

It would also be nice to add a test for the checker in flycheck-test.el.

@Wilfred Wilfred force-pushed the Wilfred:add_mypy branch from 52f680c to aba15a6 Jun 1, 2018

@Wilfred

This comment has been minimized.

Contributor

Wilfred commented Jun 1, 2018

I've updated CHANGES.rst.

I'm not sure how to add a test. Looking at e.g. (flycheck-ert-def-checker-test python-flake8 python nil in flycheck-test.el then it looks like the test depends on flake8 being installed.

I can see main/requirements.txt is installing flake8, but that doesn't look related. If I look at another test like (flycheck-ert-def-checker-test puppet-parser puppet parser-error-puppet-4 in flycheck-test.el then it seems to be shelling out to puppet, but I can't see puppet being installed either.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jun 4, 2018

@Wilfred right, so make integ can be run locally and it runs on our CI. For both, the command will run only the tests of checkers for which there is an executable present. The tests do not install the executable.

So, if you have mypy installed, you can add a test, and it should run locally with make integ. Then, it will be skipped on CI because there mypy is not installed. For that, we need to add mypy to the docker image we use for running the tests, over at flycheck/docker-tools.

I've just added mypy there flycheck/docker-tools@f32347c, so that should be good.

For writing the test, you probably want to set the flycheck-disabled-checkers variable (as in the python-plint tests), so that Flycheck picks python-mypy for the buffer being tested.

Add mypy as a flycheck checker
Rather than allowing the user to specify each argument, allow the user
to set a path to a mypy configuration file.

Output parsing is based on
https://github.com/lbolla/emacs-flycheck-mypy and adds support for
warnings too.

@Wilfred Wilfred force-pushed the Wilfred:add_mypy branch from aba15a6 to d169cab Jun 5, 2018

@Wilfred

This comment has been minimized.

Contributor

Wilfred commented Jun 5, 2018

@fmdkdd that's really helpful, thanks. I've had a crack at writing a test and I think it's correct now.

I also noticed an issue with --disable-incremental not actually doing what I expected, so I've added a new configuration variable to disable writing .mypy_cache properly.

Finally, I've corrected the docs regarding the minimum mypy version required.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jun 5, 2018

@Wilfred Thanks! That's real nice. Looks like your test is passing on the CI, but now some checkers are freaking out because of we are using Python3 instead of Python2.

I'll look into that and restart the build if necessary.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jun 5, 2018

Ok so the python-pylint failures are due to pylint not being found when using python3. Looks like a consequence of #1113. In the Docker image:

$ pylint

invokes pylint correctly, but what we actually run by default is:

$ python -c 'import sys,runpy;sys.path.pop(0); runpy.run_module("pylint")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib/python2.7/runpy.py", line 182, in run_module
    mod_name, loader, code, fname = _get_module_details(mod_name)
  File "/usr/lib/python2.7/runpy.py", line 107, in _get_module_details
    raise error(format(e))
ImportError: No module named pylint

Changing python to python3 in the checker fixes it, but we probably don't want to do that.

Now, my understanding is that as it stands, python checkers need to be configured in order to work properly. This was not case before #1113 if installing tools with pip.

Of course, one solution for the CI is to use python2 for all the Python tools, and use python3 for mypy. But using python3 for all seemed simpler.

@cpitclaudel What is your take on this?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Jun 5, 2018

@fmdkdd it should be enough to let-bind flycheck-python-pylint-executable to python3 around the test — does that work?

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jun 5, 2018

it should be enough to let-bind flycheck-python-pylint-executable to python3 around the test — does that work?

Yes it works. And so does let-binding it to pylint more directly. But what I was worried about was that users who had a similar setup would have to jump through the same hoops. I thought the point of #1113 was to make the switch between pylint for python2 and pylint for python3 easier, and now I'm unclear as to how it eases that.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Jun 5, 2018

But what I was worried about was that users who had a similar setup would have to jump through the same hoops. I thought the point of #1113 was to make the switch between pylint for python2 and pylint for python3 easier, and now I'm unclear as to how it eases that.

Correct (more precisely, the point of #1113 was to make it easy to use the pylint matching your version of python. If you're using python3, you set the both python-shell-executable and flycheck-python-pylint-executable to python3; for python 2, you set both of them to python2.

(Did I get the question right?)

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jun 6, 2018

If you're using python3, you set the both python-shell-executable and flycheck-python-pylint-executable to python3; for python 2, you set both of them to python2.

Oh okay, I had a bad case of not RTFM. This is documented here. Thank you for your patience ;)

@Wilfred

This comment has been minimized.

Contributor

Wilfred commented Jun 8, 2018

Is there anything else you need from me? :)

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jun 8, 2018

@Wilfred I don't think so. I just need to fix the python integration tests on CI and we can merge.

On a related note, has anyone taken a look at https://github.com/facebook/pyre-check/, how it compares to mypy, and whether we could integrate it with Flycheck?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Jun 8, 2018

On a related note, has anyone taken a look at https://github.com/facebook/pyre-check/, how it compares to mypy, and whether we could integrate it with Flycheck?

Looks good! We could have a simple version in Flycheck that runs it from scratch every time, and a separate package that uses the server version of it.

Fix integration tests for python3
In the all-tools Docker image we use on CI, we install all Python tools
with python3.  We thus need to change the default executable in order to
the tools to be found by python.

@fmdkdd fmdkdd force-pushed the Wilfred:add_mypy branch from 7a6537b to 42bfdf8 Jun 9, 2018

@fmdkdd fmdkdd merged commit 6e46bce into flycheck:master Jun 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment