Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
add checker for accidental constants in boolean operators (#7273)
### Problem It's possible to accidentally write something like `None or 'default_value'`, which is often an error indicating that the original value is dropped. One example is in 13b8505 for #7269 just now. ### Solution - Add a checker for `and`/`or` expressions which ensures that the left-hand side is probably not a constant, and ensures that the right-hand side is probably not a constant for `and` expressions. ### Result One possible class of error that I just made is impossible.
- Loading branch information
1 parent
8da3654
commit 3c5c70f
Showing
7 changed files
with
92 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
60 changes: 60 additions & 0 deletions
60
contrib/python/src/python/pants/contrib/python/checks/checker/constant_logic.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
# coding=utf-8 | ||
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import absolute_import, division, print_function, unicode_literals | ||
|
||
import ast | ||
|
||
from future.utils import PY3 | ||
|
||
from pants.contrib.python.checks.checker.common import CheckstylePlugin | ||
|
||
|
||
class ConstantLogic(CheckstylePlugin): | ||
"""Check for constants provided to boolean operators which result in a constant expression.""" | ||
|
||
@classmethod | ||
def name(cls): | ||
return 'constant-logic' | ||
|
||
@classmethod | ||
def iter_bool_ops(cls, tree): | ||
for ast_node in ast.walk(tree): | ||
if isinstance(ast_node, ast.BoolOp) and isinstance(ast_node.op, (ast.And, ast.Or)): | ||
yield ast_node | ||
|
||
@classmethod | ||
def is_name_constant(cls, expr): | ||
if PY3: | ||
if isinstance(expr, ast.NameConstant): | ||
return True | ||
else: | ||
if isinstance(expr, ast.Name) and expr.id in ['True', 'False', 'None']: | ||
return True | ||
return False | ||
|
||
@classmethod | ||
def is_probably_constant(cls, expr): | ||
if isinstance(expr, (ast.Num, ast.Str)): | ||
return True | ||
if cls.is_name_constant(expr): | ||
return True | ||
return False | ||
|
||
def nits(self): | ||
for bool_op in self.iter_bool_ops(self.python_file.tree): | ||
# We don't try to check anything in the middle of logical expressions with more than two | ||
# values. | ||
leftmost = bool_op.values[0] | ||
rightmost = bool_op.values[-1] | ||
if self.is_probably_constant(leftmost): | ||
yield self.error('T804', | ||
'You are using a constant on the left-hand side of a logical operator. ' | ||
'This is probably an error.', | ||
bool_op) | ||
if isinstance(bool_op.op, ast.And) and self.is_probably_constant(rightmost): | ||
yield self.error('T805', | ||
'You are using a constant on the right-hand side of an `and` operator. ' | ||
'This is probably an error.', | ||
bool_op) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 changes: 26 additions & 0 deletions
26
contrib/python/tests/python/pants_test/contrib/python/checks/checker/test_constant_logic.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# coding=utf-8 | ||
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import absolute_import, division, print_function, unicode_literals | ||
|
||
from pants_test.contrib.python.checks.checker.plugin_test_base import CheckstylePluginTestBase | ||
|
||
from pants.contrib.python.checks.checker.constant_logic import ConstantLogic | ||
|
||
|
||
class ConstantLogicTest(CheckstylePluginTestBase): | ||
plugin_type = ConstantLogic | ||
|
||
def test_or(self): | ||
self.assertNit('None or x', 'T804') | ||
self.assertNit('True or x', 'T804') | ||
self.assertNit('False or x', 'T804') | ||
self.assertNit('1 or x', 'T804') | ||
self.assertNit('"a" or x', 'T804') | ||
self.assertNoNits('x or y') | ||
|
||
def test_and(self): | ||
self.assertNit('None and x', 'T804') | ||
self.assertNit('x and False', 'T805') | ||
self.assertNoNits('x and y') |