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

mypy incremental issues with `--warn-unused-ignores` #2960

Closed
timabbott opened this Issue Mar 4, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@timabbott

timabbott commented Mar 4, 2017

I just had the following sequence happen, which suggests to me that the mypy cache is broken with this option:

(zulip-py3-venv) tabbott@zaset:~/zulip$ ./tools/run-mypy 
(zulip-py3-venv) tabbott@zaset:~/zulip$ ./tools/run-mypy --py3
zerver/lib/cache.py:287: note: unused 'type: ignore' comment
zproject/jinja2/__init__.py:32: note: unused 'type: ignore' comment
zerver/tornado/event_queue.py:474: note: unused 'type: ignore' comment
zerver/lib/actions.py:1199: note: unused 'type: ignore' comment
(zulip-py3-venv) tabbott@zaset:~/zulip$ ./tools/run-mypy --py3^C
(zulip-py3-venv) tabbott@zaset:~/zulip$ rm -rf var/mypy-cache/
(zulip-py3-venv) tabbott@zaset:~/zulip$ ./tools/run-mypy 
api/integrations/asana/zulip_asana_mirror:81: note: unused 'type: ignore' comment
api/integrations/asana/zulip_asana_mirror:82: note: unused 'type: ignore' comment
api/integrations/google/google-calendar:133: note: unused 'type: ignore' comment
api/integrations/google/google-calendar:143: note: unused 'type: ignore' comment
scripts/setup/generate_secrets.py:54: note: unused 'type: ignore' comment
tools/replace-tarball-shebang:22: note: unused 'type: ignore' comment
tools/show-profile-results.py:22: note: unused 'type: ignore' comment
tools/tests/test_template_parser.py:31: note: unused 'type: ignore' comment
zerver/lib/actions.py:1199: note: unused 'type: ignore' comment
zerver/lib/camo.py:18: note: unused 'type: ignore' comment
zerver/lib/parallel.py:32: note: unused 'type: ignore' comment
zerver/lib/test_helpers.py:175: note: unused 'type: ignore' comment
zerver/management/commands/email_mirror.py:112: note: unused 'type: ignore' comment
zerver/management/commands/email_mirror.py:119: note: unused 'type: ignore' comment
zerver/management/commands/email_mirror.py:128: note: unused 'type: ignore' comment
zerver/storage.py:45: note: unused 'type: ignore' comment
zerver/storage.py:75: note: unused 'type: ignore' comment
zerver/tests/test_upload.py:746: note: unused 'type: ignore' comment
zerver/tornado/ioloop_logging.py:24: note: unused 'type: ignore' comment
zproject/jinja2/__init__.py:32: note: unused 'type: ignore' comment
zproject/local_settings.py:14: note: unused 'type: ignore' comment
(zulip-py3-venv) tabbott@zaset:~/zulip$ ./tools/run-mypy 
(zulip-py3-venv) tabbott@zaset:~/zulip$
@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Mar 5, 2017

I think what happens here is that the data for --warn-unused-ignores is only collected when it parses a file, not when it reads the data for a file from the cache. So I guess in your first run some files were already in the cache.

I don't believe there's a bug to be fixed here (or at least I would prefer not to have to serialize the # type: ignore map into the cache). Maybe we can add a note to the docs.

@timabbott

This comment has been minimized.

timabbott commented Mar 5, 2017

Hmm. Our model for how we use mypy is that we want to run it as part of our linter suite, along with pyflakes and friends (with the caching, it's basically fast enough). And the easy thing to do there is to just pass the -i option to mypy unconditionally (we do that today).

But if we do that, then suddenly this flag doesn't work at all, unless you know to delete the mypy cache each time you want to run it.

So I guess my vote is for updating the cache to include that map.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Mar 5, 2017

I hear you -- but supporting this use case is a low priority for us.

@Daenyth

This comment has been minimized.

Daenyth commented Jun 6, 2017

I've been able to work around this for CI by making sure mypy_cache is deleted between runs.

@msullivan msullivan changed the title from mypy caching issues with `--warn-unused-ignores` to mypy incremental issues with `--warn-unused-ignores` Feb 27, 2018

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Feb 27, 2018

I am deep in a rabbit hole of trying to understand if there are going to be any unexpected ramifications to a change to the handling of suppressed that I want to make, and in the process I produced these two test cases that I believe are for this bug:

[case testIgnoreWarning1]
# cmd: mypy -m a b
# cmd2: mypy -m a b
# cmd3: mypy -m a b c
# flags: --follow-imports=error --warn-unused-ignores --verbose
[file a.py]
import b
import c  # type: ignore
[file b.py]
x = 1
[file b.py.2]
x = 'hi'
[file c.py]
pass
[out]
[out2]
[out3]
tmp/a.py:1: note: unused 'type: ignore' comment

[case testIgnoreWarning2]
# flags: --warn-unused-ignores --verbose
import a
[file a.py]
import b
import c  # type: ignore
[file b.py]
x = 1
[file b.py.2]
x = 'hi'
[file c.py.3]
pass
[out]
[out2]
[out3]
tmp/a.py:1: note: unused 'type: ignore' comment

They both fail in stage 2 with spurious unused type:ignore warnings.

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Mar 27, 2018

We may want to address this somehow (perhaps just by disallowing the combination for now) before making incremental the default mode per #4800.

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Mar 27, 2018

The snag here also does not actually seem to be that we don't write out the ignore errors map (which I think is fine), but that we don't generate error messages for failed imports from modules that are loaded from the cache initially but then later parsed due to changes in their dependencies.

@JukkaL

This comment has been minimized.

Collaborator

JukkaL commented Mar 27, 2018

Good point, I had forgotten about this. I've encountered myself. This may affect a non-trivial number of users.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Mar 27, 2018

@JukkaL

This comment has been minimized.

Collaborator

JukkaL commented Mar 27, 2018

We have to figure out what's the best way to get rid of these bogus messages -- either fix the underlying issue or disable the flag in incremental mode. I haven't spent enough time thinking about this to say how hard this would be to fix. If somebody has spare cycles to look at this, great. Otherwise I can probably look at this next week or so.

@msullivan msullivan self-assigned this Mar 27, 2018

@msullivan

This comment has been minimized.

Collaborator

msullivan commented Mar 27, 2018

I can take this. I have a half-finished refactor of the production of import diagnostics that should make both this and #4798 easy to fix

@Daenyth

This comment has been minimized.

Daenyth commented Mar 28, 2018

msullivan added a commit that referenced this issue Apr 2, 2018

Don't produce spurious unused type ignore messages in incremental mode
This is accomplished by generating diagnostics for suppressed
dependencies before generating unused ignore notes.  This requires
that we store line information for suppressed dependencies in our
cache files.

Fixes #2960.

msullivan added a commit that referenced this issue Apr 3, 2018

Don't produce spurious unused type ignore messages in incremental mode
This is accomplished by generating diagnostics for suppressed
dependencies before generating unused ignore notes.  This requires
that we store line information for suppressed dependencies in our
cache files.

Fixes #2960.

msullivan added a commit that referenced this issue Apr 3, 2018

Don't produce spurious unused type ignore messages in incremental mode
This is accomplished by generating diagnostics for suppressed
dependencies before generating unused ignore notes.  This requires
that we store line information for suppressed dependencies in our
cache files.

Fixes #2960

msullivan added a commit that referenced this issue Apr 4, 2018

Don't produce spurious unused type ignore messages in incremental mode (
#4841)

This is accomplished by generating diagnostics for suppressed
dependencies before generating unused ignore notes.  This requires
that we store line information for suppressed dependencies in our
cache files.

Fixes #2960
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment