Repository: elm-tooling/elm-language-server
Pull request #299 (2020-07-12 02:29:53)

Better completions context
This PR begins to address the that problem that `nodeAtPosition` is often determined to be very different nodes based on similar positions. For example `var.|` and `var.pr|` can product vastly different results (lower level node vs higher level) and we have a lot of specific pattern matching to try to catch all of these cases (and the result is ugly). There are also many rarer variations that can occur (like in deeper nested statements) that we don't cover, and completions don't work there.

To solve this, I took heavy inspiration from the Typescript completion provider and introduce a new `contextNode` and a `TreeUtils.findPreviousNode()` to use instead of the existing `nodeAtPosition`. `contextNode` is the node right before the caret (but is also adjusted when typing after a dot). 

We can continue to use both nodes and transition some completions to use the new `contextNode` when and where it makes sense. Currently I am mostly just using it for completions that come after a dot. Doing this means I am able to consistently get the node we want completions on (before the dot, Ex: `Module.v|` -> `Module`, `model.|` -> `model`) no matter the surrounding code (I was impressed with the testing results of this). This also means that we can get rid of a lot of the ugly node type matching code, and use a more consistent, unified approach. This should also help get better results with some of those edge cases. 

Razzeee: This probably superseeds what I tried to do with https://github.com/elm-tooling/elm-language-server/commit/a05078889a9036fb49e90b07292d6dd68f8a8a33#diff-21245dcc42cbe9ffe996a08b05568766R19
Razzeee: Looks good otherwise
jmbockhorst: Thanks, should be good now.

Patch:
diff --git a/src/positionUtil.ts b/src/positionUtil.ts
index e8dd144..6751e72 100644
--- a/src/positionUtil.ts
+++ b/src/positionUtil.ts
@@ -29,3 +29,18 @@ export class PositionUtil {
     };
   }
 }
+
+export function comparePosition(pos1: VSPosition, pos2: TSPosition): number {
+  if (pos1.line === pos2.row && pos1.character === pos2.column) {
+    return 0;
+  }
+
+  if (
+    pos1.line < pos2.row ||
+    (pos1.line === pos2.row && pos1.character < pos2.column)
+  ) {
+    return -1;
+  }
+
+  return 1;
+}
diff --git a/src/providers/completionProvider.ts b/src/providers/completionProvider.ts
index da82d72..5cf6431 100644
--- a/src/providers/completionProvider.ts
+++ b/src/providers/completionProvider.ts
@@ -23,6 +23,7 @@ import RANKING_LIST from "./ranking";
 import { ImportUtils } from "../util/importUtils";
 import { RefactorEditUtils } from "../util/refactorEditUtils";
 import { Utils } from "../util/utils";
+import { comparePosition, PositionUtil } from "../positionUtil";
 
 export type CompletionResult =
   | CompletionItem[]
@@ -88,7 +89,7 @@ export class CompletionProvider {
         currentCharacter--;
       }
 
-      const replaceRange = Range.create(
+      let replaceRange = Range.create(
         Position.create(params.position.line, currentCharacter),
         params.position,
       );
@@ -97,11 +98,32 @@ export class CompletionProvider {
 
       const isAtStartOfLine = replaceRange.start.character === 0;
 
-      const targetWord = targetLine.substring(
+      let targetWord = targetLine.substring(
         replaceRange.start.character,
         replaceRange.end.character,
       );
 
+      let contextNode = TreeUtils.findPreviousNode(
+        tree.rootNode,
+        params.position,
+      );
+
+      // If we are in a partial identifier, skip that and adjust the contextNode to be the previous node
+      if (
+        contextNode &&
+        comparePosition(params.position, contextNode.endPosition) <= 0 &&
+        TreeUtils.isIdentifier(contextNode)
+      ) {
+        contextNode = TreeUtils.findPreviousNode(
+          tree.rootNode,
+          PositionUtil.FROM_TS_POSITION(
+            contextNode.startPosition,
+          ).toVSPosition(),
+        );
+      }
+
+      const isAfterDot = contextNode?.type === "dot";
+
       if (
         TreeUtils.findParentOfType("block_comment", nodeAtPosition) ||
         TreeUtils.findParentOfType("line_comment", nodeAtPosition)
@@ -210,8 +232,6 @@ export class CompletionProvider {
           nodeAtPosition.type === "right_parenthesis")
       ) {
         return this.getSameFileTopLevelCompletions(tree, replaceRange, true);
-      } else if (previousWord && previousWord === "import") {
-        return this.getImportableModules(forest, replaceRange);
       } else if (
         nodeAtPosition.parent?.type === "exposing_list" &&
         nodeAtPosition.parent.parent?.type === "import_clause" &&
@@ -245,98 +265,73 @@ export class CompletionProvider {
           params.textDocument.uri,
           forest,
         );
-      } else if (
-        nodeAtPosition.type === "field_access_segment" ||
-        nodeAtPosition.parent?.type === "field_access_segment" ||
-        RegExp("^[a-z]+.*.$").exec(targetWord)
-      ) {
-        let accessSegmentNode =
-          nodeAtPosition.type === "field_access_segment"
-            ? nodeAtPosition
-            : nodeAtPosition.parent?.type === "field_access_segment"
-            ? nodeAtPosition.parent
-            : undefined;
-
-        if (!accessSegmentNode) {
-          accessSegmentNode =
-            TreeUtils.getNamedDescendantForPosition(tree.rootNode, {
-              ...params.position,
-              character: params.position.character - 1,
-            }).parent ?? undefined;
-        }
+      }
 
-        if (accessSegmentNode) {
-          let dotIndex = TreeUtils.findFirstNamedChildOfType(
-            "dot",
-            accessSegmentNode,
-          )?.startPosition.column;
+      let targetNode;
 
-          if (!dotIndex) {
-            const wordDot = targetWord.lastIndexOf(".");
+      if (contextNode) {
+        const parent = contextNode.parent;
+        if (isAfterDot) {
+          targetWord = targetLine.substring(
+            replaceRange.start.character,
+            contextNode.startPosition.column,
+          );
 
-            if (wordDot >= 0) {
-              dotIndex =
-                params.position.character - (targetWord.length - wordDot);
-            }
-          }
+          replaceRange = Range.create(
+            Position.create(
+              params.position.line,
+              contextNode.startPosition.column + 1,
+            ),
+            params.position,
+          );
 
-          if (dotIndex) {
-            return this.getRecordCompletions(
-              accessSegmentNode,
-              tree,
-              Range.create(
-                Position.create(params.position.line, dotIndex + 1),
-                params.position,
-              ),
-              elmWorkspace.getImports(),
-              params.textDocument.uri,
-              forest,
+          if (parent?.type === "value_qid") {
+            // Qualified submodule and value access
+            targetNode = contextNode.previousNamedSibling;
+          } else if (parent?.type === "field_access_segment") {
+            // Record field access
+            targetNode =
+              parent?.previousNamedSibling?.lastNamedChild?.lastNamedChild ??
+              parent.previousNamedSibling?.lastNamedChild;
+          } else if (parent?.type === "upper_case_qid") {
+            // Imports
+            targetNode = contextNode.previousNamedSibling;
+          } else if (parent?.type === "ERROR") {
+            targetNode = TreeUtils.findPreviousNode(
+              tree.rootNode,
+              PositionUtil.FROM_TS_POSITION(
+                contextNode.startPosition,
+              ).toVSPosition(),
             );
           }
+        } else {
+          if (contextNode.type === "import") {
+            return this.getImportableModules(tree, forest, replaceRange);
+          }
         }
-      } else if (
-        (nodeAtPosition.parent?.type === "value_qid" &&
-          nodeAtPosition.previousNamedSibling?.type === "dot") ||
-        RegExp("^[A-Z]+.*.$").exec(targetWord) // Handles the 'Html.Attributes.' case
-      ) {
-        let targetNode = nodeAtPosition.parent;
-
-        if (targetNode?.type !== "value_qid") {
-          targetNode = TreeUtils.getNamedDescendantForPosition(tree.rootNode, {
-            ...params.position,
-            character: params.position.character - 1,
-          }).parent;
-        }
-
-        if (targetNode) {
-          const dotNodes = TreeUtils.findAllNamedChildrenOfType(
-            "dot",
-            targetNode,
-          );
+      }
 
-          // Get the last dot index
-          const dotIndex =
-            dotNodes && dotNodes.length > 0
-              ? dotNodes[dotNodes.length - 1].startPosition.column
-              : targetWord.endsWith(".")
-              ? params.position.character - 1
-              : undefined;
+      if (targetNode) {
+        const moduleCompletions = this.getSubmodulesOrValues(
+          targetNode,
+          params.textDocument.uri,
+          tree,
+          elmWorkspace.getImports(),
+          forest,
+          replaceRange,
+          targetWord,
+        );
 
-          if (dotIndex) {
-            return this.getSubmodulesOrValues(
+        return moduleCompletions.length > 0
+          ? moduleCompletions
+          : this.getRecordCompletions(
               targetNode,
-              params.textDocument.uri,
               tree,
+              replaceRange,
               elmWorkspace.getImports(),
+              params.textDocument.uri,
               forest,
-              Range.create(
-                Position.create(params.position.line, dotIndex + 1),
-                params.position,
-              ),
-              targetLine.substring(replaceRange.start.character, dotIndex),
             );
-          }
-        }
       }
 
       completions.push(
@@ -391,19 +386,34 @@ export class CompletionProvider {
   }
 
   private getImportableModules(
+    tree: Tree,
     forest: IForest,
     range: Range,
+    targetModule?: string,
   ): CompletionItem[] {
+    const currentModuleNameNode = TreeUtils.getModuleNameNode(tree);
     return forest.treeIndex
-      .map((a) => a.moduleName)
-      .filter(Utils.notUndefined)
-      .map((label) =>
-        this.createModuleCompletion({
-          label,
-          range,
+      .filter(
+        (t) =>
+          t.moduleName &&
+          (!targetModule || t.moduleName?.startsWith(targetModule + ".")) &&
+          t.moduleName !== currentModuleNameNode?.text &&
+          t.moduleName !== targetModule,
+      )
+      .map((t) => {
+        const moduleNode = TreeUtils.findModuleDeclaration(t.tree);
+        const markdownDocumentation = HintHelper.createHint(moduleNode);
+
+        return this.createModuleCompletion({
+          label:
+            (targetModule
+              ? t.moduleName?.slice(targetModule.length + 1) ?? t.moduleName
+              : t.moduleName) ?? "",
           sortPrefix: "b",
-        }),
-      );
+          range,
+          markdownDocumentation,
+        });
+      });
   }
 
   private getExposedFromModule(
@@ -1032,101 +1042,79 @@ export class CompletionProvider {
     const result: CompletionItem[] = [];
 
     // Handle possible submodules
-    forest.treeIndex
-      .filter(
-        (t) =>
-          t.moduleName?.startsWith(targetModule + ".") &&
-          t.moduleName !== targetModule,
-      )
-      .forEach((t) => {
-        const moduleNode = TreeUtils.findModuleDeclaration(t.tree);
-        if (t.moduleName) {
-          const markdownDocumentation = HintHelper.createHint(moduleNode);
-
-          result.push(
-            this.createModuleCompletion({
-              label: t.moduleName.slice(targetModule.length + 1),
-              sortPrefix: "b",
-              range,
-              markdownDocumentation,
-            }),
-          );
-        }
-      });
-
-    const moduleNodes = TreeUtils.findAllNamedChildrenOfType(
-      "upper_case_identifier",
-      node,
-    )?.filter((node) => node.text !== "");
+    result.push(
+      ...this.getImportableModules(tree, forest, range, targetModule),
+    );
 
-    if (moduleNodes && moduleNodes.length > 0) {
-      const moduleNode = moduleNodes[moduleNodes.length - 1];
+    // If we are in an import completion, don't return any values
+    if (TreeUtils.isImport(node)) {
+      return result;
+    }
 
-      let alreadyImported = true;
+    let alreadyImported = true;
 
-      // Try to find the module definition that is already imported
-      const definitionNode = TreeUtils.findDefinitionNodeByReferencingNode(
-        moduleNode,
-        uri,
-        tree,
-        imports,
-      );
+    // Try to find the module definition that is already imported
+    const definitionNode = TreeUtils.findDefinitionNodeByReferencingNode(
+      node,
+      uri,
+      tree,
+      imports,
+    );
 
-      let moduleTree: ITreeContainer | undefined;
+    let moduleTree: ITreeContainer | undefined;
 
-      if (definitionNode && definitionNode.nodeType === "Module") {
-        moduleTree = forest.getByUri(definitionNode.uri);
-      } else {
-        // Try to find this module in the forest to import
-        moduleTree = forest.getByModuleName(targetModule);
-        alreadyImported = false;
-      }
+    if (definitionNode && definitionNode.nodeType === "Module") {
+      moduleTree = forest.getByUri(definitionNode.uri);
+    } else {
+      // Try to find this module in the forest to import
+      moduleTree = forest.getByModuleName(targetModule);
+      alreadyImported = false;
+    }
 
-      if (moduleTree) {
-        // Get exposed values
-        const imports = ImportUtils.getPossibleImportsOfTree(moduleTree);
-        imports.forEach((value) => {
-          const markdownDocumentation = HintHelper.createHint(value.node);
-          let additionalTextEdits: TextEdit[] | undefined;
-          let detail: string | undefined;
-
-          // Add the import text edit if not imported
-          if (!alreadyImported) {
-            const importEdit = RefactorEditUtils.addImport(tree, targetModule);
-
-            if (importEdit) {
-              additionalTextEdits = [importEdit];
-              detail = `Auto import module '${targetModule}'`;
-            }
+    if (moduleTree) {
+      // Get exposed values
+      const imports = ImportUtils.getPossibleImportsOfTree(moduleTree);
+      imports.forEach((value) => {
+        const markdownDocumentation = HintHelper.createHint(value.node);
+        let additionalTextEdits: TextEdit[] | undefined;
+        let detail: string | undefined;
+
+        // Add the import text edit if not imported
+        if (!alreadyImported) {
+          const importEdit = RefactorEditUtils.addImport(tree, targetModule);
+
+          if (importEdit) {
+            additionalTextEdits = [importEdit];
+            detail = `Auto import module '${targetModule}'`;
           }
+        }
 
-          const completionOptions: ICompletionOptions = {
-            label: value.value,
-            sortPrefix: "a",
-            range,
-            markdownDocumentation,
-            additionalTextEdits,
-            detail,
-          };
-
-          switch (value.type) {
-            case "Function":
-              result.push(this.createFunctionCompletion(completionOptions));
-              break;
-            case "Type":
-              result.push(this.createTypeCompletion(completionOptions));
-              break;
-            case "TypeAlias":
-              result.push(this.createTypeAliasCompletion(completionOptions));
-              break;
-            case "UnionConstructor":
-              result.push(
-                this.createUnionConstructorCompletion(completionOptions),
-              );
-              break;
-          }
-        });
-      }
+        const completionOptions: ICompletionOptions = {
+          label: value.value,
+          sortPrefix: "a",
+          range,
+          markdownDocumentation,
+          additionalTextEdits,
+          detail,
+        };
+
+        switch (value.type) {
+          case "Function":
+            result.push(this.createFunctionCompletion(completionOptions));
+            break;
+          case "Type":
+            result.push(this.createTypeCompletion(completionOptions));
+            break;
+          case "TypeAlias":
+            result.push(this.createTypeAliasCompletion(completionOptions));
+            break;
+          case "UnionConstructor":
+            result.push(
+              this.createUnionConstructorCompletion(completionOptions),
+            );
+            break;
+        }
+      });
     }
 
     return result;
diff --git a/src/util/treeUtils.ts b/src/util/treeUtils.ts
index 13fdb2f..1345842 100644
--- a/src/util/treeUtils.ts
+++ b/src/util/treeUtils.ts
@@ -3,6 +3,7 @@ import { SyntaxNode, Tree } from "web-tree-sitter";
 import { IImport, IImports } from "../imports";
 import { IForest } from "../forest";
 import { Utils } from "./utils";
+import { PositionUtil, comparePosition } from "../positionUtil";
 
 export type NodeType =
   | "Function"
@@ -1624,25 +1625,6 @@ export class TreeUtils {
           node.parent,
         );
 
-      if (!type) {
-        const valueNodes =
-          TreeUtils.findAllNamedChildrenOfType(
-            ["value_expr", "field_access_segment"],
-            node.parent,
-          ) ??
-          TreeUtils.findAllNamedChildrenOfType(
-            ["value_expr", "field_access_segment"],
-            node.parent.parent,
-          );
-
-        // We don't want the current access segment
-        valueNodes?.pop();
-
-        if (valueNodes?.length) {
-          type = valueNodes[valueNodes.length - 1].lastNamedChild;
-        }
-      }
-
       // Handle records of function returns
       if (!type && node.parent.parent.parent) {
         type =
@@ -1651,12 +1633,7 @@ export class TreeUtils {
           )?.parent ?? undefined;
       }
 
-      // Handle when the record and dot are not group, like the end of let expression
-      if (
-        !type &&
-        node.type === "value_qid" &&
-        node.parent.nextNamedSibling?.type === "dot"
-      ) {
+      if (!type) {
         type = node;
       }
 
@@ -1774,6 +1751,70 @@ export class TreeUtils {
     }
   }
 
+  public static findPreviousNode(
+    node: SyntaxNode,
+    position: Position,
+  ): SyntaxNode | undefined {
+    function nodeHasTokens(n: SyntaxNode): boolean {
+      return n.endIndex - n.startIndex !== 0;
+    }
+
+    function findRightmostChildWithTokens(
+      childrenList: SyntaxNode[],
+      startIndex: number,
+    ): SyntaxNode | undefined {
+      for (let i = startIndex - 1; i >= 0; i--) {
+        if (nodeHasTokens(childrenList[i])) {
+          return childrenList[i];
+        }
+      }
+    }
+
+    function findRightmostNode(n: SyntaxNode): SyntaxNode | undefined {
+      if (n.children.length === 0) {
+        return n;
+      }
+
+      const candidate = findRightmostChildWithTokens(
+        n.children,
+        n.children.length,
+      );
+
+      if (candidate) {
+        return findRightmostNode(candidate);
+      }
+    }
+
+    const children = node.children;
+
+    if (children.length === 0) {
+      return node;
+    }
+
+    for (let i = 0; i < children.length; i++) {
+      const child = children[i];
+      if (comparePosition(position, child.endPosition) < 0) {
+        const lookInPreviousChild =
+          comparePosition(position, child.startPosition) <= 0 ||
+          !nodeHasTokens(child);
+
+        if (lookInPreviousChild) {
+          const candidate = findRightmostChildWithTokens(children, i);
+          if (candidate) {
+            return findRightmostNode(candidate);
+          }
+        } else {
+          return this.findPreviousNode(child, position);
+        }
+      }
+    }
+
+    const candidate = findRightmostChildWithTokens(children, children.length);
+    if (candidate) {
+      return findRightmostNode(candidate);
+    }
+  }
+
   public static getNamedDescendantForLineBeforePosition(
     node: SyntaxNode,
     position: Position,
@@ -1868,4 +1909,18 @@ export class TreeUtils {
 
     return result.length === 0 ? undefined : result;
   }
+
+  public static isIdentifier(node: SyntaxNode): boolean {
+    return (
+      node.type === "lower_case_identifier" ||
+      node.type === "upper_case_identifier"
+    );
+  }
+
+  public static isImport(node: SyntaxNode): boolean {
+    return (
+      node.parent?.firstNamedChild?.type === "import" ||
+      node.parent?.parent?.firstNamedChild?.type === "import"
+    );
+  }
 }


Test patch:
diff --git a/test/completionProvider.test.ts b/test/completionProvider.test.ts
index 5825c90..3bc04fe 100644
--- a/test/completionProvider.test.ts
+++ b/test/completionProvider.test.ts
@@ -408,11 +408,7 @@ module Test exposing (..)
 import {-caret-}
 `;
 
-    await testCompletions(
-      otherSource + source,
-      ["Test", "OtherModule"],
-      "exactMatch",
-    );
+    await testCompletions(otherSource + source, ["OtherModule"], "exactMatch");
 
     const source2 = `
 --@ Test.elm
@@ -421,11 +417,7 @@ module Test exposing (..)
 import T{-caret-}
 `;
 
-    await testCompletions(
-      otherSource + source2,
-      ["Test", "OtherModule"],
-      "exactMatch",
-    );
+    await testCompletions(otherSource + source2, ["OtherModule"], "exactMatch");
   });
 
   it("Exposing a value from another module should have completions", async () => {
@@ -871,6 +863,96 @@ test = Module.sub{-caret-}
     );
   });
 
+  it("Possible import completions", async () => {
+    const source = `
+--@ Module.elm
+module Module exposing (..)
+
+func = ""
+
+--@ Module/Submodule.elm
+module Module.Submodule exposing (..)
+
+func = ""
+
+--@ Module/Submodule/AnotherSubmodule.elm
+module Module.Submodule.AnotherSubmodule exposing (..)
+
+func = ""
+
+--@ Test.elm
+module Test exposing (..)
+
+import Module.{-caret-}
+
+`;
+
+    await testCompletions(
+      source,
+      ["Submodule", "Submodule.AnotherSubmodule"],
+      "exactMatch",
+      "triggeredByDot",
+    );
+
+    const source2 = `
+--@ Module.elm
+module Module exposing (..)
+
+func = ""
+
+--@ Module/Submodule.elm
+module Module.Submodule exposing (..)
+
+func = ""
+
+--@ Module/Submodule/AnotherSubmodule.elm
+module Module.Submodule.AnotherSubmodule exposing (..)
+
+func = ""
+
+--@ Test.elm
+module Test exposing (..)
+
+import Module.Sub{-caret-}
+
+`;
+
+    await testCompletions(
+      source2,
+      ["Submodule", "Submodule.AnotherSubmodule"],
+      "exactMatch",
+    );
+
+    const source3 = `
+--@ Module.elm
+module Module exposing (..)
+
+func = ""
+
+--@ Module/Submodule.elm
+module Module.Submodule exposing (..)
+
+func = ""
+
+--@ Module/Submodule/AnotherSubmodule.elm
+module Module.Submodule.AnotherSubmodule exposing (..)
+
+func = ""
+
+--@ Test.elm
+module Test exposing (..)
+
+import {-caret-}
+
+`;
+
+    await testCompletions(
+      source3,
+      ["Module", "Module.Submodule", "Module.Submodule.AnotherSubmodule"],
+      "exactMatch",
+    );
+  });
+
   it("Imported qualified modules should have value completions", async () => {
     const source = `
 --@ Module.elm