Repository: elm-tooling/elm-language-server
Pull request #374 (2020-11-24 13:34:40)

Fix union pattern when arg is a unit expr
Fixes the elm-spa-example.


Patch:
diff --git a/src/forest.ts b/src/forest.ts
index d9a89bc..392f471 100644
--- a/src/forest.ts
+++ b/src/forest.ts
@@ -7,6 +7,7 @@ import { existsSync } from "fs";
 import { URI } from "vscode-uri";
 import { SyntaxNodeMap } from "./util/types/syntaxNodeMap";
 import { SymbolMap } from "./util/types/binder";
+import { Diagnostic } from "./util/types/typeInference";
 
 export interface ITreeContainer {
   uri: string;
@@ -16,6 +17,8 @@ export interface ITreeContainer {
   tree: Tree;
   isExposed: boolean; // Is this file exposed by the elm.json
 
+  parseDiagnostics: Diagnostic[];
+
   // These are resolved in the synchronize step and are cached until the file is changed
   moduleName?: string;
   resolvedModules?: Map<string, string>; // Map of modules to uris
@@ -92,6 +95,7 @@ export class Forest implements IForest {
       uri,
       writeable,
       isExposed,
+      parseDiagnostics: [],
     };
 
     this.treeMap.set(uri, treeContainer);
diff --git a/src/imports.ts b/src/imports.ts
index b3fead6..3577851 100644
--- a/src/imports.ts
+++ b/src/imports.ts
@@ -101,7 +101,7 @@ export class Imports extends MultiMap<string, IImport> {
                     );
                     if (foundNode) {
                       this.exposedNodesToImports(
-                        [foundNode],
+                        foundNode,
                         moduleName,
                         foundModule,
                       ).forEach((imp) => result.set(imp.alias, imp));
@@ -118,7 +118,7 @@ export class Imports extends MultiMap<string, IImport> {
                     );
                     if (foundNode) {
                       this.exposedNodesToImports(
-                        [foundNode],
+                        foundNode,
                         moduleName,
                         foundModule,
                       ).forEach((imp) => result.set(imp.alias, imp));
@@ -136,13 +136,19 @@ export class Imports extends MultiMap<string, IImport> {
                       exposedType,
                     )?.text;
 
+                    const exposedUnionConstructors = !!TreeUtils.findFirstNamedChildOfType(
+                      "exposed_union_constructors",
+                      exposedType,
+                    );
+
                     if (typeName) {
                       const foundNode = exposedFromRemoteModule.get(typeName);
                       if (foundNode) {
                         this.exposedNodesToImports(
-                          [foundNode],
+                          foundNode,
                           moduleName,
                           foundModule,
+                          exposedUnionConstructors,
                         ).forEach((imp) => result.set(imp.alias, imp));
                       }
                     }
@@ -311,24 +317,24 @@ export class Imports extends MultiMap<string, IImport> {
   }
 
   private static exposedNodesToImports(
-    exposedNodes: IExposed[],
+    exposedNode: IExposed,
     moduleName: string,
     foundModule: ITreeContainer,
+    includeUnionConstructors = false,
   ): IImport[] {
-    return exposedNodes
-      .map((a) => {
-        return [
-          {
-            alias: a.name,
-            fromModuleName: moduleName,
-            fromUri: foundModule.uri,
-            maintainerAndPackageName: foundModule.maintainerAndPackageName,
-            node: a.syntaxNode,
-            type: a.type,
-            explicitlyExposed: true,
-          },
-        ].concat(
-          a.exposedUnionConstructors?.map((b) => {
+    return [
+      {
+        alias: exposedNode.name,
+        fromModuleName: moduleName,
+        fromUri: foundModule.uri,
+        maintainerAndPackageName: foundModule.maintainerAndPackageName,
+        node: exposedNode.syntaxNode,
+        type: exposedNode.type,
+        explicitlyExposed: true,
+      },
+    ].concat(
+      includeUnionConstructors
+        ? exposedNode.exposedUnionConstructors?.map((b) => {
             return {
               alias: b.name,
               fromModuleName: moduleName,
@@ -338,9 +344,8 @@ export class Imports extends MultiMap<string, IImport> {
               type: "UnionConstructor",
               explicitlyExposed: false,
             };
-          }) ?? [],
-        );
-      })
-      .reduce((a, b) => a.concat(b), []);
+          }) ?? []
+        : [],
+    );
   }
 }
diff --git a/src/providers/completionProvider.ts b/src/providers/completionProvider.ts
index 68fd070..c5a72e2 100644
--- a/src/providers/completionProvider.ts
+++ b/src/providers/completionProvider.ts
@@ -26,6 +26,7 @@ import RANKING_LIST from "./ranking";
 import { DiagnosticsProvider } from ".";
 import { TypeChecker } from "../util/types/typeChecker";
 import escapeStringRegexp from "escape-string-regexp";
+import { TRecord } from "../util/types/typeInference";
 
 export type CompletionResult =
   | CompletionItem[]
@@ -744,6 +745,8 @@ export class CompletionProvider {
     range: Range,
     elmWorkspace: IElmWorkspace,
   ): CompletionItem[] {
+    const checker = elmWorkspace.getTypeChecker();
+
     const result: CompletionItem[] = [];
     let typeDeclarationNode = TreeUtils.getTypeAliasOfRecord(
       node,
@@ -759,22 +762,58 @@ export class CompletionProvider {
       )?.node;
     }
 
+    let recordType: TRecord | undefined;
     if (!typeDeclarationNode && node.parent?.parent) {
-      typeDeclarationNode = TreeUtils.getTypeOrTypeAliasOfFunctionRecordParameter(
+      recordType = TreeUtils.getRecordTypeOfFunctionRecordParameter(
         node.parent.parent,
-        treeContainer,
         elmWorkspace,
       );
     }
 
+    if (
+      !typeDeclarationNode &&
+      !recordType &&
+      node.parent?.parent?.type === "record_expr"
+    ) {
+      const recordExpr = node.parent.parent;
+
+      const foundType = checker.findType(recordExpr);
+
+      if (foundType.nodeType === "Record") {
+        recordType = foundType;
+      }
+    }
+
+    if (recordType) {
+      if (recordType.baseType?.nodeType === "Record") {
+        for (const field in recordType.baseType.fields) {
+          const hint = HintHelper.createHintForTypeAliasReference(
+            checker.typeToString(recordType.fields[field]),
+            field,
+            recordType.alias?.name ?? "",
+          );
+
+          result.push(
+            this.createFieldOrParameterCompletion(hint, field, range),
+          );
+        }
+      }
+      for (const field in recordType.fields) {
+        const hint = HintHelper.createHintForTypeAliasReference(
+          checker.typeToString(recordType.fields[field]),
+          field,
+          recordType.alias?.name ?? "",
+        );
+
+        result.push(this.createFieldOrParameterCompletion(hint, field, range));
+      }
+    }
+
     if (typeDeclarationNode) {
       const fields = TreeUtils.getAllFieldsFromTypeAlias(typeDeclarationNode);
 
       const typeName =
-        TreeUtils.findFirstNamedChildOfType(
-          "upper_case_identifier",
-          typeDeclarationNode,
-        )?.text ?? "";
+        typeDeclarationNode.childForFieldName("name")?.text ?? "";
 
       fields?.forEach((element) => {
         const hint = HintHelper.createHintForTypeAliasReference(
diff --git a/src/providers/diagnostics/diagnosticsProvider.ts b/src/providers/diagnostics/diagnosticsProvider.ts
index 8a33cbe..d86a35a 100644
--- a/src/providers/diagnostics/diagnosticsProvider.ts
+++ b/src/providers/diagnostics/diagnosticsProvider.ts
@@ -290,7 +290,7 @@ export class DiagnosticsProvider {
             }
           };
 
-          const checkOne = async (): Promise<void> => {
+          const checkOne = (): void => {
             if (this.changeSeq !== seq) {
               return;
             }
@@ -307,15 +307,26 @@ export class DiagnosticsProvider {
               return;
             }
 
-            this.updateDiagnostics(
-              uri,
-              DiagnosticKind.TypeInference,
-              await this.typeInferenceDiagnostics.getDiagnosticsForFile(
+            this.typeInferenceDiagnostics
+              .getDiagnosticsForFile(
                 treeContainer,
                 workspace,
                 cancellationToken,
-              ),
-            );
+              )
+              .then((diagnostics) => {
+                if (this.changeSeq !== seq) {
+                  return;
+                }
+
+                this.updateDiagnostics(
+                  uri,
+                  DiagnosticKind.TypeInference,
+                  diagnostics,
+                );
+              })
+              .catch(() => {
+                // Cancelled
+              });
 
             if (this.changeSeq !== seq) {
               return;
diff --git a/src/providers/diagnostics/typeInferenceDiagnostics.ts b/src/providers/diagnostics/typeInferenceDiagnostics.ts
index 3d38828..4a12689 100644
--- a/src/providers/diagnostics/typeInferenceDiagnostics.ts
+++ b/src/providers/diagnostics/typeInferenceDiagnostics.ts
@@ -24,6 +24,7 @@ import {
   ThrottledCancellationToken,
 } from "../../cancellation";
 import { container } from "tsyringe";
+import { Diagnostic as InternalDiagnostic } from "../../util/types/typeInference";
 
 export class TypeInferenceDiagnostics {
   TYPE_INFERENCE = "Type Inference";
@@ -51,6 +52,10 @@ export class TypeInferenceDiagnostics {
 
     const diagnostics: Diagnostic[] = [];
 
+    diagnostics.push(
+      ...treeContainer.parseDiagnostics.map(this.convertDiagnostic.bind(this)),
+    );
+
     const allTopLevelFunctions =
       TreeUtils.findAllTopLevelFunctionDeclarations(treeContainer.tree) ?? [];
 
@@ -119,15 +124,7 @@ export class TypeInferenceDiagnostics {
         const nodeUri = diagnostic.node.tree.uri;
 
         if (nodeUri === treeContainer.uri) {
-          diagnostics.push({
-            range: {
-              start: this.getNodeRange(diagnostic.node).start,
-              end: this.getNodeRange(diagnostic.endNode).end,
-            },
-            message: diagnostic.message,
-            severity: DiagnosticSeverity.Error,
-            source: this.TYPE_INFERENCE,
-          });
+          diagnostics.push(this.convertDiagnostic(diagnostic));
         }
       });
 
@@ -154,6 +151,18 @@ export class TypeInferenceDiagnostics {
     return diagnostics;
   }
 
+  private convertDiagnostic(diagnostic: InternalDiagnostic): Diagnostic {
+    return {
+      range: {
+        start: this.getNodeRange(diagnostic.node).start,
+        end: this.getNodeRange(diagnostic.endNode).end,
+      },
+      message: diagnostic.message,
+      severity: DiagnosticSeverity.Error,
+      source: this.TYPE_INFERENCE,
+    };
+  }
+
   public onCodeAction(params: CodeActionParams): CodeAction[] {
     const { uri } = params.textDocument;
     const typeInferenceDiagnostics: Diagnostic[] = this.filterTypeInferenceDiagnostics(
diff --git a/src/util/treeUtils.ts b/src/util/treeUtils.ts
index b569915..a1da0f5 100644
--- a/src/util/treeUtils.ts
+++ b/src/util/treeUtils.ts
@@ -2,8 +2,12 @@ import { Position } from "vscode-languageserver";
 import { SyntaxNode, Tree } from "web-tree-sitter";
 import { ITreeContainer } from "../forest";
 import { comparePosition } from "../positionUtil";
-import { Type } from "./types/typeInference";
+import { TRecord, Type } from "./types/typeInference";
 import { IElmWorkspace } from "../elmWorkspace";
+import {
+  EFunctionCallExpr,
+  mapSyntaxNodeToExpression,
+} from "./types/expressionTree";
 
 export type NodeType =
   | "Function"
@@ -512,50 +516,30 @@ export class TreeUtils {
     }
   }
 
-  public static getTypeOrTypeAliasOfFunctionRecordParameter(
+  public static getRecordTypeOfFunctionRecordParameter(
     node: SyntaxNode | undefined,
-    treeContainer: ITreeContainer,
     elmWorkspace: IElmWorkspace,
-  ): SyntaxNode | undefined {
+  ): TRecord | undefined {
     const checker = elmWorkspace.getTypeChecker();
     if (
       node?.parent?.type === "function_call_expr" &&
       node.parent.firstNamedChild
     ) {
-      const parameterIndex =
-        node.parent.namedChildren.map((c) => c.text).indexOf(node.text) - 1;
-
-      const functionName = TreeUtils.descendantsOfType(
-        node.parent.firstNamedChild,
-        "lower_case_identifier",
-      );
-
-      const functionDefinition = checker.findDefinition(
-        functionName[functionName.length - 1],
-        treeContainer,
-      );
+      const functionCallExpr = mapSyntaxNodeToExpression(
+        node.parent,
+      ) as EFunctionCallExpr;
 
-      if (functionDefinition?.node.previousNamedSibling?.lastNamedChild) {
-        const typeAnnotationNodes = TreeUtils.findAllNamedChildrenOfType(
-          ["type_ref", "record_type"],
-          functionDefinition.node.previousNamedSibling.lastNamedChild,
-        );
+      const parameterIndex =
+        functionCallExpr.namedChildren.map((c) => c.text).indexOf(node.text) -
+        1;
 
-        if (typeAnnotationNodes) {
-          const typeNode = typeAnnotationNodes[parameterIndex];
+      const foundType = checker.findType(functionCallExpr.target);
 
-          if (typeNode?.type === "type_ref") {
-            const typeNodes = TreeUtils.descendantsOfType(
-              typeNode,
-              "upper_case_identifier",
-            );
+      if (foundType.nodeType === "Function") {
+        const paramType = foundType.params[parameterIndex];
 
-            if (typeNodes.length > 0) {
-              return checker.findDefinition(typeNodes[0], treeContainer)?.node;
-            }
-          } else {
-            return typeNode || undefined;
-          }
+        if (paramType.nodeType === "Record") {
+          return paramType;
         }
       }
     }
diff --git a/src/util/types/binder.ts b/src/util/types/binder.ts
index f60cec4..e72ceda 100644
--- a/src/util/types/binder.ts
+++ b/src/util/types/binder.ts
@@ -81,6 +81,13 @@ export function bindTreeContainer(treeContainer: ITreeContainer): void {
       case "import_clause":
         bindImportClause(node);
         break;
+      case "ERROR":
+        treeContainer.parseDiagnostics.push({
+          node,
+          endNode: node,
+          message: `Parsing error`,
+        });
+        break;
       default:
         forEachChild(bind);
     }
diff --git a/src/util/types/expressionTree.ts b/src/util/types/expressionTree.ts
index 4aea5d5..894e6e8 100644
--- a/src/util/types/expressionTree.ts
+++ b/src/util/types/expressionTree.ts
@@ -617,7 +617,9 @@ export function mapSyntaxNodeToExpression(
           .slice(1)
           .filter(
             (node) =>
-              node.type.includes("pattern") || node.type.includes("constant"),
+              node.type.includes("pattern") ||
+              node.type.includes("constant") ||
+              node.type === "unit_expr",
           )
           .map(
             (node) => mapSyntaxNodeToExpression(node) ?? node,
@@ -721,11 +723,12 @@ export function mapSyntaxNodeToExpression(
         recordExpr.baseRecord = node.childForFieldName(
           "baseRecord",
         ) as SyntaxNode;
-        recordExpr.fields = TreeUtils.findAllNamedChildrenOfType("field", node)
-          ?.map(mapSyntaxNodeToExpression)
-          .filter(
-            Utils.notUndefined.bind(mapSyntaxNodeToExpression),
-          ) as EField[];
+        recordExpr.fields =
+          (TreeUtils.findAllNamedChildrenOfType("field", node)
+            ?.map(mapSyntaxNodeToExpression)
+            .filter(
+              Utils.notUndefined.bind(mapSyntaxNodeToExpression),
+            ) as EField[]) ?? [];
         addTime("recordExpr", performance.now() - start);
         return recordExpr;
       }


Test patch:
diff --git a/.github/workflows/test-performance.yaml b/.github/workflows/test-performance.yaml
index cdb7e70..dde18fa 100644
--- a/.github/workflows/test-performance.yaml
+++ b/.github/workflows/test-performance.yaml
@@ -1,12 +1,6 @@
 name: Test Performance
 
-on:
-  push:
-    branches:
-      - master
-  pull_request:
-    branches:
-      - master
+on: [push, pull_request]
 
 jobs:
   build:
diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml
index cd25f68..77a8c69 100644
--- a/.github/workflows/test.yaml
+++ b/.github/workflows/test.yaml
@@ -1,12 +1,6 @@
 name: Lint and test
 
-on:
-  push:
-    branches:
-      - master
-  pull_request:
-    branches:
-      - master
+on: [push, pull_request]
 
 jobs:
   build:
diff --git a/test/completionProvider.test.ts b/test/completionProvider.test.ts
index 374f3e6..0fcfdeb 100644
--- a/test/completionProvider.test.ts
+++ b/test/completionProvider.test.ts
@@ -292,9 +292,8 @@ type alias Model =
 view : Model -> Model
 view model =
   let
-    var : String
     var = 
-      model.{-caret-}
+      model.p{-caret-}
 
   in
     model
@@ -1338,4 +1337,18 @@ func model =
 
     await testCompletions(source, ["field1", "field2"], "exactMatch");
   });
+
+  it("Record completions in function params", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+type Maybe a = Just { prop1: String, prop2: Int } | Nothing
+
+func model =
+    Just { p{-caret-} }
+`;
+
+    await testCompletions(source, ["prop1", "prop2"], "exactMatch");
+  });
 });
diff --git a/test/definitionProviderTests/importDefinition.test.ts b/test/definitionProviderTests/importDefinition.test.ts
index 5199593..199a863 100644
--- a/test/definitionProviderTests/importDefinition.test.ts
+++ b/test/definitionProviderTests/importDefinition.test.ts
@@ -472,6 +472,27 @@ bar = 42
 module FooExtra exposing (..)
 quux = 99
 --X
+`;
+    await testBase.testDefinition(source);
+  });
+
+  it(`test union constructor resolves to the exposed one`, async () => {
+    const source = `
+--@ main.elm
+import App exposing (Page)
+import OtherApp exposing (Page2(..))
+
+func = Home
+       --^OtherApp.elm
+
+--@ App.elm
+module App exposing (..)
+type Page = Home
+
+--@ OtherApp.elm
+module OtherApp exposing (..)
+type Page2 = Home
+            --X
 `;
     await testBase.testDefinition(source);
   });