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

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

Closed
fischbacher opened this issue May 9, 2022 · 3 comments · Fixed by #7990
Closed

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

fischbacher opened this issue May 9, 2022 · 3 comments · Fixed by #7990
Labels
Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation Optional Checkers Related to a checked, disabled by default
Milestone

Comments

@fischbacher
Copy link

fischbacher commented May 9, 2022

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 fischbacher added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label May 9, 2022
@Pierre-Sassoulas
Copy link
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 ?

@Pierre-Sassoulas Pierre-Sassoulas changed the title Warn about "if x is None == y is None" Warn aboutif x is None == y is None May 9, 2022
@Pierre-Sassoulas Pierre-Sassoulas changed the title Warn aboutif x is None == y is None Warn about if x is None == y is None May 9, 2022
@fischbacher
Copy link
Author

fischbacher commented May 9, 2022

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.

@Pierre-Sassoulas Pierre-Sassoulas added Proposal 📨 Optional Checkers Related to a checked, disabled by default and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 9, 2022
@gpshead
Copy link

gpshead commented May 9, 2022

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

@Pierre-Sassoulas Pierre-Sassoulas added Help wanted 🙏 Outside help would be appreciated, good for new contributors Good first issue Friendly and approachable by new contributors labels May 9, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Jun 24, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.17.0 milestone Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation Optional Checkers Related to a checked, disabled by default
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants