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

PEP647 typing.TypeGuard based is_ok() and is_err() helpers #69

Closed
wbolster opened this issue Oct 30, 2021 · 10 comments · Fixed by #135
Closed

PEP647 typing.TypeGuard based is_ok() and is_err() helpers #69

wbolster opened this issue Oct 30, 2021 · 10 comments · Fixed by #135

Comments

@wbolster
Copy link
Member

wbolster commented Oct 30, 2021

PEP 647 (https://www.python.org/dev/peps/pep-0647/) makes it possible to write custom type guard functions.

this makes it possible to write shorthand is_ok(...) and is_err(...) functions that effectively work as shorthands for the longer isinstance(..., ...) forms, resulting in denser and more readable code, while still keeping all the benefits of strict type checking:

def is_ok(result: Result[T, E]) -> TypeGuard[Ok[T]]:
    return isinstance(result, Ok)


def is_err(result: Result[T, E]) -> TypeGuard[Err[E]]:
    return isinstance(result, Err)

not all supported python versions support TypeGuard, but the widely used typing_extensions package (https://github.com/python/typing/blob/master/typing_extensions/README.rst) has a functional backport that works with (at least) mypy. i personally do not see a problem adding a dependency on typing_extensions, since it's maintained ‘upstream’, close to python itself, and it will already be pulled in by many other libraries in many real-world applications.

thoughts? opinions? (cc @francium @dbrgn)

@francium
Copy link
Member

francium commented Nov 1, 2021

I can only speak briefly about this as I do more TypeScript work than Python (very little Python actually). In TS it's very common to use typeguards and providing a more descriptive and terser is_x style DSL function I think would be nice to have.

@wbolster
Copy link
Member Author

wbolster commented Nov 1, 2021

ok, we agree 👍🏼

i'll open a pr later, including a conditional typing_extensions dependency for older python versions.

(unlike #33 / #71 this seems to be well-supported by mypy)

@wbolster
Copy link
Member Author

wbolster commented Nov 1, 2021

first investigation shows that mypy may have some issues in this area; i've reported python/mypy#11428 about that.

@dbrgn
Copy link
Member

dbrgn commented Nov 1, 2021

If only older Python versions need it, I don't think adding a dependency is a problem. By upgrading your Python, you can get rid of the dependency.

We did the same thing with typing when we still supported Python versions that didn't have this built-in yet.

From my experience with TS, type guards are pretty nice to have.

wbolster added a commit to wbolster/result that referenced this issue Dec 13, 2021
@jack-michaud
Copy link

jack-michaud commented Jun 9, 2022

It seems like mypy generic typeguards are fixed, but there's still no negation.

from result import Ok, Result
from typing import TypeVar
from typing_extensions import TypeGuard


E = TypeVar("E")
T = TypeVar("T")


def is_ok(result: Result[T, E]) -> TypeGuard[Ok[T]]:
    return isinstance(result, Ok)

def main(result: Result[str, int]):
    if is_ok(result):
        reveal_type(result)
    else:
        reveal_type(result)
test.py:20: note: Revealed type is "result.result.Ok[builtins.str]"
test.py:22: note: Revealed type is "Union[result.result.Ok[builtins.str], result.result.Err[builtins.int]]"

Doing something like this would work:

def main(result: Result[str, int]):
    if is_ok(result):
        reveal_type(result)
    elif is_err(result):
        reveal_type(result)
test.py:20: note: Revealed type is "result.result.Ok[builtins.str]"
test.py:22: note: Revealed type is "result.result.Err[builtins.int]"

...but ideally we wouldn't need to do this.

Related discussion for typeguard negations: python/typing#1013

wbolster added a commit to wbolster/result that referenced this issue Sep 16, 2022
wbolster added a commit to wbolster/result that referenced this issue Sep 16, 2022
wbolster added a commit to wbolster/result that referenced this issue Sep 16, 2022
@harahu
Copy link
Contributor

harahu commented Dec 9, 2022

@qexat as @jack-michaud touches upon, we'd need a StrictTypeGuard annotation to make these type guard functions truly useful. StrictTypeGuard hasn't been proposed in a PEP yet, so it's not certain that it will ever be included in Python. I'm optimistic, but it is going to take time.

@charbonnierg
Copy link

charbonnierg commented Apr 9, 2023

Although it's not possible to do it using is_ok() and is_err(), it's possible to narrow type without typeguard when using the __bool__ method (at least with mypy and pyright):

from __future__ import annotations

import typing as t
from typing_extensions import Literal, TypeAlias
from result import Ok as _Ok, Err as _Err


T = t.TypeVar("T")
E = t.TypeVar("E")


class Ok(_Ok[T]):
    def __bool__(self) -> Literal[True]:
        return True


class Err(_Err[E]):
    def __bool__(self) -> Literal[False]:
        return False


Result: TypeAlias = "Ok[T] | Err[E]"


def foo(ok_t: t.Type[T], err_t: t.Type[E]) -> Result[T, E]:
    raise NotImplementedError


x = foo(int, Exception)

reveal_type(x)

if x:
    reveal_type(x)
else:
    reveal_type(x)
src/synopsys/tasks/result.py:31:13: note: Revealed type is "Union[synopsys.tasks.result.Ok[builtins.int], synopsys.tasks.result.Err[builtins.Exception]]"
src/synopsys/tasks/result.py:34:17: note: Revealed type is "synopsys.tasks.result.Ok[builtins.int]"
src/synopsys/tasks/result.py:36:17: note: Revealed type is "synopsys.tasks.result.Err[builtins.Exception]"

It eliminates the need for isinstance, but it may not be obvious that a "truthy" result designates a result which is Ok instead of a result with a "truthy" value (I.E, bool(Ok(False)) would be True).

@mishamsk
Copy link
Contributor

Hey, just stumbled upon the lib yesterday and immediately thought of type guards. Wonder if there is any particular reason it hasn't been done yet? Should be a pretty simple PR, happy to contribute if this just a matter of lack of time.

P.S. I wish the PEP didn't drop the self narrowing. This would make bound is_ok and is_err not only more succinct, but also more performant, since there would be no need for an instance check at all...

@francium
Copy link
Member

@mishamsk You're welcome to help out with PRs :)

mishamsk added a commit to mishamsk/result that referenced this issue Aug 1, 2023
update changelog & docs
add typeguard mypy tests
@mishamsk
Copy link
Contributor

mishamsk commented Aug 1, 2023

@francium #135 here you go. I saw #72, but IMHO my implementation makes a bit more sense as calls to isinstance are not necessary and are marginally slower than using is_ok and is_err methods under the hood.

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

Successfully merging a pull request may close this issue.

7 participants