False positive when NoReturn is annotated using a text annotation

[NeilGirdhar]

Bug description

def distribution_info(request: Any):
    if request is not None:  # dummy condition
        return request.param
    pytest.skip(f"Deselected {info_name}")

Produces

tests/conftest.py:45:0: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)

even though pytest.skip is marked NoReturn.

Pylint version

pylint 2.14.0
astroid 2.11.5
Python 3.10.0 (default, Nov  6 2021, 07:41:13) [Clang 12.0.5 (clang-1205.0.22.9)]

[DanielNoord]

@NeilGirdhar Where are you suppressing inconsistent-return-statements in your example? I don't see a disable comment.

[NeilGirdhar]

@DanielNoord Sorrry I meant

tests/conftest.py:45:0: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)

My mistake.

[DanielNoord]

Since this is related to pylint I would recommend opening this against either pylint-pytest or pytest-pylint.

Those are community maintained extensions for pylint which offer support for pytest. Pylint itself doesn't offer this.

[NeilGirdhar]

@DanielNoord I'm not using either of those extensions. Isn't pylint just using its own analysis to evaluate this error?

[DanielNoord]

@DanielNoord I'm not using either of those extensions. Isn't pylint just using its own analysis to evaluate this error?

Yes, but for some external packages we need to rely on some hints to make sure we understand the packages correctly. However, we can't do this for all packages, as we are also limited in our maintainers' time. That's why some external packages are supported through community plugins, like pylint-django. The same goes for pytest.

[NeilGirdhar]

Yes, but for some external packages we need to rely on some hints to make sure we understand the packages correctly

I understand, but isn't the type annotation enough? I'll try to construct a MWE later tonight. My guess is that pylint isn't preserving the NoReturn annotation through the decorator even though the decorator is properly annotated with a type variable.

Incidentally, thanks for teaching me about pylint-pytest. That looks really useful to me!

[NeilGirdhar]

Here's an example that doesn't use pytest:

from typing import Any, Callable, Protocol, Type, TypeVar, cast

_F = TypeVar("_F", bound=Callable[..., object])
_ET = TypeVar("_ET", bound=Type[BaseException])


class _WithException(Protocol[_F, _ET]):
    Exception: _ET
    __call__: _F


def _with_exception(exception_type: _ET) -> Callable[[_F], _WithException[_F, _ET]]:
    def decorate(func: _F) -> _WithException[_F, _ET]:
        func_with_exception = cast(_WithException[_F, _ET], func)
        func_with_exception.Exception = exception_type
        return func_with_exception

    return decorate


@_with_exception(BaseException)
def skip(reason: str = "") -> "NoReturn":
    print(reason)


def distribution_info(request: Any):
    if request is not None:  # dummy condition
        return None
    skip("Deselected")

@DanielNoord Is this something that's worth fixing?

[Pierre-Sassoulas]

This is not a false positive, the issue here is that the return are inconsistent in the function. I.e. all return statements in a function should return an expression, or none of them should, if you return request.param you should also return everywhere else. This should work.

from typing import Any

import pytest


def distribution_info(request: Any):
    if request is not None:  # dummy condition
        return request.param
    return pytest.skip("Deselected {info_name}")

See https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/inconsistent-return-statements.html?highlight=inconsistent-return

[NeilGirdhar]

@Pierre-Sassoulas I believe you're mistaken. pytest.skip is marked NoReturn, therefore, it is exempt from the rule that you "should also return elsewhere". Your fix doesn't make any sense either since returning the value of NoReturn function doesn't make sense.

One fix that does work is changing the annotation in Pytest from "NoReturn" to NoReturn as I did in my linked pull request. Another fix would be for Pylint to understand text annotations.

If you plan on supporting text annotations, then please reopen the issue.

[Pierre-Sassoulas]

Well adding a return is easier to understand for both human and linter imo, it's just a clue that it's going to "return" without having to check what skip does so you just have to read the function to understand it easily. If you think this check and the fix it currently require does not make sense you can disable it.

We have an issue for taking typing into account #4813, it's not going to be in pylint before a long time. pylint is not mypy, mypy has 2k+ open issues, and pylint has it's own issues to take care off.

[NeilGirdhar]

without having to check what skip does so you just have to read the function to understand it easily.

I understand what you're saying, but if you really need to remind the reader, then I think a comment would be better. Writing code that never runs is confusing.

We have an issue for taking typing into account #4813, it's not going to be in pylint before a long time. pylint is not mypy, mypy has 2k+ open issues, and pylint has it's own issues to take care off.

Fair enough! Let's leave this closed then as low priority, and hopefully Pytest accepts my pull request, which will resolve this issue for me.