Skip to content
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

false report of multiple definitions #649

Closed
rbtcollins opened this issue Apr 20, 2015 · 18 comments
Closed

false report of multiple definitions #649

rbtcollins opened this issue Apr 20, 2015 · 18 comments
Labels
bug mypy got something wrong

Comments

@rbtcollins
Copy link
Member

The following code in six fails to type check:

try:
    advance_iterator = next
except NameError:
    def advance_iterator(it):
        return it.next()
next = advance_iterator

.../lib/python3.4/site-packages/six.py, line 501: Name 'advance_iterator' already defined

But advance_iterator can't be already defined: for NameError to be thrown, the lookup of next must have failed.

Whats the right way to handle this? Just mask it?

@refi64
Copy link
Contributor

refi64 commented Apr 20, 2015

This is a hack...but, whenever I encounter stuff like this, I do:

try:
    advance_iterator = next
except NameError:
    globals['advance_iterator'] = lambda it: it.next()
    next = advance_iterator

@JukkaL JukkaL added the bug mypy got something wrong label Apr 29, 2015
@JukkaL
Copy link
Collaborator

JukkaL commented Apr 29, 2015

This is a known, long-standing issue. Conditional definitions are often rejected because mypy is too picky.

A potential fix would be to treat def more like an assignment, rather than a special 'function definition'.

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 25, 2015

Another, slightly different example:

if condition:
    from module import name
else:
    name = None

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 25, 2015

Merging with #212, which is similar.

JukkaL added a commit that referenced this issue Nov 27, 2015
Work towards #649. Still only addresses a subset of
issues.
@JukkaL
Copy link
Collaborator

JukkaL commented Nov 27, 2015

Now some more conditional definitions are accepted by mypy. These are all okay:

try:
    from m import CONST
except ImportError:
    CONST = 'const'
try:
    from m import f
except ImportError:
    def f(): ...
def f(): ...
try:
    from m import f
except ImportError: pass

There's still a lot more work to do before mypy can deal with all interesting cases, but this should cover a good fraction of cases that used to fail.

@gvanrossum
Copy link
Member

Thanks, it's a bit better. There's still this case that doesn't seem to
work right:

import sys
if sys.platform == 'darwin':
    import os
else:
    os = None

I get an error on each line mentioning os:

/Users/guido/mypy_tests/mypy_imp.py:3: error: Need type annotation for
variable
/Users/guido/mypy_tests/mypy_imp.py:5: error: Name 'os' already defined

On Fri, Nov 27, 2015 at 1:58 PM, Jukka Lehtosalo notifications@github.com
wrote:

Now some more conditional definitions are accepted by mypy. These are all
okay:

try:
from m import CONST
except ImportError:
CONST = 'const'

try:
from m import f
except ImportError:
def f(): ...

def f(): ...
try:
from m import f
except ImportError: pass

There's still a lot more work to do before mypy can deal with all
interesting cases, but this should cover a good fraction of cases that used
to fail.


Reply to this email directly or view it on GitHub
#649 (comment).

--Guido van Rossum (python.org/~guido)

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 28, 2015

That should be pretty easy to support as well.

@gvanrossum
Copy link
Member

gvanrossum commented Nov 28, 2015 via email

@gvanrossum
Copy link
Member

OK, another case found in the wild:

if ignore_case:
    correct_case = None
else:
    def correct_case(args):  # E: error: Name 'correct_case' already defined
        ...

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 11, 2015

I also have a long, unprocessed list of related issues from the Python 2.7 std library that I should analyze one of these days.

JukkaL added a commit that referenced this issue Dec 31, 2015
@JukkaL
Copy link
Collaborator

JukkaL commented Dec 31, 2015

These now work:

if x:
    def f(): ...
else:
    f = g
try:
    from m import f
except ...:
    f = None

These still don't work:

if x:
    f = g
else:
    def f(): ...
if x:
    import m
else:
    m = None
if x:
    m = None
else:
     import m

@gvanrossum
Copy link
Member

Here's another one that still doesn't work:

if x:
    f = None
else:
    def f(): ...

Since it's the exact opposite of one that does work, how hard would it be
to make it work?

(In general I am beginning to become more interested in more symmetrical
unification of branches when the type info gathered from one branch is of a
clearly subordinate nature -- similar to x = [] if cond else [1, 2, 3],
which IMO should infer a type of List[int] for x regardless of context.)

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 6, 2016

I don't expect that supporting the f = None / def f(): ... case would be hard once we support f = g / def f(): ....

[Here's some background for why these are hard. Mypy treats function definitions (and modules and classes) as special and different from variables. This is important for modules as it allows expressions like mod.ClassName to be bound early during semantic analysis, but less so for functions. We could unify functions and variables, but it would be pretty big change and there are some benefits to how things work currently.]

@gvanrossum
Copy link
Member

gvanrossum commented Jan 6, 2016 via email

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 8, 2016

Treating a function definition like an assignment if it's already defined as a variable would work. Consider code like this:

if x:
    f = None   # f variable
else:
    def f(): ...

We would treat it like this code:

if x:
    f = None
else:
    def _xyz(): ...   # _xyz must be unique
    f = _xyz

JukkaL added a commit that referenced this issue Jan 8, 2016
JukkaL added a commit that referenced this issue Jan 8, 2016
@JukkaL
Copy link
Collaborator

JukkaL commented Jan 8, 2016

These now work:

if x:
    f = g
else:
    def f(): ...


if x:
    f = None
else:
    def f(): ...

if x:
    import m
else:
    m = None

The m = None / import m case is harder to support. I gave it a try but we should have a new type for modules for that.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 8, 2016

Okay, getting back to the original reported issue -- the fixes above don't actually solve it, as it also has the feature of both using a builtin in a module and redefining it. However, now there is fairly simple (though perhaps quite non-obvious) workaround:

try:
    advance_iterator = __builtins__.next    # Note __builtins__!
except NameError:
    def advance_iterator(it):
        return it.next()
next = advance_iterator

As we've pretty much implemented all the low hanging fruit improvements, I'm downgrading priority. Let me know if there are still common issues and I'll bump priority again.

We could also close this issue and create separate issues for the remaining unsupported cases.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 8, 2016

Closing this now, even though this is not fully fixed. The above issues cover some remaining missing features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

No branches or pull requests

4 participants