Store mros in cache files in order to fix a really pernicious bug

Author: msullivan

mypy/build.py

@@ -47,7 +47,7 @@
 from mypy.util import DecodeError
 from mypy.report import Reports
 from mypy import moduleinfo
-from mypy.fixup import fixup_module_pass_one, fixup_module_pass_two
+from mypy.fixup import fixup_module
 from mypy.nodes import Expression
 from mypy.options import Options
 from mypy.parse import parse
@@ -1704,13 +1704,9 @@ def fix_cross_refs(self) -> None:
         assert self.tree is not None, "Internal error: method must be called on parsed file only"
         # We need to set quick_and_dirty when doing a fine grained
         # cache load because we need to gracefully handle missing modules.
-        fixup_module_pass_one(self.tree, self.manager.modules,
-                              self.manager.options.quick_and_dirty or
-                              self.options.use_fine_grained_cache)
-
-    def calculate_mros(self) -> None:
-        assert self.tree is not None, "Internal error: method must be called on parsed file only"
-        fixup_module_pass_two(self.tree, self.manager.modules)
+        fixup_module(self.tree, self.manager.modules,
+                     self.manager.options.quick_and_dirty or
+                     self.options.use_fine_grained_cache)
 
     def patch_dependency_parents(self) -> None:
         """
@@ -2600,8 +2596,6 @@ def process_fresh_scc(graph: Graph, scc: List[str], manager: BuildManager) -> No
         graph[id].load_tree()
     for id in scc:
         graph[id].fix_cross_refs()
-    for id in scc:
-        graph[id].calculate_mros()
     for id in scc:
         graph[id].patch_dependency_parents()
 
@@ -2635,8 +2629,6 @@ def process_stale_scc(graph: Graph, scc: List[str], manager: BuildManager) -> No
         graph[id].semantic_analysis()
     for id in stale:
         graph[id].semantic_analysis_pass_three()
-    for id in fresh:
-        graph[id].calculate_mros()
     for id in stale:
         graph[id].semantic_analysis_apply_patches()
     for id in stale:

mypy/fixup.py

@@ -19,25 +19,12 @@
 # N.B: we do a quick_and_dirty fixup in both quick_and_dirty mode and
 # when fixing up a fine-grained incremental cache load (since there may
 # be cross-refs into deleted modules)
-def fixup_module_pass_one(tree: MypyFile, modules: Dict[str, MypyFile],
-                          quick_and_dirty: bool) -> None:
+def fixup_module(tree: MypyFile, modules: Dict[str, MypyFile],
+                 quick_and_dirty: bool) -> None:
     node_fixer = NodeFixer(modules, quick_and_dirty)
     node_fixer.visit_symbol_table(tree.names)
 
 
-def fixup_module_pass_two(tree: MypyFile, modules: Dict[str, MypyFile]) -> None:
-    compute_all_mros(tree.names, modules)
-
-
-def compute_all_mros(symtab: SymbolTable, modules: Dict[str, MypyFile]) -> None:
-    for key, value in symtab.items():
-        if value.kind in (LDEF, MDEF, GDEF) and isinstance(value.node, TypeInfo):
-            info = value.node
-            info.calculate_mro()
-            assert info.mro, "No MRO calculated for %s" % (info.fullname(),)
-            compute_all_mros(info.names, modules)
-
-
 # TODO: Fix up .info when deserializing, i.e. much earlier.
 class NodeFixer(NodeVisitor[None]):
     current_info = None  # type: Optional[TypeInfo]
@@ -69,6 +56,10 @@ def visit_type_info(self, info: TypeInfo) -> None:
                 info.declared_metaclass.accept(self.type_fixer)
             if info.metaclass_type:
                 info.metaclass_type.accept(self.type_fixer)
+            if info._mro_refs:
+                info.mro = [lookup_qualified_typeinfo(self.modules, name, self.quick_and_dirty)
+                            for name in info._mro_refs]
+                info._mro_refs = None
         finally:
             self.current_info = save_info
 
@@ -162,18 +153,12 @@ def visit_instance(self, inst: Instance) -> None:
         if type_ref is None:
             return  # We've already been here.
         del inst.type_ref
-        node = lookup_qualified(self.modules, type_ref, self.quick_and_dirty)
-        if isinstance(node, TypeInfo):
-            inst.type = node
-            # TODO: Is this needed or redundant?
-            # Also fix up the bases, just in case.
-            for base in inst.type.bases:
-                if base.type is NOT_READY:
-                    base.accept(self)
-        else:
-            # Looks like a missing TypeInfo in quick mode, put something there
-            assert self.quick_and_dirty, "Should never get here in normal mode"
-            inst.type = stale_info(self.modules)
+        inst.type = lookup_qualified_typeinfo(self.modules, type_ref, self.quick_and_dirty)
+        # TODO: Is this needed or redundant?
+        # Also fix up the bases, just in case.
+        for base in inst.type.bases:
+            if base.type is NOT_READY:
+                base.accept(self)
         for a in inst.args:
             a.accept(self)
 
@@ -251,6 +236,17 @@ def visit_type_type(self, t: TypeType) -> None:
         t.item.accept(self)
 
 
+def lookup_qualified_typeinfo(modules: Dict[str, MypyFile], name: str,
+                              quick_and_dirty: bool) -> TypeInfo:
+    node = lookup_qualified(modules, name, quick_and_dirty)
+    if isinstance(node, TypeInfo):
+        return node
+    else:
+        # Looks like a missing TypeInfo in quick mode, put something there
+        assert quick_and_dirty, "Should never get here in normal mode"
+        return stale_info(modules)
+
+
 def lookup_qualified(modules: Dict[str, MypyFile], name: str,
                      quick_and_dirty: bool) -> Optional[SymbolNode]:
     stnode = lookup_qualified_stnode(modules, name, quick_and_dirty)

mypy/nodes.py

@@ -1960,6 +1960,9 @@ class is generic then it will be a type constructor of higher kind.
     # Method Resolution Order: the order of looking up attributes. The first
     # value always to refers to this class.
     mro = None  # type: List[TypeInfo]
+    # Used to stash the names of the mro classes temporarily between
+    # deserialization and fixup. See deserialize() for why.
+    _mro_refs = None  # type: Optional[List[str]]
 
     declared_metaclass = None  # type: Optional[mypy.types.Instance]
     metaclass_type = None  # type: Optional[mypy.types.Instance]
@@ -2276,6 +2279,7 @@ def serialize(self) -> JsonDict:
                 'protocol_members': self.protocol_members,
                 'type_vars': self.type_vars,
                 'bases': [b.serialize() for b in self.bases],
+                'mro': [c.fullname() for c in self.mro],
                 '_promote': None if self._promote is None else self._promote.serialize(),
                 'declared_metaclass': (None if self.declared_metaclass is None
                                        else self.declared_metaclass.serialize()),
@@ -2307,7 +2311,17 @@ def deserialize(cls, data: JsonDict) -> 'TypeInfo':
                                  else mypy.types.Instance.deserialize(data['declared_metaclass']))
         ti.metaclass_type = (None if data['metaclass_type'] is None
                              else mypy.types.Instance.deserialize(data['metaclass_type']))
-        # NOTE: ti.mro will be set in the fixup phase.
+        # NOTE: ti.mro will be set in the fixup phase based on these
+        # names.  The reason we need to store the mro instead of just
+        # recomputing it from base classes has to do with a subtle
+        # point about fine-grained incremental: the cache files might
+        # not be loaded until after a class in the mro has changed its
+        # bases, which causes the mro to change. If we recomputed our
+        # mro, we would compute the *new* mro, which leaves us with no
+        # way to detact that the mro has changed! Thus we need to make
+        # sure to load the original mro so that once the class is
+        # rechecked, it can tell that the mro has changed.
+        ti._mro_refs = data['mro']
         ti.tuple_type = (None if data['tuple_type'] is None
                          else mypy.types.TupleType.deserialize(data['tuple_type']))
         ti.typeddict_type = (None if data['typeddict_type'] is None

test-data/unit/fine-grained.test

@@ -5504,7 +5504,7 @@ from typing import overload
 ==
 main:3: error: Module has no attribute "f"
 
-[case testOverloadsUpdatedTypeRecheckImplementation-skip]
+[case testOverloadsUpdatedTypeRecheckImplementation]
 from typing import overload
 import mod
 class Outer: