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

[RET503] Interaction with mypy assert_never #3304

Closed
eddiebergman opened this issue Mar 2, 2023 · 7 comments
Closed

[RET503] Interaction with mypy assert_never #3304

eddiebergman opened this issue Mar 2, 2023 · 7 comments

Comments

@eddiebergman
Copy link
Contributor

This is rather a niche circumstance but thought it's worth mentioning. For exhaustive checking of Enums, there's an idiom to use with Mypy, checking each possible value and ending with assert_never. This is not a runtime check but purely a Mypy static type check.

Ruff will give a RET503 here which makes sense without mypy but given mypy is telling us it's safe, it would be nice for Ruff to detect this too. In the example above, using ruff with --fix will actually insert the line return None after the assert which then further causes linting issues of unreachable code.

from enum import Enum, auto

from typing_extensions import assert_never


class E(Enum):
    A = auto()
    B = auto()
    C = auto()


def f(value: E) -> bool:
    if value == E.A:
        return True

    if value == E.B:
        return True

    if value == E.C:
        return True

    assert_never(value) # ruff: [RET503] [*] Missing explicit `return` at the end of function able to return non-`None` │ value (ruff) 

My naive solution to this is check for assert_never(.*) where the return should be and accept that as a valid explicit return statement.

Looking into the checker code. It seems like it could be added as a StmntKind to the following lines as I'm guessing this code block is where it says to do nothing.

https://github.com/charliermarsh/ruff/blob/3ed539d50ed6260358c97d2e00b49bad4abfa37e/crates/ruff/src/rules/flake8_return/rules.rs#L272-L275

However I'm really not a rustacian so just grasping at straws.

@NeilGirdhar
Copy link

NeilGirdhar commented Mar 2, 2023

For the purpose of this error, assert False should also do the same thing.

@charliermarsh
Copy link
Member

I actually thought we did account for this -- lemme look back at the code today.

@eddiebergman
Copy link
Contributor Author

Major apologies, latest version does seem to solve this! The releases are just too fast to keep up :)

@charliermarsh
Copy link
Member

Oh no worries at all! Glad to hear it :)

@henryiii
Copy link
Contributor

@henryiii
Copy link
Contributor

Is it possible that the code (I think here) isn't accounting for the typing module reexport location?

@charliermarsh
Copy link
Member

Ah yeah, you're exactly right.

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

4 participants