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

needless bool for needless-bool #15323

Open
nickdrozd opened this issue Jan 7, 2025 · 5 comments
Open

needless bool for needless-bool #15323

nickdrozd opened this issue Jan 7, 2025 · 5 comments
Assignees
Labels
bug Something isn't working fixes Related to suggested fixes for violations type-inference Requires more advanced type inference.

Comments

@nickdrozd
Copy link
Contributor

Function to check random numbers:

from random import randint

def check_zero(end: int) -> bool:
    """Check if two random numbers from 0 to END are both 0."""
    if randint(0, end) == 0 and randint(0, end) == 0:
        return True
    else:
        return False

Raises warning SIM103 Return the condition directly. Good. Fix with --unsafe-fixes --fix:

from random import randint

def check_zero(end: int) -> bool:
    """Check if two random numbers from 0 to END are both 0."""
    return bool(randint(0, end) == 0 and randint(0, end) == 0)

Multi-return if/else expression replaced with boolean expression. Good. But also wrapped with bool cast. Bad! bool cast of a boolean expression is unnecessary.

@dylwil3 dylwil3 added bug Something isn't working fixes Related to suggested fixes for violations labels Jan 7, 2025
@dylwil3 dylwil3 self-assigned this Jan 7, 2025
@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 7, 2025

Multi-return if/else expression replaced with boolean expression. Good. But also wrapped with bool cast. Bad! bool cast of a boolean expression is unnecessary.

We need to be careful here! If we can say for sure that both the left-hand side and the right-hand side is of type bool, then yes, we can safely remove the bool() call wrapping the expression. Otherwise we cannot, or a type checker might complain that the function's annotation has bool for the return type when actually it returns something else:

>>> 5 and 42
42

Perhaps you might say that we can say for sure that equality comparisons will always evaluate to a bool, but... sadly that's also not true!

>>> class Foo:
...     def __eq__(self, other):
...         return 42
...         
>>> Foo() == Foo() and Foo() == Foo()
42

@dylwil3
Copy link
Collaborator

dylwil3 commented Jan 7, 2025

Excellent point! We can at least add in the cases where ruff's semantic model can infer the type as bool (which will be fairly limited, and I think will not include the example in this issue due to the function call).

@dylwil3 dylwil3 added the type-inference Requires more advanced type inference. label Jan 7, 2025
@dylwil3
Copy link
Collaborator

dylwil3 commented Jan 7, 2025

In fact, here is a case where the autofix is wrong because it doesn't cast to bool:

import numpy as np 

def f(x:np.ndarray,y:np.ndarray):
    if x<y:
        return True
    else:
        return False

gets fixed to

import numpy as np 

def f(x:np.ndarray,y:np.ndarray):
    return x < y

Arguably comparison operators are more likely to produce false positives than boolean operators like and/or (since numpy does not override the latter - it only overrides the bitwise versions).

In any event the fix is marked as unsafe... so there's a question of whether we should be restricting the fix behavior further, extending it (as in the OP), or some mixture (eg avoiding comparison operators but allowing boolean operators).

I'm gonna push a few different versions and see what the ecosystem check reveals and then maybe we can discuss further. (Assuming the ecosystem check will tell me anything... I remember we made a change recently around what it does with fix behavior...)

@AlexWaygood
Copy link
Member

Right. We could also take whether or not the function is explicitly annotated into consideration, possibly? In untyped code, there's maybe not much of a difference between returning something truthy and something that is the literal True constant; the bool() cast maybe just adds overhead there. But if the function is annotated as returning bool, they're probably using a type checker, so we might break their CI by removing the bool() cast... anyway, looking forward to seeing the results of your experiments :D

@nickdrozd
Copy link
Contributor Author

Python's boolean handling never ceases to surprise! I see what you mean about being careful. Feel free to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations type-inference Requires more advanced type inference.
Projects
None yet
Development

No branches or pull requests

3 participants