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

Nonsensical returns are not reported as useless-return if they are indented #9449

Closed
e-gebes opened this issue Feb 20, 2024 · 2 comments · Fixed by #9492 or #9619
Closed

Nonsensical returns are not reported as useless-return if they are indented #9449

e-gebes opened this issue Feb 20, 2024 · 2 comments · Fixed by #9492 or #9619
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@e-gebes
Copy link

e-gebes commented Feb 20, 2024

Bug description

# pylint: disable=missing-module-docstring,missing-function-docstring


def function1(parameter):
    parameter.do()
    return  # this is reported as `useless-return` as expected


def function2(parameter):
    if parameter:
        return
    return  # this is not reported as `useless-return`


def function3(parameter):
    if parameter:
        pass
    else:
        return  # this is not reported as `useless-return`


def function4(parameter):
    if parameter:
        return
    else:  # this is reported as `no-else-return` as expected
        return  # this is not reported as `useless-return`


def function5(parameter):
    try:
        parameter.do()
    except RuntimeError:
        parameter.other()
        return  # this is not reported as `useless-return`


def function6(parameter):
    try:
        parameter.do()
    except RuntimeError:
        return  # maybe even this should be reported - `pass` can be used

Configuration

No response

Command used

pylint a.py

Pylint output

************* Module a
4:0: R1711: Useless return at end of function or method (useless-return)
23:4: R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it (no-else-return)

Expected behavior

Maybe more useless-return messages should be reported, see the comments in the code snippet.

Pylint version

pylint 2.17.7
astroid 2.15.8
Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]

OS / Environment

No response

Additional dependencies

No response

@e-gebes e-gebes added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Feb 20, 2024
@jacobtylerwalls
Copy link
Member

Agree we could extend the check to the last line of a (potentially nested) try/if/with

@jacobtylerwalls jacobtylerwalls added Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Feb 20, 2024
@nickdrozd
Copy link
Collaborator

These are great examples.

For anyone who wants to fix this, the warning is raised from this function:

    def _check_return_at_the_end(self, node: nodes.FunctionDef) -> None:
        """Check for presence of a *single* return statement at the end of a
        function.

        "return" or "return None" are useless because None is the
        default return type if they are missing.

        NOTE: produces a message only if there is a single return statement
        in the function body. Otherwise _check_consistent_returns() is called!
        Per its implementation and PEP8 we can have a "return None" at the end
        of the function body if there are other return statements before that!
        """
        if len(self._return_nodes[node.name]) > 1:
            return
        if len(node.body) <= 1:
            return

        last = node.body[-1]
        if isinstance(last, nodes.Return):
            # e.g. "return"
            if last.value is None:
                self.add_message("useless-return", node=node)
            # return None"
            elif isinstance(last.value, nodes.Const) and (last.value.value is None):
                self.add_message("useless-return", node=node)

The docstring refers to another function, _check_consistent_returns. The logic there is murky.

crazybolillo added a commit to crazybolillo/pylint that referenced this issue Mar 9, 2024
Returns inside try or if/else conditions were not being detected as
useless. Said nodes are now checked for return statements to ensure
their last component is not a useless return as well.

Closes pylint-dev#9449.
crazybolillo added a commit to crazybolillo/pylint that referenced this issue Mar 9, 2024
Returns inside try or if/else conditions were not being detected as
useless. Said nodes are now checked for return statements to ensure
their last component is not a useless return as well.

Closes pylint-dev#9449.
crazybolillo added a commit to crazybolillo/pylint that referenced this issue Mar 14, 2024
Returns inside try or if/else conditions were not being detected as
useless. Said nodes are now checked for return statements to ensure
their last component is not a useless return as well.

Closes pylint-dev#9449.
Pierre-Sassoulas pushed a commit that referenced this issue Mar 15, 2024
Returns inside try or if/else conditions were not being detected as
useless. Said nodes are now checked for return statements to ensure
their last component is not a useless return as well.

Closes #9449.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants