Improves renaming functionality

gnikit · gnikit · commit 8b01495a90a7 · 2022-03-23T18:03:19.000Z
- Added unittests for renaming - Properly implemented solution for hansec/fortran-language-server#104 Now it does not add to the source code. - Improves coverage of renaming and all_references - Added warnings for edge cases i.e. renaming intrinsics
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,13 @@
 # CHANGELONG
 
+## 2.2.8
+
+### Changed
+
+- Changed how renaming of implicitly named type-bound procedures and their
+  implementations is handled. Unittest was added.
+- Rewrote the Fortran parser to be clearer and more modular
+
 ## 2.2.7
 
 ### Changed
diff --git a/fortls/langserver.py b/fortls/langserver.py
@@ -41,6 +41,7 @@
     set_keyword_ordering,
 )
 from fortls.intrinsics import (
+    fortran_intrinsic_obj,
     get_intrinsic_keywords,
     load_intrinsics,
     set_lowercase_intrinsics,
@@ -905,6 +906,8 @@ def get_all_references(
             file_set = self.workspace.items()
         else:
             file_set = ((file_obj.path, file_obj),)
+        # A container that includes all the FQSN signatures for objects that
+        # are linked to the rename request and that should also be replaced
         override_cache: list[str] = []
         refs = {}
         ref_objs = []
@@ -923,26 +926,52 @@ def get_all_references(
                     if var_def is None:
                         continue
                     ref_match = False
-                    if def_fqsn == var_def.FQSN or var_def.FQSN in override_cache:
-                        ref_match = True
-                    elif var_def.parent and var_def.parent.get_type() == CLASS_TYPE_ID:
-                        if type_mem:
-                            for inherit_def in var_def.parent.get_overridden(def_name):
-                                if def_fqsn == inherit_def.FQSN:
-                                    ref_match = True
-                                    override_cache.append(var_def.FQSN)
-                                    break
-                        if (
-                            (var_def.sline - 1 == i)
-                            and (var_def.file_ast.path == filename)
-                            and (line.count("=>") == 0)
+                    try:
+                        # NOTE: throws AttributeError if object is intrinsic since
+                        # it will not have a FQSN
+                        # BUG: intrinsic objects should be excluded, but get_definition
+                        # does not recognise the arguments
+                        if def_fqsn == var_def.FQSN or var_def.FQSN in override_cache:
+                            ref_match = True
+                        # NOTE: throws AttributeError if object is None
+                        elif var_def.parent.get_type() == CLASS_TYPE_ID:
+                            if type_mem:
+                                for inherit_def in var_def.parent.get_overridden(
+                                    def_name
+                                ):
+                                    if def_fqsn == inherit_def.FQSN:
+                                        ref_match = True
+                                        override_cache.append(var_def.FQSN)
+                                        break
+
+                            # Standalone definition of a type-bound procedure,
+                            # no pointer replace all its instances in the current scope
+                            # NOTE: throws AttributeError if object has no link_obj
+                            if (
+                                var_def.sline - 1 == i
+                                and var_def.file_ast.path == filename
+                                and line.count("=>") == 0
+                                and var_def.link_obj is def_obj
+                            ):
+                                ref_objs.append(var_def)
+                                override_cache.append(var_def.FQSN)
+                                ref_match = True
+
+                        # Object is a Method and the linked object i.e. the
+                        # implementation
+                        # shares the same parent signature as the current variable
+                        # NOTE:: throws and AttributeError if the link_object or
+                        # parent are not present OR they are set to None
+                        # hence not having a FQSN
+                        elif (
+                            def_obj.get_type(True) == METH_TYPE_ID
+                            and def_obj.link_obj.parent.FQSN == var_def.parent.FQSN
                         ):
-                            try:
-                                if var_def.link_obj is def_obj:
-                                    ref_objs.append(var_def)
-                                    ref_match = True
-                            except:
-                                pass
+                            ref_match = True
+                            override_cache.append(var_def.FQSN)
+                    except AttributeError:
+                        ref_match = False
+
                     if ref_match:
                         file_refs.append([i, match.start(1), match.end(1)])
             if len(file_refs) > 0:
@@ -1114,6 +1143,9 @@ def serve_rename(self, request: dict):
         def_obj = self.get_definition(file_obj, def_line, def_char)
         if def_obj is None:
             return None
+        if isinstance(def_obj, fortran_intrinsic_obj):
+            self.post_message("Rename failed: Cannot rename intrinsics", Severity.warn)
+            return None
         # Determine global accesibility and type membership
         restrict_file = None
         type_mem = False
@@ -1132,36 +1164,14 @@ def serve_rename(self, request: dict):
             return None
         # Create rename changes
         new_name = params["newName"]
-        changes = {}
-        for (filename, file_refs) in all_refs.items():
+        changes: dict[str, list[dict]] = {}
+        for filename, file_refs in all_refs.items():
             file_uri = path_to_uri(filename)
             changes[file_uri] = []
             for ref in file_refs:
                 changes[file_uri].append(
                     change_json(new_name, ref[0], ref[1], ref[0], ref[2])
                 )
-        # Check for implicit procedure implementation naming
-        bind_obj = None
-        if def_obj.get_type(no_link=True) == METH_TYPE_ID:
-            _, curr_line, post_lines = def_obj.file_ast.file.get_code_line(
-                def_obj.sline - 1, backward=False, strip_comment=True
-            )
-            if curr_line is not None:
-                full_line = curr_line + "".join(post_lines)
-                if full_line.find("=>") < 0:
-                    bind_obj = def_obj
-                    bind_change = f"{new_name} => {def_obj.name}"
-        elif (len(ref_objs) > 0) and (
-            ref_objs[0].get_type(no_link=True) == METH_TYPE_ID
-        ):
-            bind_obj = ref_objs[0]
-            bind_change = f"{ref_objs[0].name} => {new_name}"
-        # Replace definition statement with explicit implementation naming
-        if bind_obj is not None:
-            def_uri = path_to_uri(bind_obj.file_ast.file.path)
-            for change in changes[def_uri]:
-                if change["range"]["start"]["line"] == bind_obj.sline - 1:
-                    change["newText"] = bind_change
         return {"changes": changes}
 
     def serve_codeActions(self, request: dict):
diff --git a/test/setup_tests.py b/test/setup_tests.py
@@ -18,6 +18,11 @@
 test_dir = root_dir / "test" / "test_source"
 
 
+def check_post_msg(result: dict, msg: str, severity: int):
+    assert result["type"] == severity
+    assert result["message"] == msg
+
+
 def run_request(request, fortls_args: list[str] = None):
     command = [
         sys.executable,
diff --git a/test/test_server_rename.py b/test/test_server_rename.py
@@ -0,0 +1,201 @@
+from setup_tests import (
+    check_post_msg,
+    path_to_uri,
+    run_request,
+    test_dir,
+    write_rpc_request,
+)
+
+
+def rename_request(new_name: str, file_path, ln: int, ch: int):
+    return write_rpc_request(
+        1,
+        "textDocument/rename",
+        {
+            "newName": new_name,
+            "textDocument": {"uri": str(file_path)},
+            "position": {"line": ln, "character": ch},
+        },
+    )
+
+
+def check_rename_response(response: dict, references: dict):
+    # Loop over URI's if the change spans multiple files there will be more than 1
+    for uri, changes in response.items():
+        refs = references[uri]
+        # Loop over all the changes in the current URI, instances of object
+        for c, r in zip(changes, refs):
+            assert c["range"] == r["range"]
+            assert c["newText"] == r["newText"]
+
+
+def create(new_text: str, sln: int, sch: int, eln: int, ech: int):
+    return {
+        "range": {
+            "start": {"line": sln, "character": sch},
+            "end": {"line": eln, "character": ech},
+        },
+        "newText": new_text,
+    }
+
+
+def test_rename_var():
+    """ "Test simple variable rename"""
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir)})
+    file_path = test_dir / "test_prog.f08"
+    string += rename_request("str_rename", file_path, 5, 25)
+    errcode, results = run_request(string)
+    assert errcode == 0
+    ref = {}
+    ref[path_to_uri(str(file_path))] = [create("str_rename", 5, 20, 5, 29)]
+    check_rename_response(results[1]["changes"], ref)
+
+
+def test_rename_var_across_module():
+    """Test renaming objects like variables across modules works"""
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir)})
+    file_path = test_dir / "test_prog.f08"
+    string += rename_request("new_module_var", file_path, 26, 15)
+    errcode, results = run_request(string)
+    assert errcode == 0
+    ref = {}
+    ref[path_to_uri(str(test_dir / "subdir" / "test_free.f90"))] = [
+        create("new_module_var", 32, 11, 32, 26)
+    ]
+    ref[path_to_uri(str(file_path))] = [create("new_module_var", 2, 44, 2, 59)]
+    ref[path_to_uri(str(file_path))].append(create("new_module_var", 26, 8, 26, 23))
+
+    check_rename_response(results[1]["changes"], ref)
+
+
+def test_rename_empty():
+    """Test that renaming nothing will not error"""
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir / "rename")})
+    file_path = test_dir / "rename" / "test_rename_imp_type_bound_proc.f90"
+    string += rename_request("bar", file_path, 9, 0)
+    errcode, results = run_request(string, ["-n", "1"])
+    assert errcode == 0
+    assert results[1] is None
+
+
+def test_rename_member_type_ptr():
+    """Test that renaming type bound pointers of procedure methods rename
+    only the pointer and not the implementation, even if the pointer and the
+    implementation share the same name
+    """
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir)})
+    file_path = test_dir / "test_prog.f08"
+    string += rename_request("bp_rename", file_path, 18, 25)
+    errcode, results = run_request(string)
+    assert errcode == 0
+    ref = {}
+    ref[path_to_uri(str(file_path))] = [create("bp_rename", 18, 16, 18, 26)]
+    ref[path_to_uri(str(test_dir / "subdir" / "test_free.f90"))] = [
+        create("bp_rename", 15, 27, 15, 37)
+    ]
+    check_rename_response(results[1]["changes"], ref)
+
+
+def test_rename_member_type_ptr_null():
+    """Test renaming type bound pointers of procedure methods works when pointing to
+    null
+    """
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir)})
+    file_path = test_dir / "test_prog.f08"
+    string += rename_request("bp_rename", file_path, 17, 25)
+    errcode, results = run_request(string)
+    assert errcode == 0
+    ref = {}
+    ref[path_to_uri(str(file_path))] = [create("bp_rename", 17, 16, 17, 28)]
+    ref[path_to_uri(str(test_dir / "subdir" / "test_free.f90"))] = [
+        create("bp_rename", 11, 43, 11, 55)
+    ]
+    check_rename_response(results[1]["changes"], ref)
+
+
+def test_rename_type_bound_proc_no_ptr():
+    """Test renaming type bound pointers of procedure methods works when no pointer
+    is setup. Requesting to rename the procedure should rename, the implementation
+    and the Method itself i.e. call self%foo()
+
+    Requesting to rename the implementation should also rename the procedure
+    and all the locations it is called in
+    """
+    root = test_dir / "rename"
+    string = write_rpc_request(1, "initialize", {"rootPath": str(root)})
+    file_path = root / "test_rename_imp_type_bound_proc.f90"
+    # Rename the procedure name and check if implementation also renames
+    string += rename_request("bar", file_path, 5, 23)
+    # Rename the implementation name and check if declaration, references change
+    string += rename_request("bar", file_path, 10, 18)
+    errcode, results = run_request(string)
+    assert errcode == 0
+    ref = {}
+    ref[path_to_uri(str(file_path))] = [create("bar", 5, 21, 5, 24)]
+    ref[path_to_uri(str(file_path))].append(create("bar", 10, 15, 10, 18))
+    ref[path_to_uri(str(file_path))].append(create("bar", 12, 18, 12, 21))
+    ref[path_to_uri(str(file_path))].append(create("bar", 13, 19, 13, 22))
+    check_rename_response(results[1]["changes"], ref)
+    check_rename_response(results[2]["changes"], ref)
+
+
+def test_rename_non_existent_file():
+    """Test renaming type bound pointers of procedure methods works when pointing to
+    null
+    """
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir)})
+    file_path = test_dir / "fake.f90"
+    string += rename_request("bar", file_path, 5, 23)
+    errcode, results = run_request(string)
+    assert errcode == 0
+    assert results[1] is None
+
+
+def test_rename_nested():
+    """Test renaming heavily nested constructs works"""
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir / "rename")})
+    file_path = test_dir / "rename" / "test_rename_nested.f90"
+    string += rename_request("bar", file_path, 6, 23)
+    errcode, results = run_request(string, ["-n", "1"])
+    assert errcode == 0
+    ref = {}
+    ref[path_to_uri(str(file_path))] = [create("bar", 6, 23, 6, 26)]
+    ref[path_to_uri(str(file_path))].append(create("bar", 9, 27, 9, 30))
+    check_rename_response(results[1]["changes"], ref)
+
+
+def test_rename_intrinsic():
+    """Test renaming an intrinsic function, while no other function exists
+    with the same name, will throw an error
+    """
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir / "rename")})
+    file_path = test_dir / "rename" / "test_rename_nested.f90"
+    string += rename_request("bar", file_path, 8, 27)
+    errcode, results = run_request(string, ["-n", "1"])
+    assert errcode == 0
+    check_post_msg(results[1], "Rename failed: Cannot rename intrinsics", 2)
+    assert results[2] is None
+
+
+def test_rename_use_only_rename():
+    """Test renaming constructs of `use mod, only: val => root_val
+    are handled correctly
+    """
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir / "subdir")})
+    file_path = test_dir / "subdir" / "test_rename.F90"
+    string += rename_request("bar", file_path, 13, 5)
+    errcode, results = run_request(string, ["-n", "1"])
+    # FIXME: to be implemented
+    assert errcode == 0
+
+
+def test_rename_skip_intrinsic():
+    """Test that renaming functions named the same as intrinsic functions e.g. size()
+    will only rename the user defined functions
+    """
+    string = write_rpc_request(1, "initialize", {"rootPath": str(test_dir / "rename")})
+    file_path = test_dir / "rename" / "test_rename_intrinsic.f90"
+    string += rename_request("bar", file_path, 22, 13)
+    errcode, results = run_request(string, ["-n", "1"])
+    # FIXME: to be implemented
+    assert errcode == 0
diff --git a/test/test_source/.fortls b/test/test_source/.fortls
@@ -5,7 +5,8 @@
   "excl_paths": [
     "excldir/**",
     "./diag/",
-    "docs"
+    "docs",
+    "rename"
   ]
 
 }
diff --git a/test/test_source/rename/test_rename_imp_type_bound_proc.f90 b/test/test_source/rename/test_rename_imp_type_bound_proc.f90
@@ -0,0 +1,15 @@
+module mod
+    implicit none
+
+    type :: t
+    contains
+        procedure :: foo
+    end type t
+
+contains
+
+    subroutine foo(self)
+        class(t), intent(in) :: self
+        call self%foo()
+    end subroutine foo
+end module mod
diff --git a/test/test_source/rename/test_rename_intrinsic.f90 b/test/test_source/rename/test_rename_intrinsic.f90
diff --git a/test/test_source/rename/test_rename_nested.f90 b/test/test_source/rename/test_rename_nested.f90
diff --git a/test/test_source/test_prog.f08 b/test/test_source/test_prog.f08

Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,8 @@`
`5`	`5`	`"excl_paths": [`
`6`	`6`	`"excldir/**",`
`7`	`7`	`"./diag/",`
`8`		`- "docs"`
	`8`	`+ "docs",`
	`9`	`+ "rename"`
`9`	`10`	`]`
`10`	`11`
`11`	`12`	`}`