From 2ca2164a50ef7d5c94888fc6143ab5c8d313b903 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Thu, 11 Jan 2024 09:26:58 -0300 Subject: [PATCH 01/10] can fix if statements empty seq comparisons --- .../fix_empty_sequence_comparison.py | 65 ++++ .../test_fix_empty_sequence_comparison.py | 289 ++++++++++++++++++ 2 files changed, 354 insertions(+) create mode 100644 src/core_codemods/fix_empty_sequence_comparison.py create mode 100644 tests/codemods/test_fix_empty_sequence_comparison.py 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..50ff7730 --- /dev/null +++ b/src/core_codemods/fix_empty_sequence_comparison.py @@ -0,0 +1,65 @@ +import libcst as cst +from typing import Union +from codemodder.codemods.api import BaseCodemod, ReviewGuidance +from codemodder.codemods.utils_mixin import NameResolutionMixin, AncestorPatternsMixin + + +class FixEmptySequenceComparison( + BaseCodemod, NameResolutionMixin, AncestorPatternsMixin +): + NAME = "fix-empty-sequence-comparison" + SUMMARY = "TODO" + REVIEW_GUIDANCE = ReviewGuidance.MERGE_WITHOUT_REVIEW + DESCRIPTION = "TODO" + REFERENCES: list = [] + + def leave_If( + self, original_node: cst.If, updated_node: cst.If + ) -> cst.BaseStatement: + if not self.filter_by_path_includes_or_excludes( + self.node_position(original_node) + ): + return original_node + + match original_node.test: + 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) + if isinstance(target.operator, cst.NotEqual): + return updated_node.with_changes( + test=right if empty_left else left, + ) + return updated_node.with_changes( + test=cst.UnaryOperation( + operator=cst.Not(), + expression=right if empty_left else left, + ) + ) + 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 + + # def leave_Assert(self, original_node: cst.Assert, updated_node: cst.Assert) -> cst.BaseSmallStatement: + # #filter + # return self._simplify_empty_check(updated_node) + + # def _simplify_empty_check(self, node): # add type + + # if isinstance(node.test, cst.BooleanOperation) and isinstance(node.test.operator, cst.And): + # new_left = self._simplify_comparison(node.test.left) + # new_right = self._simplify_comparison(node.test.right) + # return node.with_changes(test=cst.BooleanOperation(left=new_left, operator=cst.And(), right=new_right)) + + # return self._simplify_comparison(node) 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..0117f3e1 --- /dev/null +++ b/tests/codemods/test_fix_empty_sequence_comparison.py @@ -0,0 +1,289 @@ +import pytest +from core_codemods.fix_empty_sequence_comparison import FixEmptySequenceComparison +from tests.codemods.base_codemod_test import BaseCodemodTest + + +class TestFixEmptySequenceComparison(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_if_statements(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + assert len(self.file_context.codemod_changes) == 1 + + @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_if_statements(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + assert len(self.file_context.codemod_changes) == 1 + + @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_if_statements(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + assert len(self.file_context.codemod_changes) == 1 + + +# no change +# if x == 'hi', x is [], +# @pytest.mark.parametrize( +# "code", +# [ +# """ +# from logging import {0} +# {0}('something') +# """, +# """ +# from logging import getLogger +# getLogger('anything').{0}('something') +# """, +# ], +# ) +# def test_from_import(self, tmpdir, code): +# original_code = code.format("warn") +# new_code = code.format("warning") +# self.run_and_assert(tmpdir, original_code, new_code) +# assert len(self.file_context.codemod_changes) == 1 +# +# @pytest.mark.parametrize( +# "input_code,expected_output", +# [ +# ( +# """\ +# from logging import warn as warn_func +# warn_func('something')""", +# """\ +# from logging import warning +# warning('something')""", +# ), +# ( +# """\ +# from logging import getLogger as make_logger +# logger = make_logger('anything') +# logger.warn('something')""", +# """\ +# from logging import getLogger as make_logger +# logger = make_logger('anything') +# logger.warning('something')""", +# ), +# ], +# ) +# def test_import_alias(self, tmpdir, input_code, expected_output): +# self.run_and_assert(tmpdir, input_code, expected_output) +# assert len(self.file_context.codemod_changes) == 1 +# +# @pytest.mark.parametrize( +# "code", +# [ +# """ +# import xyz +# xyz.warn('something') +# """, +# """ +# import my_logging +# log = my_logging.getLogger('anything') +# log.warn('something') +# """, +# ], +# ) +# def test_different_warn(self, tmpdir, code): +# self.run_and_assert(tmpdir, code, code) +# assert len(self.file_context.codemod_changes) == 0 +# +# @pytest.mark.xfail(reason="Not currently supported") +# def test_log_as_arg(self, tmpdir): +# code = """\ +# import logging +# log = logging.getLogger('foo') +# def some_function(logger): +# logger.()("hi") +# some_function(log) +# """ +# original_code = code.format("warn") +# new_code = code.format("warning") +# self.run_and_assert(tmpdir, original_code, new_code) +# assert len(self.file_context.codemod_changes) == 1 From 302b1cf5e543b9605917c7f558ded6d7c0df6e0a Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Thu, 11 Jan 2024 09:56:23 -0300 Subject: [PATCH 02/10] refactor and add xfail --- .../test_fix_empty_sequence_comparison.py | 168 +++++++++--------- 1 file changed, 85 insertions(+), 83 deletions(-) diff --git a/tests/codemods/test_fix_empty_sequence_comparison.py b/tests/codemods/test_fix_empty_sequence_comparison.py index 0117f3e1..1f43c549 100644 --- a/tests/codemods/test_fix_empty_sequence_comparison.py +++ b/tests/codemods/test_fix_empty_sequence_comparison.py @@ -3,7 +3,7 @@ from tests.codemods.base_codemod_test import BaseCodemodTest -class TestFixEmptySequenceComparison(BaseCodemodTest): +class TestFixEmptySequenceComparisonIfStatements(BaseCodemodTest): codemod = FixEmptySequenceComparison @pytest.mark.parametrize( @@ -69,7 +69,7 @@ class TestFixEmptySequenceComparison(BaseCodemodTest): ), ], ) - def test_change_list_if_statements(self, tmpdir, input_code, expected_output): + def test_change_list(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) assert len(self.file_context.codemod_changes) == 1 @@ -136,7 +136,7 @@ def test_change_list_if_statements(self, tmpdir, input_code, expected_output): ), ], ) - def test_change_dict_if_statements(self, tmpdir, input_code, expected_output): + def test_change_dict(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) assert len(self.file_context.codemod_changes) == 1 @@ -203,87 +203,89 @@ def test_change_dict_if_statements(self, tmpdir, input_code, expected_output): ), ], ) - def test_change_tuple_if_statements(self, tmpdir, input_code, expected_output): + def test_change_tuple(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) assert len(self.file_context.codemod_changes) == 1 + @pytest.mark.parametrize( + "code", + [ + """ + x = [1] + if x == "hi": + pass + """, + """ + x = [1] + if x is [1]: + pass + """, + ], + ) + def test_no_change(self, tmpdir, code): + self.run_and_assert(tmpdir, code, code) + assert len(self.file_context.codemod_changes) == 0 + + +@pytest.mark.xfail( + reason="Not yet support changing multiple comparisons in a statement" +) +class TestFixEmptySequenceComparisonMultipleStatements(BaseCodemodTest): + codemod = FixEmptySequenceComparison -# no change -# if x == 'hi', x is [], -# @pytest.mark.parametrize( -# "code", -# [ -# """ -# from logging import {0} -# {0}('something') -# """, -# """ -# from logging import getLogger -# getLogger('anything').{0}('something') -# """, -# ], -# ) -# def test_from_import(self, tmpdir, code): -# original_code = code.format("warn") -# new_code = code.format("warning") -# self.run_and_assert(tmpdir, original_code, new_code) -# assert len(self.file_context.codemod_changes) == 1 -# -# @pytest.mark.parametrize( -# "input_code,expected_output", -# [ -# ( -# """\ -# from logging import warn as warn_func -# warn_func('something')""", -# """\ -# from logging import warning -# warning('something')""", -# ), -# ( -# """\ -# from logging import getLogger as make_logger -# logger = make_logger('anything') -# logger.warn('something')""", -# """\ -# from logging import getLogger as make_logger -# logger = make_logger('anything') -# logger.warning('something')""", -# ), -# ], -# ) -# def test_import_alias(self, tmpdir, input_code, expected_output): -# self.run_and_assert(tmpdir, input_code, expected_output) -# assert len(self.file_context.codemod_changes) == 1 -# -# @pytest.mark.parametrize( -# "code", -# [ -# """ -# import xyz -# xyz.warn('something') -# """, -# """ -# import my_logging -# log = my_logging.getLogger('anything') -# log.warn('something') -# """, -# ], -# ) -# def test_different_warn(self, tmpdir, code): -# self.run_and_assert(tmpdir, code, code) -# assert len(self.file_context.codemod_changes) == 0 -# -# @pytest.mark.xfail(reason="Not currently supported") -# def test_log_as_arg(self, tmpdir): -# code = """\ -# import logging -# log = logging.getLogger('foo') -# def some_function(logger): -# logger.()("hi") -# some_function(log) -# """ -# original_code = code.format("warn") -# new_code = code.format("warning") -# self.run_and_assert(tmpdir, original_code, new_code) -# assert len(self.file_context.codemod_changes) == 1 + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = [1, 2] + y = [3, 4] + if x != [] or y == []: + pass + """, + """ + if x or not y: + pass + """, + ), + ], + ) + def test_change(self, tmpdir, input_code, expected_output): + self.run_and_assert(tmpdir, input_code, expected_output) + assert len(self.file_context.codemod_changes) == 1 + + +@pytest.mark.xfail( + reason="Not yet support changing comparisons in assignment statements." +) +class TestFixEmptySequenceComparisonAssignmentStatements(BaseCodemodTest): + codemod = FixEmptySequenceComparison + + @pytest.mark.parametrize( + "input_code,expected_output", + [ + ( + """ + x = [1, 2] + res = x == [] + """, + """ + x = [1, 2] + res = not x + """, + ), + ( + """ + 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) + assert len(self.file_context.codemod_changes) == 1 From 9d48514c406744ac69e0662145e9a8cdd85f035e Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Fri, 12 Jan 2024 09:46:35 -0300 Subject: [PATCH 03/10] can simplify empty check for assert statements --- .../fix_empty_sequence_comparison.py | 57 ++++-- .../test_fix_empty_sequence_comparison.py | 192 ++++++++++++++++++ 2 files changed, 233 insertions(+), 16 deletions(-) diff --git a/src/core_codemods/fix_empty_sequence_comparison.py b/src/core_codemods/fix_empty_sequence_comparison.py index 50ff7730..4ffc06af 100644 --- a/src/core_codemods/fix_empty_sequence_comparison.py +++ b/src/core_codemods/fix_empty_sequence_comparison.py @@ -13,9 +13,47 @@ class FixEmptySequenceComparison( DESCRIPTION = "TODO" REFERENCES: list = [] - def leave_If( - self, original_node: cst.If, updated_node: cst.If - ) -> cst.BaseStatement: + def leave_If(self, original_node: cst.If, updated_node: cst.If) -> cst.If: + return self._simplify_empty_check(original_node, updated_node) + + def leave_Assert( + self, original_node: cst.Assert, updated_node: cst.Assert + ) -> cst.Assert: + if not self.filter_by_path_includes_or_excludes( + self.node_position(original_node) + ): + return original_node + + match original_node.test: + 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) + if isinstance(target.operator, cst.NotEqual): + return updated_node.with_changes( + test=right if empty_left else left, + ) + return updated_node.with_changes( + test=cst.UnaryOperation( + operator=cst.Not(), + expression=right if empty_left else left, + ) + ) + return original_node + return self._simplify_empty_check(original_node, updated_node) + + def _simplify_empty_check( + self, + original_node: Union[cst.Assert, cst.If], + updated_node: Union[cst.Assert, cst.If], + ) -> Union[cst.Assert, cst.If]: if not self.filter_by_path_includes_or_excludes( self.node_position(original_node) ): @@ -50,16 +88,3 @@ def _is_empty_sequence(self, node: cst.BaseExpression): case cst.List(elements=[]) | cst.Dict(elements=[]) | cst.Tuple(elements=[]): return True return False - - # def leave_Assert(self, original_node: cst.Assert, updated_node: cst.Assert) -> cst.BaseSmallStatement: - # #filter - # return self._simplify_empty_check(updated_node) - - # def _simplify_empty_check(self, node): # add type - - # if isinstance(node.test, cst.BooleanOperation) and isinstance(node.test.operator, cst.And): - # new_left = self._simplify_comparison(node.test.left) - # new_right = self._simplify_comparison(node.test.right) - # return node.with_changes(test=cst.BooleanOperation(left=new_left, operator=cst.And(), right=new_right)) - - # return self._simplify_comparison(node) diff --git a/tests/codemods/test_fix_empty_sequence_comparison.py b/tests/codemods/test_fix_empty_sequence_comparison.py index 1f43c549..6acc9aa3 100644 --- a/tests/codemods/test_fix_empty_sequence_comparison.py +++ b/tests/codemods/test_fix_empty_sequence_comparison.py @@ -227,6 +227,198 @@ def test_no_change(self, tmpdir, code): assert len(self.file_context.codemod_changes) == 0 +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) + assert len(self.file_context.codemod_changes) == 1 + + @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) + assert len(self.file_context.codemod_changes) == 1 + + @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) + assert len(self.file_context.codemod_changes) == 1 + + @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) + assert len(self.file_context.codemod_changes) == 0 + + @pytest.mark.xfail( reason="Not yet support changing multiple comparisons in a statement" ) From e41361c43c88ebf63a9d63ceec5156dd7ebca9f8 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Fri, 12 Jan 2024 09:47:15 -0300 Subject: [PATCH 04/10] can simplify empty check for assert statements --- .../fix_empty_sequence_comparison.py | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/src/core_codemods/fix_empty_sequence_comparison.py b/src/core_codemods/fix_empty_sequence_comparison.py index 4ffc06af..d77944f8 100644 --- a/src/core_codemods/fix_empty_sequence_comparison.py +++ b/src/core_codemods/fix_empty_sequence_comparison.py @@ -19,34 +19,6 @@ def leave_If(self, original_node: cst.If, updated_node: cst.If) -> cst.If: def leave_Assert( self, original_node: cst.Assert, updated_node: cst.Assert ) -> cst.Assert: - if not self.filter_by_path_includes_or_excludes( - self.node_position(original_node) - ): - return original_node - - match original_node.test: - 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) - if isinstance(target.operator, cst.NotEqual): - return updated_node.with_changes( - test=right if empty_left else left, - ) - return updated_node.with_changes( - test=cst.UnaryOperation( - operator=cst.Not(), - expression=right if empty_left else left, - ) - ) - return original_node return self._simplify_empty_check(original_node, updated_node) def _simplify_empty_check( From d35fc78c14e53a8984dbcf0ce91150a611314f09 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Mon, 15 Jan 2024 13:17:14 -0300 Subject: [PATCH 05/10] can simplify empty seq comparison for most statements --- .../fix_empty_sequence_comparison.py | 51 ++++++++++--------- .../test_fix_empty_sequence_comparison.py | 17 +++++-- 2 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/core_codemods/fix_empty_sequence_comparison.py b/src/core_codemods/fix_empty_sequence_comparison.py index d77944f8..fc6761b1 100644 --- a/src/core_codemods/fix_empty_sequence_comparison.py +++ b/src/core_codemods/fix_empty_sequence_comparison.py @@ -13,25 +13,17 @@ class FixEmptySequenceComparison( DESCRIPTION = "TODO" REFERENCES: list = [] - def leave_If(self, original_node: cst.If, updated_node: cst.If) -> cst.If: - return self._simplify_empty_check(original_node, updated_node) - - def leave_Assert( - self, original_node: cst.Assert, updated_node: cst.Assert - ) -> cst.Assert: - return self._simplify_empty_check(original_node, updated_node) - - def _simplify_empty_check( - self, - original_node: Union[cst.Assert, cst.If], - updated_node: Union[cst.Assert, cst.If], - ) -> Union[cst.Assert, cst.If]: + 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 - match original_node.test: + maybe_parent = self.get_parent(original_node) + match original_node: case cst.Comparison( left=left, comparisons=[cst.ComparisonTarget() as target] ): @@ -43,16 +35,27 @@ def _simplify_empty_check( empty_left := self._is_empty_sequence(left) ) or self._is_empty_sequence(right): self.report_change(original_node) - if isinstance(target.operator, cst.NotEqual): - return updated_node.with_changes( - test=right if empty_left else left, - ) - return updated_node.with_changes( - test=cst.UnaryOperation( - operator=cst.Not(), - expression=right if empty_left else left, - ) - ) + 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): diff --git a/tests/codemods/test_fix_empty_sequence_comparison.py b/tests/codemods/test_fix_empty_sequence_comparison.py index 6acc9aa3..3e419bf6 100644 --- a/tests/codemods/test_fix_empty_sequence_comparison.py +++ b/tests/codemods/test_fix_empty_sequence_comparison.py @@ -220,6 +220,11 @@ def test_change_tuple(self, tmpdir, input_code, expected_output): if x is [1]: pass """, + """ + x = [1] + if x != [1]: + pass + """, ], ) def test_no_change(self, tmpdir, code): @@ -419,6 +424,7 @@ def test_no_change(self, tmpdir, code): assert len(self.file_context.codemod_changes) == 0 +# TODO: @pytest.mark.xfail( reason="Not yet support changing multiple comparisons in a statement" ) @@ -447,9 +453,6 @@ def test_change(self, tmpdir, input_code, expected_output): assert len(self.file_context.codemod_changes) == 1 -@pytest.mark.xfail( - reason="Not yet support changing comparisons in assignment statements." -) class TestFixEmptySequenceComparisonAssignmentStatements(BaseCodemodTest): codemod = FixEmptySequenceComparison @@ -466,6 +469,14 @@ class TestFixEmptySequenceComparisonAssignmentStatements(BaseCodemodTest): res = not x """, ), + ( + """ + [1, 2] == [] + """, + """ + not [1, 2] + """, + ), ( """ x = [1, 2] From 2e5aa12fbea0430e657c66799041b7312c69f653 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Mon, 15 Jan 2024 13:33:25 -0300 Subject: [PATCH 06/10] skip changing multiple statements --- .../fix_empty_sequence_comparison.py | 1 + .../test_fix_empty_sequence_comparison.py | 27 +++++++++++++++++-- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/core_codemods/fix_empty_sequence_comparison.py b/src/core_codemods/fix_empty_sequence_comparison.py index fc6761b1..ee6817f7 100644 --- a/src/core_codemods/fix_empty_sequence_comparison.py +++ b/src/core_codemods/fix_empty_sequence_comparison.py @@ -23,6 +23,7 @@ def leave_Comparison( return original_node maybe_parent = self.get_parent(original_node) + match original_node: case cst.Comparison( left=left, comparisons=[cst.ComparisonTarget() as target] diff --git a/tests/codemods/test_fix_empty_sequence_comparison.py b/tests/codemods/test_fix_empty_sequence_comparison.py index 3e419bf6..7ca422df 100644 --- a/tests/codemods/test_fix_empty_sequence_comparison.py +++ b/tests/codemods/test_fix_empty_sequence_comparison.py @@ -424,7 +424,6 @@ def test_no_change(self, tmpdir, code): assert len(self.file_context.codemod_changes) == 0 -# TODO: @pytest.mark.xfail( reason="Not yet support changing multiple comparisons in a statement" ) @@ -446,11 +445,35 @@ class TestFixEmptySequenceComparisonMultipleStatements(BaseCodemodTest): 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) - assert len(self.file_context.codemod_changes) == 1 + assert len(self.file_context.codemod_changes) == 2 class TestFixEmptySequenceComparisonAssignmentStatements(BaseCodemodTest): From 1284d3616a603d8aba4c65b3e2515142c9795d5e Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Mon, 15 Jan 2024 13:44:25 -0300 Subject: [PATCH 07/10] document and fully test empty sequence comparison --- .../test_fix_empty_sequence_comparison.py | 18 ++++++++++++++++++ src/codemodder/scripts/generate_docs.py | 4 ++++ src/core_codemods/__init__.py | 5 +++++ ...xee_python_fix-empty-sequence-comparison.md | 11 +++++++++++ .../fix_empty_sequence_comparison.py | 7 ++++--- tests/samples/fix_empty_sequence_comparison.py | 3 +++ 6 files changed, 45 insertions(+), 3 deletions(-) create mode 100644 integration_tests/test_fix_empty_sequence_comparison.py create mode 100644 src/core_codemods/docs/pixee_python_fix-empty-sequence-comparison.md create mode 100644 tests/samples/fix_empty_sequence_comparison.py 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..365cd729 --- /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/scripts/generate_docs.py b/src/codemodder/scripts/generate_docs.py index 70e0bb9e..58d8ead9 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="Simplifying comparison expressions to empty sequences is safe.", + ), } diff --git a/src/core_codemods/__init__.py b/src/core_codemods/__init__.py index 54f77896..f6a8f288 100644 --- a/src/core_codemods/__init__.py +++ b/src/core_codemods/__init__.py @@ -44,8 +44,12 @@ from .remove_debug_breakpoint import RemoveDebugBreakpoint from .combine_startswith_endswith import CombineStartswithEndswith from .fix_deprecated_logging_warn import FixDeprecatedLoggingWarn +<<<<<<< HEAD from .flask_enable_csrf_protection import FlaskEnableCSRFProtection from .replace_flask_send_file import ReplaceFlaskSendFile +======= +from .fix_empty_sequence_comparison import FixEmptySequenceComparison +>>>>>>> 00f4616 (document and fully test empty sequence comparison) registry = CodemodCollection( origin="pixee", @@ -98,5 +102,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..1a493700 --- /dev/null +++ b/src/core_codemods/docs/pixee_python_fix-empty-sequence-comparison.md @@ -0,0 +1,11 @@ +Empty sequences in a boolean comparison expression are considered falsy so you can use implicit boolean comparisons instead of +comparing against empty sequences directly + +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 index ee6817f7..d7c22079 100644 --- a/src/core_codemods/fix_empty_sequence_comparison.py +++ b/src/core_codemods/fix_empty_sequence_comparison.py @@ -1,5 +1,4 @@ import libcst as cst -from typing import Union from codemodder.codemods.api import BaseCodemod, ReviewGuidance from codemodder.codemods.utils_mixin import NameResolutionMixin, AncestorPatternsMixin @@ -8,9 +7,11 @@ class FixEmptySequenceComparison( BaseCodemod, NameResolutionMixin, AncestorPatternsMixin ): NAME = "fix-empty-sequence-comparison" - SUMMARY = "TODO" + SUMMARY = "Replace Comparisons to Empty Sequence with Implicit Boolean Comparison" REVIEW_GUIDANCE = ReviewGuidance.MERGE_WITHOUT_REVIEW - DESCRIPTION = "TODO" + DESCRIPTION = ( + "Replace comparisons to empty sequence with implicit boolean comparison." + ) REFERENCES: list = [] def leave_Comparison( 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 From 1d54707495ab243f5b3b4b08ed5ba972f69b6255 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Thu, 18 Jan 2024 08:29:04 -0300 Subject: [PATCH 08/10] change codemod to use new api --- src/core_codemods/__init__.py | 3 --- .../fix_empty_sequence_comparison.py | 20 ++++++++++++------- .../test_fix_empty_sequence_comparison.py | 12 +---------- 3 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/core_codemods/__init__.py b/src/core_codemods/__init__.py index f6a8f288..8be5f2d2 100644 --- a/src/core_codemods/__init__.py +++ b/src/core_codemods/__init__.py @@ -44,12 +44,9 @@ from .remove_debug_breakpoint import RemoveDebugBreakpoint from .combine_startswith_endswith import CombineStartswithEndswith from .fix_deprecated_logging_warn import FixDeprecatedLoggingWarn -<<<<<<< HEAD from .flask_enable_csrf_protection import FlaskEnableCSRFProtection from .replace_flask_send_file import ReplaceFlaskSendFile -======= from .fix_empty_sequence_comparison import FixEmptySequenceComparison ->>>>>>> 00f4616 (document and fully test empty sequence comparison) registry = CodemodCollection( origin="pixee", diff --git a/src/core_codemods/fix_empty_sequence_comparison.py b/src/core_codemods/fix_empty_sequence_comparison.py index d7c22079..be27beae 100644 --- a/src/core_codemods/fix_empty_sequence_comparison.py +++ b/src/core_codemods/fix_empty_sequence_comparison.py @@ -1,18 +1,24 @@ import libcst as cst -from codemodder.codemods.api import BaseCodemod, ReviewGuidance +from core_codemods.api import Metadata, ReviewGuidance, SimpleCodemod, Reference from codemodder.codemods.utils_mixin import NameResolutionMixin, AncestorPatternsMixin class FixEmptySequenceComparison( - BaseCodemod, NameResolutionMixin, AncestorPatternsMixin + SimpleCodemod, NameResolutionMixin, AncestorPatternsMixin ): - NAME = "fix-empty-sequence-comparison" - SUMMARY = "Replace Comparisons to Empty Sequence with Implicit Boolean Comparison" - REVIEW_GUIDANCE = ReviewGuidance.MERGE_WITHOUT_REVIEW - DESCRIPTION = ( + 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." ) - REFERENCES: list = [] def leave_Comparison( self, original_node: cst.Comparison, updated_node: cst.Comparison diff --git a/tests/codemods/test_fix_empty_sequence_comparison.py b/tests/codemods/test_fix_empty_sequence_comparison.py index 7ca422df..422f31d9 100644 --- a/tests/codemods/test_fix_empty_sequence_comparison.py +++ b/tests/codemods/test_fix_empty_sequence_comparison.py @@ -71,7 +71,6 @@ class TestFixEmptySequenceComparisonIfStatements(BaseCodemodTest): ) def test_change_list(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) - assert len(self.file_context.codemod_changes) == 1 @pytest.mark.parametrize( "input_code,expected_output", @@ -138,7 +137,6 @@ def test_change_list(self, tmpdir, input_code, expected_output): ) def test_change_dict(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) - assert len(self.file_context.codemod_changes) == 1 @pytest.mark.parametrize( "input_code,expected_output", @@ -205,7 +203,6 @@ def test_change_dict(self, tmpdir, input_code, expected_output): ) def test_change_tuple(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) - assert len(self.file_context.codemod_changes) == 1 @pytest.mark.parametrize( "code", @@ -229,7 +226,6 @@ def test_change_tuple(self, tmpdir, input_code, expected_output): ) def test_no_change(self, tmpdir, code): self.run_and_assert(tmpdir, code, code) - assert len(self.file_context.codemod_changes) == 0 class TestFixEmptySequenceComparisonAssertStatements(BaseCodemodTest): @@ -290,7 +286,6 @@ class TestFixEmptySequenceComparisonAssertStatements(BaseCodemodTest): ) def test_change_list(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) - assert len(self.file_context.codemod_changes) == 1 @pytest.mark.parametrize( "input_code,expected_output", @@ -347,7 +342,6 @@ def test_change_list(self, tmpdir, input_code, expected_output): ) def test_change_dict(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) - assert len(self.file_context.codemod_changes) == 1 @pytest.mark.parametrize( "input_code,expected_output", @@ -404,7 +398,6 @@ def test_change_dict(self, tmpdir, input_code, expected_output): ) def test_change_tuple(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) - assert len(self.file_context.codemod_changes) == 1 @pytest.mark.parametrize( "code", @@ -421,7 +414,6 @@ def test_change_tuple(self, tmpdir, input_code, expected_output): ) def test_no_change(self, tmpdir, code): self.run_and_assert(tmpdir, code, code) - assert len(self.file_context.codemod_changes) == 0 @pytest.mark.xfail( @@ -472,8 +464,7 @@ class TestFixEmptySequenceComparisonMultipleStatements(BaseCodemodTest): ], ) def test_change(self, tmpdir, input_code, expected_output): - self.run_and_assert(tmpdir, input_code, expected_output) - assert len(self.file_context.codemod_changes) == 2 + self.run_and_assert(tmpdir, input_code, expected_output, num_changes=2) class TestFixEmptySequenceComparisonAssignmentStatements(BaseCodemodTest): @@ -514,4 +505,3 @@ class TestFixEmptySequenceComparisonAssignmentStatements(BaseCodemodTest): ) def test_change(self, tmpdir, input_code, expected_output): self.run_and_assert(tmpdir, input_code, expected_output) - assert len(self.file_context.codemod_changes) == 1 From 68c26a4e02ca9a133df9e2d4ec38b2a4000c6cdd Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Thu, 18 Jan 2024 08:33:32 -0300 Subject: [PATCH 09/10] test empty sequence for exclude --- .../test_fix_empty_sequence_comparison.py | 2 +- .../pixee_python_fix-empty-sequence-comparison.md | 3 +-- .../codemods/test_fix_empty_sequence_comparison.py | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/integration_tests/test_fix_empty_sequence_comparison.py b/integration_tests/test_fix_empty_sequence_comparison.py index 365cd729..c2420d0c 100644 --- a/integration_tests/test_fix_empty_sequence_comparison.py +++ b/integration_tests/test_fix_empty_sequence_comparison.py @@ -15,4 +15,4 @@ class TestFixEmptySequenceComparison(BaseIntegrationTest): "--- \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 + change_description = FixEmptySequenceComparison.change_description 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 index 1a493700..d8f91d4c 100644 --- a/src/core_codemods/docs/pixee_python_fix-empty-sequence-comparison.md +++ b/src/core_codemods/docs/pixee_python_fix-empty-sequence-comparison.md @@ -1,5 +1,4 @@ -Empty sequences in a boolean comparison expression are considered falsy so you can use implicit boolean comparisons instead of -comparing against empty sequences directly +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 diff --git a/tests/codemods/test_fix_empty_sequence_comparison.py b/tests/codemods/test_fix_empty_sequence_comparison.py index 422f31d9..e3019ef7 100644 --- a/tests/codemods/test_fix_empty_sequence_comparison.py +++ b/tests/codemods/test_fix_empty_sequence_comparison.py @@ -227,6 +227,20 @@ def test_change_tuple(self, tmpdir, input_code, expected_output): 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 From bffa115391382654bda634de068934e28362f5b7 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Thu, 18 Jan 2024 08:42:01 -0300 Subject: [PATCH 10/10] fix empty str comparison is a default excluded codemod --- src/codemodder/registry.py | 2 ++ src/codemodder/scripts/generate_docs.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) 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 58d8ead9..c0f9fe93 100644 --- a/src/codemodder/scripts/generate_docs.py +++ b/src/codemodder/scripts/generate_docs.py @@ -212,7 +212,7 @@ class DocMetadata: ), "fix-empty-sequence-comparison": DocMetadata( importance="Low", - guidance_explained="Simplifying comparison expressions to empty sequences is safe.", + guidance_explained="Values compared to empty sequences should be verified in case they are falsy values that are not a sequence.", ), }