Delete cache for a module if errors are found #4045
Conversation
mypy/build.py
Outdated
| try: | ||
| os.remove(filename) | ||
| except OSError: | ||
| manager.log("Error deleting cache file {}".format(filename)) |
There was a problem hiding this comment.
This would cause a bit noisy logging. If the file doesn't exist I'd rather not log anything (or specifically log that the file doesn't exist).
| @@ -1813,6 +1831,7 @@ def write_cache(self) -> None: | |||
| else: | |||
| is_errors = self.manager.errors.is_errors() | |||
| if is_errors: | |||
| delete_cache(self.id, self.path, self.manager) | |||
There was a problem hiding this comment.
Shouldn't this also mark the interface as stale (as below on line 1845-1847) and set self.interface_hash to something unmatchable? Otherwise I worry we could still repro the same issues by adding another chain to the reference link.
There was a problem hiding this comment.
Yes, you are right. I even have found a longer crasher with four files that still happens unless interface is marked as stale (I added it to the tests). Do we also need to set the interface_hash to something? It looks like it is not necessary.
It looks like marking files with errors as stale breaks several tests. I will check if this can be fixed somehow.
There was a problem hiding this comment.
Hm, there is an explicit comment in tests:
Note that a file that ends up producing an error does not create
a new cache file and so is not considered stale.
Should I just mark them as stale now and update this comment?
There was a problem hiding this comment.
Yeah, I think the comment just reflects the state of the world at the time (i.e. that no cache file was written).
I presume this only affects some of the tests and not all tests with a [stale] marker?
Alternatively you could have a flag to mark_interface_stale() to skip adding the module to stale_modules since that is used purely by the tests. (The other two things it sets are used for the graph algorithm too.)
There was a problem hiding this comment.
I presume this only affects some of the tests and not all tests with a [stale] marker?
It affects 42 tests, i.e. around one third of all tests with [stale].
Alternatively you could have a flag to
mark_interface_stale()to skip adding the module tostale_modulessince that is used purely by the tests. (The other two things it sets are used for the graph algorithm too.)
OK, I did this. Now all tests should pass. By the way, what do you think about potential performance issues because of this change? Intuitively it should be minor (since it only affects situations where new errors appear after previous incremental runs), but maybe you could measure it somehow?
|
Whee! |
Fixes #4043
Fixes #3852
Fixes #3052
This is a simple-minded solution for inconsistent cache state problem discovered in #4043.