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

Add line # of previous definition (resubmit #3396) #3424

Merged
merged 7 commits into from Jun 16, 2017

Conversation

Projects
None yet
3 participants
@pkch
Contributor

pkch commented May 23, 2017

Partial fix for #1874: improve the error message. The duplicate definitions in different if/else branches are still not allowed (this is much harder to fix).

This is a resubmission of #3396 which caused the crash in #3415 and was reverted. The old PR is in commit 39c3058; after that, I added 2 more commits: the failing test that repros the crash, and the fix to it.

pkch added some commits May 22, 2017

@pkch

This comment has been minimized.

Show comment
Hide comment
@pkch

pkch May 23, 2017

Contributor

In case the context is None, or line number is -1, I put a message suggesting that the user look at imports. Is it ok? There could be other cases where this branch is hit that I don't list, but I thought this partial suggestion might still help.

Contributor

pkch commented May 23, 2017

In case the context is None, or line number is -1, I put a message suggesting that the user look at imports. Is it ok? There could be other cases where this branch is hit that I don't list, but I thought this partial suggestion might still help.

Show outdated Hide outdated mypy/semanal.py
if original_ctx.node and original_ctx.node.get_line() != -1:
extra_msg = ' on line {}'.format(original_ctx.node.get_line())
else:
extra_msg = ' (line unavailable; possibly an import)'

This comment has been minimized.

@JukkaL

JukkaL May 23, 2017

Collaborator

We could simplify the message a bit. What about just (possibly by an import). The 'line unavailable' part doesn't provide useful information really.

@JukkaL

JukkaL May 23, 2017

Collaborator

We could simplify the message a bit. What about just (possibly by an import). The 'line unavailable' part doesn't provide useful information really.

Show outdated Hide outdated test-data/unit/semanal-errors.test
class A:
pass
[out]
main:2: error: Name 'A' already defined (line unavailable; possibly an import)

This comment has been minimized.

@JukkaL

JukkaL May 23, 2017

Collaborator

Here are further ideas for test cases:

  • Original definition is a decorated function.
  • Original definition is an overloaded function.
  • Original definition is a named tuple definition.
  • Original definition is a typed dict definition.
  • Original definition is a module (import m).
@JukkaL

JukkaL May 23, 2017

Collaborator

Here are further ideas for test cases:

  • Original definition is a decorated function.
  • Original definition is an overloaded function.
  • Original definition is a named tuple definition.
  • Original definition is a typed dict definition.
  • Original definition is a module (import m).

This comment has been minimized.

@pkch

pkch May 23, 2017

Contributor

If a module is simply imported twice, there's no error message. Did you mean some other test?

@pkch

pkch May 23, 2017

Contributor

If a module is simply imported twice, there's no error message. Did you mean some other test?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

@pkch Maybe Jukka means something like this:

import re
re = 1
@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

@pkch Maybe Jukka means something like this:

import re
re = 1

This comment has been minimized.

@pkch

pkch May 24, 2017

Contributor

Yes, but that will cause a completely different error message (types are not compatible). It won't get to the point of complaining about duplicate definition.

@pkch

pkch May 24, 2017

Contributor

Yes, but that will cause a completely different error message (types are not compatible). It won't get to the point of complaining about duplicate definition.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

And what about

import re
import math
re = math
@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

And what about

import re
import math
re = math

This comment has been minimized.

@pkch

pkch May 24, 2017

Contributor

In this case, there's no error message at all (as expected?).

@pkch

pkch May 24, 2017

Contributor

In this case, there's no error message at all (as expected?).

@JukkaL

Almost there! Some style nits and ideas for additional test cases only.

Show outdated Hide outdated test-data/unit/semanal-errors.test
class A:
pass
[out]
main:2: error: Name 'A' already defined (possibly by an import)

This comment has been minimized.

@JukkaL

JukkaL May 31, 2017

Collaborator

Can you use # E: ... comments in these cases instead?

@JukkaL

JukkaL May 31, 2017

Collaborator

Can you use # E: ... comments in these cases instead?

Show outdated Hide outdated test-data/unit/semanal-errors.test
[out]
main:4: error: Name 'Point' already defined on line 2

This comment has been minimized.

@JukkaL

JukkaL May 31, 2017

Collaborator

Remove extra empty lines at the end of file.

@JukkaL

JukkaL May 31, 2017

Collaborator

Remove extra empty lines at the end of file.

Show outdated Hide outdated test-data/unit/semanal-errors.test
[builtins fixtures/dict.pyi]
[out]
main:4: error: Name 'Point' already defined on line 2

This comment has been minimized.

@JukkaL

JukkaL May 31, 2017

Collaborator

Suggestions for additional test cases:

  • import m followed by def m(): ...
  • T = TypeVar('T') followed by class T: ...
  • Ignored import (import m # type: ignore or from m import f # type: ignore) followed by definition.
  • Type alias like A = List[int] followed by a redefinition.
@JukkaL

JukkaL May 31, 2017

Collaborator

Suggestions for additional test cases:

  • import m followed by def m(): ...
  • T = TypeVar('T') followed by class T: ...
  • Ignored import (import m # type: ignore or from m import f # type: ignore) followed by definition.
  • Type alias like A = List[int] followed by a redefinition.
Show outdated Hide outdated test-data/unit/semanal-errors.test
pass
[out]
main:12: error: Name 'f' already defined on line 3

This comment has been minimized.

@JukkaL

JukkaL May 31, 2017

Collaborator

Remove second empty line (here and below).

@JukkaL

JukkaL May 31, 2017

Collaborator

Remove second empty line (here and below).

CR
@pkch

This comment has been minimized.

Show comment
Hide comment
@pkch

pkch Jun 8, 2017

Contributor

This may be ready to merge (the last CR fixes was only to add more tests, no code changes).

Contributor

pkch commented Jun 8, 2017

This may be ready to merge (the last CR fixes was only to add more tests, no code changes).

@JukkaL

JukkaL approved these changes Jun 16, 2017

Looks good now. Thanks for the updates!

@JukkaL JukkaL merged commit 94e3f9c into python:master Jun 16, 2017

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