From f91a1aab67b386c1726556ce081c57ab51cc1cf9 Mon Sep 17 00:00:00 2001 From: Alex Kapranoff Date: Sun, 20 Sep 2020 02:06:38 -0700 Subject: [PATCH 1/2] Add a rule to catch some incorrect uses of assertTrue() assertTrue() calls with 2 arguments when the intention is clearly to compare the values are surprisingly common. This is valid syntax, but completely incorrect semantics as no comparison is done and the second argument is used as the assertion failure message. See some examples: https://grep.app/search?q=self%5C.assertTrue%5C%28.%2B%2C%5Cs%2A%28%5B%2B-%5D%3F%280%5Bxb%5D%29%3F%5Cd%2B%28%5C.%5Cd%2B%29%3F%7CTrue%7C%5C%5B.%2A%5C%5D%29%5C%29%24®exp=true&filter[lang][0]=Python Some incorrect cases are not caught by this rule because there's no way to always reliably determine the role of the second argument. One example is: https://github.com/vibora-io/vibora/blob/master/tests/schemas/schemas.py#L18 --- fixit/rules/no_assert_true_for_comparison.py | 112 +++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 fixit/rules/no_assert_true_for_comparison.py diff --git a/fixit/rules/no_assert_true_for_comparison.py b/fixit/rules/no_assert_true_for_comparison.py new file mode 100644 index 00000000..33c4ea8b --- /dev/null +++ b/fixit/rules/no_assert_true_for_comparison.py @@ -0,0 +1,112 @@ +# Copyright (c) Facebook, Inc. and its affiliates. +# +# This source code is licensed under the MIT license found in the +# LICENSE file in the root directory of this source tree. + +import libcst as cst +import libcst.matchers as m + +from fixit import CstLintRule, InvalidTestCase as Invalid, ValidTestCase as Valid + + +class NoAssertTrueForComparisonsRule(CstLintRule): + """ + Finds incorrect use of ``assertTrue`` when the intention is to compare two values. + These calls are replaced with ``assertEqual``. + Comparisons with True, False and None are replaced with one-argument + calls to ``assertTrue``, ``assertFalse`` and ``assertIsNone``. + """ + + MESSAGE: str = ( + '"assertTrue" does not compare its arguments, use "assertEqual" or other ' + + "appropriate functions." + ) + + VALID = [ + Valid("self.assertTrue(a == b)"), + Valid('self.assertTrue(data.is_valid(), "is_valid() method")'), + Valid("self.assertTrue(validate(len(obj.getName(type=SHORT))))"), + Valid("self.assertTrue(condition, message_string)"), + ] + + INVALID = [ + Invalid("self.assertTrue(a, 3)", expected_replacement="self.assertEqual(a, 3)"), + Invalid( + "self.assertTrue(hash(s[:4]), 0x1234)", + expected_replacement="self.assertEqual(hash(s[:4]), 0x1234)", + ), + Invalid( + "self.assertTrue(list, [1, 3])", + expected_replacement="self.assertEqual(list, [1, 3])", + ), + Invalid( + "self.assertTrue(optional, None)", + expected_replacement="self.assertIsNone(optional)", + ), + Invalid( + "self.assertTrue(b == a, True)", + expected_replacement="self.assertTrue(b == a)", + ), + Invalid( + "self.assertTrue(b == a, False)", + expected_replacement="self.assertFalse(b == a)", + ), + ] + + def visit_Call(self, node: cst.Call) -> None: + result = m.extract( + node, + m.Call( + func=m.Attribute(value=m.Name("self"), attr=m.Name("assertTrue")), + args=[ + m.DoNotCare(), + m.Arg( + value=m.SaveMatchedNode( + m.OneOf( + m.Integer(), + m.Float(), + m.Imaginary(), + m.Tuple(), + m.List(), + m.Set(), + m.Dict(), + m.Name("None"), + m.Name("True"), + m.Name("False"), + ), + "second", + ) + ), + ], + ), + ) + + if result: + second_arg = result["second"] + if m.matches(second_arg, m.Name("True")): + new_call = node.with_changes( + args=[node.args[0].with_changes(comma=cst.MaybeSentinel.DEFAULT)], + ) + elif m.matches(second_arg, m.Name("None")): + new_call = node.with_changes( + func=node.func.with_deep_changes( + old_node=cst.ensure_type(node.func, cst.Attribute).attr, + value="assertIsNone", + ), + args=[node.args[0].with_changes(comma=cst.MaybeSentinel.DEFAULT)], + ) + elif m.matches(second_arg, m.Name("False")): + new_call = node.with_changes( + func=node.func.with_deep_changes( + old_node=cst.ensure_type(node.func, cst.Attribute).attr, + value="assertFalse", + ), + args=[node.args[0].with_changes(comma=cst.MaybeSentinel.DEFAULT)], + ) + else: + new_call = node.with_deep_changes( + old_node=cst.ensure_type(node.func, cst.Attribute).attr, + value="assertEqual", + ) + + self.report(node, replacement=new_call) From dd187ff1e4792c80baa65ba74c816ca73ab1bf4e Mon Sep 17 00:00:00 2001 From: Alex Kapranoff Date: Sun, 20 Sep 2020 03:36:00 -0700 Subject: [PATCH 2/2] Fix a typing error --- fixit/rules/no_assert_true_for_comparison.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fixit/rules/no_assert_true_for_comparison.py b/fixit/rules/no_assert_true_for_comparison.py index 33c4ea8b..c29136fd 100644 --- a/fixit/rules/no_assert_true_for_comparison.py +++ b/fixit/rules/no_assert_true_for_comparison.py @@ -3,6 +3,8 @@ # This source code is licensed under the MIT license found in the # LICENSE file in the root directory of this source tree. +from typing import Sequence + import libcst as cst import libcst.matchers as m @@ -83,6 +85,9 @@ def visit_Call(self, node: cst.Call) -> None: if result: second_arg = result["second"] + if isinstance(second_arg, Sequence): + second_arg = second_arg[0] + if m.matches(second_arg, m.Name("True")): new_call = node.with_changes( args=[node.args[0].with_changes(comma=cst.MaybeSentinel.DEFAULT)],