Mypy: Inner method discards type narrowing done in outer method

[euresti]

So everybody knows not to write
def foo(arg={'bar': 'baz'}):

So instead one usually writes something like:

def foo(arg=None):
    if arg is None:
        arg = {'bar', 'baz'}

Now add some types:

def func(arg=None):
    # type: (Dict[str, str]) -> Dict[str,str]
    reveal_type(arg)  # Revealed type is 'Union[builtins.dict[builtins.str, builtins.str], builtins.None]'
    if arg is None:
        arg = {'foo': 'bar'}
    reveal_type(arg)  #  Revealed type is 'builtins.dict[builtins.str, builtins.str]'
    return arg

So all is good, unless you try to create an inner method:

def func(arg=None):
    # type: (Dict[str, str]) -> Dict[str,str]
    reveal_type(arg)  # Revealed type is 'Union[builtins.dict[builtins.str, builtins.str], builtins.None]'
    if arg is None:
        arg = {'foo': 'bar'}
    reveal_type(arg)  #  Revealed type is 'builtins.dict[builtins.str, builtins.str]'

    def inner():
        # type: () -> Dict[str, str]
        reveal_type(arg)  # Revealed type is 'Union[builtins.dict[builtins.str, builtins.str], builtins.None]'
        return arg  # Incompatible return value type (got "Optional[Dict[str, str]]", expected Dict[str, str])

    return inner()

I guess there are lots of workarounds (assign to temporary variable, pull out the inner method, pass arguments into the inner) but they all require editing existing code which is always scary. Hmm. I guess the easiest thing is to just add an assert arg is not None in the inner. That's not too intrusive, anyway thought you might want to know.

[gvanrossum]

This looks like the same as #2145 which was closed because the binder doesn't keep track of this -- maybe we should reopen that? Or at least keep this one open.

[JukkaL]

Let's keep this open.

[ilevkivskyi]

(Rising priority to normal, since this appeared third time.)

[gvanrossum]

How should we do this? I think it's safe to trust the narrower type if it
prevails over the entire rest of the (outer) function -- maybe that's
enough? After all the "None" is an artifact of the idiom for mutable
default. (I almost wish there was a way to say "this None isn't really
here" like we have at the class level.)

[carljm]

FWIW you can work around this by early-binding arg to the inner function as an argument default (def inner(a=arg):).

And yes, I think it’s safe to trust the narrowed type iff it applies to the entire rest of the outer function.

[ilevkivskyi]

And yes, I think it’s safe to trust the narrowed type iff it applies to the entire rest of the outer function.

+1 also from me.

[ilevkivskyi]

(I am raising priority to high since this appears quite often.)

[gvanrossum]

Can we change the topic of this (important) issue to something more descriptive?

[euresti]

Hope this helps!

[sergeyk]

Ran into this today, here's another minimal example:

def test(foo: 'Foo' = None) -> str:
    if foo is None:
        foo = Foo()
    return foo.bar()
# mypy reports no error


def test2(foo: 'Foo' = None) -> str:
    if foo is None:
        foo = Foo()
    def inner_test() -> str:
        return foo.bar()  # mypy:error        Item "None" of "Optional[Foo]" has no attribute "bar"
    return inner_test()


class Foo:
    def bar(self) -> str:
        return 'bar'

[ilevkivskyi]

Another example (with a subclass narrowing) is https://github.com/python/mypy/issues/4679

[mthuurne]

I'm running into this as well. I tried to work around it by capturing the value from the outer context as the default value for an inner parameter, as @carljm suggested above, but that doesn't help:

from typing import Optional

def outer(key: Optional[str]) -> None:
    if key:
        reveal_type(key)
        def inner(key: str = key) -> bool:
            return True

mypy 0.701 output: (default configuration)

5: error: Revealed type is 'builtins.str'
6: error: Incompatible default for argument "key" (default has type "Optional[str]", argument has type "str")

So it seems that mypy is not only cautious when the _body_ of the inner function uses values from the outer function, but it also ignores narrowing when _defining_ the inner function. As far as I know, default parameter values don't need to be treated differently from any other expression evaluation.

[JukkaL]

@mthuurne A good point, default arguments at least should use the narrowed type.

[hauntsaninja]

Since this comes up a lot / I link here often, here's a quick summary of why this is tricky: closures in Python are late binding https://docs.python-guide.org/writing/gotchas/#late-binding-closures

Example:

def foo(x: Optional[int]):
    if x is None:
        x = 5

    def inner():
        return x + 1

    x = None    
    return inner

foo(5)()  # will crash

You could probably come up with an even more fiendish example using mutable data structures, e.g. type narrowing an element of an input list.

The following fiendish program actually passes type checking with mypy 0.790. Probably too complicated for mypy to ever really understand.

from typing import *

def foo(x: List[Optional[int]]):
    assert isinstance(x[0], int)
    def inner():
        return x[0] + 1
    return inner


bad: List[Optional[int]] = [1, 2, 3]
fn = foo(bad)
bad[0] = None
fn()

[NeilGirdhar]

@hauntsaninja That's an excellent example! So, that explains that @euresti and @sergeyk's examples are actually not mypy's problem. Perhaps it might be possible (if it's not too difficult) to output a better error message when the variable is a closure. Also, you might want to consider adding your succinct example to the common issues?

However, this doesn't have anything to do with @mthuurne's example, right? Isn't the parameter value bound when function is defined? In that case, MyPy should use the narrowed type.

What do you think?

[hauntsaninja]

Yes, as JukkaL mentioned, mthuurne's example is always correct, potentially understandable by mypy and should be fixed.

Since this issue comes up often and is surprising to users, there's room to be more pragmatic. E.g. something like mypy uses the narrowed type if the captured variable wasn't ever assigned to again (but that's probably complicated to implement / would have to think through it more). Agreed that adding this to common issues and improving error messages where possible is a good idea.