Repository: elm-tooling/elm-language-server
Pull request #921 (2023-03-27 17:14:25)

Improve hover and completion type strings when there is a module prefix that uses an alias
This fixes some cases of the module prefix being missing in a hover or completion type string when a module alias was used.


Patch:
diff --git a/src/compiler/imports.ts b/src/compiler/imports.ts
index b8a9a10..83262a6 100644
--- a/src/compiler/imports.ts
+++ b/src/compiler/imports.ts
@@ -21,6 +21,7 @@ type FromModule = {
 
 export interface IImport extends ISymbol {
   fromModule: FromModule;
+  importNode?: SyntaxNode;
 }
 
 function importModuleEqual(a: IImport, b: IImport): boolean {
@@ -118,6 +119,7 @@ export class Imports {
               node: foundModuleNode,
               type: "Module",
               fromModule,
+              importNode,
             });
 
             const exposedFromRemoteModule = foundModule.exposing;
diff --git a/src/compiler/references.ts b/src/compiler/references.ts
index c3c7d4a..5ef5c87 100644
--- a/src/compiler/references.ts
+++ b/src/compiler/references.ts
@@ -442,38 +442,22 @@ export class References {
                   continue;
                 }
 
-                const moduleNameToLookFor =
-                  TreeUtils.findImportAliasOfModule(
-                    moduleNameNode.text,
-                    sourceFileToCheck.tree,
-                  ) ?? moduleNameNode.text;
-
-                // Check if it is imported
-                const imported = sourceFileToCheck.symbolLinks
-                  ?.get(sourceFileToCheck.tree.rootNode)
-                  ?.get(
-                    moduleNameToLookFor,
-                    (symbol) => symbol.type === "Import",
-                  );
+                const imported = checker
+                  .getAllImports(sourceFileToCheck)
+                  .getModule(moduleNameNode.text)
+                  ?.importNode?.childForFieldName("moduleName");
 
                 if (imported) {
-                  const importNameNode = checker.findImportModuleNameNodes(
-                    moduleNameToLookFor,
-                    sourceFileToCheck,
-                  )[0];
-
-                  if (importNameNode) {
-                    references.push({ node: importNameNode, uri });
-                  }
+                  references.push({ node: imported, uri });
                 }
 
-                // If it is not imported as an alias, find all references in file
-                if (imported && moduleNameToLookFor === moduleNameNode.text) {
+                // Find all references in file
+                if (imported) {
                   sourceFileToCheck.tree.rootNode
                     .descendantsOfType("value_expr")
                     .forEach((valueNode) => {
                       if (
-                        RegExp(`${moduleNameToLookFor}.[a-z].*`).exec(
+                        RegExp(`${moduleNameNode.text}.[a-z].*`).exec(
                           valueNode.text,
                         )
                       ) {
diff --git a/src/compiler/typeChecker.ts b/src/compiler/typeChecker.ts
index 0767787..bf35278 100644
--- a/src/compiler/typeChecker.ts
+++ b/src/compiler/typeChecker.ts
@@ -76,7 +76,7 @@ export interface TypeChecker {
     sourceFile: ISourceFile,
     module: string,
     name: string,
-  ) => string | undefined;
+  ) => string;
   typeToString: (t: Type, sourceFile?: ISourceFile) => string;
   getDiagnostics: (
     sourceFile: ISourceFile,
@@ -564,6 +564,7 @@ export function createTypeChecker(program: IProgram): TypeChecker {
       if (TreeUtils.nextNode(nodeAtPosition)?.type === "dot") {
         const endPos = upperCaseQidText.indexOf(nodeText) + nodeText.length;
 
+        // There may be multiple imports with the same name/alias, but we just go to the first one
         const moduleNameOrAlias = nodeParent.text.substring(0, endPos);
         const moduleName =
           findImportModuleNameNodes(moduleNameOrAlias, sourceFile)[0]?.text ??
@@ -579,6 +580,7 @@ export function createTypeChecker(program: IProgram): TypeChecker {
         }
       }
 
+      // There may be multiple imports with the same name/alias, but we just go to the first one
       const moduleImport = findImport(
         sourceFile,
         findImportModuleNameNodes(upperCaseQidText, sourceFile)[0]?.text ??
@@ -849,9 +851,8 @@ export function createTypeChecker(program: IProgram): TypeChecker {
     sourceFile: ISourceFile,
     module: string,
     name: string,
-  ): string | undefined {
-    const found = findImport(sourceFile, name)[0];
-    if (found) {
+  ): string {
+    if (findImport(sourceFile, name).length > 0) {
       return "";
     }
 
@@ -859,11 +860,11 @@ export function createTypeChecker(program: IProgram): TypeChecker {
       return "";
     }
 
-    const moduleImport = findImportModuleNameNodes(module, sourceFile)[0]
-      ?.parent;
+    const moduleImport = findImport(sourceFile, module, "Module")[0]
+      ?.importNode;
 
     if (!moduleImport) {
-      return;
+      return "";
     }
 
     const asClause = TreeUtils.findFirstNamedChildOfType(
@@ -889,14 +890,28 @@ export function createTypeChecker(program: IProgram): TypeChecker {
     moduleNameOrAlias: string,
     sourceFile: ISourceFile,
   ): SyntaxNode[] {
-    return (
+    // This will find an import based on module name only, not alias
+    const moduleImport = getAllImports(sourceFile)
+      .getModule(moduleNameOrAlias)
+      ?.importNode?.childForFieldName("moduleName");
+
+    // This will not find a module by name if it is aliased
+    const moduleOrAliasImports =
       sourceFile.symbolLinks
         ?.get(sourceFile.tree.rootNode)
         ?.getAll(moduleNameOrAlias)
         ?.filter((s) => s.type === "Import")
         .map((s) => s.node.childForFieldName("moduleName"))
-        .filter(Utils.notUndefinedOrNull) ?? []
-    );
+        .filter(Utils.notUndefinedOrNull) ?? [];
+
+    if (
+      moduleImport &&
+      moduleOrAliasImports.every((n) => n.id !== moduleImport.id)
+    ) {
+      return [moduleImport, ...moduleOrAliasImports];
+    }
+
+    return moduleOrAliasImports;
   }
 
   function getSymbolsInScope(
diff --git a/src/compiler/typeRenderer.ts b/src/compiler/typeRenderer.ts
index 60ca2d4..796caa5 100644
--- a/src/compiler/typeRenderer.ts
+++ b/src/compiler/typeRenderer.ts
@@ -38,7 +38,7 @@ export class TypeRenderer {
           )
           .join(" -> ")}`;
       case "Tuple":
-        return `(${t.types.map(this.render.bind(this)).join(", ")})`;
+        return `( ${t.types.map(this.render.bind(this)).join(", ")} )`;
       case "Union":
         return this.renderUnion(t);
       case "Record":
diff --git a/src/providers/completionProvider.ts b/src/providers/completionProvider.ts
index a6be3cb..0d795c5 100644
--- a/src/providers/completionProvider.ts
+++ b/src/providers/completionProvider.ts
@@ -563,10 +563,9 @@ export class CompletionProvider {
       const dotIndex = label.lastIndexOf(".");
       const valuePart = label.slice(dotIndex + 1);
 
-      const importNode = checker.findImportModuleNameNodes(
-        element.fromModule.name,
-        sourceFile,
-      )[0]?.parent;
+      const importNode = checker
+        .getAllImports(sourceFile)
+        .getModule(element.fromModule.name)?.importNode;
 
       // Check if a value is already imported for this module using the exposing list
       // In this case, we want to prefex the unqualified value since they are using the import exposing list
@@ -1107,14 +1106,12 @@ export class CompletionProvider {
             } else if (!aMatches && bMatches) {
               return 1;
             } else {
-              const aModuleImported = !!checker.findImportModuleNameNodes(
-                a.module,
-                sourceFile,
-              )[0];
-              const bModuleImported = !!checker.findImportModuleNameNodes(
-                b.module,
-                sourceFile,
-              )[0];
+              const aModuleImported = !!checker
+                .getAllImports(sourceFile)
+                .getModule(a.module);
+              const bModuleImported = !!checker
+                .getAllImports(sourceFile)
+                .getModule(b.module);
 
               if (aModuleImported && !bModuleImported) {
                 return -1;
@@ -1255,7 +1252,13 @@ export class CompletionProvider {
     matchedSourceFiles
       .flatMap(ImportUtils.getPossibleImportsOfTree.bind(this))
       .forEach((value) => {
-        const markdownDocumentation = HintHelper.createHint(value.node);
+        const type = checker.findType(value.node);
+        const typeString = checker.typeToString(type, sourceFile);
+
+        const markdownDocumentation = HintHelper.createHint(
+          value.node,
+          typeString,
+        );
         let additionalTextEdits: TextEdit[] | undefined;
         let detail: string | undefined;
 
diff --git a/src/providers/hoverProvider.ts b/src/providers/hoverProvider.ts
index 5385f6e..42a5c71 100644
--- a/src/providers/hoverProvider.ts
+++ b/src/providers/hoverProvider.ts
@@ -14,7 +14,7 @@ import { HintHelper } from "../util/hintHelper";
 import { TreeUtils } from "../util/treeUtils";
 import { ITextDocumentPositionParams } from "./paramsExtensions";
 
-type HoverResult = Hover | null | undefined;
+export type HoverResult = Hover | null | undefined;
 
 export class HoverProvider {
   private connection: Connection;
diff --git a/src/util/hintHelper.ts b/src/util/hintHelper.ts
index 3275dc4..54aeb28 100644
--- a/src/util/hintHelper.ts
+++ b/src/util/hintHelper.ts
@@ -6,37 +6,39 @@ export class HintHelper {
     node: SyntaxNode | undefined,
     typeString?: string,
   ): string | undefined {
-    if (node) {
-      if (node.type === "module_declaration") {
-        return this.createHintFromModule(node);
-      } else if (
-        node.parent?.type === "let_in_expr" ||
-        (node.type === "lower_pattern" &&
-          !!TreeUtils.findParentOfType("let_in_expr", node))
-      ) {
-        return this.createHintFromDefinitionInLet(node, typeString);
-      } else if (node.type === "field_type") {
-        return this.createHintFromFieldType(node);
-      } else if (node.type === "port_annotation") {
-        const name = node.childForFieldName("name");
-        const typeExpression = node.childForFieldName("typeExpression");
-        if (name && typeExpression) {
-          let comment = "";
-          if (
-            node.previousNamedSibling &&
-            node.previousNamedSibling.type === "block_comment"
-          ) {
-            comment = node.previousNamedSibling.text;
-          }
-
-          return this.formatHint(
-            `${name.text} : ${typeExpression.text}`,
-            comment,
-          );
+    if (!node) {
+      return;
+    }
+
+    if (node.type === "module_declaration") {
+      return this.createHintFromModule(node);
+    } else if (
+      node.parent?.type === "let_in_expr" ||
+      (node.type === "lower_pattern" &&
+        !!TreeUtils.findParentOfType("let_in_expr", node))
+    ) {
+      return this.createHintFromDefinitionInLet(node, typeString);
+    } else if (node.type === "field_type") {
+      return this.createHintFromFieldType(node);
+    } else if (node.type === "port_annotation") {
+      const name = node.childForFieldName("name");
+      const typeExpression = node.childForFieldName("typeExpression");
+      if (name && typeExpression) {
+        let comment = "";
+        if (
+          node.previousNamedSibling &&
+          node.previousNamedSibling.type === "block_comment"
+        ) {
+          comment = node.previousNamedSibling.text;
         }
-      } else {
-        return this.createHintFromDefinition(node);
+
+        return this.formatHint(
+          `${name.text} : ${typeExpression.text}`,
+          comment,
+        );
       }
+    } else {
+      return this.createHintFromDefinition(node, typeString);
     }
   }
 
@@ -118,67 +120,74 @@ export class HintHelper {
 
   private static createHintFromDefinition(
     declaration: SyntaxNode | undefined,
+    typeString?: string,
   ): string | undefined {
-    if (declaration) {
-      let code: string | undefined;
-      let comment = "";
-      let annotation = "";
-      if (
-        declaration.type === "type_declaration" ||
-        declaration.type === "type_alias_declaration"
-      ) {
+    if (!declaration) {
+      return;
+    }
+
+    let code: string | undefined;
+    let comment = "";
+    let annotation = "";
+    if (
+      declaration.type === "type_declaration" ||
+      declaration.type === "type_alias_declaration"
+    ) {
+      code = declaration.text;
+    }
+    if (declaration.type === "union_variant") {
+      if (declaration.parent?.previousNamedSibling?.type !== "block_comment") {
         code = declaration.text;
+
+        if (declaration.parent) {
+          const typeName = TreeUtils.findFirstNamedChildOfType(
+            "upper_case_identifier",
+            declaration.parent,
+          )?.text;
+          comment =
+            `A variant on the union type \`${typeName ?? "unknown"}\`` || "";
+        }
+      } else {
+        declaration = declaration.parent ? declaration.parent : declaration;
       }
-      if (declaration.type === "union_variant") {
+    }
+
+    if (
+      declaration.parent &&
+      declaration.type === "function_declaration_left"
+    ) {
+      declaration = declaration.parent;
+    }
+
+    if (declaration.previousNamedSibling) {
+      if (declaration.previousNamedSibling.type === "type_annotation") {
+        annotation = declaration.previousNamedSibling.text;
         if (
-          declaration.parent?.previousNamedSibling?.type !== "block_comment"
+          declaration.previousNamedSibling.previousNamedSibling &&
+          declaration.previousNamedSibling.previousNamedSibling.type ===
+            "block_comment"
         ) {
-          code = declaration.text;
-
-          if (declaration.parent) {
-            const typeName = TreeUtils.findFirstNamedChildOfType(
-              "upper_case_identifier",
-              declaration.parent,
-            )?.text;
-            comment =
-              `A variant on the union type \`${typeName ?? "unknown"}\`` || "";
-          }
-        } else {
-          declaration = declaration.parent ? declaration.parent : declaration;
+          comment = declaration.previousNamedSibling.previousNamedSibling.text;
         }
+      } else if (declaration.previousNamedSibling.type === "block_comment") {
+        comment = declaration.previousNamedSibling.text;
       }
 
       if (
-        declaration.parent &&
-        declaration.type === "function_declaration_left"
+        declaration.type === "value_declaration" &&
+        declaration.firstNamedChild?.type === "function_declaration_left"
       ) {
-        declaration = declaration.parent;
+        code = declaration.firstNamedChild.text;
       }
+    }
 
-      if (declaration.previousNamedSibling) {
-        if (declaration.previousNamedSibling.type === "type_annotation") {
-          annotation = declaration.previousNamedSibling.text;
-          if (
-            declaration.previousNamedSibling.previousNamedSibling &&
-            declaration.previousNamedSibling.previousNamedSibling.type ===
-              "block_comment"
-          ) {
-            comment =
-              declaration.previousNamedSibling.previousNamedSibling.text;
-          }
-        } else if (declaration.previousNamedSibling.type === "block_comment") {
-          comment = declaration.previousNamedSibling.text;
-        }
+    const name = declaration.firstNamedChild?.firstNamedChild?.text;
 
-        if (
-          declaration.type === "value_declaration" &&
-          declaration.firstNamedChild?.type === "function_declaration_left"
-        ) {
-          code = declaration.firstNamedChild.text;
-        }
-      }
-      return this.formatHint(annotation, comment, code);
+    if (name && typeString) {
+      annotation = `${name} : ${typeString}`;
     }
+
+    return this.formatHint(annotation, comment, code);
   }
 
   private static createHintFromModule(


Test patch:
diff --git a/src/providers/findTestsProvider.ts b/src/providers/findTestsProvider.ts
index b67a67c..8a14b4a 100644
--- a/src/providers/findTestsProvider.ts
+++ b/src/providers/findTestsProvider.ts
@@ -131,18 +131,16 @@ function isTestSuite(
   const funName = findExpr("ValueExpr", call.target)?.name;
   const dot = funName?.lastIndexOf(".") ?? -1;
   const prefix = dot > -1 ? funName?.substring(0, dot) : undefined;
-  const qualifier: string =
+  const qualifier =
     prefix !== undefined
-      ? typeChecker.getQualifierForName(sourceFile, prefix, "describe") ?? ""
+      ? typeChecker.getQualifierForName(sourceFile, "Test", "describe")
       : "";
-  const moduleName =
-    prefix !== undefined
-      ? typeChecker.findImportModuleNameNodes(prefix, sourceFile)[0]?.text
-      : "Test";
   return (
-    qualifier !== undefined &&
     funName === `${qualifier}describe` &&
-    moduleName === "Test"
+    (!prefix ||
+      typeChecker
+        .findImportModuleNameNodes(prefix, sourceFile)
+        .some((n) => n.text === "Test"))
   );
 }
 
diff --git a/test/completionProvider.test.ts b/test/completionProvider.test.ts
index 0f4a02a..58beb66 100644
--- a/test/completionProvider.test.ts
+++ b/test/completionProvider.test.ts
@@ -70,7 +70,7 @@ describe("CompletionProvider", () => {
       const completions =
         completionProvider.handleCompletion({
           textDocument: { uri: testUri },
-          position: position!,
+          position,
           context,
           program,
           sourceFile,
diff --git a/test/diagnosticTests/elmDiagnostics.test.ts b/test/diagnosticTests/elmDiagnostics.test.ts
index 6675c4e..4d5b37b 100644
--- a/test/diagnosticTests/elmDiagnostics.test.ts
+++ b/test/diagnosticTests/elmDiagnostics.test.ts
@@ -1541,7 +1541,7 @@ foo asInt myNumber =
     ]);
   });
 
-  it.only("operators from different files should not have an errors", async () => {
+  it("operators from different files should not have an errors", async () => {
     const source = `
 --@ Other.elm
 module Other exposing (foo)
diff --git a/test/hoverProvider.test.ts b/test/hoverProvider.test.ts
new file mode 100644
index 0000000..1c3cab9
--- /dev/null
+++ b/test/hoverProvider.test.ts
@@ -0,0 +1,143 @@
+import path from "path";
+import { MarkupContent } from "vscode-languageserver";
+import { URI } from "vscode-uri";
+import { HoverProvider, HoverResult } from "../src/providers";
+import { ITextDocumentPositionParams } from "../src/providers/paramsExtensions";
+import { getInvokePositionFromSource } from "./utils/sourceParser";
+import { baseUri, SourceTreeParser, srcUri } from "./utils/sourceTreeParser";
+
+class MockHoverProvider extends HoverProvider {
+  handleHover = (params: ITextDocumentPositionParams): HoverResult => {
+    return this.handleHoverRequest(params);
+  };
+}
+
+describe("HoverProvider", () => {
+  const treeParser = new SourceTreeParser();
+
+  async function testHover(source: string, expectContains: string) {
+    await treeParser.init();
+    const hoverProvider = new MockHoverProvider();
+
+    const { invokePosition, invokeFile, sources } =
+      getInvokePositionFromSource(source);
+
+    if (!invokePosition) {
+      throw new Error("Getting position failed");
+    }
+
+    const testUri = URI.file(
+      path.join(invokeFile.startsWith("tests") ? baseUri : srcUri, invokeFile),
+    ).toString();
+
+    const program = await treeParser.getProgram(sources);
+    const sourceFile = program.getSourceFile(testUri);
+
+    if (!sourceFile) throw new Error("Getting source file failed");
+
+    const hover = hoverProvider.handleHover({
+      textDocument: { uri: testUri },
+      position: invokePosition,
+      program,
+      sourceFile,
+    });
+
+    if (!hover) {
+      expect(hover).toBeTruthy();
+      return;
+    }
+
+    if (MarkupContent.is(hover.contents)) {
+      expect(hover.contents.value).toContain(expectContains);
+    } else {
+      expect(MarkupContent.is(hover.contents)).toBeTruthy();
+    }
+  }
+
+  it("type should not have module prefix if it from the current module", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+type alias Foo = 
+		String
+
+foo : Foo
+foo = ""
+
+bar = foo
+     --^
+    `;
+
+    await testHover(source, "foo : Foo");
+  });
+
+  it("should have module prefix if it is from the another module and the current module doesn't have one", async () => {
+    const source = `
+--@ Another.elm
+module Another exposing (..)
+
+type alias Foo = 
+		String
+
+foo : Foo
+foo = ""
+
+--@ Test.elm
+module Test exposing (..)
+
+import Another exposing (foo)
+
+bar = foo
+     --^
+    `;
+
+    await testHover(source, "foo : Another.Foo");
+  });
+
+  it("should have module prefix if it is from the another module and the current module has one already", async () => {
+    const source = `
+--@ Another.elm
+module Another exposing (..)
+
+type alias Foo = 
+		String
+
+foo : Foo
+foo = ""
+
+--@ Test.elm
+module Test exposing (..)
+
+import Another exposing (foo)
+
+bar = foo
+     --^
+    `;
+
+    await testHover(source, "foo : Another.Foo");
+  });
+
+  it("should have aliases module prefix if it from the another module", async () => {
+    const source = `
+--@ Another.elm
+module Another exposing (..)
+
+type alias Foo = 
+		String
+
+foo : Foo
+foo = ""
+
+--@ Test.elm
+module Test exposing (..)
+
+import Another as AnotherAlias exposing (foo)
+
+bar = foo
+     --^
+    `;
+
+    await testHover(source, "foo : AnotherAlias.Foo");
+  });
+});
diff --git a/test/typeInference.test.ts b/test/typeInference.test.ts
index aa614ca..3d99620 100644
--- a/test/typeInference.test.ts
+++ b/test/typeInference.test.ts
@@ -386,7 +386,7 @@ func a b =
 `;
     await testTypeInference(
       basicsSources + source,
-      "number -> number -> (List number, ())",
+      "number -> number -> ( List number, () )",
     );
   });
 
@@ -489,7 +489,7 @@ func a =
 `;
     await testTypeInference(
       basicsSources + source,
-      "({ a | d : (), e : b }, c) -> number",
+      "( { a | d : (), e : b }, c ) -> number",
     );
   });
 
@@ -693,7 +693,7 @@ bug =
 `;
     await testTypeInference(
       basicsSources + source,
-      "a -> b -> (c -> (d -> (e -> (f -> (g -> (h -> (i -> (j -> (k -> (l -> (m -> (n -> (o -> (p -> (q -> (r -> (s -> (t -> (u -> (v -> (w -> (x -> (y -> (z -> (a1 -> (b1 -> (a, b1), a1), z), y), x), w), v), u), t), s), r), q), p), o), n), m), l), k), j), i), h), g), f), e), d), c), b)",
+      "a -> b -> ( c -> ( d -> ( e -> ( f -> ( g -> ( h -> ( i -> ( j -> ( k -> ( l -> ( m -> ( n -> ( o -> ( p -> ( q -> ( r -> ( s -> ( t -> ( u -> ( v -> ( w -> ( x -> ( y -> ( z -> ( a1 -> ( b1 -> ( a, b1 ), a1 ), z ), y ), x ), w ), v ), u ), t ), s ), r ), q ), p ), o ), n ), m ), l ), k ), j ), i ), h ), g ), f ), e ), d ), c ), b )",
     );
   });
 
diff --git a/test/utils/sourceParser.ts b/test/utils/sourceParser.ts
index 42d8442..22a4b4e 100644
--- a/test/utils/sourceParser.ts
+++ b/test/utils/sourceParser.ts
@@ -1,3 +1,4 @@
+import { fail } from "assert";
 import { Position, Range } from "vscode-languageserver";
 
 export function getCaretPositionFromSource(source: string): {
@@ -155,6 +156,36 @@ export function getInvokeAndTargetPositionFromSource(source: string): TestType {
   }
 }
 
+export function getInvokePositionFromSource(source: string): IInvokeTest {
+  const sources = getSourceFiles(source);
+
+  let invokePosition;
+  let invokeFile = "";
+
+  for (const fileName in sources) {
+    sources[fileName].split("\n").forEach((s, line) => {
+      const invokeCharacter = s.search(/--(\^)/);
+
+      if (invokeCharacter >= 0) {
+        invokePosition = {
+          line: line - 1,
+          character: invokeCharacter + 2,
+        };
+        invokeFile = fileName;
+      }
+    });
+  }
+
+  if (!invokePosition) {
+    fail();
+  }
+  return {
+    invokePosition,
+    sources,
+    invokeFile,
+  };
+}
+
 export function getTargetPositionFromSource(
   source: string,
 ): { range: Range; sources: { [K: string]: string } } | undefined {