New semantic analyzer: Make assignment analysis logic more straightforward (#6527)

Fixes #6412 
Fixes #6453 

In this PR:
* Wait until we can classify r.h.s. of an assignment as normal, type alias, named tuple etc.
* Choose the corresponding branch instead of performing all of them (as it used to be before)
* Never create (or rely on creation of) temporary `Var`s
* Add every symbol to symbol table only once (as it is done for classes)
* Don't "patch" existing symbols (with one exception for type variables, see below)
* Add and update docstrings and comments

Notes:
* I allow placeholders for type alias targets and NewTypes targets, they essentially behave as base classes. In general I make logic for type aliases and NewTypes much more similar to logic of class definitions (especially NewTypes), this allows to support a whole bunch of tricky forward references
* I don't store types with placeholders in annotations, and instead if an alias target got updated (e.g. placeholders are resolved), I set `progress=True`

Follow-ups:
* There is still a small hack to keep compatibility with old analyzer in type checker, but now it is basically "don't use binder for l.h.s. of special forms"
* Type variable declarations still have a bug that duplicate type variable definition is not reported as error, second one silently wins, this will be fixed in a separate PR
* This PR doesn't aim to fix aspects of assignment analysis related to #6422, that will be also a separate PR

Test updates:
* Add and enable more tests for type aliases and NewTypes
* Add test for cyclic definition error messages
* Change order of error messages on the same line in few tests
* Enable one test file where previously only some tests were opted in (I think it was not enabled by mistake in one of the previous PRs, I enable it here since order of errors in one test in that file is affected by this PR.)

Author: ilevkivskyi

mypy/checker.py

@@ -1715,6 +1715,11 @@ def visit_assignment_stmt(self, s: AssignmentStmt) -> None:
         with self.enter_final_context(s.is_final_def):
             self.check_assignment(s.lvalues[-1], s.rvalue, s.type is None, s.new_syntax)
 
+        if s.is_alias_def:
+            # We do this mostly for compatibility with old semantic analyzer.
+            # TODO: should we get rid of this?
+            self.store_type(s.lvalues[-1], self.expr_checker.accept(s.rvalue))
+
         if (s.type is not None and
                 self.options.disallow_any_unimported and
                 has_any_from_unimported_type(s.type)):
@@ -1824,7 +1829,9 @@ def check_assignment(self, lvalue: Lvalue, rvalue: Expression, infer_lvalue_type
                     self.msg.concrete_only_assign(lvalue_type, rvalue)
                     return
                 if rvalue_type and infer_lvalue_type and not isinstance(lvalue_type, PartialType):
-                    self.binder.assign_type(lvalue, rvalue_type, lvalue_type, False)
+                    # Don't use type binder for definitions of special forms, like named tuples.
+                    if not (isinstance(lvalue, NameExpr) and lvalue.is_special_form):
+                        self.binder.assign_type(lvalue, rvalue_type, lvalue_type, False)
 
             elif index_lvalue:
                 self.check_indexed_assignment(index_lvalue, rvalue, lvalue)

mypy/newsemanal/semanal.py

@@ -19,20 +19,22 @@ def __init__(self, options: Options, api: SemanticAnalyzerInterface) -> None:
         self.options = options
         self.api = api
 
-    def process_enum_call(self, s: AssignmentStmt, is_func_scope: bool) -> None:
-        """Check if s defines an Enum; if yes, store the definition in symbol table."""
+    def process_enum_call(self, s: AssignmentStmt, is_func_scope: bool) -> bool:
+        """Check if s defines an Enum; if yes, store the definition in symbol table.
+
+        Return True if this looks like an Enum definition (but maybe with errors),
+        otherwise return False.
+        """
         if len(s.lvalues) != 1 or not isinstance(s.lvalues[0], NameExpr):
-            return
+            return False
         lvalue = s.lvalues[0]
         name = lvalue.name
         enum_call = self.check_enum_call(s.rvalue, name, is_func_scope)
         if enum_call is None:
-            return
+            return False
         # Yes, it's a valid Enum definition. Add it to the symbol table.
-        node = self.api.lookup(name, s)
-        if node:
-            node.kind = GDEF   # TODO locally defined Enum
-            node.node = enum_call
+        self.api.add_symbol(name, enum_call, s)
+        return True
 
     def check_enum_call(self,
                         node: Expression,
@@ -62,18 +64,19 @@ class A(enum.Enum):
         items, values, ok = self.parse_enum_call_args(call, fullname.split('.')[-1])
         if not ok:
             # Error. Construct dummy return value.
-            return self.build_enum_call_typeinfo(var_name, [], fullname)
-        name = cast(Union[StrExpr, UnicodeExpr], call.args[0]).value
-        if name != var_name or is_func_scope:
-            # Give it a unique name derived from the line number.
-            name += '@' + str(call.line)
-        info = self.build_enum_call_typeinfo(name, items, fullname)
-        # Store it as a global just in case it would remain anonymous.
-        # (Or in the nearest class if there is one.)
-        stnode = SymbolTableNode(GDEF, info)
-        self.api.add_symbol_table_node(name, stnode)
+            info = self.build_enum_call_typeinfo(var_name, [], fullname)
+        else:
+            name = cast(Union[StrExpr, UnicodeExpr], call.args[0]).value
+            if name != var_name or is_func_scope:
+                # Give it a unique name derived from the line number.
+                name += '@' + str(call.line)
+            info = self.build_enum_call_typeinfo(name, items, fullname)
+            # Store generated TypeInfo under both names, see semanal_namedtuple for more details.
+            if name != var_name or is_func_scope:
+                self.api.add_symbol_skip_local(name, info)
         call.analyzed = EnumCallExpr(info, items, values)
         call.analyzed.set_line(call.line, call.column)
+        info.line = node.line
         return info
 
     def build_enum_call_typeinfo(self, name: str, items: List[str], fullname: str) -> TypeInfo:
@@ -93,6 +96,10 @@ def build_enum_call_typeinfo(self, name: str, items: List[str], fullname: str) -
     def parse_enum_call_args(self, call: CallExpr,
                              class_name: str) -> Tuple[List[str],
                                                        List[Optional[Expression]], bool]:
+        """Parse arguments of an Enum call.
+
+        Return a tuple of fields, values, was there an error.
+        """
         args = call.args
         if len(args) < 2:
             return self.fail_enum_call_arg("Too few arguments for %s()" % class_name, call)

mypy/newsemanal/semanal_enum.py

@@ -5,10 +5,14 @@
 
 from typing import Tuple, Optional
 
-from mypy.types import Type, Instance, CallableType, NoneTyp, TupleType, AnyType, TypeOfAny
+from mypy.types import (
+    Type, Instance, CallableType, NoneTyp, TupleType, AnyType, PlaceholderType,
+    TypeOfAny
+)
 from mypy.nodes import (
     AssignmentStmt, NewTypeExpr, CallExpr, NameExpr, RefExpr, Context, StrExpr, BytesExpr,
-    UnicodeExpr, Block, FuncDef, Argument, TypeInfo, Var, SymbolTableNode, GDEF, MDEF, ARG_POS
+    UnicodeExpr, Block, FuncDef, Argument, TypeInfo, Var, SymbolTableNode, MDEF, ARG_POS,
+    PlaceholderNode
 )
 from mypy.newsemanal.semanal_shared import SemanticAnalyzerInterface
 from mypy.options import Options
@@ -26,17 +30,36 @@ def __init__(self,
         self.api = api
         self.msg = msg
 
-    def process_newtype_declaration(self, s: AssignmentStmt) -> None:
-        """Check if s declares a NewType; if yes, store it in symbol table."""
-        # Extract and check all information from newtype declaration
+    def process_newtype_declaration(self, s: AssignmentStmt) -> bool:
+        """Check if s declares a NewType; if yes, store it in symbol table.
+
+        Return True if it's a NewType declaration. The current target may be
+        deferred as a side effect if the base type is not ready, even if
+        the return value is True.
+
+        The logic in this function mostly copies the logic for visit_class_def()
+        with a single (non-Generic) base.
+        """
         name, call = self.analyze_newtype_declaration(s)
         if name is None or call is None:
-            return
-
-        old_type = self.check_newtype_args(name, call, s)
-        call.analyzed = NewTypeExpr(name, old_type, line=call.line)
+            return False
+        # OK, now we know this is a NewType. But the base type may be not ready yet,
+        # add placeholder as we do for ClassDef.
+
+        fullname = self.api.qualified_name(name)
+        if (not call.analyzed or
+                isinstance(call.analyzed, NewTypeExpr) and not call.analyzed.info):
+            # Start from labeling this as a future class, as we do for normal ClassDefs.
+            self.api.add_symbol(name, PlaceholderNode(fullname, s, becomes_typeinfo=True), s)
+
+        old_type, should_defer = self.check_newtype_args(name, call, s)
+        if not call.analyzed:
+            call.analyzed = NewTypeExpr(name, old_type, line=call.line)
         if old_type is None:
-            return
+            if should_defer:
+                # Base type is not ready.
+                self.api.defer()
+                return True
 
         # Create the corresponding class definition if the aliased type is subtypeable
         if isinstance(old_type, TupleType):
@@ -48,9 +71,14 @@ def process_newtype_declaration(self, s: AssignmentStmt) -> None:
                 self.fail("NewType cannot be used with protocol classes", s)
             newtype_class_info = self.build_newtype_typeinfo(name, old_type, old_type)
         else:
-            message = "Argument 2 to NewType(...) must be subclassable (got {})"
-            self.fail(message.format(self.msg.format(old_type)), s)
-            return
+            if old_type is not None:
+                message = "Argument 2 to NewType(...) must be subclassable (got {})"
+                self.fail(message.format(self.msg.format(old_type)), s)
+            # Otherwise the error was already reported.
+            old_type = AnyType(TypeOfAny.from_error)
+            object_type = self.api.named_type('__builtins__.object')
+            newtype_class_info = self.build_newtype_typeinfo(name, old_type, object_type)
+            newtype_class_info.fallback_to_any = True
 
         check_for_explicit_any(old_type, self.options, self.api.is_typeshed_stub_file, self.msg,
                                context=s)
@@ -59,13 +87,16 @@ def process_newtype_declaration(self, s: AssignmentStmt) -> None:
             self.msg.unimported_type_becomes_any("Argument 2 to NewType(...)", old_type, s)
 
         # If so, add it to the symbol table.
-        node = self.api.lookup(name, s)
-        if node is None:
-            self.fail("Could not find {} in current namespace".format(name), s)
-            return
-        # TODO: why does NewType work in local scopes despite always being of kind GDEF?
-        node.kind = GDEF
-        call.analyzed.info = node.node = newtype_class_info
+        assert isinstance(call.analyzed, NewTypeExpr)
+        # As we do for normal classes, create the TypeInfo only once, then just
+        # update base classes on next iterations (to get rid of placeholders there).
+        if not call.analyzed.info:
+            call.analyzed.info = newtype_class_info
+        else:
+            call.analyzed.info.bases = newtype_class_info.bases
+        self.api.add_symbol(name, call.analyzed.info, s)
+        newtype_class_info.line = s.line
+        return True
 
     def analyze_newtype_declaration(self,
             s: AssignmentStmt) -> Tuple[Optional[str], Optional[CallExpr]]:
@@ -76,13 +107,18 @@ def analyze_newtype_declaration(self,
                 and isinstance(s.rvalue, CallExpr)
                 and isinstance(s.rvalue.callee, RefExpr)
                 and s.rvalue.callee.fullname == 'typing.NewType'):
-            lvalue = s.lvalues[0]
             name = s.lvalues[0].name
-            if not lvalue.is_inferred_def:
-                if s.type:
-                    self.fail("Cannot declare the type of a NewType declaration", s)
-                else:
-                    self.fail("Cannot redefine '%s' as a NewType" % name, s)
+
+            if s.type:
+                self.fail("Cannot declare the type of a NewType declaration", s)
+
+            names = self.api.current_symbol_table()
+            existing = names.get(name)
+            # Give a better error message than generic "Name already defined",
+            # like the old semantic analyzer does.
+            if (existing and
+                    not isinstance(existing.node, PlaceholderNode) and not s.rvalue.analyzed):
+                self.fail("Cannot redefine '%s' as a NewType" % name, s)
 
             # This dummy NewTypeExpr marks the call as sufficiently analyzed; it will be
             # overwritten later with a fully complete NewTypeExpr if there are no other
@@ -91,12 +127,17 @@ def analyze_newtype_declaration(self,
 
         return name, call
 
-    def check_newtype_args(self, name: str, call: CallExpr, context: Context) -> Optional[Type]:
+    def check_newtype_args(self, name: str, call: CallExpr,
+                           context: Context) -> Tuple[Optional[Type], bool]:
+        """Ananlyze base type in NewType call.
+
+        Return a tuple (type, should defer).
+        """
         has_failed = False
         args, arg_kinds = call.args, call.arg_kinds
         if len(args) != 2 or arg_kinds[0] != ARG_POS or arg_kinds[1] != ARG_POS:
             self.fail("NewType(...) expects exactly two positional arguments", context)
-            return None
+            return None, False
 
         # Check first argument
         if not isinstance(args[0], (StrExpr, BytesExpr, UnicodeExpr)):
@@ -113,19 +154,22 @@ def check_newtype_args(self, name: str, call: CallExpr, context: Context) -> Opt
             unanalyzed_type = expr_to_unanalyzed_type(args[1])
         except TypeTranslationError:
             self.fail(msg, context)
-            return None
+            return None, False
 
         # We want to use our custom error message (see above), so we suppress
         # the default error message for invalid types here.
         old_type = self.api.anal_type(unanalyzed_type, report_invalid_types=False)
+        should_defer = False
+        if old_type is None or isinstance(old_type, PlaceholderType):
+            should_defer = True
 
         # The caller of this function assumes that if we return a Type, it's always
         # a valid one. So, we translate AnyTypes created from errors into None.
         if isinstance(old_type, AnyType) and old_type.is_from_error:
             self.fail(msg, context)
-            return None
+            return None, False
 
-        return None if has_failed else old_type
+        return None if has_failed else old_type, should_defer
 
     def build_newtype_typeinfo(self, name: str, old_type: Type, base_type: Instance) -> TypeInfo:
         info = self.api.basic_new_typeinfo(name, base_type)

mypy/newsemanal/semanal_newtype.py

@@ -6,7 +6,7 @@
 
 from mypy.nodes import (
     Context, SymbolTableNode, MypyFile, ImportedName, FuncDef, Node, TypeInfo, Expression, GDEF,
-    SymbolNode
+    SymbolNode, SymbolTable
 )
 from mypy.util import correct_relative_import
 from mypy.types import Type, FunctionLike, Instance, TupleType
@@ -123,6 +123,14 @@ def add_symbol_table_node(self, name: str, stnode: SymbolTableNode) -> bool:
         """Add node to the current symbol table."""
         raise NotImplementedError
 
+    @abstractmethod
+    def current_symbol_table(self) -> SymbolTable:
+        """Get currently active symbol table.
+
+        May be module, class, or local namespace.
+        """
+        raise NotImplementedError
+
     @abstractmethod
     def add_symbol(self, name: str, node: SymbolNode, context: Optional[Context],
                    module_public: bool = True, module_hidden: bool = False) -> bool:

mypy/newsemanal/semanal_shared.py

@@ -75,48 +75,6 @@ def analyze_type_alias(node: Expression,
     full names of type aliases it depends on (directly or indirectly).
     Return None otherwise. 'node' must have been semantically analyzed.
     """
-    # Quickly return None if the expression doesn't look like a type. Note
-    # that we don't support straight string literals as type aliases
-    # (only string literals within index expressions).
-    if isinstance(node, RefExpr):
-        # Note that this misses the case where someone tried to use a
-        # class-referenced type variable as a type alias.  It's easier to catch
-        # that one in checkmember.py
-        if isinstance(node.node, TypeVarExpr):
-            api.fail('Type variable "{}" is invalid as target for type alias'.format(
-                node.fullname), node)
-            return None
-        if not (isinstance(node.node, TypeInfo) or
-                node.fullname in ('typing.Any', 'typing.Tuple', 'typing.Callable') or
-                isinstance(node.node, TypeAlias)):
-            return None
-    elif isinstance(node, IndexExpr):
-        base = node.base
-        if isinstance(base, RefExpr):
-            if not (isinstance(base.node, TypeInfo) or
-                    base.fullname in type_constructors or
-                    isinstance(base.node, TypeAlias)):
-                return None
-            # Enums can't be generic, and without this check we may incorrectly interpret indexing
-            # an Enum class as creating a type alias.
-            if isinstance(base.node, TypeInfo) and base.node.is_enum:
-                return None
-        else:
-            return None
-    elif isinstance(node, CallExpr):
-        if (isinstance(node.callee, NameExpr) and len(node.args) == 1 and
-                isinstance(node.args[0], NameExpr)):
-            call = api.lookup_qualified(node.callee.name, node.callee)
-            arg = api.lookup_qualified(node.args[0].name, node.args[0])
-            if (call is not None and call.node and call.node.fullname() == 'builtins.type' and
-                    arg is not None and arg.node and arg.node.fullname() == 'builtins.None'):
-                return NoneTyp(), set()
-            return None
-        return None
-    else:
-        return None
-
-    # It's a type alias (though it may be an invalid one).
     try:
         type = expr_to_unanalyzed_type(node)
     except TypeTranslationError:
@@ -414,14 +372,10 @@ def analyze_unbound_type_without_type_info(self, t: UnboundType, sym: SymbolTabl
                         (not self.tvar_scope or self.tvar_scope.get_binding(sym) is None))
         if self.allow_unbound_tvars and unbound_tvar and not self.third_pass:
             return t
+
         # None of the above options worked, we give up.
-        # NOTE: 'final_iteration' is iteration when we hit the maximum number of iterations limit.
-        if unbound_tvar or self.api.final_iteration:
-            # TODO: This is problematic, since we will have to wait until the maximum number
-            #       of iterations to report an invalid type.
-            self.fail('Invalid type "{}"'.format(name), t)
-        else:
-            self.api.defer()
+        self.fail('Invalid type "{}"'.format(name), t)
+
         if self.third_pass and isinstance(sym.node, TypeVarExpr):
             self.note_func("Forward references to type variables are prohibited", t)
             return AnyType(TypeOfAny.from_error)

mypy/newsemanal/typeanal.py

@@ -1418,11 +1418,13 @@ class NameExpr(RefExpr):
     This refers to a local name, global name or a module.
     """
 
-    __slots__ = ('name',)
+    __slots__ = ('name', 'is_special_form')
 
     def __init__(self, name: str) -> None:
         super().__init__()
         self.name = name  # Name referred to (may be qualified)
+        # Is this a l.h.s. of a special form assignment like typed dict or type variable?
+        self.is_special_form = False
 
     def accept(self, visitor: ExpressionVisitor[T]) -> T:
         return visitor.visit_name_expr(self)

mypy/nodes.py

@@ -345,7 +345,9 @@ def visit_star_expr(self, o: 'mypy.nodes.StarExpr') -> str:
         return self.dump([o.expr], o)
 
     def visit_name_expr(self, o: 'mypy.nodes.NameExpr') -> str:
-        pretty = self.pretty_name(o.name, o.kind, o.fullname, o.is_inferred_def, o.node)
+        pretty = self.pretty_name(o.name, o.kind, o.fullname,
+                                  o.is_inferred_def or o.is_special_form,
+                                  o.node)
         if isinstance(o.node, mypy.nodes.Var) and o.node.is_final:
             pretty += ' = {}'.format(o.node.final_value)
         return short_type(o) + '(' + pretty + ')'

mypy/strconv.py

@@ -15,10 +15,8 @@
     'check-incremental.test',
     'check-literal.test',
     'check-modules.test',
-    'check-newtype.test',
     'check-overloading.test',
     'check-python2.test',
-    'check-semanal-error.test',
     'check-statements.test',
     'check-unions.test',
     'check-unreachable-code.test',