[pylint] C1901: compare-to-empty-string

[AreamanM]

There's a few finishing touches left to make(docs, diagnostic message, make the logic a little cleaner) and I think this is autofixable, so if the diagnostic impl itself looks good, I'll have a go at autofix for this as well.

[charliermarsh]

My recommendation would be to instead match on the value, and check for value.is_empty() in Constant::Str(value). Would that have any problems? (It could also catch cases in which the string had a prefix, like u, which this would miss.)

[charliermarsh]

Separately: does Pylint catch x == b""? Or x == f""?

[AreamanM]

I think matching on the value definitely makes sense.

I just tested the latest stable release of pylint and it does not catch the b"" or f"" cases, it only shows the lint warning for "" without any specifiers. Pylint also shows the lint warning for x is "" and x is not "" which seems sensible as well.

[charliermarsh]

I'd suggest just making a new enum in this file specifically for this violation, to encode this assumption in the types themselves.

[AreamanM]

I've fixed the diagnostics message and tested the code and it all seems to work, unfortunately I've got a busier week than anticipated so I'll see if I can implement autofix in another PR sometime later

[charliermarsh]

No prob, thanks for this!

[gil-cohen]

hey, i encountered this rule just now and i think its problematic, take the following example:

def foo(a: str | None):
    if a == '':
        print('empty string')
    if a is None:
        print('none')
    if not a:
        print('not a')
        
        
foo('')
foo(None)

which outputs:

empty string
not a
none
not a

running ruff --select PL a.py will output:
a.py:2:13: PLC1901 `a == ''` can be simplified to `not a` as an empty string is falsey Found 1 error.

so following this suggestion creates code that doesnt differentiate between None and an empty string :(

[charliermarsh]

Yeah this is a known limitation of this rule unfortunately -- see, e.g., #3503. Pylint has the same problem. I've considered removing it for this reason.

You can restructure your code in various ways to ignore the lint, or disable the rule, but the rule itself can't really be improved without being made useless:

def foo(a: str | None):
    if a is None:
        print('none')
    if not a:
        print('empty string')

Or:

def foo(a: str | None):
    if not a:
        if isinstance(a, str):
            print('empty string')
        else:
            print('none')

(I'm not suggesting that these are better in any subjective sense, only that they avoid the issue.)

[NJindia]

Going to echo that I think the rule is a bit of an issue. My team prefers to declare falsey values explicitly to avoid being unsure of None vs "" and thus have to ignore this rule, which is fine, but is the functionality to enforce explicit checks of falsey values possible? i.e. finding an error when code passes PLC1901 instead of when it fails.

Thanks