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

Add new checker bad-chained-comparison #7990

Merged
merged 21 commits into from
Feb 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
842d459
Add bad chained comparison extension
zenlyj Dec 27, 2022
efe18b1
Add tests for bad chained comparison
zenlyj Dec 27, 2022
dbb7d08
Add doc for bad chained comparison
zenlyj Dec 27, 2022
4c11715
Add fragment for bad chained comparison
zenlyj Dec 27, 2022
38fc79e
Update docs
zenlyj Dec 27, 2022
d1a683e
Code style improvements for bad-chained-comparison
zenlyj Dec 28, 2022
28e28bc
Update confidence level in bad chained comparison
zenlyj Dec 28, 2022
8d3691d
Reassign message id to bad chained comparison
zenlyj Dec 28, 2022
24c3351
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 27, 2022
dd7f524
Improve message clarity of bad chained comparison
zenlyj Dec 29, 2022
b3f06a3
Make bad chained comparison a default check
zenlyj Dec 29, 2022
dbbe982
Move bad chained comparison functional tests to the correct dir
zenlyj Dec 29, 2022
be54dba
Remove redundant rc file for documentation
zenlyj Dec 29, 2022
f82a0b0
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 29, 2022
47efafe
Rename variable for consistency in bad chained comparison
zenlyj Dec 30, 2022
dc9f6c0
Shorten long line length in bad chained comparisons
zenlyj Dec 30, 2022
3ea3c6d
Refactor bad chained comparison to use sort
zenlyj Dec 30, 2022
943ee69
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 30, 2022
f4ddd8b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 30, 2022
6e004e2
Apply suggestions from code review
Pierre-Sassoulas Dec 30, 2022
05ae84a
Change line endings from CRLF to LF
zenlyj Jan 1, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/data/messages/b/bad-chained-comparison/bad.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
def xor_check(*, left=None, right=None):
if left is None != right is None: # [bad-chained-comparison]
raise ValueError('Either both left= and right= need to be provided or none should.')
3 changes: 3 additions & 0 deletions doc/data/messages/b/bad-chained-comparison/good.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
def xor_check(*, 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.')
1 change: 1 addition & 0 deletions doc/data/messages/b/bad-chained-comparison/related.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- `Comparison Chaining <https://docs.python.org/3/reference/expressions.html#comparisons>`_
13 changes: 13 additions & 0 deletions doc/user_guide/checkers/features.rst
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,19 @@ Async checker Messages
function. This message can't be emitted when using Python < 3.5.


Bad-Chained-Comparison checker
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Verbatim name of the checker is ``bad-chained-comparison``.

Bad-Chained-Comparison checker Messages
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
:bad-chained-comparison (W3501): *Suspicious %s-part chained comparison using semantically incompatible operators (%s)*
Used when there is a chained comparison where one expression is part of two
comparisons that belong to different semantic groups ("<" does not mean the
same thing as "is", chaining them in "0 < x is None" is probably a mistake).


Basic checker
~~~~~~~~~~~~~

Expand Down
1 change: 1 addition & 0 deletions doc/user_guide/messages/messages_overview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ All messages in the warning category:
warning/assert-on-tuple
warning/attribute-defined-outside-init
warning/bad-builtin
warning/bad-chained-comparison
warning/bad-dunder-name
warning/bad-format-string
warning/bad-format-string-key
Expand Down
4 changes: 4 additions & 0 deletions doc/whatsnew/fragments/6559.new_check
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Add a ``bad-chained-comparison`` check that emits a warning when
there is a chained comparison where one expression is semantically incompatible with the other.

Closes #6559
61 changes: 61 additions & 0 deletions pylint/checkers/bad_chained_comparison.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE
# Copyright (c) https://github.com/PyCQA/pylint/blob/main/CONTRIBUTORS.txt

from __future__ import annotations

from typing import TYPE_CHECKING

from astroid import nodes

from pylint.checkers import BaseChecker
from pylint.interfaces import HIGH

if TYPE_CHECKING:
from pylint.lint import PyLinter

COMPARISON_OP = frozenset(("<", "<=", ">", ">=", "!=", "=="))
IDENTITY_OP = frozenset(("is", "is not"))
MEMBERSHIP_OP = frozenset(("in", "not in"))


class BadChainedComparisonChecker(BaseChecker):
"""Checks for unintentional usage of chained comparison."""

name = "bad-chained-comparison"
msgs = {
"W3501": (
"Suspicious %s-part chained comparison using semantically incompatible operators (%s)",
"bad-chained-comparison",
"Used when there is a chained comparison where one expression is part "
"of two comparisons that belong to different semantic groups "
'("<" does not mean the same thing as "is", chaining them in '
'"0 < x is None" is probably a mistake).',
)
}

def _has_diff_semantic_groups(self, operators: list[str]) -> bool:

# Check if comparison operators are in the same semantic group
for semantic_group in (COMPARISON_OP, IDENTITY_OP, MEMBERSHIP_OP):
if operators[0] in semantic_group:
group = semantic_group
return not all(o in group for o in operators)

def visit_compare(self, node: nodes.Compare) -> None:
operators = sorted({op[0] for op in node.ops})
if self._has_diff_semantic_groups(operators):
num_parts = f"{len(node.ops)}"
incompatibles = (
", ".join(f"'{o}'" for o in operators[:-1]) + f" and '{operators[-1]}'"
)
self.add_message(
"bad-chained-comparison",
node=node,
args=(num_parts, incompatibles),
confidence=HIGH,
)


def register(linter: PyLinter) -> None:
linter.register_checker(BadChainedComparisonChecker(linter))
51 changes: 51 additions & 0 deletions tests/functional/b/bad_chained_comparison.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# pylint: disable=invalid-name
"""Checks for chained comparisons with comparisons belonging to different groups"""

primes = set(2, 3, 5, 7, 11)

def valid(x, y, z):
"""valid usage of chained comparisons"""
if x < y <= z:
pass
elif x > y >= z:
pass
elif x == y != z:
pass
elif x is y is not z:
pass
elif x in y not in z:
pass

def id_comparison_invalid(*, left=None, right=None):
"""identity mixed with comparison"""
if left is None != right is None: # [bad-chained-comparison]
raise ValueError('Either both left= and right= need to be provided or none should.')
if left is not None == right is not None: # [bad-chained-comparison]
pass

def member_comparison_invalid(x:int, y:int, z:int):
"""membership mixed with comparison"""
if x in primes == y in primes: # [bad-chained-comparison]
pass
elif x in primes == z not in primes: # [bad-chained-comparison]
pass
elif x not in primes == y in primes != z not in primes: # [bad-chained-comparison]
pass
elif x not in primes <= y not in primes > z in primes: # [bad-chained-comparison]
pass

def id_member_invalid(x:int, y:int, z:int):
"""identity mixed with membership"""
if x in primes is y in primes: # [bad-chained-comparison]
pass
elif x in primes is z not in primes: # [bad-chained-comparison]
pass
elif x not in primes is y in primes is not z not in primes: # [bad-chained-comparison]
pass

def complex_invalid(x:int, y:int, z:int):
"""invalid complex chained comparisons"""
if x in primes == y not in primes is not z in primes: # [bad-chained-comparison]
pass
elif x < y <= z != x not in primes is y in primes: # [bad-chained-comparison]
pass
11 changes: 11 additions & 0 deletions tests/functional/b/bad_chained_comparison.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
bad-chained-comparison:21:7:21:36:id_comparison_invalid:Suspicious 3-part chained comparison using semantically incompatible operators ('!=' and 'is'):HIGH
bad-chained-comparison:23:7:23:44:id_comparison_invalid:Suspicious 3-part chained comparison using semantically incompatible operators ('==' and 'is not'):HIGH
bad-chained-comparison:28:7:28:33:member_comparison_invalid:Suspicious 3-part chained comparison using semantically incompatible operators ('==' and 'in'):HIGH
bad-chained-comparison:30:9:30:39:member_comparison_invalid:Suspicious 3-part chained comparison using semantically incompatible operators ('==', 'in' and 'not in'):HIGH
bad-chained-comparison:32:9:32:58:member_comparison_invalid:Suspicious 5-part chained comparison using semantically incompatible operators ('!=', '==', 'in' and 'not in'):HIGH
bad-chained-comparison:34:9:34:57:member_comparison_invalid:Suspicious 5-part chained comparison using semantically incompatible operators ('<=', '>', 'in' and 'not in'):HIGH
bad-chained-comparison:39:7:39:33:id_member_invalid:Suspicious 3-part chained comparison using semantically incompatible operators ('in' and 'is'):HIGH
bad-chained-comparison:41:9:41:39:id_member_invalid:Suspicious 3-part chained comparison using semantically incompatible operators ('in', 'is' and 'not in'):HIGH
bad-chained-comparison:43:9:43:62:id_member_invalid:Suspicious 5-part chained comparison using semantically incompatible operators ('in', 'is', 'is not' and 'not in'):HIGH
bad-chained-comparison:48:7:48:56:complex_invalid:Suspicious 5-part chained comparison using semantically incompatible operators ('==', 'in', 'is not' and 'not in'):HIGH
bad-chained-comparison:50:9:50:53:complex_invalid:Suspicious 6-part chained comparison using semantically incompatible operators ('!=', '<', '<=', 'in', 'is' and 'not in'):HIGH
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# pylint: disable=missing-docstring, expression-not-assigned, too-few-public-methods
# pylint: disable=missing-docstring, expression-not-assigned, too-few-public-methods, bad-chained-comparison
# pylint: disable=no-member, import-error, line-too-long
# pylint: disable=unnecessary-comprehension, use-dict-literal, use-implicit-booleaness-not-comparison

Expand Down
2 changes: 1 addition & 1 deletion tests/functional/ext/set_membership/use_set_membership.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# pylint: disable=invalid-name,missing-docstring,pointless-statement,unnecessary-comprehension,undefined-variable
# pylint: disable=invalid-name,missing-docstring,pointless-statement,unnecessary-comprehension,undefined-variable,bad-chained-comparison

x = 1
var = frozenset({1, 2, 3})
Expand Down