RET508 potential false positive after elif

[LefterisJP]

This is with ruff==0.0.155

I tried to drill it down to a simpler example but it did not happen there. Can you try to run ruff with RET508 enabled on this file, like ruff rotkehlchen/tests/api/test_async.py

It's returning

rotkehlchen/tests/api/test_async.py:75:13: RET508 Unnecessary `else` after `break` statement
rotkehlchen/tests/api/test_async.py:137:9: RET508 Unnecessary `else` after `break` statement

Which I am confused how it makes sense.

If we remove the else and put the Assertion (or any other code) after the if/elif the resulting code won't be the same.

I got similar questionable results with the other RET codes btw.

Also a question on pyproject toml. I have ended up with somethine like:

select = ["E", "F", "W", "C", "N", "B", "T", "UP", "YTT", "C4", "T10", "T201"]

Isn't T a superset of T201 and T210? So I can omit those? Same for C4 and C?

[charliermarsh]

Will clone and take a look. It could just be a limitation in the plugin logic (i.e., does it reproduce in the original flake8-return?). Or it could be a bug in the translation to Rust...

[charliermarsh]

And yeah, your configuration is equivalent to:

select = ["E", "F", "W", "C", "N", "B", "T", "UP", "YTT"]

[LefterisJP]

Will clone and take a look. It could just be a limitation in the plugin logic (i.e., does it reproduce in the original flake8-return?). Or it could be a bug in the translation to Rust...

Well I think I have added some flake8 checkers thanks to ruff, that I did not use before. So yes we never used flake8-return in rotki before.

I can see the exact same error for that file with flake8-return.

Perhaps I am too tired or not focused but does it not seem wrong?

[charliermarsh]

rotkehlchen/tests/api/test_async.py:75:13: RET508 Unnecessary else after break statement

I think the idea here is that the code, as written, is:

while True:
    # and now query for the task result and assert on it
    response = requests.get(
        api_url_for(server, "specific_async_tasks_resource", task_id=task_id),
    )
    assert_proper_response(response)
    json_data = response.json()
    if json_data['result']['status'] == 'pending':
        # context switch so that the greenlet to query balances can operate
        gevent.sleep(1)
    elif json_data['result']['status'] == 'completed':
        break
    else:
        raise AssertionError(f"Unexpected status: {json_data['result']['status']}")

But this is equivalent to the following:

while True:
    # and now query for the task result and assert on it
    response = requests.get(
        api_url_for(server, "specific_async_tasks_resource", task_id=task_id),
    )
    assert_proper_response(response)
    json_data = response.json()
    if json_data['result']['status'] == 'pending':
        # context switch so that the greenlet to query balances can operate
        gevent.sleep(1)
    elif json_data['result']['status'] == 'completed':
        break    
    raise AssertionError(f"Unexpected status: {json_data['result']['status']}")

...since this will only ever be reached if the elif evaluates to False.

So I think the check is right, based on a very cursory look, but whether you want to enforce those rules is maybe a different question :)

[LefterisJP]

rotkehlchen/tests/api/test_async.py:75:13: RET508 Unnecessary else after break statement

I think the idea here is that the code, as written, is:

while True:
    # and now query for the task result and assert on it
    response = requests.get(
        api_url_for(server, "specific_async_tasks_resource", task_id=task_id),
    )
    assert_proper_response(response)
    json_data = response.json()
    if json_data['result']['status'] == 'pending':
        # context switch so that the greenlet to query balances can operate
        gevent.sleep(1)
    elif json_data['result']['status'] == 'completed':
        break
    else:
        raise AssertionError(f"Unexpected status: {json_data['result']['status']}")

But this is equivalent to the following:

while True:
    # and now query for the task result and assert on it
    response = requests.get(
        api_url_for(server, "specific_async_tasks_resource", task_id=task_id),
    )
    assert_proper_response(response)
    json_data = response.json()
    if json_data['result']['status'] == 'pending':
        # context switch so that the greenlet to query balances can operate
        gevent.sleep(1)
    elif json_data['result']['status'] == 'completed':
        break    
    raise AssertionError(f"Unexpected status: {json_data['result']['status']}")

...since this will only ever be reached if the elif evaluates to False.

So I think the check is right, based on a very cursory look, but whether you want to enforce those rules is maybe a different question :)

Huh ... so weird. I could swear I thought it made no sense but now it does look like it makes sense. Brain fart maybe? Apologies for wasting your time with this.

[charliermarsh]

No worries at all, it’s a hard one to reason about.

[actionless]

i made a simpler example for reproducing this bug:

def func(arg1: int) -> None:
    for i in range(2):
        if arg1 == i:
            if arg1 == 1:
                break
            print("don't break")
        elif arg1 == 2:
            break
        else:
            print('else')

$ ruff test_ruff_ret_508.py
Found 1 error(s).
test_ruff_ret_508.py:7:9: RET508 Unnecessary `else` after `break` statement

[LefterisJP]

No worries at all, it’s a hard one to reason about.

Hey I took another stab at this @charliermarsh, by activating the RET module again and I think that what you posted above (and have 2-3 similar false positives in our repo for) is after all incorrect -- or I am totally asleep.

Original Code

while True:
    # and now query for the task result and assert on it
    response = requests.get(
        api_url_for(server, "specific_async_tasks_resource", task_id=task_id),
    )
    assert_proper_response(response)
    json_data = response.json()
    if json_data['result']['status'] == 'pending':
        # context switch so that the greenlet to query balances can operate
        gevent.sleep(1)
    elif json_data['result']['status'] == 'completed':
        break
    else:
        raise AssertionError(f"Unexpected status: {json_data['result']['status']}")

Code you posted as equivalent due to the linting warning

while True:
    # and now query for the task result and assert on it
    response = requests.get(
        api_url_for(server, "specific_async_tasks_resource", task_id=task_id),
    )
    assert_proper_response(response)
    json_data = response.json()
    if json_data['result']['status'] == 'pending':
        # context switch so that the greenlet to query balances can operate
        gevent.sleep(1)
    elif json_data['result']['status'] == 'completed':
        break    
    raise AssertionError(f"Unexpected status: {json_data['result']['status']}")

The problem here is that it's not equivalent in the first if condition being True. So if json_data['result']['status'] == 'pending':.

The original code will wait for 1 second (gevent.sleep()), and then continue after the code.

The modified as equivalent code will wait for 1 second then get out of the if/elif and hit the assertion.

[charliermarsh]

Yeah I think you’re right.

[charliermarsh]

(They’re not equivalent, not sure why I thought they were.)

[LefterisJP]

The problem is with the following.

RET505, RET506, RET507, RET508

Simple program to reproduce them all

def foo(a: int) -> int:
    if a == 1:
        result = 2
    elif a == 3:
        raise ValueError()
    else:
        raise AssertionError('Evil')

    return result

def foo2(a: int) -> int:
    while True:
        if a == 1:
            result = 2
        elif a == 3:
            break
        else:
            raise AssertionError('Evil')

        return result

def foo3(a: int) -> int:
    if a == 1:
        result = 2
    elif a == 2:
        return 42
    else:
        raise AssertionError('Evil')

    return result

def foo4(a: int) -> int:
    while True:
        if a == 1:
            result = 2
        elif a == 3:
            continue
        else:
            raise AssertionError('Evil')

        return result

Seems to only happen for indented code. So if it's inside a function. If it's in the global space it does not happen.

var = 1
if var == 1:
    var_result = 2
elif var == 3:
    raise ValueError()
else:
    raise AssertionError('Evil')

print(var)

[charliermarsh]

Taking a look at this now.

[charliermarsh]

flake8-return seems to have the same behavior (not that it's correct).

[charliermarsh]

I'm considering just disabling this for elif for now. It's pretty hard to reason about.

[charliermarsh]

I kind of think the logic is not very good for any of these rules beyond RET501, RET502, and RET503.

[charliermarsh]

I can change it to only operate on simple if-else blocks (no elif), but I'm guessing that the plugin wants you to change...

def foo(a: int) -> int:
    if a == 1:
        result = 2
    elif a == 3:
        raise ValueError()
    else:
        raise AssertionError('Evil')

    return result

...to...

def foo(a: int) -> int:
    if a == 1:
        result = 2
    else:
        if a == 3:
            raise ValueError()
        raise AssertionError('Evil')

    return result

Or maybe the plugin logic is just not good.

[LefterisJP]

In any case such a suggestion is bad as it's not an improvement in anyway. I could swear I have a flake8 or other tool with similar~ish suggestions for no else after break or raise. But as you can see these ones seem to be wrong.

[henryiii]

PyLint's handling of this is much better, FWIW. I don't think I've ever had a false positive from PyLint's else after control flow checks.

[doolio]

I'm hitting this error (RET508 Unnecessary elif after break statement) with the below using ruff v0.1.4. I'm just a beginner so quite likely it could be avoided with better code.

def main() -> None:
    """Play a game of tic-tac-toe."""
    print('Welcome to tic-tac-toe!')
    game: dict[str, str] = create_board()
    current_player, other_player = X, O  # X goes first.

    while True:
        print(game_state(game))
        move: str = BLANK
        # Keep asking the player for a non-blank space to mark.
        while not is_space_blank(game, move):
            move = input(f"What is {current_player}'s move? (1-9) ")
        make_move(game, move, current_player)
        # Check if the game is over.
        if is_player_winner(game, current_player):  # First check for victory.
            print(game_state(game))
            print(f'{current_player} has won the game!')
            break
        elif is_board_full(game):  # Then check for a tie.
            print(game_state(game))
            print('The game is a tie!')
            break
        # Swap turns.
        current_player, other_player = other_player, current_player
    print('Thanks for playing!')

[henryiii]

You don’t need else after a control flow statement. That’s return, yield, break, or continue. Remove the “el” in front of the elif.

[doolio]

I see. Thank you so much for pointing that out.