diff --git a/integration_tests/test_fix_missing_self_or_cls.py b/integration_tests/test_fix_missing_self_or_cls.py new file mode 100644 index 00000000..afe6bac5 --- /dev/null +++ b/integration_tests/test_fix_missing_self_or_cls.py @@ -0,0 +1,47 @@ +from codemodder.codemods.test import ( + BaseIntegrationTest, + original_and_expected_from_code_path, +) +from core_codemods.fix_missing_self_or_cls import ( + FixMissingSelfOrCls, + FixMissingSelfOrClsTransformer, +) + + +class TestFixMissingSelfOrCls(BaseIntegrationTest): + codemod = FixMissingSelfOrCls + code_path = "tests/samples/fix_missing_self_or_cls.py" + original_code, expected_new_code = original_and_expected_from_code_path( + code_path, + [ + ( + 1, + """ def instance_method(self):\n""", + ), + ( + 5, + """ def class_method(cls):\n""", + ), + ], + ) + + # fmt: off + expected_diff = ( + """--- \n""" + """+++ \n""" + """@@ -1,7 +1,7 @@\n""" + """ class MyClass:\n""" + """- def instance_method():\n""" + """+ def instance_method(self):\n""" + """ print("instance_method")\n""" + """ \n""" + """ @classmethod\n""" + """- def class_method():\n""" + """+ def class_method(cls):\n""" + """ print("class_method")\n""" + ) + # fmt: on + + expected_line_change = "2" + change_description = FixMissingSelfOrClsTransformer.change_description + num_changes = 2 diff --git a/integration_tests/test_sonar_fix_missing_self_or_cls.py b/integration_tests/test_sonar_fix_missing_self_or_cls.py new file mode 100644 index 00000000..01bcc6b5 --- /dev/null +++ b/integration_tests/test_sonar_fix_missing_self_or_cls.py @@ -0,0 +1,45 @@ +from codemodder.codemods.test import ( + BaseIntegrationTest, + original_and_expected_from_code_path, +) +from core_codemods.fix_missing_self_or_cls import FixMissingSelfOrClsTransformer +from core_codemods.sonar.sonar_fix_missing_self_or_cls import SonarFixMissingSelfOrCls + + +class TestSonarFixMissingSelfOrCls(BaseIntegrationTest): + codemod = SonarFixMissingSelfOrCls + code_path = "tests/samples/fix_missing_self_or_cls.py" + original_code, expected_new_code = original_and_expected_from_code_path( + code_path, + [ + ( + 1, + """ def instance_method(self):\n""", + ), + ( + 5, + """ def class_method(cls):\n""", + ), + ], + ) + sonar_issues_json = "tests/samples/sonar_issues.json" + # fmt: off + expected_diff = ( + """--- \n""" + """+++ \n""" + """@@ -1,7 +1,7 @@\n""" + """ class MyClass:\n""" + """- def instance_method():\n""" + """+ def instance_method(self):\n""" + """ print("instance_method")\n""" + """ \n""" + """ @classmethod\n""" + """- def class_method():\n""" + """+ def class_method(cls):\n""" + """ print("class_method")\n""" + ) + # fmt: on + + expected_line_change = "2" + change_description = FixMissingSelfOrClsTransformer.change_description + num_changes = 2 diff --git a/integration_tests/test_unnecessary_f_str.py b/integration_tests/test_unnecessary_f_str.py index 338733b9..7708708a 100644 --- a/integration_tests/test_unnecessary_f_str.py +++ b/integration_tests/test_unnecessary_f_str.py @@ -1,5 +1,3 @@ -import pytest - from codemodder.codemods.test import ( BaseIntegrationTest, original_and_expected_from_code_path, @@ -10,9 +8,6 @@ ) -@pytest.mark.skipif( - True, reason="May fail if it runs after test_sql_parameterization. See Issue #378." -) class TestFStr(BaseIntegrationTest): codemod = RemoveUnnecessaryFStr code_path = "tests/samples/unnecessary_f_str.py" diff --git a/src/codemodder/codemods/utils_mixin.py b/src/codemodder/codemods/utils_mixin.py index 5047bd57..a7876a00 100644 --- a/src/codemodder/codemods/utils_mixin.py +++ b/src/codemodder/codemods/utils_mixin.py @@ -277,6 +277,18 @@ def is_builtin_function(self, node: cst.Call): return matchers.matches(node.func, matchers.Name()) return False + def is_staticmethod(self, node: cst.FunctionDef) -> bool: + for decorator in node.decorators: + if self.find_base_name(decorator.decorator) == "builtins.staticmethod": + return True + return False + + def is_classmethod(self, node: cst.FunctionDef) -> bool: + for decorator in node.decorators: + if self.find_base_name(decorator.decorator) == "builtins.classmethod": + return True + return False + def find_accesses(self, node) -> Collection[Access]: if scope := self.get_metadata(ScopeProvider, node, None): return scope.accesses[node] @@ -437,10 +449,10 @@ def find_immediate_class_def(self, node: cst.CSTNode) -> Optional[cst.ClassDef]: def path_to_root(self, node: cst.CSTNode) -> list[cst.CSTNode]: """ - Returns node's path to root. Includes self. + Returns node's path to `node` (excludes `node`). """ path = [] - maybe_parent = node + maybe_parent = self.get_parent(node) while maybe_parent: path.append(maybe_parent) maybe_parent = self.get_parent(maybe_parent) diff --git a/src/codemodder/scripts/generate_docs.py b/src/codemodder/scripts/generate_docs.py index 6fd25321..975773a8 100644 --- a/src/codemodder/scripts/generate_docs.py +++ b/src/codemodder/scripts/generate_docs.py @@ -251,6 +251,10 @@ class DocMetadata: importance="Medium", guidance_explained="This change is safe and will prevent runtime `ValueError`.", ), + "fix-missing-self-or-cls": DocMetadata( + importance="Medium", + guidance_explained="This change is safe and will prevent errors when calling on these instance or class methods..", + ), } METADATA = CORE_METADATA | { @@ -305,6 +309,11 @@ class DocMetadata: guidance_explained=CORE_METADATA["jwt-decode-verify"].guidance_explained, need_sarif="Yes (Sonar)", ), + "fix-missing-self-or-cls-S5719": DocMetadata( + importance=CORE_METADATA["fix-missing-self-or-cls"].importance, + guidance_explained=CORE_METADATA["fix-missing-self-or-cls"].guidance_explained, + need_sarif="Yes (Sonar)", + ), } diff --git a/src/core_codemods/__init__.py b/src/core_codemods/__init__.py index 8461c339..f5a731f5 100644 --- a/src/core_codemods/__init__.py +++ b/src/core_codemods/__init__.py @@ -17,6 +17,7 @@ from .fix_deprecated_logging_warn import FixDeprecatedLoggingWarn from .fix_empty_sequence_comparison import FixEmptySequenceComparison from .fix_hasattr_call import TransformFixHasattrCall +from .fix_missing_self_or_cls import FixMissingSelfOrCls from .fix_mutable_params import FixMutableParams from .flask_enable_csrf_protection import FlaskEnableCSRFProtection from .flask_json_response_type import FlaskJsonResponseType @@ -37,8 +38,7 @@ from .remove_debug_breakpoint import RemoveDebugBreakpoint from .remove_future_imports import RemoveFutureImports from .remove_module_global import RemoveModuleGlobal - -# from .remove_unnecessary_f_str import RemoveUnnecessaryFStr +from .remove_unnecessary_f_str import RemoveUnnecessaryFStr from .remove_unused_imports import RemoveUnusedImports from .replace_flask_send_file import ReplaceFlaskSendFile from .requests_verify import RequestsVerify @@ -49,6 +49,7 @@ from .sonar.sonar_django_receiver_on_top import SonarDjangoReceiverOnTop from .sonar.sonar_exception_without_raise import SonarExceptionWithoutRaise from .sonar.sonar_fix_assert_tuple import SonarFixAssertTuple +from .sonar.sonar_fix_missing_self_or_cls import SonarFixMissingSelfOrCls from .sonar.sonar_flask_json_response_type import SonarFlaskJsonResponseType from .sonar.sonar_jwt_decode_verify import SonarJwtDecodeVerify from .sonar.sonar_literal_or_new_object_identity import SonarLiteralOrNewObjectIdentity @@ -89,7 +90,7 @@ OrderImports, ProcessSandbox, RemoveFutureImports, - # RemoveUnnecessaryFStr, # Temporarely disabled due to potential error. See Issue #378. + RemoveUnnecessaryFStr, RemoveUnusedImports, RequestsVerify, SecureFlaskCookie, @@ -128,6 +129,7 @@ DjangoModelWithoutDunderStr, TransformFixHasattrCall, FixDataclassDefaults, + FixMissingSelfOrCls, ], ) @@ -143,5 +145,6 @@ SonarFlaskJsonResponseType, SonarDjangoJsonResponseType, SonarJwtDecodeVerify, + SonarFixMissingSelfOrCls, ], ) diff --git a/src/core_codemods/docs/pixee_python_fix-missing-self-or-cls.md b/src/core_codemods/docs/pixee_python_fix-missing-self-or-cls.md new file mode 100644 index 00000000..b06bc5bf --- /dev/null +++ b/src/core_codemods/docs/pixee_python_fix-missing-self-or-cls.md @@ -0,0 +1,15 @@ +Python instance methods must be defined with `self` as the first argument. Likewise, class methods must have `cls` as the first argument. This codemod will add these arguments when the method/class method has no arguments defined. + +Our changes look something like this: + +```diff + class MyClass: +- def instance_method(): ++ def instance_method(self): + print("instance_method") + + @classmethod +- def class_method(): ++ def class_method(cls): + print("class_method") +``` diff --git a/src/core_codemods/fix_missing_self_or_cls.py b/src/core_codemods/fix_missing_self_or_cls.py new file mode 100644 index 00000000..56b5d4d7 --- /dev/null +++ b/src/core_codemods/fix_missing_self_or_cls.py @@ -0,0 +1,87 @@ +import libcst as cst + +from codemodder.codemods.libcst_transformer import ( + LibcstResultTransformer, + LibcstTransformerPipeline, +) +from codemodder.codemods.utils_mixin import NameAndAncestorResolutionMixin +from core_codemods.api import Metadata, ReviewGuidance +from core_codemods.api.core_codemod import CoreCodemod + + +class FixMissingSelfOrClsTransformer( + LibcstResultTransformer, NameAndAncestorResolutionMixin +): + change_description = "Add `self` or `cls` parameter to instance or class method." + + def leave_FunctionDef( + self, original_node: cst.FunctionDef, updated_node: cst.FunctionDef + ) -> cst.FunctionDef: + # TODO: add filter by include or exclude that works for nodes + # that that have different start/end numbers. + + if not self.find_immediate_class_def(original_node): + # If `original_node` is not inside a class, nothing to do. + return original_node + + if self.find_immediate_function_def(original_node): + # If `original_node` is inside a class but also nested within a function/method + # We won't touch it. + return original_node + + if original_node.decorators: + if self.is_staticmethod(original_node): + return updated_node + if self.is_classmethod(original_node): + if self.has_no_args(original_node): + self.report_change(original_node) + return updated_node.with_changes( + params=updated_node.params.with_changes( + params=[cst.Param(name=cst.Name("cls"))] + ) + ) + else: + if self.has_no_args(original_node): + self.report_change(original_node) + return updated_node.with_changes( + params=updated_node.params.with_changes( + params=[cst.Param(name=self._pick_arg_name(original_node))] + ) + ) + return updated_node + + def _pick_arg_name(self, node: cst.FunctionDef) -> cst.Name: + match node.name: + case cst.Name(value="__new__") | cst.Name(value="__init_subclass__"): + new_name = "cls" + case _: + new_name = "self" + return cst.Name(value=new_name) + + def has_no_args(self, node: cst.FunctionDef) -> bool: + converted_star_arg = ( + None + if node.params.star_arg is cst.MaybeSentinel.DEFAULT + else node.params.star_arg + ) + return not any( + ( + node.params.params, + converted_star_arg, + node.params.kwonly_params, + node.params.star_kwarg, + node.params.posonly_params, + ) + ) + + +FixMissingSelfOrCls = CoreCodemod( + metadata=Metadata( + name="fix-missing-self-or-cls", + review_guidance=ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW, + summary="Add Missing Positional Parameter for Instance and Class Methods", + references=[], + ), + transformer=LibcstTransformerPipeline(FixMissingSelfOrClsTransformer), + detector=None, +) diff --git a/src/core_codemods/remove_unnecessary_f_str.py b/src/core_codemods/remove_unnecessary_f_str.py index 26480be0..f8d4dda6 100644 --- a/src/core_codemods/remove_unnecessary_f_str.py +++ b/src/core_codemods/remove_unnecessary_f_str.py @@ -1,6 +1,7 @@ +from typing import cast + import libcst as cst import libcst.matchers as m -from libcst.codemod import CodemodContext from libcst.codemod.commands.unnecessary_format_string import UnnecessaryFormatString from codemodder.codemods.libcst_transformer import ( @@ -11,18 +12,9 @@ from core_codemods.api.core_codemod import CoreCodemod -class RemoveUnnecessaryFStrTransform(LibcstResultTransformer, UnnecessaryFormatString): - +class RemoveUnnecessaryFStrTransform(LibcstResultTransformer): change_description = "Remove unnecessary f-string" - def __init__( - self, codemod_context: CodemodContext, *codemod_args, **codemod_kwargs - ): - UnnecessaryFormatString.__init__(self, codemod_context) - LibcstResultTransformer.__init__( - self, codemod_context, *codemod_args, **codemod_kwargs - ) - @m.leave(m.FormattedString(parts=(m.FormattedStringText(),))) def _check_formatted_string( self, @@ -34,7 +26,9 @@ def _check_formatted_string( ): return updated_node - transformed_node = super()._check_formatted_string(_original_node, updated_node) + transformed_node = UnnecessaryFormatString._check_formatted_string( + cast(UnnecessaryFormatString, self), _original_node, updated_node + ) if not _original_node.deep_equals(transformed_node): self.report_change(_original_node) return transformed_node diff --git a/src/core_codemods/sonar/sonar_fix_missing_self_or_cls.py b/src/core_codemods/sonar/sonar_fix_missing_self_or_cls.py new file mode 100644 index 00000000..d624c86c --- /dev/null +++ b/src/core_codemods/sonar/sonar_fix_missing_self_or_cls.py @@ -0,0 +1,10 @@ +from codemodder.codemods.sonar import SonarCodemod +from core_codemods.fix_missing_self_or_cls import FixMissingSelfOrCls + +SonarFixMissingSelfOrCls = SonarCodemod.from_core_codemod( + name="fix-missing-self-or-cls-S5719", + other=FixMissingSelfOrCls, + rule_id="python:S5719", + rule_name="Instance and class methods should have at least one positional parameter", + rule_url="https://rules.sonarsource.com/python/RSPEC-5719/", +) diff --git a/tests/codemods/test_fix_missing_self_or_cls.py b/tests/codemods/test_fix_missing_self_or_cls.py new file mode 100644 index 00000000..3116c6c1 --- /dev/null +++ b/tests/codemods/test_fix_missing_self_or_cls.py @@ -0,0 +1,137 @@ +import pytest + +from codemodder.codemods.test import BaseCodemodTest +from core_codemods.fix_missing_self_or_cls import FixMissingSelfOrCls + + +class TestFixMissingSelfOrCls(BaseCodemodTest): + codemod = FixMissingSelfOrCls + + def test_name(self): + assert self.codemod.name == "fix-missing-self-or-cls" + + def test_change(self, tmpdir): + input_code = """ + class A: + def method(): + pass + + @classmethod + def clsmethod(): + pass + + def __new__(): + pass + def __init_subclass__(): + pass + """ + expected = """ + class A: + def method(self): + pass + + @classmethod + def clsmethod(cls): + pass + + def __new__(cls): + pass + def __init_subclass__(cls): + pass + """ + self.run_and_assert(tmpdir, input_code, expected, num_changes=4) + + def test_change_not_nested(self, tmpdir): + input_code = """ + class A: + def method(): + def inner(): + pass + + @classmethod + def clsmethod(): + def other_inner(): + pass + + def wrapper(): + class B: + def method(): + pass + """ + expected = """ + class A: + def method(self): + def inner(): + pass + + @classmethod + def clsmethod(cls): + def other_inner(): + pass + + def wrapper(): + class B: + def method(): + pass + """ + self.run_and_assert(tmpdir, input_code, expected, num_changes=2) + + @pytest.mark.parametrize( + "code", + [ + """ + def my_function(): + pass + """, + """ + class A: + def method(self, arg): + pass + + @classmethod + def clsmethod(cls, arg): + pass + + @staticmethod + def my_static(): + pass + """, + """ + class MyBaseClass: + def __new__(*args, **kwargs): + pass + def __init_subclass__(**kwargs): + pass + """, + """ + class A(): + def f(self): + def g(): + pass + """, + ], + ) + def test_no_change(self, tmpdir, code): + self.run_and_assert(tmpdir, code, code) + + def test_with_args_no_change(self, tmpdir): + input_code = """ + class A: + def method(one, two): + pass + + def method(*args, two=2): + pass + + def method(**kwargs): + pass + + @classmethod + def clsmethod(one, two): + pass + + @classmethod + def kls(**kwargs): + pass + """ + self.run_and_assert(tmpdir, input_code, input_code) diff --git a/tests/codemods/test_remove_unnecessary_f_str.py b/tests/codemods/test_remove_unnecessary_f_str.py index 39505e8c..3bf6d849 100644 --- a/tests/codemods/test_remove_unnecessary_f_str.py +++ b/tests/codemods/test_remove_unnecessary_f_str.py @@ -1,5 +1,3 @@ -import pytest - from codemodder.codemods.test import BaseCodemodTest from core_codemods.remove_unnecessary_f_str import RemoveUnnecessaryFStr @@ -7,9 +5,6 @@ class TestFStr(BaseCodemodTest): codemod = RemoveUnnecessaryFStr - @pytest.mark.skip( - reason="May fail if it runs after the test_sql_parameterization. See Issue #378." - ) def test_no_change(self, tmpdir): before = r""" good: str = "good" @@ -23,9 +18,6 @@ def test_no_change(self, tmpdir): """ self.run_and_assert(tmpdir, before, before) - @pytest.mark.skip( - reason="May fail if it runs after the test_sql_parameterization. See Issue #378." - ) def test_change(self, tmpdir): before = r""" bad: str = f"bad" + "bad" @@ -39,9 +31,6 @@ def test_change(self, tmpdir): """ self.run_and_assert(tmpdir, before, after, num_changes=3) - @pytest.mark.skip( - reason="May fail if it runs after the test_sql_parameterization. See Issue #378." - ) def test_exclude_line(self, tmpdir): input_code = ( expected diff --git a/tests/codemods/test_sonar_fix_missing_self_or_cls.py b/tests/codemods/test_sonar_fix_missing_self_or_cls.py new file mode 100644 index 00000000..39bda2ec --- /dev/null +++ b/tests/codemods/test_sonar_fix_missing_self_or_cls.py @@ -0,0 +1,65 @@ +import json + +from codemodder.codemods.test import BaseSASTCodemodTest +from core_codemods.sonar.sonar_fix_missing_self_or_cls import SonarFixMissingSelfOrCls + + +class TestSonarFixMissingSelfOrCls(BaseSASTCodemodTest): + codemod = SonarFixMissingSelfOrCls + tool = "sonar" + + def test_name(self): + assert self.codemod.name == "fix-missing-self-or-cls-S5719" + + def test_simple(self, tmpdir): + input_code = """ + class A: + def method(): + pass + + @classmethod + def clsmethod(): + pass + """ + expected_output = """ + class A: + def method(self): + pass + + @classmethod + def clsmethod(cls): + pass + """ + issues = { + "issues": [ + { + "rule": "python:S5719", + "status": "OPEN", + "component": "code.py", + "textRange": { + "startLine": 2, + "endLine": 2, + "startOffset": 4, + "endOffset": 25, + }, + }, + { + "rule": "python:S5719", + "status": "OPEN", + "component": "code.py", + "textRange": { + "startLine": 6, + "endLine": 6, + "startOffset": 4, + "endOffset": 22, + }, + }, + ] + } + self.run_and_assert( + tmpdir, + input_code, + expected_output, + results=json.dumps(issues), + num_changes=2, + ) diff --git a/tests/samples/fix_missing_self_or_cls.py b/tests/samples/fix_missing_self_or_cls.py new file mode 100644 index 00000000..9cfad558 --- /dev/null +++ b/tests/samples/fix_missing_self_or_cls.py @@ -0,0 +1,7 @@ +class MyClass: + def instance_method(): + print("instance_method") + + @classmethod + def class_method(): + print("class_method") diff --git a/tests/test_ancestorpatterns_mixin.py b/tests/test_ancestorpatterns_mixin.py index ba86e824..8c683b6e 100644 --- a/tests/test_ancestorpatterns_mixin.py +++ b/tests/test_ancestorpatterns_mixin.py @@ -46,7 +46,7 @@ def transform_module_impl(self, tree: cst.Module) -> cst.Module: node = stmt.body[0] path = self.path_to_root(node) - assert len(path) == 3 + assert len(path) == 2 return tree input_code = dedent(