analysis: fix namespaced reference lookup

analysis/src/Cmt.ml

@@ -8,6 +8,30 @@ let fullForCmt ~moduleName ~package ~uri cmt =
     let extra = ProcessExtra.getExtra ~file ~infos in
     Some {file; extra; package}
 
+let fullForIncrementalCmt ~package ~moduleName ~uri =
+  if !Cfg.inIncrementalTypecheckingMode then
+    let path = Uri.toPath uri in
+    let incrementalCmtPath =
+      package.rootPath ^ "/lib/bs/___incremental" ^ "/" ^ moduleName
+      ^
+      match Files.classifySourceFile path with
+      | Resi -> ".cmti"
+      | _ -> ".cmt"
+    in
+    match fullForCmt ~moduleName ~package ~uri incrementalCmtPath with
+    | Some cmtInfo ->
+      if Debug.verbose () then Printf.printf "[cmt] Found incremental cmt\n";
+      Some cmtInfo
+    | None -> None
+  else None
+
+let fullFromModuleUri ~package ~moduleName ~uri ~paths =
+  match fullForIncrementalCmt ~package ~moduleName ~uri with
+  | Some cmtInfo -> Some cmtInfo
+  | None ->
+    let cmt = getCmtPath ~uri paths in
+    fullForCmt ~moduleName ~package ~uri cmt
+
 let fullFromUri ~uri =
   let path = Uri.toPath uri in
   match Packages.getPackage ~uri with
@@ -16,22 +40,8 @@ let fullFromUri ~uri =
     let moduleName =
       BuildSystem.namespacedName package.namespace (FindFiles.getName path)
     in
-    let incremental =
-      if !Cfg.inIncrementalTypecheckingMode then
-        let incrementalCmtPath =
-          package.rootPath ^ "/lib/bs/___incremental" ^ "/" ^ moduleName
-          ^
-          match Files.classifySourceFile path with
-          | Resi -> ".cmti"
-          | _ -> ".cmt"
-        in
-        fullForCmt ~moduleName ~package ~uri incrementalCmtPath
-      else None
-    in
-    match incremental with
-    | Some cmtInfo ->
-      if Debug.verbose () then Printf.printf "[cmt] Found incremental cmt\n";
-      Some cmtInfo
+    match fullForIncrementalCmt ~package ~moduleName ~uri with
+    | Some cmtInfo -> Some cmtInfo
     | None -> (
       match Hashtbl.find_opt package.pathsForModule moduleName with
       | Some paths ->
@@ -41,12 +51,20 @@ let fullFromUri ~uri =
         prerr_endline ("can't find module " ^ moduleName);
         None))
 
+let fullFromModule ~package ~moduleName =
+  Option.bind (Hashtbl.find_opt package.pathsForModule moduleName)
+  @@ fun paths ->
+  let uri = getUri paths in
+  fullFromModuleUri ~package ~moduleName ~uri ~paths
+
 let fullsFromModule ~package ~moduleName =
-  if Hashtbl.mem package.pathsForModule moduleName then
-    let paths = Hashtbl.find package.pathsForModule moduleName in
+  match Hashtbl.find_opt package.pathsForModule moduleName with
+  | None -> []
+  | Some paths ->
     let uris = getUris paths in
-    uris |> List.filter_map (fun uri -> fullFromUri ~uri)
-  else []
+    uris
+    |> List.filter_map (fun uri ->
+           fullFromModuleUri ~package ~moduleName ~uri ~paths)
 
 let loadFullCmtFromPath ~path =
   let uri = Uri.fromPath path in

analysis/src/References.ml

@@ -18,6 +18,20 @@ let locItemsForPos ~extra pos =
 
 let lineColToCmtLoc ~pos:(line, col) = (line + 1, col)
 
+(** External references in namespaced projects are indexed by the public
+  * namespace path, e.g. MyNamespace.MyModule1.myFunc1, while definitions live
+  * in hidden compiled modules like MyModule1-MyNamespace.
+  * We return the lookup key pair used by the external reference index.
+  *)
+let normalizeExternalReferenceKey ~namespace ~moduleName ~path =
+  match namespace with
+  | Some namespace when Utils.endsWith moduleName ("-" ^ namespace) ->
+    let suffixLen = String.length namespace + 1 in
+    let sourceModuleLen = String.length moduleName - suffixLen in
+    let sourceModule = String.sub moduleName 0 sourceModuleLen in
+    (namespace, sourceModule :: path)
+  | _ -> (moduleName, path)
+
 let getLocItem ~full ~pos ~debug =
   let log n msg = if debug then Printf.printf "getLocItem #%d: %s\n" n msg in
   let pos = lineColToCmtLoc ~pos in
@@ -485,6 +499,10 @@ let forLocalStamp ~full:{file; extra; package} stamp (tip : Tip.t) =
             in
             maybeLog ("Now checking path " ^ pathToString path);
             let thisModuleName = file.moduleName in
+            let normalizedModuleName, normalizedPath =
+              normalizeExternalReferenceKey ~namespace:package.namespace
+                ~moduleName:thisModuleName ~path
+            in
             let externals =
               package.projectFiles |> FileSet.elements
               |> List.filter (fun name -> name <> file.moduleName)
@@ -493,14 +511,15 @@ let forLocalStamp ~full:{file; extra; package} stamp (tip : Tip.t) =
                      |> List.map (fun {file; extra} ->
                             match
                               Hashtbl.find_opt extra.externalReferences
-                                thisModuleName
+                                normalizedModuleName
                             with
                             | None -> []
                             | Some refs ->
                               let locs =
                                 refs
                                 |> Utils.filterMap (fun (p, t, locs) ->
-                                       if p = path && t = tip then Some locs
+                                       if p = normalizedPath && t = tip then
+                                         Some locs
                                        else None)
                               in
                               locs
@@ -522,10 +541,8 @@ let allReferencesForLocItem ~full:({file; package} as full) locItem =
   | TopLevelModule moduleName ->
     let otherModulesReferences =
       package.projectFiles |> FileSet.elements
-      |> Utils.filterMap (fun name ->
-             match ProcessCmt.fileForModule ~package name with
-             | None -> None
-             | Some file -> Cmt.fullFromUri ~uri:file.uri)
+      |> Utils.filterMap (fun moduleName ->
+             Cmt.fullFromModule ~package ~moduleName)
       |> List.map (fun full ->
              match Hashtbl.find_opt full.extra.fileReferences moduleName with
              | None -> []
@@ -563,7 +580,7 @@ let allReferencesForLocItem ~full:({file; package} as full) locItem =
       match exportedForTip ~env ~path ~package ~tip with
       | None -> []
       | Some (env, _name, stamp) -> (
-        match Cmt.fullFromUri ~uri:env.file.uri with
+        match Cmt.fullFromModule ~package ~moduleName:env.file.moduleName with
         | None -> []
         | Some full ->
           maybeLog

tests/analysis_tests/Makefile

@@ -4,6 +4,7 @@ test-analysis-binary:
 	make -C tests test
 	make -C tests-generic-jsx-transform test
 	make -C tests-incremental-typechecking test
+	make -C tests-namespaced-references test
 	make -C tests-sourcedirs-dependency test
 
 test-reanalyze:
@@ -15,6 +16,7 @@ clean:
 	make -C tests clean
 	make -C tests-generic-jsx-transform clean
 	make -C tests-incremental-typechecking clean
+	make -C tests-namespaced-references clean
 	make -C tests-sourcedirs-dependency clean
 	make -C tests-reanalyze clean
 

tests/analysis_tests/tests-namespaced-references/.gitignore

@@ -0,0 +1,2 @@
+lib/
+node_modules/

tests/analysis_tests/tests-namespaced-references/Makefile

@@ -0,0 +1,14 @@
+SHELL = /bin/bash
+
+build:
+	yarn build
+
+test: build
+	./test.sh
+
+clean:
+	yarn clean
+
+.DEFAULT_GOAL := test
+
+.PHONY: clean test

tests/analysis_tests/tests-namespaced-references/package.json

@@ -0,0 +1,11 @@
+{
+  "name": "@tests/namespaced-references",
+  "private": true,
+  "scripts": {
+    "build": "rescript build",
+    "clean": "rescript clean"
+  },
+  "dependencies": {
+    "rescript": "workspace:^"
+  }
+}

tests/analysis_tests/tests-namespaced-references/rescript.json

@@ -0,0 +1,17 @@
+{
+  "name": "@tests/namespaced-references",
+  "namespace": "my-namespace",
+  "sources": [
+    {
+      "dir": "src",
+      "subdirs": true
+    }
+  ],
+  "package-specs": [
+    {
+      "module": "commonjs",
+      "in-source": false
+    }
+  ],
+  "suffix": ".res.js"
+}

tests/analysis_tests/tests-namespaced-references/src/MyModule1.res

@@ -0,0 +1,2 @@
+let myFunc1 = () => MyModule2.myFunc2()
+//                            ^ref

tests/analysis_tests/tests-namespaced-references/src/MyModule2.res

@@ -0,0 +1,2 @@
+let myFunc2 = () => 42
+//  ^ref

tests/analysis_tests/tests-namespaced-references/src/expected/MyModule1.res.txt

@@ -0,0 +1,18 @@
+References src/MyModule1.res 0:30
+[
+  {
+    "range": {
+      "end": { "character": 37, "line": 0 },
+      "start": { "character": 30, "line": 0 }
+    },
+    "uri": "file:///MyModule1.res"
+  },
+  {
+    "range": {
+      "end": { "character": 11, "line": 0 },
+      "start": { "character": 4, "line": 0 }
+    },
+    "uri": "file:///MyModule2.res"
+  }
+]
+

tests/analysis_tests/tests-namespaced-references/src/expected/MyModule2.res.txt

@@ -0,0 +1,18 @@
+References src/MyModule2.res 0:4
+[
+  {
+    "range": {
+      "end": { "character": 37, "line": 0 },
+      "start": { "character": 30, "line": 0 }
+    },
+    "uri": "file:///MyModule1.res"
+  },
+  {
+    "range": {
+      "end": { "character": 11, "line": 0 },
+      "start": { "character": 4, "line": 0 }
+    },
+    "uri": "file:///MyModule2.res"
+  }
+]
+

tests/analysis_tests/tests-namespaced-references/test.sh

@@ -0,0 +1,21 @@
+for file in src/*.res; do
+  output="$(dirname $file)/expected/$(basename $file).txt"
+  ../../../_build/install/default/bin/rescript-editor-analysis test $file &> $output
+  # CI. We use LF, and the CI OCaml fork prints CRLF. Convert.
+  if [ "$RUNNER_OS" == "Windows" ]; then
+    perl -pi -e 's/\r\n/\n/g' -- $output
+  fi
+done
+
+warningYellow='\033[0;33m'
+successGreen='\033[0;32m'
+reset='\033[0m'
+
+diff=$(git ls-files --modified src/expected)
+if [[ $diff = "" ]]; then
+  printf "${successGreen}✅ No unstaged tests difference.${reset}\n"
+else
+  printf "${warningYellow}⚠️ There are unstaged differences in tests/! Did you break a test?\n${diff}\n${reset}"
+  git --no-pager diff src/expected
+  exit 1
+fi

tests/ounit_tests/ounit_analysis_references_tests.ml

@@ -0,0 +1,25 @@
+open OUnit
+
+let show_reference (module_name, path) =
+  Printf.sprintf "%s:%s" module_name (String.concat "." path)
+
+let assert_ref_key_eq ~expected ~actual =
+  assert_equal ~printer:show_reference expected actual
+
+let suites =
+  __FILE__
+  >::: [
+         ( "without namespace, external reference is unchanged" >:: fun _ ->
+           assert_ref_key_eq ~expected:("MyModule2", ["myFunc2"])
+             ~actual:
+               (Analysis.References.normalizeExternalReferenceKey
+                  ~namespace:None ~moduleName:"MyModule2" ~path:["myFunc2"]) );
+         ( "with namespace, hidden module resolves to public namespace path"
+         >:: fun _ ->
+           assert_ref_key_eq
+             ~expected:("MyNamespace", ["MyModule2"; "myFunc2"])
+             ~actual:
+               (Analysis.References.normalizeExternalReferenceKey
+                  ~namespace:(Some "MyNamespace")
+                  ~moduleName:"MyModule2-MyNamespace" ~path:["myFunc2"]) );
+       ]

tests/ounit_tests/ounit_tests_main.ml

@@ -20,6 +20,7 @@ let suites =
       Ounit_js_analyzer_tests.suites;
       Ounit_jsx_loc_tests.suites;
       Ounit_analysis_config_tests.suites;
+      Ounit_analysis_references_tests.suites;
     ]
 
 let _ = OUnit.run_test_tt_main suites

yarn.lock

@@ -1133,6 +1133,14 @@ __metadata:
   languageName: unknown
   linkType: soft
 
+"@tests/namespaced-references@workspace:tests/analysis_tests/tests-namespaced-references":
+  version: 0.0.0-use.local
+  resolution: "@tests/namespaced-references@workspace:tests/analysis_tests/tests-namespaced-references"
+  dependencies:
+    rescript: "workspace:^"
+  languageName: unknown
+  linkType: soft
+
 "@tests/reanalyze-benchmark@workspace:tests/analysis_tests/tests-reanalyze/deadcode-benchmark":
   version: 0.0.0-use.local
   resolution: "@tests/reanalyze-benchmark@workspace:tests/analysis_tests/tests-reanalyze/deadcode-benchmark"