Skip to content

Guarded return value #56

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

Closed
DavidMertz opened this issue Feb 2, 2015 · 13 comments
Closed

Guarded return value #56

DavidMertz opened this issue Feb 2, 2015 · 13 comments

Comments

@DavidMertz
Copy link

I just presented on this topic for PyCon Belarus, and again at WarGaming, located here in Minsk. One question raised (and emailed to me) at the second presentation was this example. What would be the best approach to coding this to pass the type checker (and do we need the PEP to address a similar example)? This pattern of guarding a return value seems commonplace, and avoiding superfluous warnings is desirable.

% cat sergey_example.py

from typing import Union, Any
from random import random

def db_get(key: str) -> Union[str, None]:
    if random() < .5:
        return "string"
    else:
        return None

def handle_result(res: str) -> Any:
    pass

result = db_get("my_key")
if result is None:
    pass
else:
    handle_result(result)  # ???

% mypy sergey_example.py
sergey_example.py, line 17: Argument 1 to "handle_result" has incompatible type "Union[str, None]"; expected "str"

@gvanrossum
Copy link
Member

@JukkaL can you help with this? I thought there was something in mypy that would let you do this, but I can't seem to get it to work. Isn't there a feature where code guarded by isinstance() is type-checked with the more constrained type? E.g.

    result = db_get("my_key")
    if isinstance(result, str):
        handle_result(result)  # ???

@vlasovskikh
Copy link
Member

BTW the PEP doesn't specify (or recommend to the implementers of type checkers) that A is incompatible to Union[A, B] (which is supposed in this issue).

And it doesn't say anything about the subtyping relationship between Union[A, B] and Union[A, B, C].

@gvanrossum
Copy link
Member

The closing of python/mypy#245 made me think that this was supported in mypy. Maybe it's not understanding the Union correctly?

I wrote up the subtyping relationship between Unions and their constituents in PEP 483, which is supposed to be included by reference in PEP 484.

@vlasovskikh
Copy link
Member

@gvanrossum According to PEP 483: """Union[t1, t2, ...] . Classes that are subclass of at least one of t1 etc. are subclasses of this.""" So str is a subtype ("subclass") of Union[str, None], since str is a subclass of at least one of {str, None}. Is that correct?

@gvanrossum
Copy link
Member

That's correct. I'm guessing mypy's Union implementation has some bugs.

@seriyps
Copy link

seriyps commented Feb 2, 2015

@vlasovskikh this is correct. But we have reverse situation here: handle_result awaits str, but receives Union[str, None]. Resulting check will look like issubclass(Union[str, None], str).

@gvanrossum
Copy link
Member

@seriyps Yes, but this is a run-time guard, which mypy is supposed to handle. The handle_result(result) call will only be called when isinstance(result, str) is true. So we should be in the second case described by the mypy docs here: http://mypy.readthedocs.org/en/latest/common_issues.html#complex-isinstance-tests (# Mypy understands a lone isinstance check).

@vlasovskikh
Copy link
Member

@seriyps I misread the example, sorry.

@seriyps
Copy link

seriyps commented Feb 2, 2015

@gvanrossum ah, nice!

So, the correct example should look like

if result is None:
    handle_none()
elif isinstance(result, str):
    handle_result(result)

This looks reasonably. But if analyzer can calculate last if clause's type automatically it will be nice.

May also suggest assert isinstance(A, B) apart from if for type hinting/guarding. One possible usecase:

def main() -> str:
    result = None # type: Union[str, None]
    while result is None:
        result = pool_worker(key)  ## -> Union[str, None]
    assert isinstance(result, str)
    return result

@JukkaL
Copy link
Contributor

JukkaL commented Feb 3, 2015

Type checking None types is broken: python/mypy#359

Since None is currently a subtype of every other type, the original example can be written like this:

def db_get(key: str) -> str:   # instead of Union[str, None]:
    if random() < .5:
        return "string"
    else:
        return None   # Okay!

Type checking the isinstance checks works for types other than None. This should be fine:

from typing import Union, Any
from random import random

def db_get(key: str) -> Union[str, int]:
    if random() < .5:
        return "string"
    else:
        return 0

def handle_result(res: str) -> Any:
    pass

result = db_get("my_key")
if isinstance(result, int):
    pass
else:
    handle_result(result)  # Okay

There is also an issue for assert isinstance(...) type checking: python/mypy#475

python/mypy#473, python/mypy#476 and python/mypy#477 are other related issues.

Type checking None values is probably one of the next major features I'm going to work on. There are many related issues that can't implemented in a few weeks, so my plan is to add a mode to mypy that, when explicitly enabled, turns on None type checking, and I will fix issues gradually. I won't turn this on by default unless it works smoothly for most common cases.

@JukkaL JukkaL added the bug label Feb 3, 2015
@JukkaL
Copy link
Contributor

JukkaL commented Feb 3, 2015

And the original code is an example of a specific mypy bug, so I'm keeping this open -- Union[str, None] should be the same type as str, since None is a subtype of str, but it clearly isn't right now.

@JukkaL
Copy link
Contributor

JukkaL commented Feb 3, 2015

Ah, I forgot that this is not the mypy issue tracker :-)

I added a new mypy issue: python/mypy#572

@JukkaL
Copy link
Contributor

JukkaL commented Apr 14, 2015

I think that this issue can be closed -- this was about a mypy bug and current limitations of mypy.

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

No branches or pull requests

5 participants