New check: Simplify boolean returns

[nickdrozd]

Current problem

Here's a Pylint internal function:

def is_super(node: nodes.NodeNG) -> bool:
    """Return True if the node is referencing the "super" builtin function."""
    if getattr(node, "name", None) == "super" and node.root().name == "builtins":
        return True
    return False

This is a very common pattern, and it is easy to simplify:

def is_super(node: nodes.NodeNG) -> bool:
    """Return True if the node is referencing the "super" builtin function."""
    return getattr(node, "name", None) == "super" and node.root().name == "builtins"

IMO, returning booleans conditionally can be a code smell. For example, here is another Pylint internal function:

def is_ancestor_name(frame: nodes.ClassDef, node: nodes.NodeNG) -> bool:
    """Return whether `frame` is an astroid.Class node with `node` in the
    subtree of its bases attribute.
    """
    if not isinstance(frame, nodes.ClassDef):
        return False
    return any(node in base.nodes_of_class(nodes.Name) for base in frame.bases)

This can be simplifed to:

def is_ancestor_name(frame: nodes.ClassDef, node: nodes.NodeNG) -> bool:
    """Return whether `frame` is an astroid.Class node with `node` in the
    subtree of its bases attribute.
    """
    return (
        isinstance(frame, nodes.ClassDef)
        and any(node in base.nodes_of_class(nodes.Name) for base in frame.bases)
    )

Maybe one checker could handle both of these cases. It might be tricky to get the right coordination between not and False, so the various cases could be implemented incrementally.

Desired solution

Add new checker to simplify boolean returns

Additional context

No response

[clavedeluna]

I thought pylint already had something like this, but it does not. The closest I think would be simplifiable-if-statement so this could be simplifiable-bool-return.

Potential false positives need to be handled, such as NOT raising on something like

    if arg == "any":
        return True
    else:
        return 0

but yes raising on

def is_super(node: nodes.NodeNG) -> bool:
    """Return True if the node is referencing the "super" builtin function."""
    if getattr(node, "name", None) == "super" and node.root().name == "builtins":
        return True
    else:
       return False

Also consider non-bool return values. I'm sure the primer will help with catching lots of these.

[nickdrozd]

I thought pylint already had something like this, but it does not. The closest I think would be simplifiable-if-statement so this could be simplifiable-bool-return.

That's a good point. If it's easier, this functionality could be folded into that checker.

Potential false positives need to be handled, such as NOT raising on something like

    if arg == "any":
        return True
    else:
        return 0

Yes, although bool | int not a great return type to begin with 😆

[Pierre-Sassoulas]

This one need to be an extension it's a little opinionated. We had an argument about this because extending the code is easier with the other design. In pylint in particular we often have a long list of check like this:

def is_a_red_apple_with_worm(fruit):
    if not is_an_apple(fruit):
        return False
    if not is_red(fruit):
        return False
    if not has_worm(fruit):
        return False
    return True

In this context it does not necessarily make a lot more sense to do:

def is_a_red_apple_with_worm(fruit):
    if not is_an_apple(fruit):
        return False
    if not is_red(fruit):
        return False
    return has_worm(fruit):

Even if it's true that it's denser. For this simple example, this can be done:

def is_a_red_apple_with_worm(fruit):
    return is_an_apple(fruit) and is_red(fruit) and has_worm(fruit):

But in "real life pylint" the list of condition is so big that you kinda have to use the other step by step pattern with each condition after the other or you get an impossible to understand "if behemoth" that would be a nightmare to debug and extend.

[nickdrozd]

My perspective here is that it's awkward to use procedural logic to describe something that is just a set of conditions.

Question: How do I know if X is a red apple with worms?

Answer 1: X is a red apple with worms if it's red and it's an apple and it has worms.

Answer 2: If X is not red, it isn't. And if X is not an apple, it isn't. And if X doesn't have worms, it isn't. Otherwise, it is.

Answer 2 is a strange way to speak, but it's how a lot of code ends up reading. Actual Pylint code is sometimes written as a list of and/or and sometimes as a sequence of if/return blocks.

Keep in mind that Pylint by default warns against having too many return statements, so this check would actually help address that in many cases. The current pylintrc has max-returns set to 11. That's pretty high! (The default is 6.) A lot of the returning-est functions could benefit from this. Here is a typical example:

def _supports_protocol(
    value: nodes.NodeNG, protocol_callback: Callable[[nodes.NodeNG], bool]
) -> bool:
    if isinstance(value, nodes.ClassDef):
        if not has_known_bases(value):
            return True
        # classobj can only be iterable if it has an iterable metaclass
        meta = value.metaclass()
        if meta is not None:
            if protocol_callback(meta):
                return True
    if isinstance(value, astroid.BaseInstance):
        if not has_known_bases(value):
            return True
        if value.has_dynamic_getattr():
            return True
        if protocol_callback(value):
            return True

    if isinstance(value, nodes.ComprehensionScope):
        return True

    if (
        isinstance(value, astroid.bases.Proxy)
        and isinstance(value._proxied, astroid.BaseInstance)
        and has_known_bases(value._proxied)
    ):
        value = value._proxied
        return protocol_callback(value)

    return False

I can't really tell at a glance what this is doing, and the only way to figure it out is to procedurally step through it. Three levels of nested if branches! I think clarity could be improved by batching these returns together.

Actually, a lot of many-return functions don't return anything, so maybe there's a more general way to approach this.

[nickdrozd]

Also the walrus operator really helps to streamline this kind of logic because you don't need to stop to assign variables. But Pylint still supports 3.7, so we can't use it 😿

[nickdrozd]

Ruff has this check: https://docs.astral.sh/ruff/rules/needless-bool/