Use module path instead of simply the id to find references across res/resi.

cristianoc · cristianoc · commit 6cced215bb8b · 2022-12-15T17:08:39.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -43,6 +43,8 @@
 
 - Fix issue with references from implementation files which also happen to have interface files https://github.com/rescript-lang/rescript-vscode/issues/645
 
+- Fix issue where jump to definition would go to the wrong place when there are aliased identifiers in submodules https://github.com/rescript-lang/rescript-vscode/pull/653
+
 ## v1.8.2
 
 #### :rocket: New Feature
diff --git a/analysis/src/Hover.ml b/analysis/src/Hover.ml
@@ -106,7 +106,8 @@ let hoverWithExpandedTypes ~docstring ~file ~package ~supportsMarkdownLinks typ
                Markdown.goToDefinitionText ~env ~pos:loc.Warnings.loc_start
              else ""
            in
-           Markdown.divider ^ (if supportsMarkdownLinks then Markdown.spacing else "")
+           Markdown.divider
+           ^ (if supportsMarkdownLinks then Markdown.spacing else "")
            ^ Markdown.codeBlock
                (decl
                |> Shared.declToString ~printNameAsIs:true
@@ -186,7 +187,7 @@ let newHover ~full:{file; package} ~supportsMarkdownLinks locItem =
       match ResolvePath.resolvePath ~env ~path ~package with
       | None -> None
       | Some (env, name) -> (
-        match References.exportedForTip ~env name tip with
+        match References.exportedForTip ~env ~name tip with
         | None -> None
         | Some stamp -> (
           match Stamps.findModule file.stamps stamp with
@@ -250,4 +251,4 @@ let newHover ~full:{file; package} ~supportsMarkdownLinks locItem =
           let typeString, docstring = t |> fromType ~docstring in
           typeString :: docstring)
     in
-    Some (String.concat "\n\n" parts)
+    Some (String.concat "\n\n" parts)
diff --git a/analysis/src/ProcessCmt.ml b/analysis/src/ProcessCmt.ml
@@ -249,7 +249,17 @@ let rec forSignatureItem ~env ~(exported : Exported.t)
            decl |> forTypeDeclaration ~env ~exported ~recStatus)
   | Tsig_module
       {md_id; md_attributes; md_loc; md_name = name; md_type = {mty_type}} ->
-    let item = forTypeModule env mty_type in
+    let item =
+      let env =
+        {
+          env with
+          modulePath =
+            ExportedModule
+              {name = name.txt; modulePath = env.modulePath; isType = false};
+        }
+      in
+      forTypeModule env mty_type
+    in
     let declared =
       addDeclared ~item ~name ~extent:md_loc ~stamp:(Ident.binding_time md_id)
         ~env md_attributes
diff --git a/analysis/src/References.ml b/analysis/src/References.ml
@@ -145,7 +145,7 @@ let getConstructor (file : File.t) stamp name =
       | Some const -> Some const)
     | _ -> None)
 
-let exportedForTip ~(env : QueryEnv.t) name (tip : Tip.t) =
+let exportedForTip ~(env : QueryEnv.t) ~name (tip : Tip.t) =
   match tip with
   | Value -> Exported.find env.exported Exported.Value name
   | Field _ | Constructor _ | Type ->
@@ -185,7 +185,7 @@ let definedForLoc ~file ~package locKind =
         Log.log ("Cannot resolve path " ^ pathToString path);
         None
       | Some (env, name) -> (
-        match exportedForTip ~env name tip with
+        match exportedForTip ~env ~name tip with
         | None ->
           Log.log
             ("Exported not found for tip " ^ name ^ " > " ^ Tip.toString tip);
@@ -200,23 +200,6 @@ let definedForLoc ~file ~package locKind =
             maybeLog "Yes!! got it";
             Some res))))
 
-let declaredForExportedTip ~(stamps : Stamps.t) ~(exported : Exported.t) name
-    (tip : Tip.t) =
-  let bind f x = Option.bind x f in
-  match tip with
-  | Value ->
-    Exported.find exported Exported.Value name
-    |> bind (fun stamp -> Stamps.findValue stamps stamp)
-    |> Option.map (fun x -> {x with Declared.item = ()})
-  | Field _ | Constructor _ | Type ->
-    Exported.find exported Exported.Type name
-    |> bind (fun stamp -> Stamps.findType stamps stamp)
-    |> Option.map (fun x -> {x with Declared.item = ()})
-  | Module ->
-    Exported.find exported Exported.Module name
-    |> bind (fun stamp -> Stamps.findModule stamps stamp)
-    |> Option.map (fun x -> {x with Declared.item = ()})
-
 (** Find alternative declaration: from res in case of interface, or from resi in case of implementation  *)
 let alternateDeclared ~(file : File.t) ~package (declared : _ Declared.t) tip =
   match Hashtbl.find_opt package.pathsForModule file.moduleName with
@@ -230,10 +213,18 @@ let alternateDeclared ~(file : File.t) ~package (declared : _ Declared.t) tip =
       match Cmt.fullFromUri ~uri:(Uri.fromPath alternateUri) with
       | None -> None
       | Some {file; extra} -> (
-        match
-          declaredForExportedTip ~stamps:file.stamps
-            ~exported:file.structure.exported declared.name.txt tip
-        with
+        let env = QueryEnv.fromFile file in
+        let path = ModulePath.toPath declared.modulePath declared.name.txt in
+        maybeLog ("find declared for path " ^ pathToString path);
+        let declaredOpt =
+          match ResolvePath.resolvePath ~env ~path ~package with
+          | None -> None
+          | Some (env, name) -> (
+            match exportedForTip ~env ~name tip with
+            | None -> None
+            | Some stamp -> declaredForTip ~stamps:file.stamps stamp tip)
+        in
+        match declaredOpt with
         | None -> None
         | Some declared -> Some (file, extra, declared)))
     | _ ->
@@ -386,7 +377,7 @@ let definitionForLocItem ~full:{file; package} locItem =
       | None -> None
       | Some (env, name) -> (
         maybeLog ("resolved path:" ^ name);
-        match exportedForTip ~env name tip with
+        match exportedForTip ~env ~name tip with
         | None -> None
         | Some stamp ->
           (* oooh wht do I do if the stamp is inside a pseudo-file? *)
@@ -425,7 +416,7 @@ let typeDefinitionForLocItem ~full:{file; package} locItem =
 let isVisible (declared : _ Declared.t) =
   declared.isExported
   &&
-  let rec loop v =
+  let rec loop (v : ModulePath.t) =
     match v with
     | File _ -> true
     | NotVisible -> false
@@ -434,17 +425,6 @@ let isVisible (declared : _ Declared.t) =
   in
   loop declared.modulePath
 
-let rec pathFromVisibility visibilityPath current =
-  match visibilityPath with
-  | File _ -> Some current
-  | IncludedModule (_, inner) -> pathFromVisibility inner current
-  | ExportedModule {name; modulePath = inner} ->
-    pathFromVisibility inner (name :: current)
-  | NotVisible -> None
-
-let pathFromVisibility visibilityPath tipName =
-  pathFromVisibility visibilityPath [tipName]
-
 type references = {
   uri: Uri.t;
   locOpt: Location.t option; (* None: reference to a toplevel module *)
@@ -497,35 +477,35 @@ let forLocalStamp ~full:{file; extra; package} stamp (tip : Tip.t) =
               (* if this file has a corresponding interface or implementation file
                  also find the references in that file *)
             in
-            match pathFromVisibility declared.modulePath declared.name.txt with
-            | None -> []
-            | Some path ->
-              maybeLog ("Now checking path " ^ pathToString path);
-              let thisModuleName = file.moduleName in
-              let externals =
-                package.projectFiles |> FileSet.elements
-                |> List.filter (fun name -> name <> file.moduleName)
-                |> List.map (fun moduleName ->
-                       Cmt.fullsFromModule ~package ~moduleName
-                       |> List.map (fun {file; extra} ->
-                              match
-                                Hashtbl.find_opt extra.externalReferences
-                                  thisModuleName
-                              with
-                              | None -> []
-                              | Some refs ->
-                                let locs =
-                                  refs
-                                  |> Utils.filterMap (fun (p, t, locs) ->
-                                         if p = path && t = tip then Some locs
-                                         else None)
-                                in
-                                locs
-                                |> List.map (fun loc ->
-                                       {uri = file.uri; locOpt = Some loc})))
-                |> List.concat |> List.concat
-              in
-              alternativeReferences @ externals)
+            let path =
+              ModulePath.toPath declared.modulePath declared.name.txt
+            in
+            maybeLog ("Now checking path " ^ pathToString path);
+            let thisModuleName = file.moduleName in
+            let externals =
+              package.projectFiles |> FileSet.elements
+              |> List.filter (fun name -> name <> file.moduleName)
+              |> List.map (fun moduleName ->
+                     Cmt.fullsFromModule ~package ~moduleName
+                     |> List.map (fun {file; extra} ->
+                            match
+                              Hashtbl.find_opt extra.externalReferences
+                                thisModuleName
+                            with
+                            | None -> []
+                            | Some refs ->
+                              let locs =
+                                refs
+                                |> Utils.filterMap (fun (p, t, locs) ->
+                                       if p = path && t = tip then Some locs
+                                       else None)
+                              in
+                              locs
+                              |> List.map (fun loc ->
+                                     {uri = file.uri; locOpt = Some loc})))
+              |> List.concat |> List.concat
+            in
+            alternativeReferences @ externals)
           else (
             maybeLog "Not visible";
             [])
@@ -580,7 +560,7 @@ let allReferencesForLocItem ~full:({file; package} as full) locItem =
       match ResolvePath.resolvePath ~env ~path ~package with
       | None -> []
       | Some (env, name) -> (
-        match exportedForTip ~env name tip with
+        match exportedForTip ~env ~name tip with
         | None -> []
         | Some stamp -> (
           match Cmt.fullFromUri ~uri:env.file.uri with
diff --git a/analysis/src/ResolvePath.ml b/analysis/src/ResolvePath.ml
@@ -133,7 +133,7 @@ let resolveFromCompilerPath ~env ~package path =
   | NotFound -> NotFound
   | Exported (env, name) -> Exported (env, name)
 
-let rec getSourceUri ~(env : QueryEnv.t) ~package path =
+let rec getSourceUri ~(env : QueryEnv.t) ~package (path : ModulePath.t) =
   match path with
   | File (uri, _moduleName) -> uri
   | NotVisible -> env.file.uri
diff --git a/analysis/src/SharedTypes.ml b/analysis/src/SharedTypes.ml
@@ -2,11 +2,27 @@ let str s = if s = "" then "\"\"" else s
 let list l = "[" ^ (l |> List.map str |> String.concat ", ") ^ "]"
 let ident l = l |> List.map str |> String.concat "."
 
-type modulePath =
-  | File of Uri.t * string
-  | NotVisible
-  | IncludedModule of Path.t * modulePath
-  | ExportedModule of {name: string; modulePath: modulePath; isType: bool}
+type path = string list
+
+let pathToString (path : path) = path |> String.concat "."
+
+module ModulePath = struct
+  type t =
+    | File of Uri.t * string
+    | NotVisible
+    | IncludedModule of Path.t * t
+    | ExportedModule of {name: string; modulePath: t; isType: bool}
+
+  let toPath modulePath tipName : path =
+    let rec loop modulePath current =
+      match modulePath with
+      | File _ -> current
+      | IncludedModule (_, inner) -> loop inner current
+      | ExportedModule {name; modulePath = inner} -> loop inner (name :: current)
+      | NotVisible -> current
+    in
+    loop modulePath [tipName]
+end
 
 type field = {stamp: int; fname: string Location.loc; typ: Types.type_expr}
 
@@ -102,7 +118,7 @@ module Declared = struct
     name: string Location.loc;
     extentLoc: Location.t;
     stamp: int;
-    modulePath: modulePath;
+    modulePath: ModulePath.t;
     isExported: bool;
     deprecated: string option;
     docstring: string list;
@@ -257,7 +273,7 @@ module Completion = struct
 end
 
 module Env = struct
-  type t = {stamps: Stamps.t; modulePath: modulePath}
+  type t = {stamps: Stamps.t; modulePath: ModulePath.t}
 end
 
 type filePath = string
@@ -321,10 +337,6 @@ module Tip = struct
     | Module -> "Module"
 end
 
-type path = string list
-
-let pathToString (path : path) = path |> String.concat "."
-
 let rec pathIdentToString (p : Path.t) =
   match p with
   | Pident {name} -> name
diff --git a/analysis/tests/src/DefinitionWithInterface.res b/analysis/tests/src/DefinitionWithInterface.res
@@ -8,4 +8,5 @@ let _ = aabbcc
 
 module Inner = {
   let y = 100
+  //  ^def
 }
diff --git a/analysis/tests/src/DefinitionWithInterface.resi b/analysis/tests/src/DefinitionWithInterface.resi
@@ -5,4 +5,5 @@ type t
 
 module Inner: {
   let y: int
+  //  ^def
 }
diff --git a/analysis/tests/src/expected/Cross.res.txt b/analysis/tests/src/expected/Cross.res.txt
@@ -100,5 +100,5 @@ Completable: Cpath Value[DefinitionWithInterface, a]
 []
 
 Definition src/Cross.res 39:39
-{"uri": "DefinitionWithInterface.res", "range": {"start": {"line": 0, "character": 4}, "end": {"line": 0, "character": 5}}}
+{"uri": "DefinitionWithInterface.res", "range": {"start": {"line": 9, "character": 6}, "end": {"line": 9, "character": 7}}}
 
diff --git a/analysis/tests/src/expected/DefinitionWithInterface.res.txt b/analysis/tests/src/expected/DefinitionWithInterface.res.txt
@@ -1,3 +1,6 @@
 Definition src/DefinitionWithInterface.res 0:4
 {"uri": "DefinitionWithInterface.resi", "range": {"start": {"line": 0, "character": 4}, "end": {"line": 0, "character": 5}}}
 
+Definition src/DefinitionWithInterface.res 9:6
+{"uri": "DefinitionWithInterface.resi", "range": {"start": {"line": 6, "character": 2}, "end": {"line": 6, "character": 12}}}
+
diff --git a/analysis/tests/src/expected/DefinitionWithInterface.resi.txt b/analysis/tests/src/expected/DefinitionWithInterface.resi.txt
@@ -1,3 +1,6 @@
 Definition src/DefinitionWithInterface.resi 0:4
 {"uri": "DefinitionWithInterface.res", "range": {"start": {"line": 0, "character": 4}, "end": {"line": 0, "character": 5}}}
 
+Definition src/DefinitionWithInterface.resi 6:6
+{"uri": "DefinitionWithInterface.res", "range": {"start": {"line": 9, "character": 6}, "end": {"line": 9, "character": 7}}}
+

Original file line number	Diff line number	Diff line change
`@@ -8,4 +8,5 @@ let _ = aabbcc`
`8`	`8`
`9`	`9`	`module Inner = {`
`10`	`10`	`let y = 100`
	`11`	`+ // ^def`
`11`	`12`	`}`
Original file line number	Diff line number	Diff line change
`@@ -5,4 +5,5 @@ type t`
`5`	`5`
`6`	`6`	`module Inner: {`
`7`	`7`	`let y: int`
	`8`	`+ // ^def`
`8`	`9`	`}`