PLC1901 false positives/incorrect response

[onerandomusername]

code:

def bug(query: str | None):
    if query == "":
        ...
    else:
        ...

this should not be flagged as "" is different from None.

Additionally, ruff provides the following error:

 PLC1901 `query == ""` can be simplified to `query` as an empty string is falsey

This is inaccurate, it should be simplified to not query (if it wasn't possible for query to be None).

possible related issue but not the same: GH-3494

[calumy]

I think this is fixed by #3497.

[Czaki]

I think this is fixed by #3497.

Only the second part. Linked PR does not touch the case where there is information about the variable type and the type is Union.

[henryiii]

I'm seeing the not:

src/scikit_build_core/settings/sources.py:159:55: PLC1901 `self.env[name] != ""` can be simplified to `not self.env[name]` as an empty string is falsey

That's the backward "not", but also, the original was:

def ...(...) -> bool:
    return name in self.env and self.env[name] != ""

It's not valid to simply that without the incorrect not, as self.env[name] is probably not a bool, and and returns the last truthy value - without the not making it a bool, it's not a bool. IIRC, pylint might suggest wrapping it in bool. (A better way to simplify it would probably be to use get & truthiness, actually)

I don't think you need variable type info; pylint doesn't do full type checking, you just can only simplify in cases were it's known to be a bool, via not or wrapping in bool.

[charliermarsh]

FWIW, Pylint treats these identically. Every example shared here is flagged by Pylint:

def bug(query: str | None):
    if query == "":
        ...
    else:
        ...

def f() -> bool:
    return name in self.env and self.env[name] != ""

Yields:

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: C0116: Missing function or method docstring (missing-function-docstring)
foo.py:2:7: C1901: Avoid comparisons to empty string (compare-to-empty-string)
foo.py:7:0: C0116: Missing function or method docstring (missing-function-docstring)
foo.py:7:0: C0103: Function name "f" doesn't conform to snake_case naming style (invalid-name)
foo.py:8:11: E0602: Undefined variable 'name' (undefined-variable)
foo.py:8:19: E0602: Undefined variable 'self' (undefined-variable)
foo.py:8:32: C1901: Avoid comparisons to empty string (compare-to-empty-string)
foo.py:8:32: E0602: Undefined variable 'self' (undefined-variable)
foo.py:8:41: E0602: Undefined variable 'name' (undefined-variable)

[cclauss]

In tests/* we will ignore rule PLC1901 because we want to be sure results are empty strings.
[], {}, 0, False, and None are all falsey, but are not empty strings.

[henryiii]

Yes, but most of the time, you either have something that's always a string, or you have an interface that can produce some of those other values, and you should also check to make sure it's a string. For example, x != "" will also be True if x is some other falsey value, which might then break the next line in your tests where you expect it to be a non-empty string. You could also overload a class to compare equal to an empty string. (I'd say this is not totally impossible to find, ROOT overloads classes to compare equal to None...)

There are certain types of tests where this can't be simplified (like testing custom __eq__), but I'd carefully think about it before doing an ignore on tests/**. But ignoring a rule is always fine, you don't have to accept any rule, and can disable them on patterns!

Above, I was only worried about the case where the value is not in a place where the bool is directly evaluated and can leak the value (and/or is itself not a boolean context unless inside if or bool()). If Pylint does this (I'm running pylint on my codebase and don't have this rule disabled, and haven't seen an error here), then that's fine. I'd expect the pylint checks to behave like pylint.

[henryiii]

Ahh, it's working now (0.256!). Pylint and Ruff are both happy. I think the not fix also fixed the FP.

Edit, wait a minute, forgot I rewrote it...

Nope, restoring the original, I still get:

src/scikit_build_core/settings/sources.py:160:55: PLC1901 `self.env[name] != ""` can be simplified to `self.env[name]` as an empty string is falsey
...
nox > pylint scikit_build_core

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

nox > Session pylint was successful.

The message is correct, though, so that's great. I'm not sure how to get pylint to flag this error.

[henryiii]

Ahh, empty string comparisons is an optional checker in pylint. Okay, I'm happy, I'd want Ruff'd Pylint checker to behave like pylint. Though if it was a hair smarter and could tell boolean context (assert, if, bool()) from places that could see the result, that'd be an improvement, but I think that could be proposed to pylint first. :)

[cclauss]

If I go to https://play.ruff.rs and in Settings enable PL and then in Source put:

s == ""  # Raises PLC1901
s != ""  # Raises PLC1901

Is there a syntax you would recommend or just # noqa: PLC1901 on all lines where we really want to test for empty str?

[henryiii]

You can use "tests/<filename>" = ["PLC1901"] or put # ruff: noqa: PLC1901 at the top for a file that has multiple of these that you'd like to silence. I have checks for a series of attributes, and it's much more symmetric to write == "" for the ones that are empty strings, rather that to make it look like a bool, so that test file is a good candidate for this sort of disable. And for any file that has just one or two instances, after verifying they are correct, you can just pass --add-noqa to Ruff to get it to add the ignores for you per line.

[charliermarsh]

Ah yeah, sorry, I should've mentioned that this is behind a Pylint extension.

I'm honestly a little tempted to remove this rule given the number of false positives 🙈 But I try to defer to upstream plugins, and so look to Pylint as a guide, and... it has the same behavior.