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

PLC1901 false positive in __bool__ methods #4282

Closed
trag1c opened this issue May 8, 2023 · 7 comments · Fixed by #5264
Closed

PLC1901 false positive in __bool__ methods #4282

trag1c opened this issue May 8, 2023 · 7 comments · Fixed by #5264
Assignees

Comments

@trag1c
Copy link
Contributor

trag1c commented May 8, 2023

class X:
    val: str

    def __bool__(self) -> bool:
        return self.val != ""
$ ruff check test.py --select=PLC1901
test.py:5:28: PLC1901 `self.val != ""` can be simplified to `self.val` as an empty string is falsey
Found 1 error.

Trying to correct this to self.val will lead to an error:

TypeError: __bool__ should return bool, returned str
@MichaReiser
Copy link
Member

Yeah, The "safe" fix for this rule should probably be bool(self.val). But even that is not entirely safe, because it now no longer reads self.val, which could have side effects.

@trag1c
Copy link
Contributor Author

trag1c commented May 8, 2023

In one case I specifically need the comparison :)

@charliermarsh
Copy link
Member

I dislike this rule, too many false positives.

@karolinepauls
Copy link

karolinepauls commented Jun 21, 2023

This rule is fundamentally incorrect for assertions or assignments, since None != "" - unless it can infer that the variable is a string.

a = 0

def test_sth() -> None:
    assert a == ""

Command: ruff --isolated --select PLC1901 a.py

Output:

a.py:4:17: PLC1901 `a == ""` can be simplified to `not a` as an empty string is falsey
Found 1 error.
$ ruff --version
ruff 0.0.274

@charliermarsh
Copy link
Member

Yes, Pylint's implementation has the same problem:

pylint --load-plugins=pylint.extensions.emptystring foo.py
************* Module foo
foo.py:1:0: C0114: Missing module docstring (missing-module-docstring)
foo.py:1:0: C0104: Disallowed name "foo" (disallowed-name)
foo.py:1:0: C0103: Constant name "a" doesn't conform to UPPER_CASE naming style (invalid-name)
foo.py:3:0: C0116: Missing function or method docstring (missing-function-docstring)
foo.py:4:11: C1901: "a == ''" can be simplified to "not a" as an empty string is falsey (compare-to-empty-string)

I'm going to move this rule to the nursery so that it's explicitly opt-in.

@charliermarsh charliermarsh self-assigned this Jun 21, 2023
charliermarsh added a commit that referenced this issue Jun 21, 2023
## Summary

This rule has too many false positives. It has parity with the Pylint
version, but the Pylint version is part of an
[extension](https://pylint.readthedocs.io/en/stable/user_guide/messages/convention/compare-to-empty-string.html),
and so requires explicit opt-in.

I'm moving this rule to the nursery to require explicit opt-in, as with
Pylint.

Closes #4282.
@adifelice-godaddy
Copy link

@bersbersbers
Copy link
Contributor

Here's another example where this rule creates false positives with numpy:

import numpy as np

array = np.array(("X", "", "X"), dtype=str)
indices = array == ""
print(indices)

print(not array)

At least in this case, not array raises a ValueError, so incorrect fixes become obvious (at least at runtime).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants