Repository: elm-tooling/elm-language-server
Pull request #380 (2020-11-28 05:38:41)

Add import resolve diagnostics and clean up API
Some major refactoring of the type checker api for better handling diagnostics with an async option and allowing us to calculate other diagnostics like import resolves.


Patch:
diff --git a/src/cancellation.ts b/src/cancellation.ts
index 7c05a28..1005721 100644
--- a/src/cancellation.ts
+++ b/src/cancellation.ts
@@ -187,6 +187,20 @@ export interface ICancellationToken {
   throwIfCancellationRequested(): void;
 }
 
+export class ServerCancellationToken implements ICancellationToken {
+  constructor(private cancellationToken: CancellationToken) {}
+
+  public isCancellationRequested(): boolean {
+    return this.cancellationToken.isCancellationRequested;
+  }
+
+  public throwIfCancellationRequested(): void {
+    if (this.isCancellationRequested()) {
+      throw new OperationCanceledException();
+    }
+  }
+}
+
 /**
  * ThrottledCancellationToken taken from Typescript: https://github.com/microsoft/TypeScript/blob/79ffd03f8b73010fa03cef624e5f1770bc9c975b/src/services/services.ts#L1152
  */
diff --git a/src/forest.ts b/src/forest.ts
index 211bddf..8949000 100644
--- a/src/forest.ts
+++ b/src/forest.ts
@@ -147,10 +147,7 @@ export class Forest implements IForest {
 
     // It should be faster to look directly at elmFolders instead of traversing the forest
     importClauses.forEach((importClause) => {
-      const moduleName = TreeUtils.findFirstNamedChildOfType(
-        "upper_case_qid",
-        importClause,
-      )?.text;
+      const moduleName = importClause.childForFieldName("moduleName")?.text;
 
       if (moduleName) {
         const modulePath = moduleName.split(".").join("/") + ".elm";
@@ -163,10 +160,7 @@ export class Forest implements IForest {
           ),
         );
 
-        if (treeContainer.writeable && !found) {
-          // TODO: Diagnostics for unresolved imports
-          console.log(`Could not resolve import for ${moduleName}`);
-        } else if (found) {
+        if (found) {
           resolvedModules.set(
             moduleName,
             URI.file(
diff --git a/src/providers/diagnostics/diagnosticsProvider.ts b/src/providers/diagnostics/diagnosticsProvider.ts
index d86a35a..6a41418 100644
--- a/src/providers/diagnostics/diagnosticsProvider.ts
+++ b/src/providers/diagnostics/diagnosticsProvider.ts
@@ -147,18 +147,20 @@ export class DiagnosticsProvider {
 
     astProvider.onTreeChange(({ treeContainer, declaration }) => {
       if (!clientInitiatedDiagnostics) {
-        if (this.pendingRequest) {
-          this.pendingRequest.cancel();
-          this.pendingRequest = undefined;
-        }
-
         this.requestDiagnostics(treeContainer.uri);
       }
     });
 
     this.documentEvents.on("change", () => {
       this.change();
-      this.typeInferenceDiagnostics.change();
+
+      // We need to cancel the request as soon as possible
+      if (!clientInitiatedDiagnostics) {
+        if (this.pendingRequest) {
+          this.pendingRequest.cancel();
+          this.pendingRequest = undefined;
+        }
+      }
     });
   }
 
@@ -307,41 +309,34 @@ export class DiagnosticsProvider {
               return;
             }
 
-            this.typeInferenceDiagnostics
-              .getDiagnosticsForFile(
+            next.promise(async () => {
+              const diagnostics = await this.typeInferenceDiagnostics.getDiagnosticsForFileAsync(
                 treeContainer,
                 workspace,
                 cancellationToken,
-              )
-              .then((diagnostics) => {
-                if (this.changeSeq !== seq) {
-                  return;
-                }
-
-                this.updateDiagnostics(
-                  uri,
-                  DiagnosticKind.TypeInference,
-                  diagnostics,
-                );
-              })
-              .catch(() => {
-                // Cancelled
-              });
+              );
 
-            if (this.changeSeq !== seq) {
-              return;
-            }
+              if (this.changeSeq !== seq) {
+                return;
+              }
 
-            next.immediate(() => {
               this.updateDiagnostics(
                 uri,
-                DiagnosticKind.ElmLS,
-                this.elmLsDiagnostics.createDiagnostics(
-                  treeContainer,
-                  workspace,
-                ),
+                DiagnosticKind.TypeInference,
+                diagnostics,
               );
-              goNext();
+
+              next.immediate(() => {
+                this.updateDiagnostics(
+                  uri,
+                  DiagnosticKind.ElmLS,
+                  this.elmLsDiagnostics.createDiagnostics(
+                    treeContainer,
+                    workspace,
+                  ),
+                );
+                goNext();
+              });
             });
           };
 
diff --git a/src/providers/diagnostics/typeInferenceDiagnostics.ts b/src/providers/diagnostics/typeInferenceDiagnostics.ts
index 5e6c887..149b869 100644
--- a/src/providers/diagnostics/typeInferenceDiagnostics.ts
+++ b/src/providers/diagnostics/typeInferenceDiagnostics.ts
@@ -1,49 +1,18 @@
 /* eslint-disable @typescript-eslint/no-use-before-define */
-import {
-  CancellationToken,
-  CodeAction,
-  CodeActionKind,
-  CodeActionParams,
-  Diagnostic,
-  DiagnosticSeverity,
-  Range,
-  TextEdit,
-} from "vscode-languageserver";
+import { CancellationToken, Diagnostic } from "vscode-languageserver";
 import { SyntaxNode } from "web-tree-sitter";
-import { PositionUtil } from "../../positionUtil";
 import { TreeUtils } from "../../util/treeUtils";
-import { URI } from "vscode-uri";
-import { ElmWorkspaceMatcher } from "../../util/elmWorkspaceMatcher";
 import { mapSyntaxNodeToExpression } from "../../util/types/expressionTree";
-import { MultistepOperation } from "../../util/multistepOperation";
 import { TypeChecker } from "../../util/types/typeChecker";
 import { ITreeContainer } from "../../forest";
 import { IElmWorkspace } from "../../elmWorkspace";
-import {
-  ICancellationToken,
-  ThrottledCancellationToken,
-} from "../../cancellation";
-import { container } from "tsyringe";
+import { ServerCancellationToken } from "../../cancellation";
 import { Diagnostics, error } from "../../util/types/diagnostics";
 
 export class TypeInferenceDiagnostics {
   TYPE_INFERENCE = "Type Inference";
 
-  private elmWorkspaceMatcher: ElmWorkspaceMatcher<URI>;
-  private changeSeq = 0;
-
-  private operation: MultistepOperation;
-
-  constructor() {
-    this.elmWorkspaceMatcher = new ElmWorkspaceMatcher((uri) => uri);
-    this.operation = new MultistepOperation(container.resolve("Connection"));
-  }
-
-  public change(): void {
-    this.changeSeq++;
-  }
-
-  public getDiagnosticsForFile(
+  public getDiagnosticsForFileAsync(
     treeContainer: ITreeContainer,
     elmWorkspace: IElmWorkspace,
     cancellationToken: CancellationToken,
@@ -58,45 +27,28 @@ export class TypeInferenceDiagnostics {
       TreeUtils.findAllTopLevelFunctionDeclarations(treeContainer.tree) ?? [];
 
     return new Promise((resolve, reject) => {
-      this.operation.startNew(
-        cancellationToken,
-        (next) => {
-          const seq = this.changeSeq;
-
-          let index = 0;
-          const goNext = (): void => {
-            index++;
-            if (allTopLevelFunctions.length > index) {
-              next.immediate(checkOne);
-            }
-          };
-
-          const checkOne = (): void => {
-            if (this.changeSeq !== seq) {
-              reject();
-            }
-
+      checker
+        .getDiagnosticsAsync(
+          treeContainer,
+          new ServerCancellationToken(cancellationToken),
+        )
+        .then((diag) => {
+          diagnostics.push(...diag);
+
+          // Get other custom diagnostics
+          allTopLevelFunctions.forEach((declaration) => {
             diagnostics.push(
               ...this.getDiagnosticsForDeclaration(
                 checker,
-                allTopLevelFunctions[index],
+                declaration,
                 treeContainer,
-                new ThrottledCancellationToken(cancellationToken),
               ),
             );
+          });
 
-            goNext();
-          };
-
-          if (allTopLevelFunctions.length > 0 && this.changeSeq === seq) {
-            next.immediate(checkOne);
-          }
-        },
-        () => {
           resolve(diagnostics);
-        },
-        reject,
-      );
+        })
+        .catch(reject);
     });
   }
 
@@ -104,7 +56,6 @@ export class TypeInferenceDiagnostics {
     checker: TypeChecker,
     declaration: SyntaxNode,
     treeContainer: ITreeContainer,
-    cancellationToken: ICancellationToken,
   ): Diagnostic[] {
     const valueDeclaration = mapSyntaxNodeToExpression(declaration);
     if (valueDeclaration?.nodeType !== "ValueDeclaration") {
@@ -113,16 +64,6 @@ export class TypeInferenceDiagnostics {
 
     const diagnostics: Diagnostic[] = [];
 
-    checker
-      .getDiagnosticsFromDeclaration(valueDeclaration, cancellationToken)
-      .forEach((diagnostic) => {
-        const nodeUri = (<any>diagnostic.data).uri;
-
-        if (nodeUri === treeContainer.uri) {
-          diagnostics.push(diagnostic);
-        }
-      });
-
     if (!valueDeclaration.typeAnnotation) {
       const typeString: string = checker.typeToString(
         checker.findType(declaration),
diff --git a/src/util/multistepOperation.ts b/src/util/multistepOperation.ts
index a5a4d25..69f2a2c 100644
--- a/src/util/multistepOperation.ts
+++ b/src/util/multistepOperation.ts
@@ -8,14 +8,15 @@ import { OperationCanceledException } from "../cancellation";
 interface INextStep {
   immediate(action: () => void): void;
   delay(ms: number, action: () => void): void;
+  promise(action: () => Promise<void>): void;
 }
 
 export class MultistepOperation implements INextStep {
   private timerHandle: NodeJS.Timeout | undefined;
   private immediateId: NodeJS.Immediate | undefined;
+  private savedPromise: Promise<void> | undefined;
   private cancellationToken: CancellationToken | undefined;
   private done: (() => void) | undefined;
-  private cancelled: (() => void) | undefined;
 
   constructor(private connection: Connection) {}
 
@@ -23,12 +24,10 @@ export class MultistepOperation implements INextStep {
     cancellationToken: CancellationToken,
     action: (next: INextStep) => void,
     done: () => void,
-    cancelled?: () => void,
   ): void {
     this.complete();
     this.cancellationToken = cancellationToken;
     this.done = done;
-    this.cancelled = cancelled;
     this.executeAction(action);
   }
 
@@ -41,6 +40,7 @@ export class MultistepOperation implements INextStep {
     this.cancellationToken = undefined;
     this.setTimerHandle(undefined);
     this.setImmediateId(undefined);
+    this.savedPromise = undefined;
   }
 
   public immediate(action: () => void): void {
@@ -61,13 +61,30 @@ export class MultistepOperation implements INextStep {
     );
   }
 
+  public promise(action: () => Promise<void>): void {
+    if (this.cancellationToken?.isCancellationRequested) {
+      this.complete();
+    } else {
+      this.savedPromise = action()
+        .then(() => {
+          if (!this.hasPendingWork()) {
+            this.complete();
+          }
+        })
+        .catch(() => {
+          this.complete();
+        })
+        .finally(() => {
+          this.savedPromise = undefined;
+        });
+    }
+  }
+
   private executeAction(action: (next: INextStep) => void): void {
     let stop = false;
-    let cancelled = false;
     try {
       if (this.cancellationToken?.isCancellationRequested) {
         stop = true;
-        cancelled = true;
       } else {
         action(this);
       }
@@ -78,13 +95,6 @@ export class MultistepOperation implements INextStep {
       }
 
       stop = true;
-      cancelled = true;
-    }
-
-    if (cancelled && this.cancelled) {
-      this.cancelled();
-      this.cancelled = undefined;
-      return;
     }
 
     if (stop || !this.hasPendingWork()) {
@@ -107,6 +117,6 @@ export class MultistepOperation implements INextStep {
   }
 
   private hasPendingWork(): boolean {
-    return !!this.timerHandle || !!this.immediateId;
+    return !!this.timerHandle || !!this.immediateId || !!this.savedPromise;
   }
 }
diff --git a/src/util/types/diagnostics.ts b/src/util/types/diagnostics.ts
index 45c90f7..f7c51f9 100644
--- a/src/util/types/diagnostics.ts
+++ b/src/util/types/diagnostics.ts
@@ -66,6 +66,11 @@ export const Diagnostics = {
     DiagnosticSeverity.Error,
   ),
   General: diag("general", "General error: {0}", DiagnosticSeverity.Error),
+  ImportMissing: diag(
+    "import_resolve",
+    "Could not find a module to import named `{0}` in `dependencies` or `source-directories`.",
+    DiagnosticSeverity.Error,
+  ),
   InfiniteRecursion: diag(
     "infinite_recursion",
     "Infinite recursion.",
diff --git a/src/util/types/typeChecker.ts b/src/util/types/typeChecker.ts
index 350f985..4b20711 100644
--- a/src/util/types/typeChecker.ts
+++ b/src/util/types/typeChecker.ts
@@ -25,6 +25,7 @@ import { Sequence } from "../sequence";
 import { Utils } from "../utils";
 import { TypeExpression } from "./typeExpression";
 import { ICancellationToken } from "../../cancellation";
+import { Diagnostics, error } from "./diagnostics";
 
 export let bindTime = 0;
 export function resetBindTime(): void {
@@ -37,6 +38,25 @@ export interface DefinitionResult {
   nodeType: NodeType;
 }
 
+class DiagnosticsCollection extends Map<string, Diagnostic[]> {
+  public add(diagnostic: Diagnostic): void {
+    const uri = (<any>diagnostic.data).uri;
+
+    let diagnostics = super.get(uri);
+
+    if (!diagnostics) {
+      diagnostics = [];
+      super.set(uri, diagnostics);
+    }
+
+    diagnostics.push(diagnostic);
+  }
+
+  public get(uri: string): Diagnostic[] {
+    return super.get(uri) ?? [];
+  }
+}
+
 export interface TypeChecker {
   findType: (node: SyntaxNode) => Type;
   findDefinition: (
@@ -58,10 +78,11 @@ export interface TypeChecker {
     treeContainer: ITreeContainer,
     cancellationToken?: ICancellationToken,
   ) => Diagnostic[];
-  getDiagnosticsFromDeclaration: (
-    valueDeclaration: SyntaxNode,
-    cancellationToken?: ICancellationToken,
-  ) => Diagnostic[];
+  getDiagnosticsAsync: (
+    treeContainer: ITreeContainer,
+    token?: ICancellationToken,
+    cancelCallback?: () => boolean,
+  ) => Promise<Diagnostic[]>;
   findImportModuleNameNode: (
     moduleNameOrAlias: string,
     treeContainer: ITreeContainer,
@@ -72,6 +93,9 @@ export function createTypeChecker(workspace: IElmWorkspace): TypeChecker {
   const forest = workspace.getForest();
   const imports = new Map<string, Imports>();
 
+  const diagnostics = new DiagnosticsCollection();
+  let cancellationToken: ICancellationToken | undefined;
+
   const start = performance.now();
   forest.treeMap.forEach((treeContainer) => {
     bindTreeContainer(treeContainer);
@@ -86,7 +110,7 @@ export function createTypeChecker(workspace: IElmWorkspace): TypeChecker {
     getQualifierForName,
     typeToString,
     getDiagnostics,
-    getDiagnosticsFromDeclaration,
+    getDiagnosticsAsync,
     findImportModuleNameNode,
   };
 
@@ -213,45 +237,59 @@ export function createTypeChecker(workspace: IElmWorkspace): TypeChecker {
 
   function getDiagnostics(
     treeContainer: ITreeContainer,
-    cancellationToken?: ICancellationToken,
+    token?: ICancellationToken,
   ): Diagnostic[] {
-    const allTopLevelFunctions = TreeUtils.findAllTopLevelFunctionDeclarations(
-      treeContainer.tree,
-    );
+    try {
+      cancellationToken = token;
 
-    return (
-      allTopLevelFunctions
-        ?.map((valueDeclaration) => {
-          return getDiagnosticsFromDeclaration(
-            valueDeclaration,
-            cancellationToken,
-          );
-        })
-        .reduce((a, b) => a.concat(b), [])
-        .filter(
-          // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-          (diagnostic) => (<any>diagnostic.data).uri === treeContainer.uri,
-        ) ?? []
-    );
-  }
+      checkNode(treeContainer.tree.rootNode);
 
-  function getDiagnosticsFromDeclaration(
-    valueDeclaration: SyntaxNode,
-    cancellationToken?: ICancellationToken,
-  ): Diagnostic[] {
-    if (valueDeclaration.type !== "value_declaration") {
-      throw new Error(
-        "getDiagnosticsFromDeclaration must take a node of type value_declaraion",
-      );
+      return diagnostics.get(treeContainer.uri);
+    } finally {
+      cancellationToken = undefined;
     }
+  }
 
-    return InferenceScope.valueDeclarationInference(
-      mapSyntaxNodeToExpression(valueDeclaration) as EValueDeclaration,
-      valueDeclaration.tree.uri,
-      workspace,
-      new Set(),
-      cancellationToken,
-    ).diagnostics;
+  function getDiagnosticsAsync(
+    treeContainer: ITreeContainer,
+    token?: ICancellationToken,
+    cancelCallback?: () => boolean,
+  ): Promise<Diagnostic[]> {
+    cancellationToken = token;
+
+    return new Promise((resolve, reject) => {
+      const children = treeContainer.tree.rootNode.children;
+      let index = 0;
+
+      const goNext = (): void => {
+        index++;
+        if (children.length > index) {
+          setImmediate(checkOne);
+        } else {
+          cancellationToken = undefined;
+          resolve(diagnostics.get(treeContainer.uri));
+        }
+      };
+
+      const checkOne = (): void => {
+        if (cancelCallback && cancelCallback()) {
+          reject();
+          return;
+        }
+
+        try {
+          checkNode(children[index]);
+        } catch {
+          cancellationToken = undefined;
+          reject();
+          return;
+        }
+
+        goNext();
+      };
+
+      checkOne();
+    });
   }
 
   function getAllImports(treeContainer: ITreeContainer): Imports {
@@ -794,4 +832,43 @@ export function createTypeChecker(workspace: IElmWorkspace): TypeChecker {
         ?.node.childForFieldName("moduleName") ?? undefined
     );
   }
+
+  function checkNode(node: SyntaxNode): void {
+    cancellationToken?.throwIfCancellationRequested();
+
+    switch (node.type) {
+      case "file":
+        node.children.forEach(checkNode);
+        break;
+      case "value_declaration":
+        checkValueDeclaration(node);
+        break;
+      case "import_clause":
+        checkImportClause(node);
+        break;
+    }
+  }
+
+  function checkValueDeclaration(valueDeclaration: SyntaxNode): void {
+    InferenceScope.valueDeclarationInference(
+      mapSyntaxNodeToExpression(valueDeclaration) as EValueDeclaration,
+      valueDeclaration.tree.uri,
+      workspace,
+      new Set(),
+      cancellationToken,
+    ).diagnostics.forEach((diagnostic) => diagnostics.add(diagnostic));
+  }
+
+  function checkImportClause(importClause: SyntaxNode): void {
+    const moduleNameNode = importClause.childForFieldName("moduleName");
+
+    if (moduleNameNode) {
+      const moduleName = moduleNameNode.text;
+      if (!workspace.getForest().getByModuleName(moduleName)) {
+        diagnostics.add(
+          error(moduleNameNode, Diagnostics.ImportMissing, moduleName),
+        );
+      }
+    }
+  }
 }


Test patch:
diff --git a/test/diagnosticTests/elmDiagnostics.test.ts b/test/diagnosticTests/elmDiagnostics.test.ts
index 932b30f..aff2d5d 100644
--- a/test/diagnosticTests/elmDiagnostics.test.ts
+++ b/test/diagnosticTests/elmDiagnostics.test.ts
@@ -1,8 +1,14 @@
 import { debug } from "console";
 import { Diagnostic } from "vscode-languageserver";
 import { URI } from "vscode-uri";
+import { SyntaxNode } from "web-tree-sitter";
 import { diagnosticsEquals } from "../../src/providers/diagnostics/fileDiagnostics";
 import { TreeUtils } from "../../src/util/treeUtils";
+import {
+  Diagnostics,
+  error,
+  IDiagnosticMessage,
+} from "../../src/util/types/diagnostics";
 import { Utils } from "../../src/util/utils";
 import { baseUri } from "../utils/mockElmWorkspace";
 import {
@@ -46,19 +52,24 @@ describe("test elm diagnostics", () => {
 
   async function testTypeInference(
     source: string,
-    expectedDiagnostics: Diagnostic[],
+    expectedDiagnostics: {
+      message: IDiagnosticMessage;
+      args: (string | number)[];
+    }[],
   ) {
     await treeParser.init();
 
-    const sources = getSourceFiles(source);
+    const result = getTargetPositionFromSource(source) ?? {
+      sources: getSourceFiles(source),
+    };
 
-    if (!sources) {
+    if (!result) {
       throw new Error("Getting sources failed");
     }
 
     const testUri = URI.file(baseUri + "Test.elm").toString();
 
-    const workspace = treeParser.getWorkspace(sources);
+    const workspace = treeParser.getWorkspace(result.sources);
     const treeContainer = workspace.getForest().getByUri(testUri);
 
     if (!treeContainer) throw new Error("Getting tree failed");
@@ -72,9 +83,24 @@ describe("test elm diagnostics", () => {
       }
     });
 
+    let nodeAtPosition: SyntaxNode;
+
+    if ("position" in result) {
+      const rootNode = workspace.getForest().treeMap.get(testUri)!.tree
+        .rootNode;
+      nodeAtPosition = TreeUtils.getNamedDescendantForPosition(
+        rootNode,
+        result.position,
+      );
+    }
+
+    const expected = expectedDiagnostics.map((exp) =>
+      error(nodeAtPosition, exp.message, ...exp.args),
+    );
+
     const diagnosticsEqual = Utils.arrayEquals(
       diagnostics,
-      expectedDiagnostics,
+      expected,
       diagnosticsEquals,
     );
 
@@ -113,4 +139,19 @@ concat comparators a b =
 `;
     await testTypeInference(basicsSources + source, []);
   });
+
+  test("missing import", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import App
+      --^
+
+func = ""
+`;
+    await testTypeInference(basicsSources + source, [
+      { message: Diagnostics.ImportMissing, args: ["App"] },
+    ]);
+  });
 });