Skip to content

Commit 339f16f

Browse files
ethan-lebaPierre-Sassoulas
authored andcommitted
Improve lint message for singleton-comparison with bools
1 parent 1e1fe19 commit 339f16f

File tree

5 files changed

+117
-46
lines changed

5 files changed

+117
-46
lines changed

ChangeLog

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ Release date: TBA
2525

2626
* Fix ``duplicate-code`` false positive when lines only contain whitespace and non-alphanumeric characters (e.g. parentheses, bracket, comman, etc.)
2727

28+
* Improve lint message for `singleton-comparison` with bools
29+
2830
What's New in Pylint 2.6.0?
2931
===========================
3032

pylint/checkers/base.py

Lines changed: 59 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -2297,7 +2297,7 @@ class ComparisonChecker(_BasicChecker):
22972297

22982298
msgs = {
22992299
"C0121": (
2300-
"Comparison to %s should be %s",
2300+
"Comparison %s should be %s",
23012301
"singleton-comparison",
23022302
"Used when an expression is compared to singleton "
23032303
"values like True, False or None.",
@@ -2339,31 +2339,63 @@ class ComparisonChecker(_BasicChecker):
23392339
),
23402340
}
23412341

2342-
def _check_singleton_comparison(self, singleton, root_node, negative_check=False):
2343-
if singleton.value is True:
2344-
if not negative_check:
2345-
suggestion = "just 'expr'"
2346-
else:
2347-
suggestion = "just 'not expr'"
2348-
self.add_message(
2349-
"singleton-comparison", node=root_node, args=(True, suggestion)
2342+
def _check_singleton_comparison(
2343+
self, left_value, right_value, root_node, checking_for_absence: bool = False
2344+
):
2345+
"""Check if == or != is being used to compare a singleton value"""
2346+
singleton_values = (True, False, None)
2347+
2348+
def _is_singleton_const(node) -> bool:
2349+
return isinstance(node, astroid.Const) and any(
2350+
node.value is value for value in singleton_values
23502351
)
2351-
elif singleton.value is False:
2352-
if not negative_check:
2353-
suggestion = "'not expr'"
2354-
else:
2355-
suggestion = "'expr'"
2356-
self.add_message(
2357-
"singleton-comparison", node=root_node, args=(False, suggestion)
2352+
2353+
if _is_singleton_const(left_value):
2354+
singleton, other_value = left_value.value, right_value
2355+
elif _is_singleton_const(right_value):
2356+
singleton, other_value = right_value.value, left_value
2357+
else:
2358+
return
2359+
2360+
singleton_comparison_example = {False: "'{} is {}'", True: "'{} is not {}'"}
2361+
2362+
# True/False singletons have a special-cased message in case the user is
2363+
# mistakenly using == or != to check for truthiness
2364+
if singleton in (True, False):
2365+
suggestion_template = (
2366+
"{} if checking for the singleton value {}, or {} if testing for {}"
23582367
)
2359-
elif singleton.value is None:
2360-
if not negative_check:
2361-
suggestion = "'expr is None'"
2362-
else:
2363-
suggestion = "'expr is not None'"
2364-
self.add_message(
2365-
"singleton-comparison", node=root_node, args=(None, suggestion)
2368+
truthiness_example = {False: "not {}", True: "{}"}
2369+
truthiness_phrase = {True: "truthiness", False: "falsiness"}
2370+
2371+
# Looks for comparisons like x == True or x != False
2372+
checking_truthiness = singleton is not checking_for_absence
2373+
2374+
suggestion = suggestion_template.format(
2375+
singleton_comparison_example[checking_for_absence].format(
2376+
left_value.as_string(), right_value.as_string()
2377+
),
2378+
singleton,
2379+
(
2380+
"'bool({})'"
2381+
if not utils.is_test_condition(root_node) and checking_truthiness
2382+
else "'{}'"
2383+
).format(
2384+
truthiness_example[checking_truthiness].format(
2385+
other_value.as_string()
2386+
)
2387+
),
2388+
truthiness_phrase[checking_truthiness],
23662389
)
2390+
else:
2391+
suggestion = singleton_comparison_example[checking_for_absence].format(
2392+
left_value.as_string(), right_value.as_string()
2393+
)
2394+
self.add_message(
2395+
"singleton-comparison",
2396+
node=root_node,
2397+
args=("'{}'".format(root_node.as_string()), suggestion),
2398+
)
23672399

23682400
def _check_literal_comparison(self, literal, node):
23692401
"""Check if we compare to a literal, which is usually what we do not want to do."""
@@ -2454,14 +2486,10 @@ def visit_compare(self, node):
24542486
if operator in COMPARISON_OPERATORS and isinstance(left, astroid.Const):
24552487
self._check_misplaced_constant(node, left, right, operator)
24562488

2457-
if operator == "==":
2458-
if isinstance(left, astroid.Const):
2459-
self._check_singleton_comparison(left, node)
2460-
elif isinstance(right, astroid.Const):
2461-
self._check_singleton_comparison(right, node)
2462-
if operator == "!=":
2463-
if isinstance(right, astroid.Const):
2464-
self._check_singleton_comparison(right, node, negative_check=True)
2489+
if operator in ("==", "!="):
2490+
self._check_singleton_comparison(
2491+
left, right, node, checking_for_absence=operator == "!="
2492+
)
24652493
if operator in ("is", "is not"):
24662494
self._check_literal_comparison(right, node)
24672495

tests/checkers/unittest_base.py

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -414,42 +414,74 @@ class TestComparison(CheckerTestCase):
414414

415415
def test_comparison(self):
416416
node = astroid.extract_node("foo == True")
417-
message = Message("singleton-comparison", node=node, args=(True, "just 'expr'"))
417+
message = Message(
418+
"singleton-comparison",
419+
node=node,
420+
args=(
421+
"'foo == True'",
422+
"'foo is True' if checking for the singleton value True, or 'bool(foo)' if testing for truthiness",
423+
),
424+
)
418425
with self.assertAddsMessages(message):
419426
self.checker.visit_compare(node)
420427

421428
node = astroid.extract_node("foo == False")
422-
message = Message("singleton-comparison", node=node, args=(False, "'not expr'"))
429+
message = Message(
430+
"singleton-comparison",
431+
node=node,
432+
args=(
433+
"'foo == False'",
434+
"'foo is False' if checking for the singleton value False, or 'not foo' if testing for falsiness",
435+
),
436+
)
423437
with self.assertAddsMessages(message):
424438
self.checker.visit_compare(node)
425439

426440
node = astroid.extract_node("foo == None")
427441
message = Message(
428-
"singleton-comparison", node=node, args=(None, "'expr is None'")
442+
"singleton-comparison", node=node, args=("'foo == None'", "'foo is None'")
429443
)
430444
with self.assertAddsMessages(message):
431445
self.checker.visit_compare(node)
432446

433447
node = astroid.extract_node("True == foo")
434448
messages = (
435449
Message("misplaced-comparison-constant", node=node, args=("foo == True",)),
436-
Message("singleton-comparison", node=node, args=(True, "just 'expr'")),
450+
Message(
451+
"singleton-comparison",
452+
node=node,
453+
args=(
454+
"'True == foo'",
455+
"'True is foo' if checking for the singleton value True, or 'bool(foo)' if testing for truthiness",
456+
),
457+
),
437458
)
438459
with self.assertAddsMessages(*messages):
439460
self.checker.visit_compare(node)
440461

441462
node = astroid.extract_node("False == foo")
442463
messages = (
443464
Message("misplaced-comparison-constant", node=node, args=("foo == False",)),
444-
Message("singleton-comparison", node=node, args=(False, "'not expr'")),
465+
Message(
466+
"singleton-comparison",
467+
node=node,
468+
args=(
469+
"'False == foo'",
470+
"'False is foo' if checking for the singleton value False, or 'not foo' if testing for falsiness",
471+
),
472+
),
445473
)
446474
with self.assertAddsMessages(*messages):
447475
self.checker.visit_compare(node)
448476

449477
node = astroid.extract_node("None == foo")
450478
messages = (
451479
Message("misplaced-comparison-constant", node=node, args=("foo == None",)),
452-
Message("singleton-comparison", node=node, args=(None, "'expr is None'")),
480+
Message(
481+
"singleton-comparison",
482+
node=node,
483+
args=("'None == foo'", "'None is foo'"),
484+
),
453485
)
454486
with self.assertAddsMessages(*messages):
455487
self.checker.visit_compare(node)

tests/functional/s/singleton_comparison.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,9 @@
1212

1313
j = x != True # [singleton-comparison]
1414
j1 = x != False # [singleton-comparison]
15-
j2 = x != None # [singleton-comparison]
15+
j2 = x != None # [singleton-comparison]
16+
assert x == True # [singleton-comparison]
17+
assert x != False # [singleton-comparison]
18+
if x == True: # [singleton-comparison]
19+
pass
20+
z = bool(x == True) # [singleton-comparison]
Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
1-
singleton-comparison:4::Comparison to None should be 'expr is None'
2-
singleton-comparison:5::Comparison to True should be just 'expr'
3-
singleton-comparison:6::Comparison to False should be 'not expr'
4-
singleton-comparison:7::Comparison to True should be just 'expr'
5-
singleton-comparison:11::Comparison to None should be 'expr is None'
6-
singleton-comparison:13::Comparison to True should be just 'not expr'
7-
singleton-comparison:14::Comparison to False should be 'expr'
8-
singleton-comparison:15::Comparison to None should be 'expr is not None'
1+
singleton-comparison:4::Comparison 'x == None' should be 'x is None'
2+
singleton-comparison:5::Comparison 'x == True' should be 'x is True' if checking for the singleton value True, or 'bool(x)' if testing for truthiness
3+
singleton-comparison:6::Comparison 'x == False' should be 'x is False' if checking for the singleton value False, or 'not x' if testing for falsiness
4+
singleton-comparison:7::Comparison 'True == True' should be 'True is True' if checking for the singleton value True, or 'bool(True)' if testing for truthiness
5+
singleton-comparison:11::Comparison 'None == x' should be 'None is x'
6+
singleton-comparison:13::Comparison 'x != True' should be 'x is not True' if checking for the singleton value True, or 'not x' if testing for falsiness
7+
singleton-comparison:14::Comparison 'x != False' should be 'x is not False' if checking for the singleton value False, or 'bool(x)' if testing for truthiness
8+
singleton-comparison:15::Comparison 'x != None' should be 'x is not None'
9+
singleton-comparison:16::Comparison 'x == True' should be 'x is True' if checking for the singleton value True, or 'x' if testing for truthiness
10+
singleton-comparison:17::Comparison 'x != False' should be 'x is not False' if checking for the singleton value False, or 'x' if testing for truthiness
11+
singleton-comparison:18::Comparison 'x == True' should be 'x is True' if checking for the singleton value True, or 'x' if testing for truthiness
12+
singleton-comparison:20::Comparison 'x == True' should be 'x is True' if checking for the singleton value True, or 'x' if testing for truthiness

0 commit comments

Comments
 (0)