python/mypy

false report of multiple definitions

rbtcollins opened this issue · 18 comments

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?

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

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'.

Another, slightly different example:

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

Merging with #212, which is similar.

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.

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)

That should be pretty easy to support as well.

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

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.

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

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.)

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.]

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

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.

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.

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