Fix crash in dmypy when packages are removed during dmypy run #19229
Conversation
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
| return [BuildSource(graph[dep].path, dep, followed=True) for dep in state.dependencies] | ||
| except KeyError: | ||
| # Dependency not found in graph, it was probably removed while dmypy was running. | ||
| return None |
There was a problem hiding this comment.
I'm afraid this will result in inconsistent state eventually. Would be really helpful to see a reproducer, but in general something like this (modules changed while (d)mypy is running) is really difficult to support. I can envision a solution that restarts the whole processing upon encountering such condition (and that should be moderately easy to support), we need to be really careful with such patching.
There was a problem hiding this comment.
I can try to find a more general fix, but just so it's a 100% clear, is dmypy supposed to support the following use case?
- I run
dmypy run ., and see the results. - I do various changes in my codebase (possibly switching branches), which results in different packages being removed or added
- I run
dmypy run .again.
There was a problem hiding this comment.
Definitely yes, and we have a bunch of tests attesting that. I don't use dmypy personally, but I've made a couple of fixes related to such module structure changes in 1.16. Does your problem reproduce on latest mypy version?
I must have misinterpreted your PR description as "file is deleted while dmypy is running", which is indeed likely unsupported.
There was a problem hiding this comment.
Just tested with 1.16.0 and sadly the issue is still there.
Thanks for the feedback, I'll see whether I can make a reproducible test and potentially a better fix.
At our company we use mypy extensively on quite a large codebase. More often than not, after switching branches dmypy will crash, because it still expects an import of a package although the package is gone in the new branch.
I did not manage to write a unit test for this, but verified it on our codebase and it solved the issue.