useless-else-on-loop doesn't take into account return statement

[chrisbeardy]

Checklist

• [ x] I have searched the Sourcery documentation for the issue, and found nothing
• [x ] I have checked there are no open bugs referencing the same bug or problem

Description

When using a For Else loop, if using return instead of break it raises the useless-else-on-loop suggestion. This should not be the case as if you are returning from a function you will not be executing the else code.

Code snippet that reproduces issue

def thing_that_checks_something():
   return True

def my_function():
    retries = 3
    for _ in range(retries):
        if thing_that_checks_something():
            return True
    else:
        print("There was an error checking something")
        return False

Debug Information

IDE Version: Pycharm Community Edition 2022.1.3 and 4

Sourcery Version: 1.6.0 Pro

Operating system and Version: Windows 10 22H2

[Hellebore]

Hi - thanks for raising.

I don't quite understand the issue - isn't the before/after refactored code equivalent regardless of the return

[chrisbeardy]

You are very correct!

Further thoughts for discussion though, if I expand my example slightly, I initially had some code like this

def thing_that_checks_something(actual, expected):
   return actual == expected

def read_from_device():
    return 6

def my_function():
    retries = 3
    expected = 5
    for _ in range(retries):
        actual = read_from_device()
        if thing_that_checks_something(actual, expected):
            break

    if not thing_that_checks_something(actual, expected):
        print("Read thing did not match")
        return False

    return True

I initially refactored this into the following, as repeating the check function just to log a specific message seemed wrong and this function is called many times with different inputs in the actual app, with the check function being more complex:

def my_function():
    retries = 3
    expected = 5
    for _ in range(retries):
        actual = read_from_device()
        if thing_that_checks_something(actual, expected):
            break
    else:
        print("Read thing did not match")
        return False

    return True

After checking this passed all tests, i further factored into the following as I prefer returning early where possible to avoid nested code, (something sourcery also seems to prefer as it suggests swap-i-else-branchs and remove-unnessecary-else):

def my_function():
    retries = 3
    expected = 5
    for _ in range(retries):
        actual = read_from_device()
        if thing_that_checks_something(actual, expected):
            return True
    else:
        print("Read thing did not match")
        return False

Which is when I then got the suggested refactoring and raised the issue, clearly confusing myself in the meantime...

My thought for discussion, is should/could sourcery recommend changing this:

def my_function():
    retries = 3
    expected = 5
    for _ in range(retries):
        actual = read_from_device()
        if thing_that_checks_something(actual, expected):
            break
    else:
        print("Read thing did not match")
        return False

    return True

into this?:

def my_function():
    retries = 3
    expected = 5
    for _ in range(retries):
        actual = read_from_device()
        if thing_that_checks_something(actual, expected):
            return True        
    print("Read thing did not match")
    return False

[Hellebore]

I see yes. The second snippet there is clearly more readable for sure.

Will have a think about whether there's a rule we could add or tweak for this that wouldn't be too specific.