Repository: python/mypy
Pull request: Fine-grained: Support "from m import *"
This a adds a new per-module "wildcard" trigger that gets triggered if 
anything gets changed in module top-level. This allows tracking
"from m import *" dependencies which depend on every name in the
target module.

Fixes #4668.

Also fixed #4671 since it was triggered by some of the new tests.


Problem statement:
Fine-grained: Support "from m import *"
`from m import *` is only partially supported in fine-grained incremental mode.

Fine-grained: Error fixed in later steps may persist
I suspect that if we refresh the same target multiple times in a single fine-grained update, errors from the initial refreshes may persist in the generated errors even if those errors are later fixed. To fix this, we should remove any generated errors for a target before refreshing it. We may want to have errors indexed by the target to make this more efficient in case there are a large number of errors.

I have a test case that depends on unrelated local changes that may reproduce this, but it's still unclear. I'm creating this issue now to avoid duplicate work in case somebody else encounters this while I'm still working on this.

Hints from issue discussion:

Commit message: Fix type check

Patch to review:
diff --git a/mypy/errors.py b/mypy/errors.py
index b1ec7ab1ece6..2a7db1da435b 100644
--- a/mypy/errors.py
+++ b/mypy/errors.py
@@ -21,7 +21,7 @@ class ErrorInfo:
     # related to this error. Each item is a (path, line number) tuple.
     import_ctx = None  # type: List[Tuple[str, int]]
 
-    # The source file that was the source of this error.
+    # The path to source file that was the source of this error.
     file = ''
 
     # The fully-qualified id of the source module for this error.
@@ -51,7 +51,7 @@ class ErrorInfo:
     # Only report this particular messages once per program.
     only_once = False
 
-    # Actual origin of the error message
+    # Actual origin of the error message as tuple (path, line number)
     origin = None  # type: Tuple[str, int]
 
     # Fine-grained incremental target where this was reported
@@ -349,6 +349,17 @@ def add_error_info(self, info: ErrorInfo) -> None:
             self.only_once_messages.add(info.message)
         self._add_error_info(file, info)
 
+    def clear_errors_in_targets(self, path: str, targets: Set[str]) -> None:
+        """Remove errors in specific fine-grained targets within a file."""
+        if path in self.error_info_map:
+            new_errors = []
+            for info in self.error_info_map[path]:
+                if info.target not in targets:
+                    new_errors.append(info)
+                elif info.only_once:
+                    self.only_once_messages.remove(info.message)
+            self.error_info_map[path] = new_errors
+
     def generate_unused_ignore_notes(self, file: str) -> None:
         ignored_lines = self.ignored_lines[file]
         if not self.is_typeshed_file(file):
diff --git a/mypy/nodes.py b/mypy/nodes.py
index ab9f80008ce3..deab32b5c97b 100644
--- a/mypy/nodes.py
+++ b/mypy/nodes.py
@@ -315,11 +315,13 @@ class ImportAll(ImportBase):
     """from m import *"""
     id = None  # type: str
     relative = None  # type: int
+    imported_names = None  # type: List[str]
 
     def __init__(self, id: str, relative: int) -> None:
         super().__init__()
         self.id = id
         self.relative = relative
+        self.imported_names = []
 
     def accept(self, visitor: StatementVisitor[T]) -> T:
         return visitor.visit_import_all(self)
diff --git a/mypy/semanal.py b/mypy/semanal.py
index 3d849f085ed6..5ac14dec4e4b 100644
--- a/mypy/semanal.py
+++ b/mypy/semanal.py
@@ -1659,7 +1659,7 @@ def visit_import_all(self, i: ImportAll) -> None:
                 # False, and we can skip checking '_' because it's been explicitly included.
                 if (new_node and new_node.module_public and
                         (not name.startswith('_') or '__all__' in m.names)):
-                    existing_symbol = self.globals.get(name)
+                    existing_symbol = self.lookup_current_scope(name)
                     if existing_symbol:
                         # Import can redefine a variable. They get special treatment.
                         if self.process_import_over_existing_name(
@@ -1669,6 +1669,7 @@ def visit_import_all(self, i: ImportAll) -> None:
                                                           new_node.type_override,
                                                           normalized=new_node.normalized,
                                                           alias_tvars=new_node.alias_tvars), i)
+                    i.imported_names.append(name)
         else:
             # Don't add any dummy symbols for 'from x import *' if 'x' is unknown.
             pass
@@ -4005,6 +4006,14 @@ def accept(self, node: Node) -> None:
         except Exception as err:
             report_internal_error(err, self.errors.file, node.line, self.errors, self.options)
 
+    def lookup_current_scope(self, name: str) -> Optional[SymbolTableNode]:
+        if self.locals[-1] is not None:
+            return self.locals[-1].get(name)
+        elif self.type is not None:
+            return self.type.names.get(name)
+        else:
+            return self.globals.get(name)
+
 
 def replace_implicit_first_type(sig: FunctionLike, new: Type) -> FunctionLike:
     if isinstance(sig, CallableType):
diff --git a/mypy/server/aststrip.py b/mypy/server/aststrip.py
index 06a1f60f8dd6..d9be05b84662 100644
--- a/mypy/server/aststrip.py
+++ b/mypy/server/aststrip.py
@@ -43,7 +43,7 @@
 from mypy.nodes import (
     Node, FuncDef, NameExpr, MemberExpr, RefExpr, MypyFile, FuncItem, ClassDef, AssignmentStmt,
     ImportFrom, Import, TypeInfo, SymbolTable, Var, CallExpr, Decorator, OverloadedFuncDef,
-    SuperExpr, UNBOUND_IMPORTED, GDEF, MDEF, IndexExpr, SymbolTableNode
+    SuperExpr, UNBOUND_IMPORTED, GDEF, MDEF, IndexExpr, SymbolTableNode, ImportAll
 )
 from mypy.semanal_shared import create_indirect_imported_name
 from mypy.traverser import TraverserVisitor
@@ -65,6 +65,7 @@ def strip_target(node: Union[MypyFile, FuncItem, OverloadedFuncDef]) -> None:
 class NodeStripVisitor(TraverserVisitor):
     def __init__(self) -> None:
         self.type = None  # type: Optional[TypeInfo]
+        # Currently active module/class symbol table
         self.names = None  # type: Optional[SymbolTable]
         self.file_node = None  # type: Optional[MypyFile]
         self.is_class_body = False
@@ -125,14 +126,16 @@ def visit_overloaded_func_def(self, node: OverloadedFuncDef) -> None:
 
     @contextlib.contextmanager
     def enter_class(self, info: TypeInfo) -> Iterator[None]:
-        # TODO: Update and restore self.names
         old_type = self.type
         old_is_class_body = self.is_class_body
+        old_names = self.names
         self.type = info
         self.is_class_body = True
+        self.names = info.names
         yield
         self.type = old_type
         self.is_class_body = old_is_class_body
+        self.names = old_names
 
     @contextlib.contextmanager
     def enter_method(self, info: TypeInfo) -> Iterator[None]:
@@ -196,6 +199,14 @@ def visit_import(self, node: Import) -> None:
                     symnode.kind = UNBOUND_IMPORTED
                     symnode.node = None
 
+    def visit_import_all(self, node: ImportAll) -> None:
+        # Reset entries in the symbol table that were added through the statement.
+        # (The description in visit_import is relevant here as well.)
+        if self.names:
+            for name in node.imported_names:
+                del self.names[name]
+        node.imported_names = []
+
     def visit_name_expr(self, node: NameExpr) -> None:
         # Global assignments are processed in semantic analysis pass 1, and we
         # only want to strip changes made in passes 2 or later.
diff --git a/mypy/server/deps.py b/mypy/server/deps.py
index 33a41e5b5e09..7eea0ab83620 100644
--- a/mypy/server/deps.py
+++ b/mypy/server/deps.py
@@ -92,7 +92,7 @@ class 'mod.Cls'. This can also refer to an attribute inherited from a
     ComparisonExpr, GeneratorExpr, DictionaryComprehension, StarExpr, PrintStmt, ForStmt, WithStmt,
     TupleExpr, ListExpr, OperatorAssignmentStmt, DelStmt, YieldFromExpr, Decorator, Block,
     TypeInfo, FuncBase, OverloadedFuncDef, RefExpr, SuperExpr, Var, NamedTupleExpr, TypedDictExpr,
-    LDEF, MDEF, GDEF, FuncItem, TypeAliasExpr, NewTypeExpr,
+    LDEF, MDEF, GDEF, FuncItem, TypeAliasExpr, NewTypeExpr, ImportAll,
     op_methods, reverse_op_methods, ops_with_inplace_method, unary_op_methods
 )
 from mypy.traverser import TraverserVisitor
@@ -101,7 +101,7 @@ class 'mod.Cls'. This can also refer to an attribute inherited from a
     TupleType, TypeType, TypeVarType, TypedDictType, UnboundType, UninhabitedType, UnionType,
     FunctionLike, ForwardRef, Overloaded
 )
-from mypy.server.trigger import make_trigger
+from mypy.server.trigger import make_trigger, make_wildcard_trigger
 from mypy.util import correct_relative_import
 from mypy.scope import Scope
 
@@ -259,6 +259,15 @@ def visit_import_from(self, o: ImportFrom) -> None:
         for name, as_name in o.names:
             self.add_dependency(make_trigger(module_id + '.' + name))
 
+    def visit_import_all(self, o: ImportAll) -> None:
+        module_id, _ = correct_relative_import(self.scope.current_module_id(),
+                                               o.relative,
+                                               o.id,
+                                               self.is_package_init_file)
+        # The current target needs to be rechecked if anything "significant" changes in the
+        # target module namespace (as the imported definitions will need to be updated).
+        self.add_dependency(make_wildcard_trigger(module_id))
+
     def visit_block(self, o: Block) -> None:
         if not o.is_unreachable:
             super().visit_block(o)
diff --git a/mypy/server/trigger.py b/mypy/server/trigger.py
index 2c161f57c57e..808a5110759d 100644
--- a/mypy/server/trigger.py
+++ b/mypy/server/trigger.py
@@ -1,5 +1,17 @@
 """AST triggers that are used for fine-grained dependency handling."""
 
+# Used as a suffix for triggers to handle "from m import *" dependencies (see also
+# make_wildcard_trigger)
+WILDCARD_TAG = '[wildcard]'
+
 
 def make_trigger(name: str) -> str:
     return '<%s>' % name
+
+
+def make_wildcard_trigger(module: str) -> str:
+    """Special trigger fired when any top-level name is changed in a module.
+
+    This is used for "from m import *" dependencies.
+    """
+    return '<%s%s>' % (module, WILDCARD_TAG)
diff --git a/mypy/server/update.py b/mypy/server/update.py
index 25e5d366137c..854f611e5c7a 100644
--- a/mypy/server/update.py
+++ b/mypy/server/update.py
@@ -131,7 +131,7 @@
 from mypy.errors import Errors, CompileError
 from mypy.nodes import (
     MypyFile, FuncDef, TypeInfo, Expression, SymbolNode, Var, FuncBase, ClassDef, Decorator,
-    Import, ImportFrom, OverloadedFuncDef, SymbolTable
+    Import, ImportFrom, OverloadedFuncDef, SymbolTable, LambdaExpr
 )
 from mypy.options import Options
 from mypy.types import Type
@@ -143,7 +143,7 @@
 from mypy.server.aststrip import strip_target
 from mypy.server.deps import get_dependencies, get_dependencies_of_target
 from mypy.server.target import module_prefix, split_target
-from mypy.server.trigger import make_trigger
+from mypy.server.trigger import make_trigger, WILDCARD_TAG
 
 
 MAX_ITER = 1000
@@ -594,7 +594,16 @@ def calculate_active_triggers(manager: BuildManager,
             names.add(id)
         else:
             snapshot2 = snapshot_symbol_table(id, new.names)
-        names |= compare_symbol_table_snapshots(id, snapshot1, snapshot2)
+        diff = compare_symbol_table_snapshots(id, snapshot1, snapshot2)
+        package_nesting_level = id.count('.')
+        for item in diff:
+            if (item.count('.') <= package_nesting_level + 1
+                    and not item.split('.')[-1].startswith('_')):
+                # Activate catch-all wildcard trigger for top-level module changes (used for
+                # "from m import *").
+                diff.add(id + WILDCARD_TAG)
+                break
+        names |= diff
     return {make_trigger(name) for name in names}
 
 
@@ -654,7 +663,7 @@ def propagate_changes_using_dependencies(
             if id is not None and id not in up_to_date_modules:
                 if id not in todo:
                     todo[id] = set()
-                manager.log_fine_grained('process: %s' % target)
+                manager.log_fine_grained('process target with error: %s' % target)
                 todo[id].update(lookup_target(manager.modules, target))
         triggered = set()
         # TODO: Preserve order (set is not optimal)
@@ -712,7 +721,7 @@ def find_targets_recursive(
                     continue
                 if module_id not in result:
                     result[module_id] = set()
-                manager.log_fine_grained('process %s' % target)
+                manager.log_fine_grained('process: %s' % target)
                 deferred = lookup_target(modules, target)
                 result[module_id].update(deferred)
 
@@ -749,6 +758,10 @@ 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}
+    manager.errors.clear_errors_in_targets(file_node.path, targets)
+
     # Strip semantic analysis information.
     for deferred in nodes:
         strip_target(deferred.node)
@@ -898,3 +911,22 @@ def lookup_target(modules: Dict[str, MypyFile], target: str) -> List[DeferredNod
 
 def is_verbose(manager: BuildManager) -> bool:
     return manager.options.verbosity >= 1 or DEBUG_FINE_GRAINED
+
+
+def target_from_node(module: str,
+                     node: Union[FuncDef, MypyFile, OverloadedFuncDef, LambdaExpr]) -> str:
+    """Return the target name corresponding to a deferred node.
+
+    Args:
+        module: Must be module id of the module that defines 'node'
+    """
+    if isinstance(node, MypyFile):
+        assert module == node.fullname()
+        return module
+    elif isinstance(node, (OverloadedFuncDef, FuncDef)):
+        if node.info is not None:
+            return '%s.%s' % (node.info.fullname(), node.name())
+        else:
+            return '%s.%s' % (module, node.name())
+    else:
+        assert False, "Lambda expressions can't be deferred in fine-grained incremental mode"
diff --git a/test-data/unit/deps.test b/test-data/unit/deps.test
index b38d8d5ce161..6245794c3c45 100644
--- a/test-data/unit/deps.test
+++ b/test-data/unit/deps.test
@@ -596,3 +596,10 @@ def foo(x: Point) -> int:
 <m.Point> -> <m.foo>, <m.p>, m, m.Point, m.foo
 <m.p> -> m
 <mypy_extensions.TypedDict> -> m
+
+[case testImportStar]
+from a import *
+[file a.py]
+x = 0
+[out]
+<a[wildcard]> -> m
diff --git a/test-data/unit/fine-grained-modules.test b/test-data/unit/fine-grained-modules.test
index e5a6cf72d413..7fd1c4b9e503 100644
--- a/test-data/unit/fine-grained-modules.test
+++ b/test-data/unit/fine-grained-modules.test
@@ -1109,3 +1109,139 @@ x = 1
 [builtins fixtures/bool.pyi]
 [out]
 ==
+
+[case testImportStarPropagateChange1]
+from b import f
+f()
+[file b.py]
+from c import *
+[file c.py]
+def f() -> None: pass
+[file c.py.2]
+def f(x: int) -> None: pass
+[out]
+==
+main:2: error: Too few arguments for "f"
+
+[case testImportStarPropagateChange2]
+from b import *
+f()
+[file b.py]
+def f() -> None: pass
+[file b.py.2]
+def f(x: int) -> None: pass
+[out]
+==
+main:2: error: Too few arguments for "f"
+
+[case testImportStarAddMissingDependency1]
+from b import f
+f()
+[file b.py]
+from c import *
+[file c.py]
+[file c.py.2]
+def f(x: int) -> None: pass
+[out]
+main:1: error: Module 'b' has no attribute 'f'
+==
+main:2: error: Too few arguments for "f"
+
+[case testImportStarAddMissingDependency2]
+from b import *
+f()
+[file b.py]
+[file b.py.2]
+def f(x: int) -> None: pass
+[out]
+main:2: error: Name 'f' is not defined
+==
+main:2: error: Too few arguments for "f"
+
+[case testImportStarAddMissingDependencyWithinClass]
+class A:
+    from b import *
+    f()
+    x: C
+[file b.py]
+[file b.py.2]
+def f(x: int) -> None: pass
+[file b.py.3]
+def f(x: int) -> None: pass
+class C: pass
+[file b.py.4]
+def f() -> None: pass
+class C: pass
+[out]
+main:3: error: Name 'f' is not defined
+main:4: error: Name 'C' is not defined
+==
+main:3: error: Too few arguments for "f"
+main:4: error: Name 'C' is not defined
+==
+main:3: error: Too few arguments for "f"
+==
+
+[case testImportStarAddMissingDependencyInsidePackage1]
+from p.b import f
+f()
+[file p/__init__.py]
+[file p/b.py]
+from p.c import *
+[file p/c.py]
+[file p/c.py.2]
+def f(x: int) -> None: pass
+[out]
+main:1: error: Module 'p.b' has no attribute 'f'
+==
+main:2: error: Too few arguments for "f"
+
+[case testImportStarAddMissingDependencyInsidePackage2]
+import p.a
+[file p/__init__.py]
+[file p/a.py]
+from p.b import *
+f()
+[file p/b.py]
+[file p/b.py.2]
+def f(x: int) -> None: pass
+[out]
+p/a.py:2: error: Name 'f' is not defined
+==
+p/a.py:2: error: Too few arguments for "f"
+
+[case testImportStarRemoveDependency1]
+from b import f
+f()
+[file b.py]
+from c import *
+[file c.py]
+def f() -> None: pass
+[file c.py.2]
+[out]
+==
+main:1: error: Module 'b' has no attribute 'f'
+
+[case testImportStarRemoveDependency2]
+from b import *
+f()
+[file b.py]
+def f() -> None: pass
+[file b.py.2]
+[out]
+==
+main:2: error: Name 'f' is not defined
+
+[case testImportStarWithinFunction]
+def f() -> None:
+    from m import *
+    f()
+[file m.py]
+[file m.py.2]
+def f(x: int) -> None: pass
+[file m.py.3]
+def f() -> None: pass
+[out]
+==
+main:3: error: Too few arguments for "f"
+==
diff --git a/test-data/unit/fine-grained.test b/test-data/unit/fine-grained.test
index a4ee06efa38d..681ca2d692c8 100644
--- a/test-data/unit/fine-grained.test
+++ b/test-data/unit/fine-grained.test
@@ -1149,8 +1149,8 @@ def g() -> None:
 [typing fixtures/typing-full.pyi]
 [builtins fixtures/list.pyi]
 [triggered]
-2: <b.h>, <b>, a.g
-3: <b.h>, <b>, a
+2: <b.h>, <b>, <b[wildcard]>, a.g
+3: <b.h>, <b>, <b[wildcard]>, a
 4: a.g
 [out]
 a.py:11: error: Too many arguments for "h"


Code review comments:
diff_hunk: @@ -594,7 +594,16 @@ def calculate_active_triggers(manager: BuildManager,
             names.add(id)
         else:
             snapshot2 = snapshot_symbol_table(id, new.names)
-        names |= compare_symbol_table_snapshots(id, snapshot1, snapshot2)
+        diff = compare_symbol_table_snapshots(id, snapshot1, snapshot2)
+        package_nesting_level = id.count('.')
+        for item in diff:
+            if (item.count('.') <= package_nesting_level + 1
+                    and not item.split('.')[-1].startswith('_')):
path: mypy/server/update.py
text: @user1:
Shouldn't we treat `.pyi` files specially here? For example ignore top level names imported from other modules (unless imported using `... as ...`).

@author:
That would improve efficiency a bit but since `.pyi` files are usually shipped as part of typeshed, they don't change very frequently and this is a minor problem. Added a comment about this.