Update protocol caches when generating fg caches on incremental build (#5012)

Previously using `--cache-fine-grained` and `--incremental` together
would produce invalid protocol caches, which would prevent using
`--incremental` to quickly generate fine-grained cache artifacts.

This also adds machinery to `testfinegrained` to test building cache
artifacts using incremental runs.

Author: msullivan

mypy/build.py

@@ -980,8 +980,16 @@ def write_protocol_deps_cache(proto_deps: Dict[str, Set[str]],
     proto_meta, proto_cache = get_protocol_deps_cache_name(manager)
     meta_snapshot = {}  # type: Dict[str, str]
     error = False
-    for id in graph:
-        meta_snapshot[id] = graph[id].source_hash
+    for id, st in graph.items():
+        # If we didn't parse a file (so it doesn't have a
+        # source_hash), then it must be a module with a fresh cache,
+        # so use the hash from that.
+        if st.source_hash:
+            meta_snapshot[id] = st.source_hash
+        else:
+            assert st.meta, "Module must be either parsed or cached"
+            meta_snapshot[id] = st.meta.hash
+
     if not atomic_write(proto_meta, json.dumps(meta_snapshot), '\n'):
         manager.log("Error writing protocol meta JSON file {}".format(proto_cache))
         error = True
@@ -1008,11 +1016,12 @@ def read_protocol_cache(manager: BuildManager,
                                     log_error='Could not load protocol metadata: ')
     if meta_snapshot is None:
         return None
-    current_meta_snapshot = {}  # type: Dict[str, str]
-    for id in graph:
-        meta = graph[id].meta
-        assert meta is not None, 'Protocol cache should be read after all other'
-        current_meta_snapshot[id] = meta.hash
+    # Take a snapshot of the source hashes from all of the metas we found.
+    # (Including the ones we rejected because they were out of date.)
+    # We use this to verify that they match up with the proto_deps.
+    current_meta_snapshot = {id: st.meta_source_hash for id, st in graph.items()
+                             if st.meta_source_hash is not None}
+
     common = set(meta_snapshot.keys()) & set(current_meta_snapshot.keys())
     if any(meta_snapshot[id] != current_meta_snapshot[id] for id in common):
         # TODO: invalidate also if options changed (like --strict-optional)?
@@ -1608,7 +1617,8 @@ class State:
     path = None  # type: Optional[str]  # Path to module source
     xpath = None  # type: str  # Path or '<string>'
     source = None  # type: Optional[str]  # Module source code
-    source_hash = None  # type: str  # Hash calculated based on the source code
+    source_hash = None  # type: Optional[str]  # Hash calculated based on the source code
+    meta_source_hash = None  # type: Optional[str]  # Hash of the source given in the meta, if any
     meta = None  # type: Optional[CacheMeta]
     data = None  # type: Optional[str]
     tree = None  # type: Optional[MypyFile]
@@ -1699,6 +1709,7 @@ def __init__(self,
             # TODO: Get mtime if not cached.
             if self.meta is not None:
                 self.interface_hash = self.meta.interface_hash
+                self.meta_source_hash = self.meta.hash
         self.add_ancestors()
         self.meta = validate_meta(self.meta, self.id, self.path, self.ignore_all, manager)
         if self.meta:
@@ -2089,6 +2100,7 @@ def write_cache(self) -> None:
             return
         dep_prios = self.dependency_priorities()
         dep_lines = self.dependency_lines()
+        assert self.source_hash is not None
         new_interface_hash, self.meta = write_cache(
             self.id, self.path, self.tree,
             {k: list(v) for k, v in self.fine_grained_deps.items()},
@@ -2333,23 +2345,29 @@ def dispatch(sources: List[BuildSource], manager: BuildManager) -> Graph:
     if manager.options.dump_graph:
         dump_graph(graph)
         return graph
-    # If we are loading a fine-grained incremental mode cache, we
-    # don't want to do a real incremental reprocess of the graph---we
-    # just want to load in all of the cache information.
-    if manager.use_fine_grained_cache():
-        process_fine_grained_cache_graph(graph, manager)
-        # Fine grained protocol dependencies are serialized separately, so we read them
-        # after we loaded cache for whole graph. The `read_protocol_cache` will also validate
-        # the protocol cache against the loaded individual cache files.
-        TypeState.proto_deps = read_protocol_cache(manager, graph)
-        if TypeState.proto_deps is None and manager.stats.get('fresh_metas', 0) > 0:
+
+    # Fine grained protocol dependencies are serialized separately, so we read them
+    # after we load the cache for whole graph.
+    # We need to read them both for running in daemon mode and if we are generating
+    # a fine-grained cache (so that we can properly update them incrementally).
+    # The `read_protocol_cache` will also validate
+    # the protocol cache against the loaded individual cache files.
+    if manager.options.cache_fine_grained or manager.use_fine_grained_cache():
+        proto_deps = read_protocol_cache(manager, graph)
+        if proto_deps is not None:
+            TypeState.proto_deps = proto_deps
+        elif manager.stats.get('fresh_metas', 0) > 0:
             # There were some cache files read, but no protocol dependencies loaded.
             manager.log("Error reading protocol dependencies cache -- aborting cache load")
             manager.cache_enabled = False
-            TypeState.proto_deps = {}
             manager.log("Falling back to full run -- reloading graph...")
             return dispatch(sources, manager)
 
+    # If we are loading a fine-grained incremental mode cache, we
+    # don't want to do a real incremental reprocess of the graph---we
+    # just want to load in all of the cache information.
+    if manager.use_fine_grained_cache():
+        process_fine_grained_cache_graph(graph, manager)
     else:
         process_graph(graph, manager)
         if manager.options.cache_fine_grained or manager.options.fine_grained_incremental:

mypy/test/data.py

@@ -308,6 +308,13 @@ def teardown(self) -> None:
                 pycache = os.path.join(path, '__pycache__')
                 if os.path.isdir(pycache):
                     shutil.rmtree(pycache)
+                # As a somewhat nasty hack, ignore any dirs with .mypy_cache in the path,
+                # to allow test cases to intentionally corrupt the cache without provoking
+                # the test suite when there are still files left over.
+                # (Looking at / should be fine on windows because these are paths specified
+                # in the test cases.)
+                if '/.mypy_cache' in path:
+                    continue
                 try:
                     rmdir(path)
                 except OSError as error:
@@ -342,6 +349,8 @@ def find_steps(self) -> List[List[FileOperation]]:
         The first list item corresponds to the first incremental step, the second for the
         second step, etc. Each operation can either be a file modification/creation (UpdateFile)
         or deletion (DeleteFile).
+
+        Defaults to having two steps if there aern't any operations.
         """
         steps = {}  # type: Dict[int, List[FileOperation]]
         for path, _ in self.files:
@@ -358,8 +367,8 @@ def find_steps(self) -> List[List[FileOperation]]:
             for path in paths:
                 module = module_from_path(path)
                 steps.setdefault(num, []).append(DeleteFile(module, path))
-        max_step = max(steps)
-        return [steps[num] for num in range(2, max_step + 1)]
+        max_step = max(steps) if steps else 2
+        return [steps.get(num, []) for num in range(2, max_step + 1)]
 
 
 def module_from_path(path: str) -> str:

mypy/test/testcheck.py

@@ -10,7 +10,7 @@
 from mypy import build, defaults
 from mypy.build import BuildSource, Graph
 from mypy.test.config import test_temp_dir
-from mypy.test.data import DataDrivenTestCase, DataSuite
+from mypy.test.data import DataDrivenTestCase, DataSuite, FileOperation, UpdateFile, DeleteFile
 from mypy.test.helpers import (
     assert_string_arrays_equal, normalize_error_messages, assert_module_equivalence,
     retry_on_error, update_testcase_output, parse_options,
@@ -93,7 +93,6 @@ def run_case(self, testcase: DataDrivenTestCase) -> None:
             # Incremental tests are run once with a cold cache, once with a warm cache.
             # Expect success on first run, errors from testcase.output (if any) on second run.
             # We briefly sleep to make sure file timestamps are distinct.
-            self.clear_cache()
             num_steps = max([2] + list(testcase.output2.keys()))
             # Check that there are no file changes beyond the last run (they would be ignored).
             for dn, dirs, files in os.walk(os.curdir):
@@ -103,18 +102,17 @@ def run_case(self, testcase: DataDrivenTestCase) -> None:
                         raise ValueError(
                             'Output file {} exists though test case only has {} runs'.format(
                                 file, num_steps))
+            steps = testcase.find_steps()
             for step in range(1, num_steps + 1):
-                self.run_case_once(testcase, step)
+                idx = step - 2
+                ops = steps[idx] if idx < len(steps) and idx >= 0 else []
+                self.run_case_once(testcase, ops, step)
         else:
             self.run_case_once(testcase)
 
-    def clear_cache(self) -> None:
-        dn = defaults.CACHE_DIR
-
-        if os.path.exists(dn):
-            shutil.rmtree(dn)
-
-    def run_case_once(self, testcase: DataDrivenTestCase, incremental_step: int = 0) -> None:
+    def run_case_once(self, testcase: DataDrivenTestCase,
+                      operations: List[FileOperation] = [],
+                      incremental_step: int = 0) -> None:
         original_program_text = '\n'.join(testcase.input)
         module_data = self.parse_module(original_program_text, incremental_step)
 
@@ -127,16 +125,15 @@ def run_case_once(self, testcase: DataDrivenTestCase, incremental_step: int = 0)
                     break
         elif incremental_step > 1:
             # In runs 2+, copy *.[num] files to * files.
-            for dn, dirs, files in os.walk(os.curdir):
-                for file in files:
-                    if file.endswith('.' + str(incremental_step)):
-                        full = os.path.join(dn, file)
-                        target = full[:-2]
-                        copy_and_fudge_mtime(full, target)
-            # Delete files scheduled to be deleted in [delete <path>.num] sections.
-            for path in testcase.deleted_paths.get(incremental_step, set()):
-                # Use retries to work around potential flakiness on Windows (AppVeyor).
-                retry_on_error(lambda: os.remove(path))
+            for op in operations:
+                if isinstance(op, UpdateFile):
+                    # Modify/create file
+                    copy_and_fudge_mtime(op.source_path, op.target_path)
+                else:
+                    # Delete file
+                    # Use retries to work around potential flakiness on Windows (AppVeyor).
+                    path = op.path
+                    retry_on_error(lambda: os.remove(path))
 
         # Parse options after moving files (in case mypy.ini is being moved).
         options = parse_options(original_program_text, testcase, incremental_step)

mypy/test/testfinegrained.py

@@ -81,22 +81,13 @@ def run_case(self, testcase: DataDrivenTestCase) -> None:
             f.write(main_src)
 
         options = self.get_options(main_src, testcase, build_cache=False)
-        for name, _ in testcase.files:
-            if 'mypy.ini' in name:
-                config = name  # type: Optional[str]
-                break
-        else:
-            config = None
-        if config:
-            parse_config_file(options, config)
+        build_options = self.get_options(main_src, testcase, build_cache=True)
         server = Server(options)
 
+        num_regular_incremental_steps = self.get_build_steps(main_src)
         step = 1
         sources = self.parse_sources(main_src, step, options)
-        if self.use_cache:
-            build_options = self.get_options(main_src, testcase, build_cache=True)
-            if config:
-                parse_config_file(build_options, config)
+        if step <= num_regular_incremental_steps:
             messages = self.build(build_options, sources)
         else:
             messages = self.run_check(server, sources)
@@ -122,7 +113,11 @@ def run_case(self, testcase: DataDrivenTestCase) -> None:
                     # Delete file
                     os.remove(op.path)
             sources = self.parse_sources(main_src, step, options)
-            new_messages = self.run_check(server, sources)
+
+            if step <= num_regular_incremental_steps:
+                new_messages = self.build(build_options, sources)
+            else:
+                new_messages = self.run_check(server, sources)
 
             updated = []  # type: List[str]
             changed = []  # type: List[str]
@@ -179,6 +174,11 @@ def get_options(self,
         if options.follow_imports == 'normal':
             options.follow_imports = 'error'
 
+        for name, _ in testcase.files:
+            if 'mypy.ini' in name:
+                parse_config_file(options, name)
+                break
+
         return options
 
     def run_check(self, server: Server, sources: List[BuildSource]) -> List[str]:
@@ -205,6 +205,15 @@ def format_triggered(self, triggered: List[List[str]]) -> List[str]:
             result.append(('%d: %s' % (n + 2, ', '.join(filtered))).strip())
         return result
 
+    def get_build_steps(self, program_text: str) -> int:
+        """Get the number of regular incremental steps to run, from the test source"""
+        if not self.use_cache:
+            return 0
+        m = re.search('# num_build_steps: ([0-9]+)$', program_text, flags=re.MULTILINE)
+        if m is not None:
+            return int(m.group(1))
+        return 1
+
     def parse_sources(self, program_text: str,
                       incremental_step: int,
                       options: Options) -> List[BuildSource]:

mypy/test/testfinegrainedcache.py

@@ -11,3 +11,5 @@
 class FineGrainedCacheSuite(mypy.test.testfinegrained.FineGrainedSuite):
     use_cache = True
     test_name_suffix = '_cached'
+    files = (
+        mypy.test.testfinegrained.FineGrainedSuite.files + ['fine-grained-cache-incremental.test'])

test-data/unit/check-incremental.test

@@ -4319,3 +4319,45 @@ def foo() -> None:
 [out]
 [out2]
 tmp/main.py:2: error: "int" has no attribute "foo"
+
+[case testIncrementalBustedFineGrainedCache1]
+# flags: --cache-fine-grained
+import a
+import b
+[file a.py]
+[file b.py]
+-- This is a heinous hack, but we simulate having a invalid cache by clobbering
+-- the proto deps file with something with hash mismatches.
+[file ../.mypy_cache/3.6/@proto_deps.meta.json.2]
+{"__main__": "00000000000000000000000000000000", "a": "d41d8cd98f00b204e9800998ecf8427e", "b": "d41d8cd98f00b204e9800998ecf8427e", "builtins": "00000000000000000000000000000000"}
+[file b.py.2]
+# uh
+-- Every file should get reloaded, since the cache was invalidated
+[stale a, b, builtins]
+[rechecked a, b, builtins]
+
+[case testIncrementalBustedFineGrainedCache2]
+# flags2: --cache-fine-grained
+import a
+import b
+[file a.py]
+[file b.py]
+[file b.py.2]
+# uh
+-- Every file should get reloaded, since the settings changed
+[stale a, b, builtins]
+[rechecked a, b, builtins]
+
+[case testIncrementalWorkingFineGrainedCache]
+# flags: --cache-fine-grained
+# flags2: --cache-fine-grained
+import a
+import b
+[file a.py]
+[file b.py]
+[file b.py.2]
+# uh
+-- b gets rechecked because it changed, but nothing is stale
+-- since the interface didn't change
+[stale]
+[rechecked b]