Skip to content

Warn about if x is None == y is None #6559

Closed
@fischbacher

Description

@fischbacher

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.

Activity

added
Needs triage 📥Just created, needs acknowledgment, triage, and proper labelling
on May 9, 2022
Pierre-Sassoulas

Pierre-Sassoulas commented on May 9, 2022

@Pierre-Sassoulas
Member

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

changed the title [-]Warn about "if x is None == y is None"[/-] [+]Warn about``if x is None == y is None``[/+] on May 9, 2022
changed the title [-]Warn about``if x is None == y is None``[/-] [+]Warn about ``if x is None == y is None``[/+] on May 9, 2022
fischbacher

fischbacher commented on May 9, 2022

@fischbacher
Author

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.

added
Optional CheckersRelated to a checked, disabled by default
and removed
Needs triage 📥Just created, needs acknowledgment, triage, and proper labelling
on May 9, 2022
gpshead

gpshead commented on May 9, 2022

@gpshead

FYI - The equivalent pyflakes issue was filed as PyCQA/pyflakes#690

added
Help wanted 🙏Outside help would be appreciated, good for new contributors
Good first issueFriendly and approachable by new contributors
on May 9, 2022
added
Needs PRThis issue is accepted, sufficiently specified and now needs an implementation
on Jun 24, 2022
added this to the 2.17.0 milestone on Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Good first issueFriendly and approachable by new contributorsHacktoberfestHelp wanted 🙏Outside help would be appreciated, good for new contributorsNeeds PRThis issue is accepted, sufficiently specified and now needs an implementationOptional CheckersRelated to a checked, disabled by default

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Warn about ``if x is None == y is None`` · Issue #6559 · pylint-dev/pylint