From 3c5c70ff07b5a80f4e58288ca7c13e3f6b16a8f2 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Fri, 1 Mar 2019 08:48:57 -0800 Subject: [PATCH] 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 13b850566a527dda67baa8398f33dc63b091c152 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. --- .../pants/contrib/python/checks/checker/BUILD | 1 + .../contrib/python/checks/checker/checker.py | 2 + .../python/checks/checker/constant_logic.py | 60 +++++++++++++++++++ .../checks/checker/except_statements.py | 2 +- .../checks/checker/missing_contextmanager.py | 2 +- .../python/checks/checker/print_statements.py | 2 +- .../checks/checker/test_constant_logic.py | 26 ++++++++ 7 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 contrib/python/src/python/pants/contrib/python/checks/checker/constant_logic.py create mode 100644 contrib/python/tests/python/pants_test/contrib/python/checks/checker/test_constant_logic.py diff --git a/contrib/python/src/python/pants/contrib/python/checks/checker/BUILD b/contrib/python/src/python/pants/contrib/python/checks/checker/BUILD index 6204308e45c..0d326a8c184 100644 --- a/contrib/python/src/python/pants/contrib/python/checks/checker/BUILD +++ b/contrib/python/src/python/pants/contrib/python/checks/checker/BUILD @@ -10,6 +10,7 @@ python_library( } ), dependencies=[ + '3rdparty/python:future', '3rdparty/python:pycodestyle', '3rdparty/python:pyflakes', '3rdparty/python:six', diff --git a/contrib/python/src/python/pants/contrib/python/checks/checker/checker.py b/contrib/python/src/python/pants/contrib/python/checks/checker/checker.py index 6940265d309..e2a80398520 100644 --- a/contrib/python/src/python/pants/contrib/python/checks/checker/checker.py +++ b/contrib/python/src/python/pants/contrib/python/checks/checker/checker.py @@ -13,6 +13,7 @@ from pants.contrib.python.checks.checker.class_factoring import ClassFactoring from pants.contrib.python.checks.checker.common import CheckSyntaxError, Nit, PythonFile +from pants.contrib.python.checks.checker.constant_logic import ConstantLogic from pants.contrib.python.checks.checker.except_statements import ExceptStatements from pants.contrib.python.checks.checker.file_excluder import FileExcluder from pants.contrib.python.checks.checker.future_compatibility import FutureCompatibility @@ -128,6 +129,7 @@ def plugins(): """ return ( ClassFactoring, + ConstantLogic, ExceptStatements, FutureCompatibility, ImportOrder, diff --git a/contrib/python/src/python/pants/contrib/python/checks/checker/constant_logic.py b/contrib/python/src/python/pants/contrib/python/checks/checker/constant_logic.py new file mode 100644 index 00000000000..f653fa72f76 --- /dev/null +++ b/contrib/python/src/python/pants/contrib/python/checks/checker/constant_logic.py @@ -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) diff --git a/contrib/python/src/python/pants/contrib/python/checks/checker/except_statements.py b/contrib/python/src/python/pants/contrib/python/checks/checker/except_statements.py index 97a16557746..8be63af8bbd 100644 --- a/contrib/python/src/python/pants/contrib/python/checks/checker/except_statements.py +++ b/contrib/python/src/python/pants/contrib/python/checks/checker/except_statements.py @@ -6,7 +6,7 @@ import ast -from six import PY3 +from future.utils import PY3 from pants.contrib.python.checks.checker.common import CheckstylePlugin diff --git a/contrib/python/src/python/pants/contrib/python/checks/checker/missing_contextmanager.py b/contrib/python/src/python/pants/contrib/python/checks/checker/missing_contextmanager.py index 64587669730..8d90fcce6d5 100644 --- a/contrib/python/src/python/pants/contrib/python/checks/checker/missing_contextmanager.py +++ b/contrib/python/src/python/pants/contrib/python/checks/checker/missing_contextmanager.py @@ -6,7 +6,7 @@ import ast -from six import PY3 +from future.utils import PY3 from pants.contrib.python.checks.checker.common import CheckstylePlugin diff --git a/contrib/python/src/python/pants/contrib/python/checks/checker/print_statements.py b/contrib/python/src/python/pants/contrib/python/checks/checker/print_statements.py index 78546f5fdc8..9b263d07d75 100644 --- a/contrib/python/src/python/pants/contrib/python/checks/checker/print_statements.py +++ b/contrib/python/src/python/pants/contrib/python/checks/checker/print_statements.py @@ -7,7 +7,7 @@ import ast import re -from six import PY3 +from future.utils import PY3 from pants.contrib.python.checks.checker.common import CheckstylePlugin diff --git a/contrib/python/tests/python/pants_test/contrib/python/checks/checker/test_constant_logic.py b/contrib/python/tests/python/pants_test/contrib/python/checks/checker/test_constant_logic.py new file mode 100644 index 00000000000..88bb8e307d3 --- /dev/null +++ b/contrib/python/tests/python/pants_test/contrib/python/checks/checker/test_constant_logic.py @@ -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')