From 05876b310107f4e6940ef6d6121435ddb4b1a5f4 Mon Sep 17 00:00:00 2001 From: Lucas Faudman Date: Fri, 19 Apr 2024 11:28:54 -0700 Subject: [PATCH 1/3] define decorators to remove repetitive code that shows up at the beginning of CodeMods --- src/codemodder/codemods/utils.py | 22 ++- src/codemodder/codemods/utils_decorators.py | 96 +++++++++++++ .../combine_isinstance_issubclass.py | 98 +++++++++++++ .../test_combine_isinstance_issubclass.py | 135 ++++++++++++++++++ 4 files changed, 350 insertions(+), 1 deletion(-) create mode 100644 src/codemodder/codemods/utils_decorators.py create mode 100644 src/core_codemods/combine_isinstance_issubclass.py create mode 100644 tests/codemods/test_combine_isinstance_issubclass.py diff --git a/src/codemodder/codemods/utils.py b/src/codemodder/codemods/utils.py index 862f362e..878f6f63 100644 --- a/src/codemodder/codemods/utils.py +++ b/src/codemodder/codemods/utils.py @@ -1,6 +1,6 @@ from enum import Enum from pathlib import Path -from typing import Any, Optional, TypeAlias +from typing import Any, Iterator, Optional, Type, TypeAlias, TypeVar import libcst as cst from libcst import MetadataDependent, matchers @@ -216,3 +216,23 @@ def is_zero(node: cst.CSTNode) -> bool: case cst.Call(func=cst.Name("int")) | cst.Call(func=cst.Name("float")): return is_zero(node.args[0].value) return False + + +_CSTNode = TypeVar("_CSTNode", bound=cst.CSTNode) + + +def extract_boolean_operands( + node: cst.BooleanOperation, ensure_type: Type[_CSTNode] = cst.CSTNode +) -> Iterator[_CSTNode]: + """ + Recursively extract operands from a cst.BooleanOperation node from left to right as an iterator of nodes. + """ + if isinstance(node.left, cst.BooleanOperation): + # Recursively yield operands from the boolean operation on the left + yield from extract_boolean_operands(node.left, ensure_type) + else: + # Yield left operand + yield cst.ensure_type(node.left, ensure_type) + + # Yield right operand + yield cst.ensure_type(node.right, ensure_type) diff --git a/src/codemodder/codemods/utils_decorators.py b/src/codemodder/codemods/utils_decorators.py new file mode 100644 index 00000000..a0f3845b --- /dev/null +++ b/src/codemodder/codemods/utils_decorators.py @@ -0,0 +1,96 @@ +from functools import wraps +from typing import Callable, TypeVar + +import libcst as cst + +from codemodder.codemods.base_visitor import BaseTransformer, BaseVisitor + +# TypeVars for decorators below +_BaseVisitorOrTransformer = TypeVar( + "_BaseVisitorOrTransformer", BaseVisitor, BaseTransformer +) +_CSTNode = TypeVar("_CSTNode", bound=cst.CSTNode) + + +def check_filter_by_path_includes_or_excludes( + func: Callable[[_BaseVisitorOrTransformer, _CSTNode, _CSTNode], cst.CSTNode] +) -> Callable[[_BaseVisitorOrTransformer, _CSTNode, _CSTNode], cst.CSTNode]: + """ + Decorator to filter nodes based on path-includes or path-excludes flags. + + Calls the decorated func only if the original node is not filtered. + otherwise, returns the updated node as is. + + ``` + @check_filter_by_path_includes_or_excludes + def leave_Call(self, original_node: cst.Call, updated_node: cst.Call) -> cst.Call + # rest of the method + ``` + + Is equivalent to: + + ``` + def leave_Call(self, original_node: cst.Call, updated_node: cst.Call) -> cst.Call + # Added by the decorator + if not self.filter_by_path_includes_or_excludes( + self.node_position(original_node) + ): + return updated_node + + # rest of the method + ``` + """ + + @wraps(func) + def wrapper( + instance: _BaseVisitorOrTransformer, + original_node: _CSTNode, + updated_node: _CSTNode, + ) -> cst.CSTNode: + if not instance.filter_by_path_includes_or_excludes( + instance.node_position(original_node) + ): + return updated_node + return func(instance, original_node, updated_node) + + return wrapper + + +def check_node_is_not_selected( + func: Callable[[_BaseVisitorOrTransformer, _CSTNode, _CSTNode], cst.CSTNode] +) -> Callable[[_BaseVisitorOrTransformer, _CSTNode, _CSTNode], cst.CSTNode]: + """ + Decorator to filter nodes based on whether the node is selected or not. + + Calls the decorated func only if the original node is not selected. + otherwise, returns the updated node as is. + + ``` + @check_node_is_not_selected + def leave_Call(self, original_node: cst.Call, updated_node: cst.Call) -> cst.Call + # rest of the method + ``` + + Is equivalent to: + + ``` + def leave_Call(self, original_node: cst.Call, updated_node: cst.Call) -> cst.Call + # Added by the decorator + if not self.node_is_selected(original_node): + return updated_node + + # rest of the method + ``` + """ + + @wraps(func) + def wrapper( + instance: _BaseVisitorOrTransformer, + original_node: _CSTNode, + updated_node: _CSTNode, + ) -> cst.CSTNode: + if not instance.node_is_selected(original_node): + return updated_node + return func(instance, original_node, updated_node) + + return wrapper diff --git a/src/core_codemods/combine_isinstance_issubclass.py b/src/core_codemods/combine_isinstance_issubclass.py new file mode 100644 index 00000000..dea6f19f --- /dev/null +++ b/src/core_codemods/combine_isinstance_issubclass.py @@ -0,0 +1,98 @@ +import libcst as cst +from libcst import matchers as m + +from codemodder.codemods.utils import extract_boolean_operands +from codemodder.codemods.utils_decorators import ( + check_filter_by_path_includes_or_excludes, +) +from codemodder.codemods.utils_mixin import NameResolutionMixin +from core_codemods.api import Metadata, ReviewGuidance, SimpleCodemod + + +class CombineIsinstanceIssubclass(SimpleCodemod, NameResolutionMixin): + metadata = Metadata( + name="combine-isinstance-issubclass", + summary="Simplify Boolean Expressions Using `isinstance` and `issubclass`", + review_guidance=ReviewGuidance.MERGE_WITHOUT_REVIEW, + references=[], + ) + change_description = "Use tuple of matches instead of boolean expression" + + @check_filter_by_path_includes_or_excludes + def leave_BooleanOperation( + self, original_node: cst.BooleanOperation, updated_node: cst.BooleanOperation + ) -> cst.CSTNode: + if self.matches_isinstance_issubclass_or_pattern(original_node): + self.report_change(original_node) + + elements = [] + seen_values = set() + for call in extract_boolean_operands(updated_node, ensure_type=cst.Call): + class_tuple_arg_value = call.args[1].value + if isinstance(class_tuple_arg_value, cst.Tuple): + arg_elements = class_tuple_arg_value.elements + else: + arg_elements = (cst.Element(value=class_tuple_arg_value),) + + for element in arg_elements: + if (value := getattr(element.value, "value", None)) in seen_values: + # If an element has a non-None evaluated value that has already been seen, continue to avoid duplicates + continue + if value is not None: + seen_values.add(value) + elements.append(element) + + instance_arg = updated_node.left.args[0] + new_class_tuple_arg = cst.Arg(value=cst.Tuple(elements=elements)) + return cst.Call(func=call.func, args=[instance_arg, new_class_tuple_arg]) + + return updated_node + + def matches_isinstance_issubclass_or_pattern( + self, node: cst.BooleanOperation + ) -> bool: + # Match the pattern: isinstance(x, cls1) or isinstance(x, cls2) or isinstance(x, cls3) or ... + # and the same but with issubclass + args = [m.Arg(value=m.Name()), m.Arg(value=m.Name() | m.Tuple())] + isinstance_call = m.Call( + func=m.Name("isinstance"), + args=args, + ) + issubclass_call = m.Call( + func=m.Name("issubclass"), + args=args, + ) + isinstance_or = m.BooleanOperation( + left=isinstance_call, operator=m.Or(), right=isinstance_call + ) + issubclass_or = m.BooleanOperation( + left=issubclass_call, operator=m.Or(), right=issubclass_call + ) + + # Check for simple case: isinstance(x, cls) or issubclass(x, cls) + if ( + m.matches(node, isinstance_or | issubclass_or) + and node.left.func.value == node.right.func.value # Same function + and node.left.args[0].value.value + == node.right.args[0].value.value # Same first argument (instance) + ): + return True + + # Check for chained case: isinstance(x, cls1) or isinstance(x, cls2) or isinstance(x, cls3) or ... + chained_or = m.BooleanOperation( + left=m.BooleanOperation(operator=m.Or()), + operator=m.Or(), + right=isinstance_call | issubclass_call, + ) + if m.matches(node, chained_or): + return all( + ( + call.func.value == node.right.func.value # Same function + and call.args[0].value.value + == node.right.args[0].value.value # Same first argument (instance) + ) + for call in extract_boolean_operands(node, ensure_type=cst.Call) + ) + + # No match + return False diff --git a/tests/codemods/test_combine_isinstance_issubclass.py b/tests/codemods/test_combine_isinstance_issubclass.py new file mode 100644 index 00000000..857efbf7 --- /dev/null +++ b/tests/codemods/test_combine_isinstance_issubclass.py @@ -0,0 +1,135 @@ +import pytest + +from codemodder.codemods.test import BaseCodemodTest +from core_codemods.combine_isinstance_issubclass import CombineIsinstanceIssubclass + +each_func = pytest.mark.parametrize("func", ["isinstance", "issubclass"]) + + +class TestCombineIsinstanceIssubclass(BaseCodemodTest): + codemod = CombineIsinstanceIssubclass + + def test_name(self): + assert self.codemod.name == "combine-isinstance-issubclass" + + @each_func + def test_combine(self, tmpdir, func): + input_code = f""" + {func}(x, str) or {func}(x, bytes) + """ + expected = f""" + {func}(x, (str, bytes)) + """ + self.run_and_assert(tmpdir, input_code, expected) + + @pytest.mark.parametrize( + "code", + [ + "isinstance(x, str)", + "isinstance(x, (str, bytes))", + "isinstance(x, str) and isinstance(x, bytes)", + "isinstance(x, str) and isinstance(x, str) or True", + "isinstance(x, str) or issubclass(x, str)", + "isinstance(x, str) or isinstance(y, str)", + ], + ) + def test_no_change(self, tmpdir, code): + self.run_and_assert(tmpdir, code, code) + + def test_exclude_line(self, tmpdir): + input_code = ( + expected + ) = """ + x = "foo" + isinstance(x, str) or isinstance(x, bytes) + """ + lines_to_exclude = [3] + self.run_and_assert( + tmpdir, + input_code, + expected, + lines_to_exclude=lines_to_exclude, + ) + + def _format_func_run_test(self, tmpdir, func, input_code, expected, num_changes=1): + self.run_and_assert( + tmpdir, + input_code.replace("{func}", func), + expected.replace("{func}", func), + num_changes, + ) + + @each_func + @pytest.mark.parametrize( + "input_code, expected", + [ + # Tuple on the left + ( + "{func}(x, (str, bytes)) or {func}(x, bytearray)", + "{func}(x, (str, bytes, bytearray))", + ), + # Tuple on the right + ( + "{func}(x, str) or {func}(x, (bytes, bytearray))", + "{func}(x, (str, bytes, bytearray))", + ), + # Tuple on both sides no duplicates + ( + "{func}(x, (str, bytes)) or {func}(x, (bytearray, memoryview))", + "{func}(x, (str, bytes, bytearray, memoryview))", + ), + # Tuple on both sides with duplicates + ( + "{func}(x, (str, bytes)) or {func}(x, (str, bytes, bytearray))", + "{func}(x, (str, bytes, bytearray))", + ), + ], + ) + def test_combine_tuples(self, tmpdir, func, input_code, expected): + self._format_func_run_test(tmpdir, func, input_code, expected) + + @each_func + @pytest.mark.parametrize( + "input_code, expected", + [ + # 3 cst.Names + ( + "{func}(x, str) or {func}(x, bytes) or {func}(x, bytearray)", + "{func}(x, (str, bytes, bytearray))", + ), + # 4 cst.Names + ( + "{func}(x, str) or {func}(x, bytes) or {func}(x, bytearray) or {func}(x, some_type)", + "{func}(x, (str, bytes, bytearray, some_type))", + ), + # 5 cst.Names + ( + "{func}(x, str) or {func}(x, bytes) or {func}(x, bytearray) or {func}(x, some_type) or {func}(x, another_type)", + "{func}(x, (str, bytes, bytearray, some_type, another_type))", + ), + # 2 cst.Names and 1 cst.Tuple + ( + "{func}(x, str) or {func}(x, bytes) or {func}(x, (bytearray, memoryview))", + "{func}(x, (str, bytes, bytearray, memoryview))", + ), + # 2 cst.Name and 2 cst.Tuples + ( + "{func}(x, str) or {func}(x, (bytes, bytearray)) or {func}(x, (memoryview, bytearray)) or {func}(x, list)", + "{func}(x, (str, bytes, bytearray, memoryview, list))", + ), + # 3 cst.Tuples + ( + "{func}(x, (str, bytes)) or {func}(x, (bytes, bytearray)) or {func}(x, (bytearray, memoryview))", + "{func}(x, (str, bytes, bytearray, memoryview))", + ), + # 4 cst.Tuples + ( + "{func}(x, (str, bytes)) or {func}(x, (bytes, bytearray)) or {func}(x, (bytearray, memoryview)) or {func}(x, (memoryview, str))", + "{func}(x, (str, bytes, bytearray, memoryview))", + ), + ], + ) + def test_more_than_two_calls(self, tmpdir, func, input_code, expected): + self._format_func_run_test( + tmpdir, func, input_code, expected, input_code.count(" or ") + ) From 7523bf7f1a0e40c959684107f079731fe9d65365 Mon Sep 17 00:00:00 2001 From: Lucas Faudman Date: Fri, 19 Apr 2024 11:29:30 -0700 Subject: [PATCH 2/3] prevent SubprocessShellFalse from causing FileNotFoundError by changing subprocess calls with strings as the first argument --- .../combine_startswith_endswith.py | 75 +++++--- src/core_codemods/subprocess_shell_false.py | 25 +-- .../test_combine_startswith_endswith.py | 168 ++++++++++++++++++ tests/codemods/test_subprocess_shell_false.py | 19 +- 4 files changed, 253 insertions(+), 34 deletions(-) diff --git a/src/core_codemods/combine_startswith_endswith.py b/src/core_codemods/combine_startswith_endswith.py index fcd43b61..95b13f4f 100644 --- a/src/core_codemods/combine_startswith_endswith.py +++ b/src/core_codemods/combine_startswith_endswith.py @@ -1,6 +1,10 @@ import libcst as cst from libcst import matchers as m +from codemodder.codemods.utils import extract_boolean_operands +from codemodder.codemods.utils_decorators import ( + check_filter_by_path_includes_or_excludes, +) from codemodder.codemods.utils_mixin import NameResolutionMixin from core_codemods.api import Metadata, ReviewGuidance, SimpleCodemod @@ -14,39 +18,53 @@ class CombineStartswithEndswith(SimpleCodemod, NameResolutionMixin): ) change_description = "Use tuple of matches instead of boolean expression" + @check_filter_by_path_includes_or_excludes def leave_BooleanOperation( self, original_node: cst.BooleanOperation, updated_node: cst.BooleanOperation ) -> cst.CSTNode: - if not self.filter_by_path_includes_or_excludes( - self.node_position(original_node) - ): - return updated_node - if self.matches_startswith_endswith_or_pattern(original_node): - left_call = cst.ensure_type(updated_node.left, cst.Call) - right_call = cst.ensure_type(updated_node.right, cst.Call) - self.report_change(original_node) - new_arg = cst.Arg( - value=cst.Tuple( - elements=[ - cst.Element(value=left_call.args[0].value), - cst.Element(value=right_call.args[0].value), - ] - ) - ) + elements = [] + seen_evaluated_values = set() + for call in extract_boolean_operands(updated_node, ensure_type=cst.Call): + arg_value = call.args[0].value + if isinstance(arg_value, cst.Tuple): + arg_elements = arg_value.elements + else: + arg_elements = (cst.Element(value=arg_value),) + + for element in arg_elements: + if ( + evaluated_value := getattr( + element.value, "evaluated_value", None + ) + ) in seen_evaluated_values: + # If an element has a non-None evaluated value that has already been seen, continue to avoid duplicates + continue + if evaluated_value is not None: + seen_evaluated_values.add(evaluated_value) + elements.append(element) - return cst.Call(func=left_call.func, args=[new_arg]) + new_arg = cst.Arg(value=cst.Tuple(elements=elements)) + return cst.Call(func=call.func, args=[new_arg]) return updated_node def matches_startswith_endswith_or_pattern( self, node: cst.BooleanOperation ) -> bool: - # Match the pattern: x.startswith("...") or x.startswith("...") + # Match the pattern: x.startswith("...") or x.startswith("...") or x.startswith("...") or ... # and the same but with endswith - args = [m.Arg(value=m.SimpleString())] + args = [ + m.Arg( + value=m.Tuple() + | m.SimpleString() + | m.ConcatenatedString() + | m.FormattedString() + | m.Name() + ) + ] startswith = m.Call( func=m.Attribute(value=m.Name(), attr=m.Name("startswith")), args=args, @@ -60,7 +78,24 @@ def matches_startswith_endswith_or_pattern( ) endswith_or = m.BooleanOperation(left=endswith, operator=m.Or(), right=endswith) - return ( + # Check for simple case: x.startswith("...") or x.startswith("...") + if ( m.matches(node, startswith_or | endswith_or) and node.left.func.value.value == node.right.func.value.value + ): + return True + + # Check for chained case: x.startswith("...") or x.startswith("...") or x.startswith("...") or ... + chained_or = m.BooleanOperation( + left=m.BooleanOperation(operator=m.Or()), + operator=m.Or(), + right=startswith | endswith, ) + if m.matches(node, chained_or): + return all( + call.func.value.value == node.right.func.value.value # Same function + for call in extract_boolean_operands(node, ensure_type=cst.Call) + ) + + # No match + return False diff --git a/src/core_codemods/subprocess_shell_false.py b/src/core_codemods/subprocess_shell_false.py index 41b0451d..90d575a3 100644 --- a/src/core_codemods/subprocess_shell_false.py +++ b/src/core_codemods/subprocess_shell_false.py @@ -1,9 +1,10 @@ import libcst as cst -from libcst import matchers +from libcst import matchers as m from libcst.metadata import ParentNodeProvider from codemodder.codemods.check_annotations import is_disabled_by_annotations from codemodder.codemods.libcst_transformer import NewArg +from codemodder.codemods.utils_decorators import check_node_is_not_selected from codemodder.codemods.utils_mixin import NameResolutionMixin from core_codemods.api import Metadata, Reference, ReviewGuidance, SimpleCodemod @@ -35,17 +36,20 @@ class SubprocessShellFalse(SimpleCodemod, NameResolutionMixin): ) IGNORE_ANNOTATIONS = ["S603"] - def leave_Call(self, original_node: cst.Call, updated_node: cst.Call): - if not self.node_is_selected(original_node): - return updated_node - - if self.find_base_name(original_node.func) in self.SUBPROCESS_FUNCS: + @check_node_is_not_selected + def leave_Call(self, original_node: cst.Call, updated_node: cst.Call) -> cst.Call: + if ( + self.find_base_name(original_node.func) in self.SUBPROCESS_FUNCS + ) and not m.matches( + original_node.args[0], + m.Arg( + value=m.SimpleString() | m.ConcatenatedString() | m.FormattedString() + ), # First argument to subprocess. cannot be a string or setting shell=False will cause a FileNotFoundError + ): for arg in original_node.args: - if matchers.matches( + if m.matches( arg, - matchers.Arg( - keyword=matchers.Name("shell"), value=matchers.Name("True") - ), + m.Arg(keyword=m.Name("shell"), value=m.Name("True")), ) and not is_disabled_by_annotations( original_node, self.metadata, # type: ignore @@ -57,4 +61,5 @@ def leave_Call(self, original_node: cst.Call, updated_node: cst.Call): [NewArg(name="shell", value="False", add_if_missing=False)], ) return self.update_arg_target(updated_node, new_args) + return updated_node diff --git a/tests/codemods/test_combine_startswith_endswith.py b/tests/codemods/test_combine_startswith_endswith.py index 30b26483..f42e05f9 100644 --- a/tests/codemods/test_combine_startswith_endswith.py +++ b/tests/codemods/test_combine_startswith_endswith.py @@ -33,6 +33,7 @@ def test_combine(self, tmpdir, func): "x.startswith('foo') and x.startswith('f') or True", "x.startswith('foo') or x.endswith('f')", "x.startswith('foo') or y.startswith('f')", + "x.startswith('foo') or y.startswith('f') or x.startswith('f')", ], ) def test_no_change(self, tmpdir, code): @@ -52,3 +53,170 @@ def test_exclude_line(self, tmpdir): expected, lines_to_exclude=lines_to_exclude, ) + + def _format_func_run_test(self, tmpdir, func, input_code, expected, num_changes=1): + self.run_and_assert( + tmpdir, + input_code.replace("{func}", func), + expected.replace("{func}", func), + num_changes, + ) + + @each_func + @pytest.mark.parametrize( + "input_code, expected", + [ + # Tuple on the left + ( + "x.{func}(('f', 'foo')) or x.{func}('bar')", + "x.{func}(('f', 'foo', 'bar'))", + ), + # Tuple on the right + ( + "x.{func}('f') or x.{func}(('foo', 'bar'))", + "x.{func}(('f', 'foo', 'bar'))", + ), + # Tuple on both sides + ( + "x.{func}(('1', '2')) or x.{func}(('3', '4'))", + "x.{func}(('1', '2', '3', '4'))", + ), + # Tuples on both sides with duplicate elements + ( + "x.{func}(('1', '2', '3')) or x.{func}(('2', '3', '4'))", + "x.{func}(('1', '2', '3', '4'))", + ), + ], + ) + def test_combine_tuples(self, tmpdir, func, input_code, expected): + self._format_func_run_test(tmpdir, func, input_code, expected) + + @each_func + @pytest.mark.parametrize( + "input_code, expected", + [ + # cst.ConcatenatedString on the left + ( + "x.{func}('foo' 'bar') or x.{func}('baz')", + "x.{func}(('foo' 'bar', 'baz'))", + ), + # cst.ConcatenatedString on the right + ( + "x.{func}('foo') or x.{func}('bar' 'baz')", + "x.{func}(('foo', 'bar' 'baz'))", + ), + # cst.ConcatenatedString on both sides + ( + "x.{func}('foo' 'bar') or x.{func}('baz' 'qux')", + "x.{func}(('foo' 'bar', 'baz' 'qux'))", + ), + ], + ) + def test_concat_string_args(self, tmpdir, func, input_code, expected): + self._format_func_run_test(tmpdir, func, input_code, expected) + + @each_func + @pytest.mark.parametrize( + "input_code, expected", + [ + # cst.FormattedString on the left + ( + "x.{func}(f'formatted {foo}') or x.{func}('bar')", + "x.{func}((f'formatted {foo}', 'bar'))", + ), + # cst.FormattedString on the right + ( + "x.{func}('foo') or x.{func}(f'formatted {bar}')", + "x.{func}(('foo', f'formatted {bar}'))", + ), + # cst.FormattedString on both sides + ( + "x.{func}(f'formatted {foo}') or x.{func}(f'formatted {bar}')", + "x.{func}((f'formatted {foo}', f'formatted {bar}'))", + ), + ], + ) + def test_format_string_args(self, tmpdir, func, input_code, expected): + self._format_func_run_test(tmpdir, func, input_code, expected) + + @each_func + @pytest.mark.parametrize( + "input_code, expected", + [ + # cst.Name on the left + ("x.{func}(y) or x.{func}('foo')", "x.{func}((y, 'foo'))"), + # cst.Name on the right + ("x.{func}('foo') or x.{func}(y)", "x.{func}(('foo', y))"), + # cst.Name on both sides + ("x.{func}(y) or x.{func}(z)", "x.{func}((y, z))"), + # cst.Name in tuple on the left + ("x.{func}((y, 'foo')) or x.{func}('bar')", "x.{func}((y, 'foo', 'bar'))"), + # cst.Name in tuple on the right + ("x.{func}('foo') or x.{func}((y, 'bar'))", "x.{func}(('foo', y, 'bar'))"), + # cst.Name in tuple on both sides + ( + "x.{func}((y, 'foo')) or x.{func}((z, 'bar'))", + "x.{func}((y, 'foo', z, 'bar'))", + ), + ], + ) + def test_name_args(self, tmpdir, func, input_code, expected): + self._format_func_run_test(tmpdir, func, input_code, expected) + + _100_of_each_type = [ + (f"'{i}'", f"'{i}con' 'cat{i}'", f"f'fmt{i}'", f"name{i}") for i in range(100) + ] + + @each_func + @pytest.mark.parametrize( + "input_code, expected", + [ + # 3 cst.SimpleStrings + ( + "x.{func}('foo') or x.{func}('bar') or x.{func}('baz')", + "x.{func}(('foo', 'bar', 'baz'))", + ), + # 3 cst.SimpleStrings with duplicates + ( + "x.{func}('foo') or x.{func}('bar') or x.{func}('foo')", + "x.{func}(('foo', 'bar'))", + ), + # 2 cst.SimpleStrings 2 cst.Tuple alternating + ( + "x.{func}('a') or x.{func}(('b', 'c')) or x.{func}('d') or x.{func}(('e', 'f', 'g', 'h'))", + "x.{func}(('a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'))", + ), + # 100 cst.SimpleStrings + ( + " or ".join("x.{func}" + f"({item[0]})" for item in _100_of_each_type), + "x.{func}" + f"(({', '.join(item[0] for item in _100_of_each_type)}))", + ), + # 100 cst.ConcatenatedStrings + ( + " or ".join("x.{func}" + f"({item[1]})" for item in _100_of_each_type), + "x.{func}" + f"(({', '.join(item[1] for item in _100_of_each_type)}))", + ), + # 100 cst.FormattedStrings + ( + " or ".join("x.{func}" + f"({item[2]})" for item in _100_of_each_type), + "x.{func}" + f"(({', '.join(item[2] for item in _100_of_each_type)}))", + ), + # 100 cst.Names + ( + " or ".join("x.{func}" + f"({item[3]})" for item in _100_of_each_type), + "x.{func}" + f"(({', '.join(item[3] for item in _100_of_each_type)}))", + ), + # 100 cst.Tuples with all types + ( + " or ".join( + "x.{func}" + f"(({', '.join(item)}))" for item in _100_of_each_type + ), + "x.{func}" + + f"(({', '.join(', '.join(item) for item in _100_of_each_type)}))", + ), + ], + ) + def test_more_than_two_calls(self, tmpdir, func, input_code, expected): + self._format_func_run_test( + tmpdir, func, input_code, expected, num_changes=input_code.count("or") + ) diff --git a/tests/codemods/test_subprocess_shell_false.py b/tests/codemods/test_subprocess_shell_false.py index 3239d6af..1f5ada2d 100644 --- a/tests/codemods/test_subprocess_shell_false.py +++ b/tests/codemods/test_subprocess_shell_false.py @@ -20,11 +20,11 @@ def test_import(self, tmpdir, func): import subprocess subprocess.{func}(args, shell=True) """ - expexted_output = f""" + expected = f""" import subprocess subprocess.{func}(args, shell=False) """ - self.run_and_assert(tmpdir, input_code, expexted_output) + self.run_and_assert(tmpdir, input_code, expected) @each_func def test_from_import(self, tmpdir, func): @@ -32,11 +32,11 @@ def test_from_import(self, tmpdir, func): from subprocess import {func} {func}(args, shell=True) """ - expexted_output = f""" + expected = f""" from subprocess import {func} {func}(args, shell=False) """ - self.run_and_assert(tmpdir, input_code, expexted_output) + self.run_and_assert(tmpdir, input_code, expected) @each_func def test_no_shell(self, tmpdir, func): @@ -89,3 +89,14 @@ def test_different_noqa_message(self, tmpdir): subprocess.run(args, shell=False) # noqa: S604 """ self.run_and_assert(tmpdir, input_code, expected) + + def test_no_change_if_first_arg_is_string(self, tmpdir): + input_code = """ + import subprocess + subprocess.run("ls -l", shell=True) + subprocess.run("ls" "-l", shell=True) + ls_args = "-l -a" + subprocess.run(f"ls {ls_args}", shell=True) + """ + + self.run_and_assert(tmpdir, input_code, input_code, num_changes=0) From 48119705bcd5894af5dd1ca2d313732d75c901e6 Mon Sep 17 00:00:00 2001 From: Lucas Faudman Date: Fri, 19 Apr 2024 11:35:35 -0700 Subject: [PATCH 3/3] Add new CodeMod CombineIsinstanceIssubclass. isistance(x, str) or isinstance(x, (bytes, list)) -> isinstance(x, (str, bytes, list) --- src/core_codemods/combine_isinstance_issubclass.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core_codemods/combine_isinstance_issubclass.py b/src/core_codemods/combine_isinstance_issubclass.py index dea6f19f..befa80df 100644 --- a/src/core_codemods/combine_isinstance_issubclass.py +++ b/src/core_codemods/combine_isinstance_issubclass.py @@ -16,7 +16,7 @@ class CombineIsinstanceIssubclass(SimpleCodemod, NameResolutionMixin): review_guidance=ReviewGuidance.MERGE_WITHOUT_REVIEW, references=[], ) - change_description = "Use tuple of matches instead of boolean expression" + change_description = "Use tuple of matches instead of boolean expression with `isinstance` or `issubclass`" @check_filter_by_path_includes_or_excludes def leave_BooleanOperation(