From 4067378105878c80d12ffb799f3a44f25dc481ff Mon Sep 17 00:00:00 2001
From: Ryan Vandersmith <ryanvandersmith@gmail.com>
Date: Fri, 3 Jun 2022 15:28:34 -0600
Subject: [PATCH] Improve support for navigating to definitions in external
 modules (#3263)

I rewrote most of `definition_handler` to account for edge cases related to finding definitions in external files.

- It's now possible to jump to the definition of an import identifier (rather than having to click on a qualified member name). For example, `import M|ap "HashMap"` (where `|` is the cursor) jumps to the top of `HashMap.mo` instead of doing nothing (fixes dfinity/vscode-motoko#17).

- The new implementation also has a more permissive default behavior for missing declarations. In cases where the module path is known but the qualified symbol is unknown, the language server attempts to direct the user to the relevant module. This is mostly intended as a fallback for situations where the index is missing an expected declaration, such as when the other file has a syntax error.

Possible future improvements:

- Using the start/end position of the `module` or equivalent keyword (when possible) rather than the top of the file
- Providing the same functionality when the cursor is on the import string literal (and perhaps even the `import` keyword)
- Supporting object pattern-style imports as mentioned in #3078 (currently experimenting with this)

Passes all tests on my WSL setup and works as expected in VSCode with the [official Motoko extension](https://github.com/dfinity/vscode-motoko).

(@matthewhammer)
---
 src/languageServer/definition.ml | 45 +++++++++++++++++++++++++-------
 test/lsp-int/Main.hs             |  7 +++++
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/src/languageServer/definition.ml b/src/languageServer/definition.ml
index 23239ccc1da..9529571a87a 100644
--- a/src/languageServer/definition.ml
+++ b/src/languageServer/definition.ml
@@ -36,17 +36,42 @@ let definition_handler index position file_contents project_root file_path =
       Source_file.identifier_at_pos project_root file_path file_contents
         position
     in
-    let* path, region =
+    (* Find the relevant path for the declaration *)
+    let* path =
       match ident with
-      | Alias _ -> None
+      | Alias (_, path) -> Some path
       | Unresolved _ -> None
-      | Resolved resolved ->
-          let* module_ = DI.lookup_module project_root resolved.path index in
-          find_named resolved.ident module_
-      | Ident ident ->
-          let* uri = Lib.FilePath.relative_to project_root file_path in
-          let* module_ = DI.lookup_module project_root uri index in
-          find_named ident module_
+      | Resolved { path; _ } -> Some path
+      | Ident _ -> Lib.FilePath.relative_to project_root file_path
+    in
+    let* module_ = DI.lookup_module project_root path index in
+    (* Update `path` from the relevant module *)
+    let path, _ = module_ in
+    (* Try to find the start/end cursor range in the relevant module *)
+    let* decl_range =
+      match
+        let* decl_ident =
+          match ident with
+          | Alias _ -> None
+          | Unresolved { ident; _ } -> Some ident (* Currently unreachable *)
+          | Resolved { ident; _ } -> Some ident
+          | Ident ident -> Some ident
+        in
+        (* Note: ignoring `path` output value from `find_named` *)
+        let* _, region = find_named decl_ident module_ in
+        Some (range_of_region region)
+      with
+      | Some range -> Some range
+      | None -> (
+          match ident with
+          | Ident _ -> None (* Unresolved identifiers in the same file *)
+          | _ ->
+              (* By default, point to the top of the relevant file *)
+              (* TODO: use the module declaration start/end positions when possible *)
+              let file_start =
+                Lsp.{ position_line = 0; position_character = 0 }
+              in
+              Some Lsp.{ range_start = file_start; range_end_ = file_start })
     in
     Some
       Lsp.
@@ -55,7 +80,7 @@ let definition_handler index position file_contents project_root file_path =
             (if Source_file.is_non_file_path path then
              Option.get (Source_file.uri_for_package path)
             else Vfs.uri_from_file path);
-          location_range = range_of_region region;
+          location_range = decl_range;
         }
   in
   `TextDocumentDefinitionResponse (Option.to_list location)
diff --git a/test/lsp-int/Main.hs b/test/lsp-int/Main.hs
index e25d3a8827e..240cf8e4970 100644
--- a/test/lsp-int/Main.hs
+++ b/test/lsp-int/Main.hs
@@ -186,6 +186,13 @@ main = do
             (Position 5 31)
             [("mydependency/lib.mo", Range (Position 5 17) (Position 5 24))]
 
+          log "Definition for an imported module alias"
+          definitionsTestCase
+            project
+            doc
+            (Position 1 10)
+            [("lib/list.mo", Range (Position 0 0) (Position 0 0))]
+
         log "Completion tests"
         log "Completing top level definitions"
         withDoc "ListClient.mo" \doc -> do