pylint icon indicating copy to clipboard operation
pylint copied to clipboard

Warn about ``if x is None == y is None``

Open fischbacher opened this issue 3 years ago • 3 comments

Current problem

We sometimes see code like this:

def foo(*, left=None, right=None):
  """ ... """
  if (left is None) != (right is None):
    raise ValueError('Either both left= and right= need to be provided or none should.')

It is very easy to make the mistake of writing the check as:

def foo(*, left=None, right=None):
  """ ... """
  if left is None != right is None:
    raise ValueError('Either both left= and right= need to be provided or none should.')

This actually is a chained comparison: (left is None != right is None) is equivalent to:

(left is None) and (None != right) and (right is None)

...which is unsatisfiable, since right is None implies not(None != right).

Desired solution

According to the comparison rule in the Python grammar (https://docs.python.org/3/reference/grammar.html), these comparison operators have the same precedence, and would lead to chained comparisons:

in, not in, is, is not, <, <=, >, >=, !=, ==

There are three groups: {'in', 'not in'}, {'is', 'is not'}, {'<', '<=', '>', '>=', '!=', '=='}.

If the linter warned about chained comparisons where one expression is part of two comparisons that belong to different groups, this would flag up checks such as "left is None != right is None".

The price to pay is that this would also trigger on code such as...:

if None is not f(x) > 0:
  ...

...but overall, it might be justified in those rare cases where a conditional just like that is appropriate to expect the author to silence the linter on that line.

fischbacher avatar May 09 '22 08:05 fischbacher

Do you want a check to suggest left^right in the first case you provided ? Or something more generic for precedence in chained operation ?

Pierre-Sassoulas avatar May 09 '22 08:05 Pierre-Sassoulas

I think a linter warning that merely explains that the code gets interpreted as a chained comparison would be appropriate here. Something along the lines of:

"Expression gets interpreted as a {num_pieces}-part chained comparison which straddles comparison-categories. If this is not the intent, please parenthesize."

So, authors then would generically want to change code such as

if left is None != right is None:

to:

if (left is None) != (right is None)

Do note that suggesting if left ^ right would be outright wrong here if the intent is to perform an if (left is None) != (right is None) check.

Note that this still would not capture code such as:

if x > 0 == True: ...

...(which again is an unsatisfiable conditional) since here, the chained comparisons are in the same category, but I would argue that this is an issue that should be dealt with entirely separately, perhaps by another rule about '== {True|False}' parts in a conditional.

fischbacher avatar May 09 '22 08:05 fischbacher

FYI - The equivalent pyflakes issue was filed as https://github.com/PyCQA/pyflakes/issues/690

gpshead avatar May 09 '22 19:05 gpshead