diff --git a/doc/data/messages/b/bad-chained-comparison/bad.py b/doc/data/messages/b/bad-chained-comparison/bad.py new file mode 100644 index 0000000000..eeca75f13e --- /dev/null +++ b/doc/data/messages/b/bad-chained-comparison/bad.py @@ -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.') diff --git a/doc/data/messages/b/bad-chained-comparison/good.py b/doc/data/messages/b/bad-chained-comparison/good.py new file mode 100644 index 0000000000..115f4a9db0 --- /dev/null +++ b/doc/data/messages/b/bad-chained-comparison/good.py @@ -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.') diff --git a/doc/data/messages/b/bad-chained-comparison/related.rst b/doc/data/messages/b/bad-chained-comparison/related.rst new file mode 100644 index 0000000000..620ba6df35 --- /dev/null +++ b/doc/data/messages/b/bad-chained-comparison/related.rst @@ -0,0 +1 @@ +- `Comparison Chaining `_ diff --git a/doc/user_guide/checkers/features.rst b/doc/user_guide/checkers/features.rst index ac17fabb66..d5bceae57a 100644 --- a/doc/user_guide/checkers/features.rst +++ b/doc/user_guide/checkers/features.rst @@ -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 ~~~~~~~~~~~~~ diff --git a/doc/user_guide/messages/messages_overview.rst b/doc/user_guide/messages/messages_overview.rst index 6c4405b429..a1962f8e0c 100644 --- a/doc/user_guide/messages/messages_overview.rst +++ b/doc/user_guide/messages/messages_overview.rst @@ -209,6 +209,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 diff --git a/doc/whatsnew/fragments/6559.new_check b/doc/whatsnew/fragments/6559.new_check new file mode 100644 index 0000000000..fa4f146e3d --- /dev/null +++ b/doc/whatsnew/fragments/6559.new_check @@ -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 diff --git a/pylint/checkers/bad_chained_comparison.py b/pylint/checkers/bad_chained_comparison.py new file mode 100644 index 0000000000..fa75c52b2a --- /dev/null +++ b/pylint/checkers/bad_chained_comparison.py @@ -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)) diff --git a/tests/functional/b/bad_chained_comparison.py b/tests/functional/b/bad_chained_comparison.py new file mode 100644 index 0000000000..c1bdab9809 --- /dev/null +++ b/tests/functional/b/bad_chained_comparison.py @@ -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 diff --git a/tests/functional/b/bad_chained_comparison.txt b/tests/functional/b/bad_chained_comparison.txt new file mode 100644 index 0000000000..276a1debce --- /dev/null +++ b/tests/functional/b/bad_chained_comparison.txt @@ -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 diff --git a/tests/functional/c/consider/consider_iterating_dictionary.py b/tests/functional/c/consider/consider_iterating_dictionary.py index 8c75b4e3ef..91402bf772 100644 --- a/tests/functional/c/consider/consider_iterating_dictionary.py +++ b/tests/functional/c/consider/consider_iterating_dictionary.py @@ -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 diff --git a/tests/functional/ext/set_membership/use_set_membership.py b/tests/functional/ext/set_membership/use_set_membership.py index 7872d7f98f..df5cd961b7 100644 --- a/tests/functional/ext/set_membership/use_set_membership.py +++ b/tests/functional/ext/set_membership/use_set_membership.py @@ -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})