consider-combining-ifs checker to suggest simplification of code with imbricated ifs

[Pierre-Sassoulas]

Current problem

The following code:

if isinstance(ancestor, nodes.If):
    if ancestor.test.as_string() in TYPING_TYPE:

Can be simplified to:

if isinstance(ancestor, nodes.If) and ancestor.test.as_string() in TYPING_TYPE:

The following code:

any(
    isinstance(a, nodes.If)
    for a in ancestors
    if a.test.as_string() in TYPING_TYPE
)

Can be simplified to:

any(
    a.test.as_string() in TYPING_TYPE and isinstance(a, nodes.If)
    for a in ancestors
)

Desired solution

A new checker suggesting to combine the ifs to simplify the code.

Additional context

Original proposition by @DanielNoord

[cdce8p]

Some details to be aware of. I noticed them while review some code.

1. If control flow statements are present, it might not be possible to simplify.

def func():
    for i in range(5):
        if some_condition():
            if some_other_condition():
                return  # same for 'break' and 'continue'
        
        do_something_else()

2. If the nested if contains an or, the combined statement isn't necessarily better just more complex.

if name in some_list:
    if some_function() or not (
        some_condition
        and some_other_condition
    ):
        pass


# "new"
if name in some_list and (
    some_function() or not (
        some_condition
        and some_other_condition
    )
):
    pass

3. Obviously, if any elif or else case is present, combining statements could break stuff.

[cdce8p]

There is also the issue that someone might explicitly want to use separate ifs to allow for easier extension.

The more I think about this check, the more I lean towards it being an optional one. There are just too many caveats (unfortunately).

[dbaty]

Just a note that Pierre's rewriting of the second example with any() seems incorrect to me:

def initial():
    # Here we call `as_string()` on each item, and call `isinstance` only
    # on items for which the first check is true.
    return any(
        isinstance(a, nodes.If)
        for a in ancestors
        if a.test.as_string() in TYPING_TYPE
    )

def transformed_wrong():  # Pierre's rewriting
    # Here it's different: we call `isinstance()` on each item, and call
    # `as_string()` only on items for which the first check is true. 
    return any(
        isinstance(a, nodes.If) and a.test.as_string() in TYPING_TYPE
        for a in ancestors
    )

def transformed_right():
    # Like above, but invert `and` operands to mimic the initial behaviour.
    return any(
        a.test.as_string() in TYPING_TYPE and isinstance(a, nodes.If)
        for a in ancestors
    )

It's probably harmless here with isinstance(), but it would be a problem if it was a resource-consuming function or a function that has side-effects.

(For what it's worth, I agree with cdce8p's comment above. It looks like it's not obvious that we can always simplify these conditions.)

[Pierre-Sassoulas]

Closing as it seems there's a lot of use case where it's not self evident it's better.