Improve "Name already defined" error message (python#5067)

In semanal.py, name_already_defined takes an optional original_ctx: Optional[SymbolTableNode] argument which is used to print the line number of the original context where a name was defined already.

In many cases, the code doesn't pass anything for this argument, even though there is an original context that makes sense. This PR does this for the name_already_defined call in add_symbol, as discussed in python#4722.

mypy/semanal.py

@@ -395,7 +395,8 @@ def _visit_func_def(self, defn: FuncDef) -> None:
                         # Redefinition. Conditional redefinition is okay.
                         n = self.type.names[defn.name()].node
                         if not self.set_original_def(n, defn):
-                            self.name_already_defined(defn.name(), defn)
+                            self.name_already_defined(defn.name(), defn,
+                                                      self.type.names[defn.name()])
                     self.type.names[defn.name()] = SymbolTableNode(MDEF, defn)
                 self.prepare_method_signature(defn, self.type)
             elif self.is_func_scope():
@@ -406,7 +407,8 @@ def _visit_func_def(self, defn: FuncDef) -> None:
                         # Redefinition. Conditional redefinition is okay.
                         n = self.locals[-1][defn.name()].node
                         if not self.set_original_def(n, defn):
-                            self.name_already_defined(defn.name(), defn)
+                            self.name_already_defined(defn.name(), defn,
+                                                      self.locals[-1][defn.name()])
                     else:
                         self.add_local(defn, defn)
             else:
@@ -554,9 +556,9 @@ def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None:
                                       "must come last", defn.items[idx])
                 else:
                     for idx in non_overload_indexes[1:]:
-                        self.name_already_defined(defn.name(), defn.items[idx])
+                        self.name_already_defined(defn.name(), defn.items[idx], first_item)
                     if defn.impl:
-                        self.name_already_defined(defn.name(), defn.impl)
+                        self.name_already_defined(defn.name(), defn.impl, first_item)
                 # Remove the non-overloads
                 for idx in reversed(non_overload_indexes):
                     del defn.items[idx]
@@ -1851,7 +1853,19 @@ def analyze_lvalue(self, lval: Lvalue, nested: bool = False,
                 self.type.names[lval.name] = SymbolTableNode(MDEF, v)
             elif explicit_type:
                 # Don't re-bind types
-                self.name_already_defined(lval.name, lval)
+                global_def = self.globals.get(lval.name)
+                if self.locals:
+                    locals_last = self.locals[-1]
+                    if locals_last:
+                        local_def = locals_last.get(lval.name)
+                    else:
+                        local_def = None
+                else:
+                    local_def = None
+                type_def = self.type.names.get(lval.name) if self.type else None
+
+                original_def = global_def or local_def or type_def
+                self.name_already_defined(lval.name, lval, original_def)
             else:
                 # Bind to an existing name.
                 lval.accept(self)
@@ -3180,7 +3194,7 @@ def add_symbol(self, name: str, node: SymbolTableNode,
                 # Flag redefinition unless this is a reimport of a module.
                 if not (node.kind == MODULE_REF and
                         self.locals[-1][name].node == node.node):
-                    self.name_already_defined(name, context)
+                    self.name_already_defined(name, context, self.locals[-1][name])
             self.locals[-1][name] = node
         elif self.type:
             self.type.names[name] = node
@@ -3197,14 +3211,14 @@ def add_symbol(self, name: str, node: SymbolTableNode,
                 if existing.type and node.type and is_same_type(existing.type, node.type):
                     ok = True
                 if not ok:
-                    self.name_already_defined(name, context)
+                    self.name_already_defined(name, context, existing)
             self.globals[name] = node
 
     def add_local(self, node: Union[Var, FuncDef, OverloadedFuncDef], ctx: Context) -> None:
         assert self.locals[-1] is not None, "Should not add locals outside a function"
         name = node.name()
         if name in self.locals[-1]:
-            self.name_already_defined(name, ctx)
+            self.name_already_defined(name, ctx, self.locals[-1][name])
         node._fullname = name
         self.locals[-1][name] = SymbolTableNode(LDEF, node)
 
@@ -3238,14 +3252,21 @@ def name_not_defined(self, name: str, ctx: Context) -> None:
                 self.add_fixture_note(fullname, ctx)
 
     def name_already_defined(self, name: str, ctx: Context,
-                             original_ctx: Optional[SymbolTableNode] = None) -> None:
-        if original_ctx:
-            if original_ctx.node and original_ctx.node.get_line() != -1:
-                extra_msg = ' on line {}'.format(original_ctx.node.get_line())
-            else:
-                extra_msg = ' (possibly by an import)'
+                    original_ctx: Optional[Union[SymbolTableNode, SymbolNode]] = None) -> None:
+        if isinstance(original_ctx, SymbolTableNode):
+            node = original_ctx.node
+        elif isinstance(original_ctx, SymbolNode):
+            node = original_ctx
+
+        if isinstance(original_ctx, SymbolTableNode) and original_ctx.kind == MODULE_REF:
+            # Since this is an import, original_ctx.node points to the module definition.
+            # Therefore its line number is always 1, which is not useful for this
+            # error message.
+            extra_msg = ' (by an import)'
+        elif node and node.line != -1:
+            extra_msg = ' on line {}'.format(node.line)
         else:
-            extra_msg = ''
+            extra_msg = ' (possibly by an import)'
         self.fail("Name '{}' already defined{}".format(name, extra_msg), ctx)
 
     def fail(self, msg: str, ctx: Context, serious: bool = False, *,

mypy/semanal_pass1.py

@@ -337,7 +337,7 @@ def add_symbol(self, name: str, node: SymbolTableNode,
                 # Flag redefinition unless this is a reimport of a module.
                 if not (node.kind == MODULE_REF and
                         self.sem.locals[-1][name].node == node.node):
-                    self.sem.name_already_defined(name, context)
+                    self.sem.name_already_defined(name, context, self.sem.locals[-1][name])
             self.sem.locals[-1][name] = node
         else:
             assert self.sem.type is None  # Pass 1 doesn't look inside classes
@@ -353,6 +353,6 @@ def add_symbol(self, name: str, node: SymbolTableNode,
                 if existing.type and node.type and is_same_type(existing.type, node.type):
                     ok = True
                 if not ok:
-                    self.sem.name_already_defined(name, context)
+                    self.sem.name_already_defined(name, context, existing)
             elif not existing:
                 self.sem.globals[name] = node

test-data/unit/check-attr.test

@@ -721,13 +721,13 @@ reveal_type(A)  # E: Revealed type is 'def (b: Any, a: Any) -> __main__.A'
 class B:
    a: int = attr.ib(default=8)
    b: int = attr.ib()
-   a: int = attr.ib()  # E: Name 'a' already defined
+   a: int = attr.ib()  # E: Name 'a' already defined on line 10
 reveal_type(B)  # E: Revealed type is 'def (b: builtins.int, a: builtins.int) -> __main__.B'
 @attr.s(auto_attribs=True)
 class C:
    a: int = 8
    b: int
-   a: int = attr.ib()  # E: Name 'a' already defined
+   a: int = attr.ib()  # E: Name 'a' already defined on line 16
 reveal_type(C)  # E: Revealed type is 'def (a: builtins.int, b: builtins.int) -> __main__.C'
 [builtins fixtures/bool.pyi]
 

test-data/unit/check-class-namedtuple.test

@@ -598,16 +598,16 @@ class AnnotationsAsAMethod(NamedTuple):
 
 class ReuseNames(NamedTuple):
     x: int
-    def x(self) -> str:  # E: Name 'x' already defined
+    def x(self) -> str:  # E: Name 'x' already defined on line 22
         return ''
 
     def y(self) -> int:
         return 0
-    y: str  # E: Name 'y' already defined
+    y: str  # E: Name 'y' already defined on line 26
 
 class ReuseCallableNamed(NamedTuple):
     z: Callable[[ReuseNames], int]
-    def z(self) -> int:  # E: Name 'z' already defined
+    def z(self) -> int:  # E: Name 'z' already defined on line 31
         return 0
 
 [builtins fixtures/dict.pyi]

test-data/unit/check-functions.test

@@ -1310,7 +1310,7 @@ if x:
     def f(x: int) -> None: pass
 else:
     # TODO: This should be okay.
-    @dec # E: Name 'f' already defined
+    @dec # E: Name 'f' already defined on line 7
     def f(): pass
 
 [case testConditionalFunctionDefinitionUsingDecorator4]
@@ -1323,7 +1323,7 @@ if x:
     def f(x: str) -> None: pass
 else:
     # TODO: We should report an incompatible redefinition.
-    @dec # E: Name 'f' already defined
+    @dec # E: Name 'f' already defined on line 7
     def f(): pass
 
 [case testConditionalRedefinitionOfAnUnconditionalFunctionDefinition1]
@@ -1490,7 +1490,7 @@ x = None # type: Any
 class A:
     if x:
         def f(self): pass
-    def f(self): pass # E: Name 'f' already defined
+    def f(self): pass # E: Name 'f' already defined on line 5
 
 [case testIncompatibleConditionalMethodDefinition]
 from typing import Any

test-data/unit/check-incremental.test

@@ -1020,7 +1020,7 @@ import a.b
 [rechecked b]
 [stale]
 [out2]
-tmp/b.py:4: error: Name 'a' already defined
+tmp/b.py:4: error: Name 'a' already defined on line 3
 
 [case testIncrementalSilentImportsAndImportsInClass]
 # flags: --ignore-missing-imports

test-data/unit/check-newsyntax.test

@@ -19,7 +19,7 @@ a = 5  # E: Incompatible types in assignment (expression has type "int", variabl
 b: str = 5  # E: Incompatible types in assignment (expression has type "int", variable has type "str")
 
 zzz: int
-zzz: str  # E: Name 'zzz' already defined
+zzz: str  # E: Name 'zzz' already defined on line 10
 [out]
 
 [case testNewSyntaxWithDict]

test-data/unit/check-overloading.test

@@ -9,18 +9,18 @@ f(0)
 
 @overload  # E: Name 'overload' is not defined
 def g(a:int): pass
-def g(a): pass  # E: Name 'g' already defined
+def g(a): pass  # E: Name 'g' already defined on line 8
 g(0)
 
 @something  # E: Name 'something' is not defined
 def r(a:int): pass
-def r(a): pass  # E: Name 'r' already defined
+def r(a): pass  # E: Name 'r' already defined on line 13
 r(0)
 [out]
 main:1: error: Name 'overload' is not defined
-main:3: error: Name 'f' already defined
+main:3: error: Name 'f' already defined on line 1
 main:3: error: Name 'overload' is not defined
-main:5: error: Name 'f' already defined
+main:5: error: Name 'f' already defined on line 1
 
 [case testTypeCheckOverloadWithImplementation]
 from typing import overload, Any
@@ -143,9 +143,9 @@ def deco(fun): ...
 
 @deco
 def f(x: 'A') -> 'B': ...
-@deco  # E: Name 'f' already defined
+@deco  # E: Name 'f' already defined on line 5
 def f(x: 'B') -> 'A': ...
-@deco  # E: Name 'f' already defined
+@deco  # E: Name 'f' already defined on line 5
 def f(x: Any) -> Any: ...
 
 class A: pass
@@ -1089,7 +1089,7 @@ def f(a: int) -> None: pass
 @overload
 def f(a: str) -> None: pass
 [out]
-tmp/foo.pyi:3: error: Name 'f' already defined
+tmp/foo.pyi:3: error: Name 'f' already defined on line 2
 tmp/foo.pyi:3: error: Single overload definition, multiple required
 
 [case testNonconsecutiveOverloads]
@@ -1103,7 +1103,7 @@ def f(a: int) -> None: pass
 def f(a: str) -> None: pass
 [out]
 tmp/foo.pyi:2: error: Single overload definition, multiple required
-tmp/foo.pyi:5: error: Name 'f' already defined
+tmp/foo.pyi:5: error: Name 'f' already defined on line 2
 tmp/foo.pyi:5: error: Single overload definition, multiple required
 
 [case testNonconsecutiveOverloadsMissingFirstOverload]
@@ -1115,7 +1115,7 @@ def f(a: int) -> None: pass
 @overload
 def f(a: str) -> None: pass
 [out]
-tmp/foo.pyi:4: error: Name 'f' already defined
+tmp/foo.pyi:4: error: Name 'f' already defined on line 2
 tmp/foo.pyi:4: error: Single overload definition, multiple required
 
 [case testNonconsecutiveOverloadsMissingLaterOverload]
@@ -1173,7 +1173,7 @@ class Test(object):
     def do_chain(self) -> int:
         return 2
 
-    @do_chain.chain  # E: Name 'do_chain' already defined
+    @do_chain.chain  # E: Name 'do_chain' already defined on line 10
     def do_chain(self) -> int:
         return 3
 

test-data/unit/check-unsupported.test

@@ -13,5 +13,5 @@ def g(): pass
 @d # E
 def g(x): pass
 [out]
-tmp/foo.pyi:5: error: Name 'f' already defined
-tmp/foo.pyi:7: error: Name 'g' already defined
+tmp/foo.pyi:5: error: Name 'f' already defined on line 3
+tmp/foo.pyi:7: error: Name 'g' already defined on line 6

test-data/unit/semanal-errors.test

@@ -141,7 +141,7 @@ class A: pass
 x = 0 # type: A
 x = 0 # type: A
 [out]
-main:4: error: Name 'x' already defined
+main:4: error: Name 'x' already defined on line 3
 
 [case testLocalVarRedefinition]
 import typing
@@ -150,15 +150,15 @@ def f() -> None:
   x = 0 # type: A
   x = 0 # type: A
 [out]
-main:5: error: Name 'x' already defined
+main:5: error: Name 'x' already defined on line 4
 
 [case testClassVarRedefinition]
 import typing
 class A:
   x = 0 # type: object
   x = 0 # type: object
 [out]
-main:4: error: Name 'x' already defined
+main:4: error: Name 'x' already defined on line 3
 
 [case testMultipleClassDefinitions]
 import typing
@@ -551,7 +551,7 @@ class A:
   def g(self) -> None: pass
   def f(self, x: object) -> None: pass
 [out]
-main:5: error: Name 'f' already defined
+main:5: error: Name 'f' already defined on line 3
 
 [case testInvalidGlobalDecl]
 import typing
@@ -640,7 +640,7 @@ def f(x) -> None:
     x = 1
     def g(): pass
 [out]
-main:5: error: Name 'g' already defined
+main:5: error: Name 'g' already defined on line 3
 
 [case testRedefinedOverloadedFunction]
 from typing import overload, Any
@@ -653,7 +653,7 @@ def f() -> None:
     def p(): pass # fail
 [out]
 main:3: error: An overloaded function outside a stub file must have an implementation
-main:8: error: Name 'p' already defined
+main:8: error: Name 'p' already defined on line 3
 
 [case testNestedFunctionInMethod]
 import typing
@@ -1049,7 +1049,7 @@ class t: pass # E: Name 't' already defined on line 2
 [case testRedefineTypevar4]
 from typing import TypeVar
 t = TypeVar('t')
-from typing import Generic as t # E: Name 't' already defined
+from typing import Generic as t # E: Name 't' already defined on line 2
 [out]
 
 [case testInvalidStrLiteralType]
@@ -1083,7 +1083,7 @@ from typing import overload
 def dec(x): pass
 @dec
 def f(): pass
-@dec  # E: Name 'f' already defined
+@dec  # E: Name 'f' already defined on line 3
 def f(): pass
 [out]
 
@@ -1180,7 +1180,7 @@ class A:
 import typing
 def f() -> None:
     import x
-    import y as x # E: Name 'x' already defined
+    import y as x # E: Name 'x' already defined (by an import)
     x.y
 [file x.py]
 y = 1
@@ -1190,7 +1190,7 @@ y = 1
 [case testImportTwoModulesWithSameNameInGlobalContext]
 import typing
 import x
-import y as x # E: Name 'x' already defined
+import y as x # E: Name 'x' already defined (by an import)
 x.y
 [file x.py]
 y = 1
@@ -1328,8 +1328,8 @@ a = 's'  # type: str
 a = ('spam', 'spam', 'eggs', 'spam')  # type: Tuple[str]
 
 [out]
-main:3: error: Name 'a' already defined
-main:4: error: Name 'a' already defined
+main:3: error: Name 'a' already defined on line 2
+main:4: error: Name 'a' already defined on line 2
 
 [case testDuplicateDefFromImport]
 from m import A
@@ -1347,7 +1347,7 @@ def dec(x: Any) -> Any:
 @dec
 def f() -> None:
     pass
-@dec  # E: Name 'f' already defined
+@dec  # E: Name 'f' already defined on line 4
 def f() -> None:
     pass
 [out]