diff --git a/integration_tests/test_fix_empty_sequence_comparison.py b/integration_tests/test_fix_empty_sequence_comparison.py new file mode 100644 index 00000000..c2420d0c --- /dev/null +++ b/integration_tests/test_fix_empty_sequence_comparison.py @@ -0,0 +1,18 @@ +from core_codemods.fix_empty_sequence_comparison import FixEmptySequenceComparison +from integration_tests.base_test import ( + BaseIntegrationTest, + original_and_expected_from_code_path, +) + + +class TestFixEmptySequenceComparison(BaseIntegrationTest): + codemod = FixEmptySequenceComparison + code_path = "tests/samples/fix_empty_sequence_comparison.py" + original_code, expected_new_code = original_and_expected_from_code_path( + code_path, [(1, "if x:\n")] + ) + expected_diff = ( + "--- \n+++ \n@@ -1,3 +1,3 @@\n x = [1]\n-if x != []:\n+if x:\n pass\n" + ) + expected_line_change = "2" + change_description = FixEmptySequenceComparison.change_description diff --git a/src/codemodder/registry.py b/src/codemodder/registry.py index 293efd9f..1c678f35 100644 --- a/src/codemodder/registry.py +++ b/src/codemodder/registry.py @@ -14,6 +14,8 @@ DEFAULT_EXCLUDED_CODEMODS = [ "pixee:python/order-imports", "pixee:python/unused-imports", + # See https://github.com/pixee/codemodder-python/pull/212 for concerns regarding this codemod. + "pixee:python/fix-empty-sequence-comparison", ] diff --git a/src/codemodder/scripts/generate_docs.py b/src/codemodder/scripts/generate_docs.py index 70e0bb9e..c0f9fe93 100644 --- a/src/codemodder/scripts/generate_docs.py +++ b/src/codemodder/scripts/generate_docs.py @@ -210,6 +210,10 @@ class DocMetadata: importance="Medium", guidance_explained="We believe this change is safe and will not cause any issues.", ), + "fix-empty-sequence-comparison": DocMetadata( + importance="Low", + guidance_explained="Values compared to empty sequences should be verified in case they are falsy values that are not a sequence.", + ), } diff --git a/src/core_codemods/__init__.py b/src/core_codemods/__init__.py index 54f77896..8be5f2d2 100644 --- a/src/core_codemods/__init__.py +++ b/src/core_codemods/__init__.py @@ -46,6 +46,7 @@ from .fix_deprecated_logging_warn import FixDeprecatedLoggingWarn from .flask_enable_csrf_protection import FlaskEnableCSRFProtection from .replace_flask_send_file import ReplaceFlaskSendFile +from .fix_empty_sequence_comparison import FixEmptySequenceComparison registry = CodemodCollection( origin="pixee", @@ -98,5 +99,6 @@ FixDeprecatedLoggingWarn, FlaskEnableCSRFProtection, ReplaceFlaskSendFile, + FixEmptySequenceComparison, ], ) diff --git a/src/core_codemods/docs/pixee_python_fix-empty-sequence-comparison.md b/src/core_codemods/docs/pixee_python_fix-empty-sequence-comparison.md new file mode 100644 index 00000000..d8f91d4c --- /dev/null +++ b/src/core_codemods/docs/pixee_python_fix-empty-sequence-comparison.md @@ -0,0 +1,10 @@ +Empty sequences in Python always evaluate to `False`. This means that comparison expressions that use empty sequences can sometimes be simplified. In these cases no explicit comparison is required: instead we can rely on the [truth value](https://docs.python.org/3/library/stdtypes.html#truth-value-testing) of the object under comparison. This is sometimes referred to as "implicit" comparison. Using implicit boolean comparison expressions is considered best practice and can lead to better code. + +Our changes look like the following: +```diff + x = [1] + +- if x != []: ++ if x: + pass +``` diff --git a/src/core_codemods/fix_empty_sequence_comparison.py b/src/core_codemods/fix_empty_sequence_comparison.py new file mode 100644 index 00000000..be27beae --- /dev/null +++ b/src/core_codemods/fix_empty_sequence_comparison.py @@ -0,0 +1,73 @@ +import libcst as cst +from core_codemods.api import Metadata, ReviewGuidance, SimpleCodemod, Reference +from codemodder.codemods.utils_mixin import NameResolutionMixin, AncestorPatternsMixin + + +class FixEmptySequenceComparison( + SimpleCodemod, NameResolutionMixin, AncestorPatternsMixin +): + metadata = Metadata( + name="fix-empty-sequence-comparison", + summary="Replace Comparisons to Empty Sequence with Implicit Boolean Comparison", + review_guidance=ReviewGuidance.MERGE_AFTER_REVIEW, + references=[ + Reference( + url="https://docs.python.org/3/library/stdtypes.html#truth-value-testing" + ), + ], + ) + change_description = ( + "Replace comparisons to empty sequence with implicit boolean comparison." + ) + + def leave_Comparison( + self, original_node: cst.Comparison, updated_node: cst.Comparison + ): + del updated_node + if not self.filter_by_path_includes_or_excludes( + self.node_position(original_node) + ): + return original_node + + maybe_parent = self.get_parent(original_node) + + match original_node: + case cst.Comparison( + left=left, comparisons=[cst.ComparisonTarget() as target] + ): + if isinstance(target.operator, cst.Equal | cst.NotEqual): + right = target.comparator + # right is empty: x == [] + # left is empty: [] == x + if ( + empty_left := self._is_empty_sequence(left) + ) or self._is_empty_sequence(right): + self.report_change(original_node) + comp_var = right if empty_left else left + match maybe_parent: + case cst.If() | cst.Assert(): + return ( + comp_var + if isinstance(target.operator, cst.NotEqual) + else cst.UnaryOperation( + operator=cst.Not(), + expression=comp_var, + ) + ) + case _: + return ( + cst.parse_expression(f"bool({comp_var.value})") + if isinstance(target.operator, cst.NotEqual) + else cst.UnaryOperation( + operator=cst.Not(), + expression=comp_var, + ) + ) + + return original_node + + def _is_empty_sequence(self, node: cst.BaseExpression): + match node: + case cst.List(elements=[]) | cst.Dict(elements=[]) | cst.Tuple(elements=[]): + return True + return False diff --git a/tests/codemods/test_fix_empty_sequence_comparison.py b/tests/codemods/test_fix_empty_sequence_comparison.py new file mode 100644 index 00000000..e3019ef7 --- /dev/null +++ b/tests/codemods/test_fix_empty_sequence_comparison.py @@ -0,0 +1,521 @@ +import pytest +from core_codemods.fix_empty_sequence_comparison import FixEmptySequenceComparison +from tests.codemods.base_codemod_test import BaseCodemodTest + + +class TestFixEmptySequenceComparisonIfStatements(BaseCodemodTest): + codemod = FixEmptySequenceComparison + + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = [1] + if x != []: + pass + """, + """ + x = [1] + if x: + pass + """, + ), + ( + """ + x = [1] + if [] != x: + pass + """, + """ + x = [1] + if x: + pass + """, + ), + ( + """ + x = [1] + if x == []: + pass + """, + """ + x = [1] + if not x: + pass + """, + ), + ( + """ + x = [1] + if [] == x: + pass + """, + """ + x = [1] + if not x: + pass + """, + ), + ( + """ + if [1, 2] == []: + pass + """, + """ + if not [1, 2]: + pass + """, + ), + ], + ) + def test_change_list(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = {1: "one", 2: "two"} + if x != {}: + pass + """, + """ + x = {1: "one", 2: "two"} + if x: + pass + """, + ), + ( + """ + x = {1: "one", 2: "two"} + if {} != x: + pass + """, + """ + x = {1: "one", 2: "two"} + if x: + pass + """, + ), + ( + """ + x = {1: "one", 2: "two"} + if x == {}: + pass + """, + """ + x = {1: "one", 2: "two"} + if not x: + pass + """, + ), + ( + """ + x = {1: "one", 2: "two"} + if {} == x: + pass + """, + """ + x = {1: "one", 2: "two"} + if not x: + pass + """, + ), + ( + """ + if {1: "one", 2: "two"} == {}: + pass + """, + """ + if not {1: "one", 2: "two"}: + pass + """, + ), + ], + ) + def test_change_dict(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = (1, 2, 3) + if x != (): + pass + """, + """ + x = (1, 2, 3) + if x: + pass + """, + ), + ( + """ + x = (1, 2, 3) + if () != x: + pass + """, + """ + x = (1, 2, 3) + if x: + pass + """, + ), + ( + """ + x = (1, 2, 3) + if x == (): + pass + """, + """ + x = (1, 2, 3) + if not x: + pass + """, + ), + ( + """ + x = (1, 2, 3) + if () == x: + pass + """, + """ + x = (1, 2, 3) + if not x: + pass + """, + ), + ( + """ + if (1, 2, 3) == (): + pass + """, + """ + if not (1, 2, 3): + pass + """, + ), + ], + ) + def test_change_tuple(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + + @pytest.mark.parametrize( + "code", + [ + """ + x = [1] + if x == "hi": + pass + """, + """ + x = [1] + if x is [1]: + pass + """, + """ + x = [1] + if x != [1]: + pass + """, + ], + ) + def test_no_change(self, tmpdir, code): + self.run_and_assert(tmpdir, code, code) + + def test_exclude_line(self, tmpdir): + input_code = expected = """\ + x = [1] + if x != []: + pass + """ + lines_to_exclude = [2] + self.run_and_assert( + tmpdir, + input_code, + expected, + lines_to_exclude=lines_to_exclude, + ) + + +class TestFixEmptySequenceComparisonAssertStatements(BaseCodemodTest): + codemod = FixEmptySequenceComparison + + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = [1] + assert x != [] + """, + """ + x = [1] + assert x + """, + ), + ( + """ + x = [1] + assert [] != x + """, + """ + x = [1] + assert x + """, + ), + ( + """ + x = [1] + assert x == [] + """, + """ + x = [1] + assert not x + """, + ), + ( + """ + x = [1] + assert [] == x + """, + """ + x = [1] + assert not x + """, + ), + ( + """ + assert [1, 2] == [] + """, + """ + assert not [1, 2] + """, + ), + ], + ) + def test_change_list(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = {1: "one", 2: "two"} + assert x != {} + """, + """ + x = {1: "one", 2: "two"} + assert x + """, + ), + ( + """ + x = {1: "one", 2: "two"} + assert {} != x + """, + """ + x = {1: "one", 2: "two"} + assert x + """, + ), + ( + """ + x = {1: "one", 2: "two"} + assert x == {} + """, + """ + x = {1: "one", 2: "two"} + assert not x + """, + ), + ( + """ + x = {1: "one", 2: "two"} + assert {} == x + """, + """ + x = {1: "one", 2: "two"} + assert not x + """, + ), + ( + """ + assert {1: "one", 2: "two"} == {} + """, + """ + assert not {1: "one", 2: "two"} + """, + ), + ], + ) + def test_change_dict(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = (1, 2, 3) + assert x != () + """, + """ + x = (1, 2, 3) + assert x + """, + ), + ( + """ + x = (1, 2, 3) + assert () != x + """, + """ + x = (1, 2, 3) + assert x + """, + ), + ( + """ + x = (1, 2, 3) + assert x == () + """, + """ + x = (1, 2, 3) + assert not x + """, + ), + ( + """ + x = (1, 2, 3) + assert () == x + """, + """ + x = (1, 2, 3) + assert not x + """, + ), + ( + """ + assert (1, 2, 3) == () + """, + """ + assert not (1, 2, 3) + """, + ), + ], + ) + def test_change_tuple(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + + @pytest.mark.parametrize( + "code", + [ + """ + x = [1] + assert x == "hi" + """, + """ + x = [1] + assert x is [1] + """, + ], + ) + def test_no_change(self, tmpdir, code): + self.run_and_assert(tmpdir, code, code) + + +@pytest.mark.xfail( + reason="Not yet support changing multiple comparisons in a statement" +) +class TestFixEmptySequenceComparisonMultipleStatements(BaseCodemodTest): + codemod = FixEmptySequenceComparison + + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = [1, 2] + y = [3, 4] + if x != [] or y == []: + pass + """, + """ + if x or not y: + pass + """, + ), + ( + """ + x = [1, 2] + y = [3, 4] + assert x != [] and y == [] + """, + """ + x = [1, 2] + y = [3, 4] + assert x and not y + """, + ), + ( + """ + x = [1, 2] + y = [3, 4] + x != [] or y == [] + """, + """ + x = [1, 2] + y = [3, 4] + bool(x) or not y + """, + ), + ], + ) + def test_change(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output, num_changes=2) + + +class TestFixEmptySequenceComparisonAssignmentStatements(BaseCodemodTest): + codemod = FixEmptySequenceComparison + + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = [1, 2] + res = x == [] + """, + """ + x = [1, 2] + res = not x + """, + ), + ( + """ + [1, 2] == [] + """, + """ + not [1, 2] + """, + ), + ( + """ + x = [1, 2] + res = x != [] + """, + """ + x = [1, 2] + res = bool(x) + """, + ), + ], + ) + def test_change(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) diff --git a/tests/samples/fix_empty_sequence_comparison.py b/tests/samples/fix_empty_sequence_comparison.py new file mode 100644 index 00000000..c89adfff --- /dev/null +++ b/tests/samples/fix_empty_sequence_comparison.py @@ -0,0 +1,3 @@ +x = [1] +if x != []: + pass