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.

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

Improve "Name already defined" error message #5067

Merged
merged 9 commits into from May 28, 2018

Conversation

Projects
None yet
3 participants
@sid-kap
Contributor

sid-kap commented May 17, 2018

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 #4722.

A few questions if you guys think this is a good idea:

  • Should it also be done in add_symbol in semanal_pass1.py and semanal_pass3.py?
  • I only made this change to cover the example that @glenjamin brought up in #4722, but there are other places in semanal.py where adding this additional argument could be helpful:
    self.name_already_defined(name, context)

    self.name_already_defined(name, ctx)

    The function is also called with just two parameters in _visit_func_def, _visit_overloaded_func_def, and analyze_lvalue, but I'm not sure if there is a original_ctx that makes sense to pass there.
  • Should I add tests?
@JelleZijlstra

This comment has been minimized.

Collaborator

JelleZijlstra commented May 17, 2018

I think anywhere this message appears it's useful to give the original definition, if possible.

Tests would be good to add. You'll probably just have to change the existing tests though.

@sid-kap

This comment has been minimized.

Contributor

sid-kap commented May 17, 2018

Thanks for taking a look at this! I updated the tests to what I think should be the correct output, but in many cases the line numbers it outputs aren't what I expected (so the tests are currently failing). I'll have to figure out why these are wrong.

sid-kap added some commits May 17, 2018

@sid-kap

This comment has been minimized.

Contributor

sid-kap commented May 17, 2018

So my code is failing on two tests because it's reporting the wrong line numbers. For imports, node.line always seems to be 1, rather than the line where import was called. So it always says Name 'x' already defined on line 1 if x was originally defined using an import.

Is anyone familiar with this issue? It seems like I either need to detect that the original definition was an import, and don't report a line number, or maybe somehow figure out on which line the import happened.

@sid-kap

This comment has been minimized.

Contributor

sid-kap commented May 21, 2018

It looks like when SymbolTableNode is created in semanal.py for an import, it stores the node of the referenced module, not the node of the import statement:
https://github.com/python/mypy/blob/master/mypy/semanal.py#L1310-L1314
Therefore, symbol_table_node.node.line is the line of the imported module, which will always be 1.

So it seems that the options are either:

  1. add a field to SymbolTableNode to store the import line numbers (to only be used if kind is MODULE_REF), or
  2. before raising a "Name already defined" error, check if the original type is a module reference (i.e. check if existing.kind == MODULE_REF) and don't report the line number in that case.

Which would be the right thing to do? cc @JelleZijlstra

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented May 25, 2018

Which would be the right thing to do?

I would just not show the additional note in this case. Anyway, conditional imports will require special support which will probably add later. I will now make a review, sorry for waiting so long with this.

@sid-kap

This comment has been minimized.

Contributor

sid-kap commented May 25, 2018

No worries! I can make that change, the tests should pass after I fix that.

@ilevkivskyi

Thank you for working on this! The more detailed errors look definitely better.

@@ -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()])

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2018

Collaborator

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

This comment has been minimized.

@sid-kap

sid-kap May 26, 2018

Contributor

I think self.check_no_global already does the right thing?

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2018

Collaborator

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

@@ -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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2018

Collaborator

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

This comment has been minimized.

@sid-kap

sid-kap May 26, 2018

Contributor

Fixed!

@@ -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:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2018

Collaborator

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?

This comment has been minimized.

@sid-kap

sid-kap May 26, 2018

Contributor

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.

This comment has been minimized.

@sid-kap

sid-kap May 26, 2018

Contributor

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?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2018

Collaborator

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2018

Collaborator

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.

This comment has been minimized.

@sid-kap

sid-kap May 28, 2018

Contributor

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

else:
node = None
if node and node.line != -1:
extra_msg = ' on line {}'.format(node.line)
else:
extra_msg = ' (possibly by an import)'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2018

Collaborator

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

This comment has been minimized.

@sid-kap

sid-kap May 26, 2018

Contributor

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?

This comment has been minimized.

@sid-kap

sid-kap May 27, 2018

Contributor

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2018

Collaborator

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.

This comment has been minimized.

@sid-kap

sid-kap May 28, 2018

Contributor

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.

@@ -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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2018

Collaborator

Wow, good job updating all these error messages! 👍

@ilevkivskyi

This is almost ready, I left few more suggestions.

self.name_already_defined(lval.name, lval)
global_def = self.globals.get(lval.name)
if self.locals:
locals_last = self.locals[-1]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2018

Collaborator

@gvanrossum probably will be happy to see another case in mypy for PEP 572 :-)

@sid-kap You can ignore this comment.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented May 28, 2018

OK, it looks like the comments I added in this round are optional, so this can be merged as soon as you fix the lint error reported by Travis.

@ilevkivskyi

LGTM now!

@ilevkivskyi ilevkivskyi merged commit 0447473 into python:master May 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment