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

Protection against using Result with if #191

Open
last-partizan opened this issue Jul 21, 2024 · 3 comments
Open

Protection against using Result with if #191

last-partizan opened this issue Jul 21, 2024 · 3 comments

Comments

@last-partizan
Copy link

Hello, i'm doing some refactoring of my app, and wanted to use result.

Old code uses if some_func(): ... constructions, and now they're returning Result(), and this should not be treated as error in type checker.

After some testing, i found this:

from typing import NoReturn

class NoIf:
    def __bool__(self) -> NoReturn:
        raise RuntimeError()


if NoIf():
    pass

This results in error in pyright (but not in mypy)

  /tmp/test.py:6:4 - error: Invalid conditional operand of type "NoIf"
    Method __bool__ for type "NoIf" returns type "NoReturn" rather than "bool" (reportGeneralTypeIssues)

Should i make a PR adding this __bool__ override, if you think this can be useful for everyone?

@francium
Copy link
Member

francium commented Aug 8, 2024

That's very cool, didn't know that could be done.

I have two concerns however,

  1. I'm not sure how many people would benefit from this if this only works with pyright. Many projects use mypy and other editors that may not support pyright (eg PyCharm).
  2. More so, the issue is we'd be introducing new runtime behavior and break existing code by making this change. We'd want to do this in a non-intrusive and opt-in way. Unfortunately there's no easy way to do this that works both at runtime and at type checker level -- the only solution I can think of is to make a new module, from result.strict import Ok, Err (src/result/strict.py), where we expose a new Ok and Err classes with extend and override the __bool__.

I'm not sure if it makes sense to do this just yet. But definitely something we can look into in the future if something changes.

@last-partizan
Copy link
Author

Yeah, you're right. Moving this into strict submodule is good idea.

In the meantime, i just added this temporarily to my local copy, to catch existing issues and it worked great.

Feel free to close this issue, or leave it open until "something changes" :)

@jonaskuske
Copy link

You could guard the behavior of returning NoReturn through an if TYPE_CHECKING condition. This way runtime behavior wouldn't change, but users would see an error in their editor. (and potentially the CI pipeline or wherever else the type checker is run)

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

No branches or pull requests

3 participants