Invalidate all cache files if plugins change #5878
Conversation
mypy/build.py Outdated
| '(in {})'.format(plugin_path)) | ||
| try: | ||
| custom_plugins.append(plugin_type(options)) | ||
| # We record _both_ hash and the version to detect more |
There was a problem hiding this comment.
This function is quite long. It would be better if this would be moved to a helper method to avoid making it even longer.
| [out] | ||
| [out2] | ||
| | ||
| [case testChangedPluginsInvalidateCache] |
There was a problem hiding this comment.
Maybe add another test where only the version changes (e.g. import version from another module so that the text of the plugin doesn't change).
| # a mapping from each file being typechecked to its possible shadow file | ||
| self.shadow_equivalence_map = {} # type: Dict[str, Optional[str]] | ||
| self.plugin = plugin | ||
| self.plugins_snapshot = plugins_snapshot |
There was a problem hiding this comment.
Document the new attributes in the class docstring.
| | ||
| [case testChangedPluginsInvalidateCache] | ||
| # flags: --config-file tmp/mypy.ini | ||
| import a |
There was a problem hiding this comment.
Would it make sense to add a fine-grained incremental test case where a plugin changes?
There was a problem hiding this comment.
As we discussed, this will have no effect.
There was a problem hiding this comment.
Hm, I have found there is another daemon command dmypy run, which unlike dmypy check restarts if options change. I think we can also restart in dmypy run if plugins change. There is however no way to test this currently.
| There is a test failure. |
| Hm, this is weird, all tests pass locally. Also the error appears on the first run, which is even more weird. |
| OK, I also added restart for daemon if plugins change (in addition to cache invalidation) and tested manually that it works. @msullivan does this make sense to you? |
| @msullivan @JukkaL I assume this looks good now. Are there any other comments? |
| manager.trace(' {}: {} != {}' | ||
| .format(key, cached_options.get(key), current_options.get(key))) | ||
| return None | ||
| if manager.old_plugins_snapshot and manager.plugins_snapshot: |
There was a problem hiding this comment.
What about adding a test for this?
There was a problem hiding this comment.
We can't yet add tests for daemon. I opened #5891 for this.
#5878 added cache invalidation for situations where a plugin version/hash changes but there is even simpler scenario -- if `plugins` option itself change, the cache should be invalidated as well.
2 participants
Fixes #3592
This invalidates all cache files if at least one plugin changes its
__version__or hash of its content (this will not catch situations when there are changes in modules imported by a plugin, and version is also imported, but such situations are probably very rare).I was not sure where to place the check to invalidate all caches at the same time, so I invalidate them one by one.