Errors not always reported for a file in incremental mode

[JukkaL]

Sometimes mypy doesn't generate an error for a module in incremental mode, even if a previous run generated errors and nothing has changed.

To reproduce, first create these two files:

# a.py
import b
b.f()

# b.py
def f() -> None: pass

Then run mypy, and no errors are reported (as expected):

$ mypy -i a.py

Now edit b.py like this:

# b.py (2)
def f(x) -> None: pass

Run mypy again, and it correctly reports an error:

$ mypy -i a.py
a.py:3: error: Too few arguments for "f"

Finally, run mypy again without changing anything, and it incorrectly gives clean output:

$ mypy -i a.py
$    <--- no error reported

The expected output is the same error as in the previous run, since nothing has changed.

[gvanrossum]

Ouch, confirmed. Let me investigate.

[ilevkivskyi]

FWIW, I think this is closely related to #3052. It seems like module dependency management is somehow broken.

[gvanrossum]

The root cause in Jukka's example seems to be that on the second run, it first re-parses b.py (since it changed), finds no fault with it and writes its cache, then re-parses a.py (since its dependency, b, changed), finds an error, and skips writing its cache file.

But then on the third run, it finds the old cache file for a.py, and decides that it is valid (since its has is still valid). However it should have been considered invalid since the cache for b.py was updated, and b.py is a dependency for a.py.

I'll dive deeper.

[gvanrossum]

I bisected this, and the root cause is over a year old: #2014. @Michael0x2a Do you remember anything about that PR?

[JukkaL]

Some random ideas (not sure if these are any good though): What about just deleting the cache file for a.py if it has errors? Or storing the generated errors in the cache file, and re-generating the errors in case we use the cache file?

[gvanrossum]

Deleting the cache file would be pretty easy.

[ilevkivskyi]

@gvanrossum

Deleting the cache file would be pretty easy.

From the two repros I have found for existing --incremental crashes, it looks like deleting the cache on error will fix them as well.

[gvanrossum]

Do you have time to make a PR? This looks like it could be a high-value fix to cherry-pick into the 0.530 release.

[ilevkivskyi]

Do you have time to make a PR? This looks like it could be a high-value fix to cherry-pick into the 0.530 release.

Yes, I will make a PR.

[Michael0x2a]

@gvanrossum: Yeah, I think making sure we delete the cache file for each file that had an error should work -- iirc, a missing cache file is basically treated the same as if it were just an outdated one, which we know works.

FWIW, I think the reason I didn't catch this when I was implementing #2014 is because at the time, the tests for incremental mode didn't support re-running mypy more then twice (and these errors emerge only if you run mypy three or more times).

I can work on fixing the existing incremental tests so they do check what happens if you run mypy 3+ times and submit a pull request sometime later today or tomorrow (assuming you agree this is a good idea).

[ilevkivskyi]

@Michael0x2a

I can work on fixing the existing incremental tests so they do check what happens if you run mypy 3+ times and submit a pull request sometime later today or tomorrow (assuming you agree this is a good idea).

Multiple runs in tests are already implemented.

[ilevkivskyi]

OK, here is the PR #4045, it fixes this problem and two crashes.

[Michael0x2a]

I can work on fixing the existing incremental tests so they do check what happens if you run mypy 3+ times and submit a pull request sometime later today or tomorrow (assuming you agree this is a good idea).

Multiple runs in tests are already implemented.

@ilevkivskyi - sure, but my point was that many of the existing tests don't seem to take advantage of this feature?

[ilevkivskyi]

my point was that many of the existing tests don't seem to take advantage of this feature?

Yes, this will be helpful.