Fine-grained: Fix crashes due to stale dependencies (#4725)

We could have a dependency pointing to a target that no longer was valid,
and various assertions could be triggered, causing a crash. Now we just
filter out bad targets to avoid crashes.

Fixes #4723 and other related issues.

Author: JukkaL

mypy/nodes.py

@@ -458,7 +458,7 @@ def set_line(self, target: Union[Context, int], column: Optional[int] = None) ->
 
 
 class FuncItem(FuncBase):
-    arguments = []  # type: List[Argument]
+    arguments = []  # type: List[Argument]  # Note: Can be None if deserialized (type is a lie!)
     arg_names = []  # type: List[str]
     arg_kinds = []  # type: List[int]
     # Minimum number of arguments

mypy/server/update.py

@@ -690,8 +690,7 @@ def propagate_changes_using_dependencies(
         if num_iter > MAX_ITER:
             raise RuntimeError('Max number of iterations (%d) reached (endless loop?)' % MAX_ITER)
 
-        todo = find_targets_recursive(manager, triggered, deps,
-                                      manager.modules, up_to_date_modules)
+        todo = find_targets_recursive(manager, triggered, deps, up_to_date_modules)
         # Also process targets that used to have errors, as otherwise some
         # errors might be lost.
         for target in targets_with_errors:
@@ -700,7 +699,7 @@ def propagate_changes_using_dependencies(
                 if id not in todo:
                     todo[id] = set()
                 manager.log_fine_grained('process target with error: %s' % target)
-                todo[id].update(lookup_target(manager.modules, target))
+                todo[id].update(lookup_target(manager, target))
         triggered = set()
         # TODO: Preserve order (set is not optimal)
         for id, nodes in sorted(todo.items(), key=lambda x: x[0]):
@@ -727,7 +726,6 @@ def find_targets_recursive(
         manager: BuildManager,
         triggers: Set[str],
         deps: Dict[str, Set[str]],
-        modules: Dict[str, MypyFile],
         up_to_date_modules: Set[str]) -> Dict[str, Set[DeferredNode]]:
     """Find names of all targets that need to reprocessed, given some triggers.
 
@@ -748,7 +746,7 @@ def find_targets_recursive(
             if target.startswith('<'):
                 worklist |= deps.get(target, set()) - processed
             else:
-                module_id = module_prefix(modules, target)
+                module_id = module_prefix(manager.modules, target)
                 if module_id is None:
                     # Deleted module.
                     continue
@@ -758,7 +756,7 @@ def find_targets_recursive(
                 if module_id not in result:
                     result[module_id] = set()
                 manager.log_fine_grained('process: %s' % target)
-                deferred = lookup_target(modules, target)
+                deferred = lookup_target(manager, target)
                 result[module_id].update(deferred)
 
     return result
@@ -794,8 +792,11 @@ def key(node: DeferredNode) -> int:
     # TODO: ignore_all argument to set_file_ignored_lines
     manager.errors.set_file_ignored_lines(file_node.path, file_node.ignored_lines)
 
-    targets = {target_from_node(module_id, node.node)
-               for node in nodes}
+    targets = set()
+    for node in nodes:
+        target = target_from_node(module_id, node.node)
+        if target is not None:
+            targets.add(target)
     manager.errors.clear_errors_in_targets(file_node.path, targets)
 
     # Strip semantic analysis information.
@@ -897,11 +898,18 @@ def update_deps(module_id: str,
             deps.setdefault(trigger, set()).update(targets)
 
 
-def lookup_target(modules: Dict[str, MypyFile], target: str) -> List[DeferredNode]:
+def lookup_target(manager: BuildManager,
+                  target: str) -> List[DeferredNode]:
     """Look up a target by fully-qualified name."""
+
+    def not_found() -> None:
+        manager.log_fine_grained(
+            "Can't find matching target for %s (stale dependency?)" % target)
+
+    modules = manager.modules
     items = split_target(modules, target)
     if items is None:
-        # Deleted target
+        not_found()  # Stale dependency
         return []
     module, rest = items
     if rest:
@@ -916,12 +924,11 @@ def lookup_target(modules: Dict[str, MypyFile], target: str) -> List[DeferredNod
         if isinstance(node, TypeInfo):
             active_class = node
             active_class_name = node.name()
-        # TODO: Is it possible for the assertion to fail?
         if isinstance(node, MypyFile):
             file = node
-        assert isinstance(node, (MypyFile, TypeInfo))
-        if c not in node.names:
-            # Deleted target
+        if (not isinstance(node, (MypyFile, TypeInfo))
+                or c not in node.names):
+            not_found()  # Stale dependency
             return []
         node = node.names[c].node
     if isinstance(node, TypeInfo):
@@ -930,18 +937,33 @@ def lookup_target(modules: Dict[str, MypyFile], target: str) -> List[DeferredNod
         # typically a module top-level, since we don't support processing class
         # bodies as separate entitites for simplicity.
         assert file is not None
+        if node.fullname() != target:
+            # This is a reference to a different TypeInfo, likely due to a stale dependency.
+            # Processing them would spell trouble -- for example, we could be refreshing
+            # a deserialized TypeInfo with missing attributes.
+            not_found()
+            return []
         result = [DeferredNode(file, None, None)]
         for name, symnode in node.names.items():
             node = symnode.node
             if isinstance(node, FuncDef):
-                result.extend(lookup_target(modules, target + '.' + name))
+                result.extend(lookup_target(manager, target + '.' + name))
         return result
     if isinstance(node, Decorator):
         # Decorator targets actually refer to the function definition only.
         node = node.func
-    assert isinstance(node, (FuncDef,
+    if not isinstance(node, (FuncDef,
                              MypyFile,
-                             OverloadedFuncDef)), 'unexpected type: %s' % type(node)
+                             OverloadedFuncDef)):
+        # The target can't be refreshed. It's possible that the target was
+        # changed to another type and we have a stale dependency pointing to it.
+        not_found()
+        return []
+    if node.fullname() != target:
+        # Stale reference points to something unexpected. We shouldn't process since the
+        # context will be wrong and it could be a partially initialized deserialized node.
+        not_found()
+        return []
     return [DeferredNode(node, active_class_name, active_class)]
 
 
@@ -950,14 +972,20 @@ def is_verbose(manager: BuildManager) -> bool:
 
 
 def target_from_node(module: str,
-                     node: Union[FuncDef, MypyFile, OverloadedFuncDef, LambdaExpr]) -> str:
+                     node: Union[FuncDef, MypyFile, OverloadedFuncDef, LambdaExpr]
+                     ) -> Optional[str]:
     """Return the target name corresponding to a deferred node.
 
     Args:
         module: Must be module id of the module that defines 'node'
+
+    Returns the target name, or None if the node is not a valid target in the given
+    module (for example, if it's actually defined in another module).
     """
     if isinstance(node, MypyFile):
-        assert module == node.fullname()
+        if module != node.fullname():
+            # Actually a reference to another module -- likely a stale dependency.
+            return None
         return module
     elif isinstance(node, (OverloadedFuncDef, FuncDef)):
         if node.info is not None:

mypy/traverser.py

@@ -34,13 +34,14 @@ def visit_block(self, block: Block) -> None:
             s.accept(self)
 
     def visit_func(self, o: FuncItem) -> None:
-        for arg in o.arguments:
-            init = arg.initializer
-            if init is not None:
-                init.accept(self)
-
-        for arg in o.arguments:
-            self.visit_var(arg.variable)
+        if o.arguments is not None:
+            for arg in o.arguments:
+                init = arg.initializer
+                if init is not None:
+                    init.accept(self)
+
+            for arg in o.arguments:
+                self.visit_var(arg.variable)
 
         o.body.accept(self)
 

test-data/unit/fine-grained.test

@@ -2833,6 +2833,151 @@ class C(Enum):
 a.py:4: error: Argument 1 to "f" has incompatible type "C"; expected "int"
 a.py:5: error: Argument 1 to "f" has incompatible type "C"; expected "int"
 
+[case testChangeFunctionToVariableAndRefreshUsingStaleDependency]
+import a
+import c
+[file a.py]
+import c
+def f() -> c.A: pass
+[file a.py.2]
+f = 1
+[file c.py]
+class A: pass
+[file c.py.3]
+[out]
+==
+==
+
+[case testChangeFunctionToTypeVarAndRefreshUsingStaleDependency]
+import a
+import c
+[file a.py]
+import c
+def f() -> c.A: pass
+[file a.py.2]
+from typing import TypeVar
+f = TypeVar('f')
+[file c.py]
+class A: pass
+[file c.py.3]
+[out]
+==
+==
+
+[case testChangeFunctionToModuleAndRefreshUsingStaleDependency]
+import a
+import c
+[file a.py]
+import c
+def f() -> c.A: pass
+[file a.py.2]
+import c as f
+[file c.py]
+class A: pass
+[file c.py.3]
+[out]
+==
+==
+
+[case testChangeFunctionToTypeAliasAndRefreshUsingStaleDependency1]
+import a
+import c
+[file a.py]
+import c
+def f() -> c.A: pass
+[file a.py.2]
+f = int
+[file c.py]
+class A: pass
+[file c.py.3]
+[out]
+==
+==
+
+[case testChangeFunctionToTypeAliasAndRefreshUsingStaleDependency2]
+import a
+import c
+[file a.py]
+import c
+def f() -> c.A: pass
+[file a.py.2]
+from typing import List
+f = List[int]
+[file c.py]
+class A: pass
+[file c.py.3]
+[builtins fixtures/list.pyi]
+[out]
+==
+==
+
+[case testChangeFunctionToClassAndRefreshUsingStaleDependency]
+import a
+import c
+[file a.py]
+import c
+def f() -> c.A: pass
+[file a.py.2]
+class f: pass
+[file c.py]
+class A: pass
+[file c.py.3]
+[out]
+==
+==
+
+[case testClassToVariableAndRefreshUsingStaleDependency]
+import a
+import c
+[file a.py]
+import c
+class A:
+    def f(self) -> c.A: pass
+[file a.py.2]
+A = 0
+[file c.py]
+class A: pass
+[file c.py.3]
+[out]
+==
+==
+
+[case testFunctionToImportedFunctionAndRefreshUsingStaleDependency]
+import a
+import c
+[file a.py]
+import c
+def f() -> c.A: pass
+[file a.py.2]
+from d import f
+[file c.py]
+class A: pass
+[file c.py.3]
+[file d.py]
+def g() -> None: pass
+def f() -> None:
+    g()
+[out]
+==
+==
+
+[case testMethodToVariableAndRefreshUsingStaleDependency]
+import a
+import c
+[file a.py]
+import c
+class B:
+    def f(self) -> c.A: pass
+[file a.py.2]
+class B:
+    f = 0
+[file c.py]
+class A: pass
+[file c.py.3]
+[out]
+==
+==
+
 [case testRefreshNestedClassWithSelfReference]
 import a
 [file a.py]