Correct node position for FuncDef nodes and fix sonar codemods (#423)

* handle node position for function def

* add exclude logic to fix-mutable-params

* end is 0-idx length

* json-ify issues

* fix missing self or cls should have node selection logic

* add unit test

* dedupe code

Author: clavedeluna

src/codemodder/codemods/base_visitor.py

@@ -1,6 +1,8 @@
 from typing import ClassVar, Collection
 
+import libcst as cst
 from libcst import MetadataDependent
+from libcst._position import CodePosition, CodeRange
 from libcst.codemod import ContextAwareVisitor, VisitorBasedCodemodCommand
 from libcst.metadata import PositionProvider, ProviderT
 
@@ -48,7 +50,18 @@ def node_is_selected(self, node) -> bool:
 
     def node_position(self, node):
         # See https://github.com/Instagram/LibCST/blob/main/libcst/_metadata_dependent.py#L112
-        return self.get_metadata(PositionProvider, node)
+        match node:
+            case cst.FunctionDef():
+                # By default a function's position includes the entire
+                # function definition. Instead, we will only use the first line
+                # of the function definition.
+                params_end = self.get_metadata(PositionProvider, node.params).end
+                return CodeRange(
+                    start=self.get_metadata(PositionProvider, node).start,
+                    end=CodePosition(params_end.line, params_end.column + 1),
+                )
+            case _:
+                return self.get_metadata(PositionProvider, node)
 
     def lineno_for_node(self, node):
         return self.node_position(node).start.line

src/codemodder/codemods/imported_call_modifier.py

@@ -92,22 +92,3 @@ def leave_Call(self, original_node: cst.Call, updated_node: cst.Call):
                 )
 
         return updated_node
-
-    def filter_by_path_includes_or_excludes(self, pos_to_match):
-        """
-        Returns False if the node, whose position in the file is pos_to_match, matches any of the lines specified in the path-includes or path-excludes flags.
-        """
-        # excludes takes precedence if defined
-        if self.line_exclude:
-            return not any(match_line(pos_to_match, line) for line in self.line_exclude)
-        if self.line_include:
-            return any(match_line(pos_to_match, line) for line in self.line_include)
-        return True
-
-    def node_position(self, node):
-        # See https://github.com/Instagram/LibCST/blob/main/libcst/_metadata_dependent.py#L112
-        return self.get_metadata(PositionProvider, node)
-
-
-def match_line(pos, line):
-    return pos.start.line == line and pos.end.line == line

src/codemodder/codemods/libcst_transformer.py

@@ -5,7 +5,6 @@
 from libcst._position import CodeRange
 from libcst.codemod import CodemodContext
 from libcst.codemod.visitors import AddImportsVisitor, RemoveImportsVisitor
-from libcst.metadata import PositionProvider
 
 from codemodder.codemods.base_transformer import BaseTransformerPipeline
 from codemodder.codemods.base_visitor import BaseTransformer
@@ -98,10 +97,6 @@ def leave_ClassDef(
     ) -> cst.ClassDef:
         return self._new_or_updated_node(original_node, updated_node)
 
-    def node_position(self, node):
-        # See https://github.com/Instagram/LibCST/blob/main/libcst/_metadata_dependent.py#L112
-        return self.get_metadata(PositionProvider, node)
-
     def add_change(self, node, description: str, start: bool = True):
         position = self.node_position(node)
         self.add_change_from_position(position, description, start)

src/core_codemods/fix_missing_self_or_cls.py

@@ -17,8 +17,8 @@ class FixMissingSelfOrClsTransformer(
     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.node_is_selected(original_node):
+            return original_node
 
         if not self.find_immediate_class_def(original_node):
             # If `original_node` is not inside a class, nothing to do.

src/core_codemods/fix_mutable_params.py

@@ -167,8 +167,8 @@ def leave_FunctionDef(
         updated_node: cst.FunctionDef,
     ):
         """Transforms function definitions with mutable default parameters"""
-        # TODO: add filter by include or exclude that works for nodes
-        # that that have different start/end numbers.
+        if not self.node_is_selected(original_node):
+            return updated_node
 
         (
             updated_params,

src/core_codemods/order_imports.py

@@ -1,14 +1,15 @@
 import libcst as cst
 from libcst.metadata import PositionProvider
 
+from codemodder.codemods.base_visitor import UtilsMixin
 from codemodder.codemods.transformations.clean_imports import (
     GatherTopLevelImportBlocks,
     OrderImportsBlocksTransform,
 )
 from core_codemods.api import Metadata, ReviewGuidance, SimpleCodemod
 
 
-class OrderImports(SimpleCodemod):
+class OrderImports(SimpleCodemod, UtilsMixin):
     metadata = Metadata(
         name="order-imports",
         summary="Order Imports",
@@ -46,22 +47,3 @@ def transform_module_impl(self, tree: cst.Module) -> cst.Module:
                     )
             return result_tree
         return tree
-
-    def filter_by_path_includes_or_excludes(self, pos_to_match):
-        """
-        Returns False if the node, whose position in the file is pos_to_match, matches any of the lines specified in the path-includes or path-excludes flags.
-        """
-        # excludes takes precedence if defined
-        if self.line_exclude:
-            return not any(match_line(pos_to_match, line) for line in self.line_exclude)
-        if self.line_include:
-            return any(match_line(pos_to_match, line) for line in self.line_include)
-        return True
-
-    def node_position(self, node):
-        # See https://github.com/Instagram/LibCST/blob/main/libcst/_metadata_dependent.py#L112
-        return self.get_metadata(PositionProvider, node)
-
-
-def match_line(pos, line):
-    return pos.start.line == line and pos.end.line == line

tests/codemods/test_base_visitor.py

@@ -1,6 +1,8 @@
 from collections import defaultdict
+from textwrap import dedent
 
 import libcst as cst
+from libcst._position import CodePosition, CodeRange
 from libcst.codemod import CodemodContext
 from libcst.metadata import PositionProvider
 
@@ -29,6 +31,30 @@ def leave_SimpleStatementLine(
         return original_node
 
 
+class AssertPositionCodemod(BaseTransformer):
+    METADATA_DEPENDENCIES = (PositionProvider,)
+
+    def __init__(
+        self,
+        context,
+        results,
+        expected_node_position,
+        line_exclude=None,
+        line_include=None,
+    ):
+        BaseTransformer.__init__(
+            self, context, results, line_include or [], line_exclude or []
+        )
+        self.expected_node_position = expected_node_position
+
+    def leave_FunctionDef(
+        self, original_node: cst.FunctionDef, updated_node: cst.FunctionDef
+    ) -> cst.FunctionDef:
+        pos_to_match = self.node_position(original_node)
+        assert pos_to_match == self.expected_node_position
+        return updated_node
+
+
 class TestBaseVisitor:
     def run_and_assert(self, input_code, expected, line_exclude, line_include):
         input_tree = cst.parse_module(input_code)
@@ -59,3 +85,43 @@ def test_includes_excludes(self):
         line_exclude = [1]
         line_include = [1]
         self.run_and_assert(input_code, expected, line_exclude, line_include)
+
+
+class TestNodePosition:
+    def run_and_assert(self, input_code, expected_pos):
+        input_tree = cst.parse_module(dedent(input_code))
+        command_instance = AssertPositionCodemod(
+            CodemodContext(), defaultdict(list), expected_pos
+        )
+        command_instance.transform_module(input_tree)
+
+    def test_funcdef(self):
+        input_code = """
+        def hello():
+            pass
+        """
+        expected_pos = CodeRange(
+            start=CodePosition(line=2, column=0), end=CodePosition(line=2, column=11)
+        )
+        self.run_and_assert(input_code, expected_pos)
+
+    def test_instance(self):
+        input_code = """
+        class MyClass:
+            def instance_method():
+                print("instance_method")
+        """
+        expected_pos = CodeRange(
+            start=CodePosition(line=3, column=4), end=CodePosition(line=3, column=25)
+        )
+        self.run_and_assert(input_code, expected_pos)
+
+    def test_funcdef_args(self):
+        input_code = """
+        def hello(one, *args, **kwargs):
+            pass
+        """
+        expected_pos = CodeRange(
+            start=CodePosition(line=2, column=0), end=CodePosition(line=2, column=31)
+        )
+        self.run_and_assert(input_code, expected_pos)

tests/codemods/test_fix_missing_self_or_cls.py

@@ -135,3 +135,19 @@ def kls(**kwargs):
                pass
            """
         self.run_and_assert(tmpdir, input_code, input_code)
+
+    def test_exclude_line(self, tmpdir):
+        input_code = (
+            expected
+        ) = """
+        class A:
+            def method():
+                pass
+        """
+        lines_to_exclude = [3]
+        self.run_and_assert(
+            tmpdir,
+            input_code,
+            expected,
+            lines_to_exclude=lines_to_exclude,
+        )

tests/codemods/test_fix_mutable_params.py

@@ -292,3 +292,18 @@ def foo(self, bar=None):
                 pass
         """
         self.run_and_assert(tmpdir, input_code, expected_output)
+
+    def test_exclude_line(self, tmpdir):
+        input_code = (
+            expected
+        ) = """
+        def foo(one, *args, bar=[]):
+            print(bar)
+        """
+        lines_to_exclude = [2]
+        self.run_and_assert(
+            tmpdir,
+            input_code,
+            expected,
+            lines_to_exclude=lines_to_exclude,
+        )

tests/codemods/test_sonar_fix_missing_self_or_cls.py

@@ -14,20 +14,20 @@ def test_name(self):
     def test_simple(self, tmpdir):
         input_code = """
         class A:
-            def method():
+            def instance_method():
                 pass
             
             @classmethod
-            def clsmethod():
+            def class_method():
                 pass
         """
         expected_output = """
         class A:
-            def method(self):
+            def instance_method(self):
                 pass
             
             @classmethod
-            def clsmethod(cls):
+            def class_method(cls):
                 pass
         """
         issues = {
@@ -37,8 +37,8 @@ def clsmethod(cls):
                     "status": "OPEN",
                     "component": "code.py",
                     "textRange": {
-                        "startLine": 2,
-                        "endLine": 2,
+                        "startLine": 3,
+                        "endLine": 3,
                         "startOffset": 4,
                         "endOffset": 25,
                     },
@@ -48,8 +48,8 @@ def clsmethod(cls):
                     "status": "OPEN",
                     "component": "code.py",
                     "textRange": {
-                        "startLine": 6,
-                        "endLine": 6,
+                        "startLine": 7,
+                        "endLine": 7,
                         "startOffset": 4,
                         "endOffset": 22,
                     },