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 comparison-of-constants
checker
#6413
Changes from 12 commits
dd414ae
06867f8
72730a5
31327ef
8a363d7
c8500f0
1ae78f4
4da21c9
23abb00
336f8c3
b0d4fa7
a24f94a
72a9e20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
def is_the_answer() -> bool: | ||
return 42 == 42 # [comparison-of-constants] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
def is_the_answer(meaning_of_life: int) -> bool: | ||
return meaning_of_life == 42 |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -9,6 +9,7 @@ | |||||||||||||
|
||||||||||||||
from pylint.checkers import utils | ||||||||||||||
from pylint.checkers.base.basic_checker import _BasicChecker | ||||||||||||||
from pylint.interfaces import HIGH | ||||||||||||||
|
||||||||||||||
LITERAL_NODE_TYPES = (nodes.Const, nodes.Dict, nodes.List, nodes.Set) | ||||||||||||||
COMPARISON_OPERATORS = frozenset(("==", "!=", "<", ">", "<=", ">=")) | ||||||||||||||
|
@@ -58,6 +59,13 @@ class ComparisonChecker(_BasicChecker): | |||||||||||||
"comparison-with-itself", | ||||||||||||||
"Used when something is compared against itself.", | ||||||||||||||
), | ||||||||||||||
"R0133": ( | ||||||||||||||
"Comparison between constants: '%s %s %s' is also a constant, remove the comparison and consider a refactor", | ||||||||||||||
"comparison-of-constants", | ||||||||||||||
"When two literals are compared with each other the result is a constant, " | ||||||||||||||
"and using the constant directly is both easier to read and more performant." | ||||||||||||||
" Initializing 'True' and 'False' this way is not required since python 2.3", | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
), | ||||||||||||||
"W0143": ( | ||||||||||||||
"Comparing against a callable, did you omit the parenthesis?", | ||||||||||||||
"comparison-with-callable", | ||||||||||||||
|
@@ -212,6 +220,24 @@ def _check_logical_tautology(self, node: nodes.Compare): | |||||||||||||
suggestion = f"{left_operand} {operator} {right_operand}" | ||||||||||||||
self.add_message("comparison-with-itself", node=node, args=(suggestion,)) | ||||||||||||||
|
||||||||||||||
def _check_two_literals_being_compared(self, node: nodes.Compare) -> None: | ||||||||||||||
"""Check if two literals are being compared; this is always a logical tautology.""" | ||||||||||||||
left_operand = node.left | ||||||||||||||
if not isinstance(left_operand, nodes.Const): | ||||||||||||||
return | ||||||||||||||
|
||||||||||||||
right_operand = node.ops[0][1] | ||||||||||||||
if not isinstance(right_operand, nodes.Const): | ||||||||||||||
return | ||||||||||||||
|
||||||||||||||
operator = node.ops[0][0] | ||||||||||||||
self.add_message( | ||||||||||||||
"comparison-of-constants", | ||||||||||||||
node=node, | ||||||||||||||
args=(left_operand.value, operator, right_operand.value), | ||||||||||||||
omarandlorraine marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
confidence=HIGH, | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
def _check_callable_comparison(self, node): | ||||||||||||||
operator = node.ops[0][0] | ||||||||||||||
if operator not in COMPARISON_OPERATORS: | ||||||||||||||
|
@@ -240,13 +266,15 @@ def _check_callable_comparison(self, node): | |||||||||||||
"unidiomatic-typecheck", | ||||||||||||||
"literal-comparison", | ||||||||||||||
"comparison-with-itself", | ||||||||||||||
"comparison-of-constants", | ||||||||||||||
"comparison-with-callable", | ||||||||||||||
"nan-comparison", | ||||||||||||||
) | ||||||||||||||
def visit_compare(self, node: nodes.Compare) -> None: | ||||||||||||||
self._check_callable_comparison(node) | ||||||||||||||
self._check_logical_tautology(node) | ||||||||||||||
self._check_unidiomatic_typecheck(node) | ||||||||||||||
self._check_two_literals_being_compared(node) | ||||||||||||||
# NOTE: this checker only works with binary comparisons like 'x == 42' | ||||||||||||||
# but not 'x == y == 42' | ||||||||||||||
if len(node.ops) != 1: | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,9 +85,9 @@ def __le__(self, other): | |
Test a correct access as the access to protected member | ||
is inside a special method even if it is deeply nested | ||
""" | ||
if 2 > 1: | ||
if 2 > 1: # pylint: disable=comparison-of-constants | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally we tend to disable at the file-level instead of in-line. We might want to keep that style here? @Pierre-Sassoulas Do you have an opinion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about this too, but I don't mind some tests for inline disabling sprinkled in our functional tests. This is something that is very hard to tests extensively if at some point we need to touch the disable code. Also not disabling everything permits to realize if there's issue with a change because our functional tests have very diverse code. A compromise has to be made between too much and zero, as it's also more work than just disabling per file. I think in this case it's ok. |
||
if isinstance(other, self.__class__): | ||
if "answer" == "42": | ||
if "answer" == "42": # pylint: disable=comparison-of-constants | ||
return self._foo == other._foo | ||
return False | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,6 +1,6 @@ | ||||||
'''Assert check example''' | ||||||
|
||||||
# pylint: disable=comparison-with-itself | ||||||
# pylint: disable=comparison-with-itself comparison-of-constants | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
assert (1 == 1, 2 == 2), "no error" | ||||||
assert (1 == 1, 2 == 2) # [assert-on-tuple] | ||||||
assert 1 == 1, "no error" | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
# pylint: disable=missing-docstring, comparison-with-itself, invalid-name | ||
|
||
|
||
if 2 is 2: # [literal-comparison, comparison-of-constants] | ||
pass | ||
|
||
while 2 == 2: # [comparison-of-constants] | ||
pass | ||
|
||
while 2 > 2: # [comparison-of-constants] | ||
pass | ||
|
||
n = 2 | ||
if 2 != n: | ||
pass | ||
|
||
if n != 1 + 1: | ||
pass | ||
|
||
if True == True: # [comparison-of-constants, singleton-comparison] | ||
pass | ||
|
||
CONST = 24 | ||
|
||
if CONST is 0: # [literal-comparison] | ||
pass | ||
|
||
if CONST is 1: # [literal-comparison] | ||
pass | ||
|
||
if CONST is 42: # [literal-comparison] | ||
pass | ||
|
||
if 0 < CONST < 42: | ||
pass | ||
|
||
if 0 < n < 42: | ||
pass | ||
|
||
if True == n != 42: | ||
pass | ||
|
||
if 0 == n != 42: | ||
pass | ||
|
||
|
||
print(0 < n < 42) | ||
print(0 <= n < 42 ) | ||
print(n < 1 < n*42 < 42) | ||
print(42> n <= 0) | ||
print(0 == n > 42) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
comparison-of-constants:4:3:4:9::"Comparison between constants: '2 is 2' is also a constant, remove the comparison and consider a refactor":HIGH | ||
literal-comparison:4:3:4:9::Comparison to literal:UNDEFINED | ||
comparison-of-constants:7:6:7:12::"Comparison between constants: '2 == 2' is also a constant, remove the comparison and consider a refactor":HIGH | ||
comparison-of-constants:10:6:10:11::"Comparison between constants: '2 > 2' is also a constant, remove the comparison and consider a refactor":HIGH | ||
comparison-of-constants:20:3:20:15::"Comparison between constants: 'True == True' is also a constant, remove the comparison and consider a refactor":HIGH | ||
singleton-comparison:20:3:20:15::Comparison 'True == True' should be 'True is True' if checking for the singleton value True, or 'True' if testing for truthiness:UNDEFINED | ||
literal-comparison:25:3:25:13::Comparison to literal:UNDEFINED | ||
literal-comparison:28:3:28:13::Comparison to literal:UNDEFINED | ||
literal-comparison:31:3:31:14::Comparison to literal:UNDEFINED |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
misplaced-comparison-constant:19:11:19:17:bad_comparisons:Comparison should be i >= 5:UNDEFINED | ||
misplaced-comparison-constant:21:11:21:17:bad_comparisons:Comparison should be i == 1:UNDEFINED | ||
misplaced-comparison-constant:23:11:23:29:bad_comparisons:Comparison should be dummy_return() > 3:UNDEFINED | ||
misplaced-comparison-constant:25:11:25:39:bad_comparisons:Comparison should be instance.dummy_return() != 4:UNDEFINED | ||
misplaced-comparison-constant:27:11:27:29:bad_comparisons:Comparison should be instance.attr == 1:UNDEFINED | ||
misplaced-comparison-constant:29:11:29:33:bad_comparisons:Comparison should be instance.attr == 'aaa':UNDEFINED | ||
misplaced-comparison-constant:20:11:20:17:bad_comparisons:Comparison should be i >= 5:UNDEFINED | ||
misplaced-comparison-constant:22:11:22:17:bad_comparisons:Comparison should be i == 1:UNDEFINED | ||
misplaced-comparison-constant:24:11:24:29:bad_comparisons:Comparison should be dummy_return() > 3:UNDEFINED | ||
misplaced-comparison-constant:26:11:26:39:bad_comparisons:Comparison should be instance.dummy_return() != 4:UNDEFINED | ||
misplaced-comparison-constant:28:11:28:29:bad_comparisons:Comparison should be instance.attr == 1:UNDEFINED | ||
misplaced-comparison-constant:30:11:30:33:bad_comparisons:Comparison should be instance.attr == 'aaa':UNDEFINED |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,11 @@ | ||
import-outside-toplevel:10:4:10:19:f:Import outside toplevel (symtable):UNDEFINED | ||
import-outside-toplevel:14:4:14:18:g:Import outside toplevel (os, sys):UNDEFINED | ||
import-outside-toplevel:18:4:18:24:h:Import outside toplevel (time):UNDEFINED | ||
import-outside-toplevel:22:4:22:41:i:Import outside toplevel (random, socket):UNDEFINED | ||
import-outside-toplevel:26:4:26:19:C:Import outside toplevel (tokenize):UNDEFINED | ||
import-outside-toplevel:29:8:29:21:C.j:Import outside toplevel (turtle):UNDEFINED | ||
import-outside-toplevel:34:8:34:23:k:Import outside toplevel (tabnanny):UNDEFINED | ||
import-outside-toplevel:38:4:38:39:j:Import outside toplevel (collections.defaultdict):UNDEFINED | ||
import-outside-toplevel:42:4:42:48:m:Import outside toplevel (math.sin, math.cos):UNDEFINED | ||
import-error:50:4:50:21:o:Unable to import 'notastroid':UNDEFINED | ||
import-outside-toplevel:50:4:50:21:o:Import outside toplevel (notastroid):UNDEFINED | ||
import-outside-toplevel:11:4:11:19:f:Import outside toplevel (symtable):UNDEFINED | ||
import-outside-toplevel:15:4:15:18:g:Import outside toplevel (os, sys):UNDEFINED | ||
import-outside-toplevel:19:4:19:24:h:Import outside toplevel (time):UNDEFINED | ||
import-outside-toplevel:23:4:23:41:i:Import outside toplevel (random, socket):UNDEFINED | ||
import-outside-toplevel:27:4:27:19:C:Import outside toplevel (tokenize):UNDEFINED | ||
import-outside-toplevel:30:8:30:21:C.j:Import outside toplevel (turtle):UNDEFINED | ||
import-outside-toplevel:35:8:35:23:k:Import outside toplevel (tabnanny):UNDEFINED | ||
import-outside-toplevel:39:4:39:39:j:Import outside toplevel (collections.defaultdict):UNDEFINED | ||
import-outside-toplevel:43:4:43:48:m:Import outside toplevel (math.sin, math.cos):UNDEFINED | ||
import-error:51:4:51:21:o:Unable to import 'notastroid':UNDEFINED | ||
import-outside-toplevel:51:4:51:21:o:Import outside toplevel (notastroid):UNDEFINED |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,21 +11,21 @@ def foo(): | |
return True | ||
elif arg <= arg: # [comparison-with-itself] | ||
return True | ||
elif None == None: # [comparison-with-itself] | ||
elif None == None: # [comparison-of-constants, comparison-with-itself] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we're updating these comments as well, could you add the double spaces here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll try using the autoformatter locally on my machine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did this work? |
||
return None | ||
elif 786 == 786: # [comparison-with-itself] | ||
elif 786 == 786: # [comparison-of-constants, comparison-with-itself] | ||
return True | ||
elif 786 is 786: # [comparison-with-itself] | ||
elif 786 is 786: # [comparison-of-constants, comparison-with-itself] | ||
return True | ||
elif 786 is not 786: # [comparison-with-itself] | ||
elif 786 is not 786: # [comparison-of-constants, comparison-with-itself] | ||
return True | ||
elif arg is arg: # [comparison-with-itself] | ||
return True | ||
elif arg is not arg: # [comparison-with-itself] | ||
return True | ||
elif True is True: # [comparison-with-itself] | ||
elif True is True: # [comparison-of-constants, comparison-with-itself] | ||
return True | ||
elif 666 == 786: | ||
elif 666 == 786: # [comparison-of-constants] | ||
return False | ||
else: | ||
return None | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
misplaced-bare-raise:5:4:5:9::The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:35:16:35:21:test1.best:The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:38:4:38:9:test1:The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:39:0:39:5::The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:48:4:48:9::The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:56:4:56:9:A:The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:67:4:67:9::The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:6:4:6:9::The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:36:16:36:21:test1.best:The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:39:4:39:9:test1:The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:40:0:40:5::The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:49:4:49:9::The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:57:4:57:9:A:The raise statement is not inside an except clause:UNDEFINED | ||
misplaced-bare-raise:68:4:68:9::The raise statement is not inside an except clause:UNDEFINED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
R
inR0125
implies that the code should be refactored so I think we can leave that out of the message here.