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

E0606: detect validation done in external function #9715

Closed
mikez opened this issue Jun 9, 2024 · 6 comments · Fixed by #9716
Closed

E0606: detect validation done in external function #9715

mikez opened this issue Jun 9, 2024 · 6 comments · Fixed by #9716

Comments

@mikez
Copy link

mikez commented Jun 9, 2024

Bug description

def f(offset):
    validate_offset(offset)

    # compute modifier
    number, suffix = int(offset[:-1]), offset[-1]

    if suffix == "d":
        modifier = f"-{number} days"
    elif suffix == "w":
        modifier = f"-{number * 7} days"
    elif suffix == "y":
        modifier = f"-{number} years"
    # else:
    #    raise ValueError() # This is here to make pylint etc. happy

    # do something with `modifier` here (omitted)
    modifier


def validate_offset(offset):
    if not isinstance(argument, str):
        raise ValueError(f"Invalid offset: {offset!r}")

    suffix = argument[-1:]  # slicing here to handle empty strings
    if suffix not in ("d", "w", "y"):
        raise ValueError(
            f"Invalid offset: {offset!r}\n"
            f"Please specify a string of the format 'X[d/w/y]' "
            "where X is a non-negative integer followed by 'd', 'w', or 'y' "
            "that indicates days, weeks, or years."
        )

Configuration

No response

Command used

pylint -sn *.py ${SRC_CORE} ${SRC_TEST}

Pylint output

************* Module things.database
things/database.py:1048:42: E0606: Possibly using variable 'modifier' before assignment (possibly-used-before-assignment)

Expected behavior

It is a common pattern for me to validate input in an external validation function. That keeps the code clean and readable.

In this case, I don't expect to have to write the additional code (commented above):

    else:
        raise ValueError() # This is here to make pylint etc. happy

since the validate_offset already raises if modified won't be set.

A potential workaround would be to explicitly mark modified will be set using some kind of asserts mechanism or comment that exists in the Typescript language (see here).

Pylint version

pylint 3.2.3
astroid 3.2.2
Python 3.11.9

OS / Environment

No response

Additional dependencies

No response

@mikez mikez added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Jun 9, 2024
@jacobtylerwalls
Copy link
Member

A potential workaround would be to explicitly mark modified will be set using some kind of asserts mechanism or comment that exists in the Typescript language (see here).

This what we've decided to support. Since pylint 3.2.1 you can use from typing import assert_never to assert exhaustiveness.

Will add your example to the docs 👍

@jacobtylerwalls jacobtylerwalls added Documentation 📗 and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jun 9, 2024
@jacobtylerwalls jacobtylerwalls added this to the 3.3.0 milestone Jun 9, 2024
@mikez
Copy link
Author

mikez commented Jun 9, 2024

@jacobtylerwalls Thanks for the fast response.

To clarify, you'd recommend a pattern like this one:

    if suffix == "d":
        modifier = f"-{number} days"
    elif suffix == "w":
        modifier = f"-{number * 7} days"
    elif suffix == "y":
        modifier = f"-{number} years"
    else:
        typing.assert_never()

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Jun 9, 2024

Yep. Or slightly more expressively typing.assert_never(suffix) in case it's ever reached at runtime to see what it was called with.

@mikez
Copy link
Author

mikez commented Jun 9, 2024

Yep. Or slightly more expressively typing.assert_never(suffix) in case it's ever reached at runtime to see what it was called with.

Great. Thank you. 😊🙏

Feel free to close this.

@mikez
Copy link
Author

mikez commented Jun 9, 2024

@jacobtylerwalls Small follow-up: how do I do this in a backwards compatible way, so it also works on Python 3.8, 3.9, and 3.10? Something like this?

try:
    from typing import assert_never
except ImportError:
    # backwards compatability with Python 3.8, 3.9, 3.10
    def assert_never(arg: NoReturn, /) -> NoReturn:
        """Assert line is unreachable."""
        raise AssertionError(arg)

@jacobtylerwalls
Copy link
Member

I think you can import it from typing_extensions, although I haven't tried it myself.

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.

2 participants