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

Inner method discards type narrowing done in outer method #2608

Closed
euresti opened this issue Dec 25, 2016 · 38 comments · Fixed by #15133
Closed

Inner method discards type narrowing done in outer method #2608

euresti opened this issue Dec 25, 2016 · 38 comments · Fixed by #15133
Assignees
Labels
false-positive mypy gave an error on correct code feature priority-0-high topic-type-narrowing Conditional type narrowing / binder topic-usability

Comments

@euresti
Copy link
Contributor

euresti commented Dec 25, 2016

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
Copy link
Member

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
Copy link
Collaborator

JukkaL commented Jan 16, 2017

Let's keep this open.

@ilevkivskyi
Copy link
Member

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

@gvanrossum
Copy link
Member

gvanrossum commented Dec 23, 2017 via email

@carljm
Copy link
Member

carljm commented Dec 23, 2017

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
Copy link
Member

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
Copy link
Member

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

@gvanrossum
Copy link
Member

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

@euresti euresti changed the title Common python idiom hard to use with inner methods and strict-optional Inner method loses type narrowing done in outer method Oct 30, 2018
@euresti euresti changed the title Inner method loses type narrowing done in outer method Inner method discards type narrowing done in outer method Oct 30, 2018
@euresti
Copy link
Contributor Author

euresti commented Oct 30, 2018

Hope this helps!

@sergeyk
Copy link

sergeyk commented Nov 14, 2018

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
Copy link
Member

Another example (with a subclass narrowing) is #4679

@mthuurne
Copy link
Contributor

mthuurne commented May 31, 2019

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
Copy link
Collaborator

JukkaL commented May 31, 2019

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

@mrolle45
Copy link

mrolle45 commented May 5, 2022

  • Lambdas and comprehensions have their own scopes. For comprehensions, the first subexpression (that calculates the outermost iterator) is evaluated in the outer scope, so any variables referenced or assigned in that subexpression should be treated as such. The rest of the comprehension is evaluated in its own scope.

Let me clarify "evaluated in its own scope" and "assigned". The scope of the comprehension includes the global scope (or whatever scope the comprehension is contained in). Assignments include assignment expressions and the variables named in the for clauses. The former assign to the nearest binding of the name starting with the scope containing the comprehension. The latter assign to variables within the comprehension scope and are not visible in any containing scopes.

In this example:

>>> a, b, c, d, x = 100, 200, 2, 3, 4
>>> [(x := a) + (y := b) for a in range(c) for b in range(x + d)]
[0, 1, 2, 3, 4, 5, 6, 1, 2, 3]    -- the initial value of x is 4 so range(x + d) is range(7)
>>> a, b, c, d, x, y
(100, 200, 2, 3, 1, 2)    -- x now set to 1, and y, formerly unbound, set to 2.
>>> [(x := a) + (y := b) for a in range(c) for b in range(x + d)]
[0, 1, 2, 3, 1, 2, 3]    -- the initial value of x is 1 so range(x + d) is range(4)
>>> a, b, c, d, x, y
(100, 200, 2, 3, 1, 2)    -- x and y are set again, to the same values as before.
  • the first subexpression is range(c), and c is found in the global scope as 2.
  • a and b are local to the comprehension as assigned by the for clauses. These variables in the global scope are not changed.
  • d is evaluated in the scope of the comprehension, and found in the global scope as 3.
  • x is leaked to the global scope. It is found there by range(x + d). In the first iteration of a, x is the current global variable (4 in the first evaluation of the comprehension and 1 in the second). In the second iteration of a, x has been assigned to 1.
  • y is leaked to the global scope and set to various values and finally to 2.
  • The only changes made in the global scope are due to the assignment expressions.

If you (i.e., mypy) are traversing a module, class, or function body, you know that any assignment expression, even if contained in a comprehension or generator, is a binding in that body's scope.

@erictraut
Copy link

Yes, I think your analysis and description are correct.

@mrolle45
Copy link

mrolle45 commented May 6, 2022

Unfortunately, mypy 0.931 doesn't handle those statements. I didn't find any corresponding Node classes.

Question

In my copy of mypy 0.931 I see nothing to handle match and case. But I see, checking on GitHub, that this code has since been added. Even so, passing a value for feature_version to ast.parse() will have no effect if this is greater than the python version that mypy is running under.

Could you put an ast_extensions module in PyPI, just as you did with typing_extensions? And make sure that your pip install script brings in the latest version (or the latest version that you have tested with mypy)?

@mrolle45
Copy link

mrolle45 commented May 7, 2022

That list looks pretty complete. Here are two more you might want to add for completeness:

* "as" patterns in a case statement

* Other capture targets in a case statement

@mrolle45
Copy link

mrolle45 commented May 7, 2022

That list looks pretty complete. Here are two more you might want to add for completeness:

* "as" patterns in a case statement

* Other capture targets in a case statement

Yes. Let me elaborate.

  • The AST has nodes of type AsPattern, StarredPattern, and MappingPattern. They correspond to
    case ... as x: where x is a plain name.
    x as a pattern, possibly part of an OR, sequence, etc.
    (1, 2, *x) as a sequence pattern.
    `{..., **x} as a mapping pattern.

  • Assignment expressions can occur anywhere in a class def or function def, other than their respective bodies. This includes function default values, base classes, etc. They bind the names in the scope containing the def statement. Statements in the body, on the other hand, bind names in the function's or class' own scope.

  • del x makes x a local variable. If the programmer forgot to assign a value to it elsewhere in the scope, any reference to it would be an UnboundLocalError, rather than looking in enclosing scopes.

Updated implementation:
temp.txt
I plan to improve it by storing a complete SymtolTable with SymbolTableNodes for the first location where the respective names were bound. I'll also use it with class defs as well as function defs, and perhaps with modules, too.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 24, 2023

I have a proof-of-concept implementation that fixes this issue. The idea is to use the narrowed type of a local variable in a nested function if the local variable is not assigned to after the nested function/lambda definition. It still needs polish and it only handles the most basic use cases currently, but it seems promising so far. Hopefully I can get a PR up soon.

@XuehaiPan
Copy link

+1 Ran into this today. Having optionally None as the argument default value is really common in practice. A minimal reproduce snippet:

from __future__ import annotations

def foo(lst: list[int] | None = None) -> None:
    if lst is None:
        lst = []

    def append(x: int) -> None:
        lst.append(x)  # mypy inferred `lst` as `Optional[List[int]]`

    append(1)

I also tried typing.cast and nonlocal but they didn't work.

from __future__ import annotations

from typing import cast


def foo(lst: list[int] | None = None) -> None:
    if lst is None:
        lst = []
    lst = cast(list[int], lst)  # redundant-cast

    def append(x: int) -> None:
        nonlocal lst  # won't work

        lst.append(x)  # mypy inferred `lst` as `Optional[List[int]]`

    append(1)

JukkaL added a commit that referenced this issue May 3, 2023
Fixes #2608.

Use the heuristic suggested in #2608 and allow narrowed types of
variables (but not attributes) to be propagated to nested functions if
the variable is not assigned to after the definition of the nested
function in the outer function.

Since we don't have a full control flow graph, we simply look for
assignments that are textually after the nested function in the outer
function. This can result in false negatives (at least in loops) and
false positives (in if statements, and if the assigned type is narrow
enough), but I expect these to be rare and not a significant issue. Type
narrowing is already unsound, and the additional unsoundness seems
minor, while the usability benefit is big.

This doesn't do the right thing for nested classes yet. I'll create an
issue to track that.

---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@bast1aan
Copy link

FYI: I run into this with following example:

def handle_now(now: datetime.datetime | None = None) -> ...:
    if now:
        class Signer(TimestampSigner):
            def get_timestamp(self) -> float:
                return now.timestamp()
    else:
        Signer = TimestampSigner
    ...

In this case it is of course not possible to change the signature of get_timestamp(). now doesn't get reassigned in the scope, so the fix of JukkaL will fix this probably. Unfortunately final or const types are not possible in mypy, that would fix a lot of issues like these.

@karlicoss
Copy link

karlicoss commented Aug 31, 2023

@bast1aan mypy does have Final, but doesn't seem like type inference takes it into the account in case of nested classes, so you'll still need to do an extra assert inside the get_timestamp call for now

That said, Final does make it a bit safer

from typing import Final

i: Final[int] = 1

class A:
    def test(self):
        print(f"Hello from {i}")

# lots of code

i = 2   # whoops, this will make the `test()` call print 2
print(A().test())

if you run it, it will print 2 because i is captured by reference. However declaring i as Final in the first place will make mypy complain about the reassignment.

@OlegAlexander
Copy link

Hello, I'm so sorry to comment on a closed issue. I was also puzzled by this error when I tried something like this:

import os
import requests
from dotenv import load_dotenv


load_dotenv()

URL = os.getenv("URL")
assert URL is not None


def ping() -> bool:
    try:
        requests.get(URL)  # Error: Argument 1 to "get" has incompatible type "str | None"; expected "str | bytes"  [arg-type]
        return True
    except requests.exceptions.ConnectionError:
        return False

I thought the assert URL is not None would be respected inside the ping function, but that isn't the case. It took me some time to even understand where the error was coming from.

The workaround that makes the error go away is:

URL = os.getenv("URL")
assert URL is not None
URL = URL

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
false-positive mypy gave an error on correct code feature priority-0-high topic-type-narrowing Conditional type narrowing / binder topic-usability
Projects
None yet
Development

Successfully merging a pull request may close this issue.