Skip to content

Improve "Name already defined" error message #5067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 28, 2018
Merged

Improve "Name already defined" error message #5067

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
33c673c
Print more context in add_symbol if name already defined
sid-kap May 17, 2018
f0c91b3
Add line number logging in more places, update test outputs
sid-kap May 17, 2018
d99eb04
Fix one more test
sid-kap May 17, 2018
5cfb4dc
Add line numbers for all "Name already defined" errors, update tests
sid-kap May 17, 2018
880d6ba
Fix more tests
sid-kap May 17, 2018
12d5111
Fix two more tests
sid-kap May 17, 2018
cfa3408
All tests passing now, address @ilevkivskyi's feedback
sid-kap May 21, 2018
4a67f14
Add '(possibly by an import)' back to error message
sid-kap May 27, 2018
7d4ef29
Fix lint warnings
sid-kap May 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions mypy/semanal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand All @@ -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()])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you missed one more case: global function definition, see else: # Top-level function below. You will likely need to patch also self.check_no_global in the same way you do above for self.name_already defined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think self.check_no_global already does the right thing?

mypy/mypy/semanal.py

Line 3225 in 7eebb8f

self.name_already_defined(n, ctx, self.globals[n])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if the previous context is always correct in check_no_global, then no action is required here.

else:
self.add_local(defn, defn)
else:
Expand Down Expand Up @@ -552,9 +554,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]
Expand Down Expand Up @@ -1848,7 +1850,12 @@ 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)
local_def = cast(SymbolTable, self.locals[-1]).get(lval.name) if self.locals and self.locals[-1] else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you rewrite it using an if statement, then the cast will be unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

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)
Expand Down Expand Up @@ -3172,7 +3179,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
Expand All @@ -3189,14 +3196,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)

Expand Down Expand Up @@ -3230,10 +3237,16 @@ 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:
original_ctx: Optional[Union[SymbolTableNode, SymbolNode]] = None) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it is not a good idea to use a union here, but it seems to me the old signature was "wrong", so it is fine in this case. Maybe you can add a short comment about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old signature was actually correct, the function used to only be called with SymbolTableNodes. I added the option for passing a SymbolNode instead because in _visit_overloaded_func_def I only have a SymbolNode to pass it, not a SymbolTableNode.

Copy link
Contributor Author

@sid-kap sid-kap May 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess to fix this, the options are

  • take Optional[SymbolNode] instead
  • take Optiona[SymbolTableNode] only, and in _visit_overloaded_func_def create a SymbolTableNode to pass to the function

Which do you think makes more sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that SymbolTableNode is logically "wrong" because it is just an entry in the symbol table wrapper, while SymbolNode is an actual error context (well to be more precise, SymbolNodes are semantic objects, not syntactic, but at least they are closer, and should ideally point to the line of syntax node defining them). So I vote for SymbolNode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you might need .kind == MODULE_REF which is only available on SymbolTableNode. Potentially, this can be replaced by isinstance(node, (MypyFile, ImportedName)), but I think it is better to just leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll leave it as-is then.

if original_ctx:
if original_ctx.node and original_ctx.node.get_line() != -1:
extra_msg = ' on line {}'.format(original_ctx.node.get_line())
if isinstance(original_ctx, SymbolTableNode):
node = original_ctx.node
elif isinstance(original_ctx, SymbolNode):
node = original_ctx
else:
node = None
if node and node.line != -1:
extra_msg = ' on line {}'.format(node.line)
else:
extra_msg = ' (possibly by an import)'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have enough tests for this case? If not you can add one more.

Copy link
Contributor Author

@sid-kap sid-kap May 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure when this would happen (when node is None or node.line is -1). There don't seem to be any tests covering it, so maybe this never happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looks like there was one test where this happened. Do you know in what situations this happens? If so, I could add tests for those cases.

Copy link
Member

@ilevkivskyi ilevkivskyi May 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might happen in case of an error or an unresolved circular import. If you can't find a simple additional test case then ignore this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, looks like there's already a test case for that situation (testDuplicateDefFromImport in semanal-errors.test). So I guess we can leave this as-is.

else:
Expand Down
4 changes: 2 additions & 2 deletions mypy/semanal_pass1.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
4 changes: 2 additions & 2 deletions test-data/unit/check-attr.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
6 changes: 3 additions & 3 deletions test-data/unit/check-class-namedtuple.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
6 changes: 3 additions & 3 deletions test-data/unit/check-functions.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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]
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test-data/unit/check-incremental.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test-data/unit/check-newsyntax.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
20 changes: 10 additions & 10 deletions test-data/unit/check-overloading.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand Down Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions test-data/unit/check-unsupported.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
26 changes: 13 additions & 13 deletions test-data/unit/semanal-errors.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]

Expand Down Expand Up @@ -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 on line 3
x.y
[file x.py]
y = 1
Expand All @@ -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 on line 2
x.y
[file x.py]
y = 1
Expand Down Expand Up @@ -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
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, good job updating all these error messages! 👍

def f() -> None:
pass
[out]
Expand Down