Repository: elm-tooling/elm-language-server
Pull request #544 (2021-03-04 08:50:21)

Removes 'Unused Value Constructor' errors on modules that expose all of their contents
Before this commit, `elmLsDiagnostics` doesn't present the 'Unused Value Constructor' warning for modules that either use the Value somewhere(correct) or expose the parent type explicitly with the '(..)' operator to expose the variants(this is speculative because the type may not be used elsewhere even though it's exported). This change just expands that logic to also include cases where the module implicitly exposes the parent type and its variants, by using the '(..)' indicator to expose all contents of the module. 

Ideally this and the other speculative case should be removed in favor of a solution that considers whether the variants are actually used in other files, but this is something that I am not familiar enough with the codebase to do quite yet. However, the tests have been modified to prepare for such a solution and a test has been added to test the multiple file variant usage case, but in the meantime, that test also tests the single file 'expose all' case.

I'm personally much more in favor of considering multiple files for this warning, so if that's not a gargantuan effort, please point me in that direction and I'll create that pull request instead.


Patch:
diff --git a/src/compiler/references.ts b/src/compiler/references.ts
index ea3bea7..49f91da 100644
--- a/src/compiler/references.ts
+++ b/src/compiler/references.ts
@@ -45,9 +45,10 @@ export class References {
               const functionNameNode =
                 TreeUtils.getFunctionNameNodeFromDefinition(
                   definitionNode.node,
-                ) ?? definitionNode.node.type === "lower_pattern"
+                ) ??
+                (definitionNode.node.type === "lower_pattern"
                   ? definitionNode.node
-                  : undefined;
+                  : undefined);
               if (functionNameNode) {
                 const functionName = functionNameNode.text;
                 if (refSourceTree.writeable) {
@@ -106,64 +107,63 @@ export class References {
                         continue;
                       }
 
-                      const otherTreeContainer = forest.getByUri(uri);
+                      const otherSourceFile = forest.getByUri(uri);
 
-                      if (!otherTreeContainer) {
+                      if (!otherSourceFile) {
                         continue;
                       }
 
                       const importedModuleAlias =
                         TreeUtils.findImportAliasOfModule(
                           moduleName,
-                          otherTreeContainer.tree,
+                          otherSourceFile.tree,
                         ) ?? moduleName;
 
                       const allImports = imports[uri];
 
                       // Find the function in the other module's imports
-                      const found =
-                        allImports.getVar(functionName, moduleName)[0] ??
-                        allImports.getVar(
+                      const found = [
+                        ...allImports.getVar(functionName, moduleName),
+                        ...allImports.getVar(
                           `${importedModuleAlias}.${functionName}`,
                           moduleName,
-                        )[0];
-
-                      if (found) {
-                        if (
-                          otherTreeContainer &&
-                          otherTreeContainer.writeable
-                        ) {
-                          const importClause = otherTreeContainer.symbolLinks
-                            ?.get(otherTreeContainer.tree.rootNode)
-                            ?.get(importedModuleAlias);
-
-                          // Add node from exposing list
-                          if (importClause?.type === "Import") {
-                            const exposedNode = TreeUtils.findExposedFunctionNode(
-                              importClause.node,
-                              functionName,
-                            );
-
-                            if (exposedNode) {
-                              references.push({
-                                node: exposedNode,
-                                uri,
-                              });
-                            }
+                        ),
+                      ];
+
+                      if (found.length > 0 && otherSourceFile.writeable) {
+                        const importClause = otherSourceFile.symbolLinks
+                          ?.get(otherSourceFile.tree.rootNode)
+                          ?.get(importedModuleAlias);
+
+                        // Add node from exposing list
+                        if (importClause?.type === "Import") {
+                          const exposedNode = TreeUtils.findExposedFunctionNode(
+                            importClause.node,
+                            functionName,
+                          );
+
+                          if (exposedNode) {
+                            references.push({
+                              node: exposedNode,
+                              uri,
+                            });
                           }
+                        }
 
-                          // Find all function calls in the other tree
-                          const functions = this.findFunctionCalls(
-                            otherTreeContainer.tree.rootNode,
-                            found?.name,
+                        // Find all function calls in the other tree
+                        const functions = found.flatMap(
+                          (imp) =>
+                            this.findFunctionCalls(
+                              otherSourceFile.tree.rootNode,
+                              imp.name,
+                            ) ?? [],
+                        );
+                        if (functions.length > 0) {
+                          references.push(
+                            ...functions.map((node) => {
+                              return { node, uri };
+                            }),
                           );
-                          if (functions) {
-                            references.push(
-                              ...functions.map((node) => {
-                                return { node, uri };
-                              }),
-                            );
-                          }
                         }
                       }
                     }
@@ -230,64 +230,63 @@ export class References {
                         continue;
                       }
 
-                      const otherTreeContainer = forest.getByUri(uri);
+                      const otherSourceFile = forest.getByUri(uri);
 
-                      if (!otherTreeContainer) {
+                      if (!otherSourceFile) {
                         continue;
                       }
 
                       const importedModuleAlias =
                         TreeUtils.findImportAliasOfModule(
                           moduleName,
-                          otherTreeContainer.tree,
+                          otherSourceFile.tree,
                         ) ?? moduleName;
 
                       const allImports = imports[uri];
 
                       // Find the function in the other module's imports
-                      const found =
-                        allImports.getVar(portName, moduleName)[0] ??
-                        allImports.getVar(
+                      const found = [
+                        ...allImports.getVar(portName, moduleName),
+                        ...allImports.getVar(
                           `${importedModuleAlias}.${portName}`,
                           moduleName,
-                        )[0];
-
-                      if (found) {
-                        if (
-                          otherTreeContainer &&
-                          otherTreeContainer.writeable
-                        ) {
-                          const importClause = otherTreeContainer.symbolLinks
-                            ?.get(otherTreeContainer.tree.rootNode)
-                            ?.get(importedModuleAlias);
-
-                          // Add node from exposing list
-                          if (importClause?.type === "Import") {
-                            const exposedNode = TreeUtils.findExposedFunctionNode(
-                              importClause.node,
-                              portName,
-                            );
-
-                            if (exposedNode) {
-                              references.push({
-                                node: exposedNode,
-                                uri,
-                              });
-                            }
+                        ),
+                      ];
+
+                      if (found.length > 0 && otherSourceFile.writeable) {
+                        const importClause = otherSourceFile.symbolLinks
+                          ?.get(otherSourceFile.tree.rootNode)
+                          ?.get(importedModuleAlias);
+
+                        // Add node from exposing list
+                        if (importClause?.type === "Import") {
+                          const exposedNode = TreeUtils.findExposedFunctionNode(
+                            importClause.node,
+                            portName,
+                          );
+
+                          if (exposedNode) {
+                            references.push({
+                              node: exposedNode,
+                              uri,
+                            });
                           }
+                        }
 
-                          // Find all function calls in the other tree
-                          const functions = this.findFunctionCalls(
-                            otherTreeContainer.tree.rootNode,
-                            found?.name,
+                        // Find all function calls in the other tree
+                        const functions = found.flatMap(
+                          (imp) =>
+                            this.findFunctionCalls(
+                              otherSourceFile.tree.rootNode,
+                              imp.name,
+                            ) ?? [],
+                        );
+                        if (functions.length > 0) {
+                          references.push(
+                            ...functions.map((node) => {
+                              return { node, uri };
+                            }),
                           );
-                          if (functions) {
-                            references.push(
-                              ...functions.map((node) => {
-                                return { node, uri };
-                              }),
-                            );
-                          }
                         }
                       }
                     }
@@ -354,66 +353,62 @@ export class References {
                         continue;
                       }
 
-                      const otherTreeContainer = forest.getByUri(uri);
+                      const otherSourceFile = forest.getByUri(uri);
 
-                      if (!otherTreeContainer) {
+                      if (!otherSourceFile) {
                         continue;
                       }
 
                       const importedModuleAlias =
                         TreeUtils.findImportAliasOfModule(
                           moduleName,
-                          otherTreeContainer.tree,
+                          otherSourceFile.tree,
                         ) ?? moduleName;
 
                       const allImports = imports[uri];
 
                       // Find the type or type alias in the other module's imports
-                      const found =
-                        allImports.getType(
-                          typeOrTypeAliasName,
-                          moduleName,
-                        )[0] ??
-                        allImports.getType(
+                      const found = [
+                        ...allImports.getType(typeOrTypeAliasName, moduleName),
+                        ...allImports.getType(
                           `${importedModuleAlias}.${typeOrTypeAliasName}`,
                           moduleName,
-                        )[0];
-
-                      if (found) {
-                        if (
-                          otherTreeContainer &&
-                          otherTreeContainer.writeable
-                        ) {
-                          const importClause = otherTreeContainer.symbolLinks
-                            ?.get(otherTreeContainer.tree.rootNode)
-                            ?.get(importedModuleAlias);
-
-                          if (importClause?.type === "Import") {
-                            const exposedNode = TreeUtils.findExposedTypeOrTypeAliasNode(
-                              importClause.node,
-                              typeOrTypeAliasNameNode.text,
-                            );
-
-                            if (exposedNode) {
-                              references.push({
-                                node: exposedNode,
-                                uri,
-                              });
-                            }
-                          }
+                        ),
+                      ];
+
+                      if (found.length > 0 && otherSourceFile.writeable) {
+                        const importClause = otherSourceFile.symbolLinks
+                          ?.get(otherSourceFile.tree.rootNode)
+                          ?.get(importedModuleAlias);
 
-                          const typeOrTypeAliasCalls = TreeUtils.findTypeOrTypeAliasCalls(
-                            otherTreeContainer.tree,
-                            found.name,
+                        if (importClause?.type === "Import") {
+                          const exposedNode = TreeUtils.findExposedTypeOrTypeAliasNode(
+                            importClause.node,
+                            typeOrTypeAliasNameNode.text,
                           );
-                          if (typeOrTypeAliasCalls) {
-                            references.push(
-                              ...typeOrTypeAliasCalls.map((node) => {
-                                return { node, uri };
-                              }),
-                            );
+
+                          if (exposedNode) {
+                            references.push({
+                              node: exposedNode,
+                              uri,
+                            });
                           }
                         }
+
+                        const typeOrTypeAliasCalls = found.flatMap(
+                          (imp) =>
+                            TreeUtils.findTypeOrTypeAliasCalls(
+                              otherSourceFile.tree,
+                              imp.name,
+                            ) ?? [],
+                        );
+                        if (typeOrTypeAliasCalls.length > 0) {
+                          references.push(
+                            ...typeOrTypeAliasCalls.map((node) => {
+                              return { node, uri };
+                            }),
+                          );
+                        }
                       }
                     }
                   }
diff --git a/src/providers/codeActionLs/removeUnusedCodeAction.ts b/src/providers/codeActionLs/removeUnusedCodeAction.ts
index a3fa9f9..7ca5886 100644
--- a/src/providers/codeActionLs/removeUnusedCodeAction.ts
+++ b/src/providers/codeActionLs/removeUnusedCodeAction.ts
@@ -15,6 +15,8 @@ const errorCodes = [
   "unused_alias",
   "unused_top_level",
   "unused_pattern",
+  "unused_value_constructor",
+  "unused_type_alias",
 ];
 const fixId = "remove_unused";
 CodeActionProvider.registerCodeAction({
@@ -129,6 +131,42 @@ function getEditsForDiagnostic(
       return { title: `Fix unused pattern \`${node.text}\``, edits: [edit] };
     }
 
+    case "unused_value_constructor": {
+      const node = TreeUtils.getNamedDescendantForPosition(
+        sourceFile.tree.rootNode,
+        diagnostic.range.start,
+      );
+
+      const edit = RefactorEditUtils.removeTypeValue(node);
+
+      if (!edit) {
+        break;
+      }
+
+      return {
+        title: `Remove unused value constructor \`${node.text}\``,
+        edits: [edit],
+      };
+    }
+
+    case "unused_type_alias": {
+      const node = TreeUtils.getNamedDescendantForPosition(
+        sourceFile.tree.rootNode,
+        diagnostic.range.start,
+      );
+
+      const edit = RefactorEditUtils.removeTypeAlias(node);
+
+      if (!edit) {
+        break;
+      }
+
+      return {
+        title: `Remove unused type alias \`${node.text}\``,
+        edits: [edit],
+      };
+    }
+
     case "unused_imported_value": {
       const node = TreeUtils.getNamedDescendantForPosition(
         sourceFile.tree.rootNode,
diff --git a/src/providers/completionProvider.ts b/src/providers/completionProvider.ts
index 2d39b91..07af2e7 100644
--- a/src/providers/completionProvider.ts
+++ b/src/providers/completionProvider.ts
@@ -1409,7 +1409,7 @@ export class CompletionProvider {
       this.createSnippet(
         "type alias",
         [
-          "type alias ${1:recordName} =",
+          "type alias ${1:RecordName} =",
           "    { ${2:key1} : ${3:ValueType1}",
           "    , ${4:key2} : ${5:ValueType2}",
           "    }",
diff --git a/src/providers/diagnostics/elmLsDiagnostics.ts b/src/providers/diagnostics/elmLsDiagnostics.ts
index 715933a..acdbc05 100644
--- a/src/providers/diagnostics/elmLsDiagnostics.ts
+++ b/src/providers/diagnostics/elmLsDiagnostics.ts
@@ -991,6 +991,19 @@ export class ElmLsDiagnostics {
   private getUnusedValueConstructorDiagnostics(tree: Tree): IDiagnostic[] {
     const diagnostics: IDiagnostic[] = [];
 
+    // Currently if the file exports the variant, either through an explicit
+    // reference to the type, or through a '(..)' to expose everything, then
+    // we won't mark the value constructor as unused. Ideally this should take
+    // multiple files into account, then these conditions can be removed.
+    const exposingAll = !!tree.rootNode
+      .childForFieldName("moduleDeclaration")
+      ?.childForFieldName("exposing")
+      ?.childForFieldName("doubleDot");
+
+    if (exposingAll) {
+      return diagnostics;
+    }
+
     const unionVariants = this.unionVariantsQuery
       .matches(tree.rootNode)
       .map((match) => [match.captures[1].node, match.captures[0].node])
diff --git a/src/providers/documentFormatingProvider.ts b/src/providers/documentFormatingProvider.ts
index 60a32cd..61b6310 100644
--- a/src/providers/documentFormatingProvider.ts
+++ b/src/providers/documentFormatingProvider.ts
@@ -78,7 +78,8 @@ export class DocumentFormattingProvider {
       this.connection.console.warn(JSON.stringify(error));
       if (
         error?.message &&
-        (error.message as string).includes("SYNTAX PROBLEM")
+        ((error.message as string).includes("SYNTAX PROBLEM") || //Elm-format 0.8.4 and below
+          (error.message as string).includes("Unable to parse file")) //Elm-format 0.8.5 and above
       ) {
         this.connection.window.showErrorMessage(
           "Running elm-format failed. Check the file for syntax errors.",
diff --git a/src/util/refactorEditUtils.ts b/src/util/refactorEditUtils.ts
index 4ad0510..649f5b6 100644
--- a/src/util/refactorEditUtils.ts
+++ b/src/util/refactorEditUtils.ts
@@ -371,6 +371,96 @@ export class RefactorEditUtils {
     }
   }
 
+  public static removeTypeAlias(node: SyntaxNode | null): TextEdit | undefined {
+    if (!node) {
+      return undefined;
+    }
+    const typeAliasDeclarationNode = TreeUtils.findParentOfType(
+      "type_alias_declaration",
+      node,
+    );
+    if (!typeAliasDeclarationNode) {
+      return undefined;
+    }
+    return TextEdit.del(
+      Range.create(
+        Position.create(
+          typeAliasDeclarationNode.startPosition.row,
+          typeAliasDeclarationNode.startPosition.column,
+        ),
+        Position.create(
+          typeAliasDeclarationNode.endPosition.row,
+          typeAliasDeclarationNode.endPosition.column,
+        ),
+      ),
+    );
+  }
+
+  public static removeTypeValue(
+    nodeAtPosition: SyntaxNode,
+  ): TextEdit | undefined {
+    const unionVariants = TreeUtils.findParentOfType(
+      "type_declaration",
+      nodeAtPosition,
+    )?.children.filter((child) => child.type == "union_variant");
+
+    if (unionVariants?.length == 1) {
+      return this.removeType(unionVariants[0].parent);
+    }
+
+    let startPosition = nodeAtPosition.startPosition;
+    let endPosition = nodeAtPosition.endPosition;
+
+    const unionVariant = unionVariants?.find(
+      (a) =>
+        a.text.startsWith(`${nodeAtPosition.text} `) ||
+        a.text === nodeAtPosition.text,
+    );
+    if (unionVariant?.previousSibling?.type == "eq") {
+      startPosition = unionVariant.previousSibling?.endPosition;
+      if (unionVariant.nextSibling?.type == "|") {
+        endPosition = unionVariant.nextSibling?.endPosition;
+      }
+    } else if (
+      unionVariant?.previousSibling?.type == "|" &&
+      unionVariant?.previousSibling?.previousSibling
+    ) {
+      startPosition = unionVariant.previousSibling?.previousSibling.endPosition;
+    }
+
+    return TextEdit.del(
+      Range.create(
+        Position.create(startPosition.row, startPosition.column),
+        Position.create(endPosition.row, endPosition.column),
+      ),
+    );
+  }
+
+  public static removeType(node: SyntaxNode | null): TextEdit | undefined {
+    if (!node) {
+      return undefined;
+    }
+    const typeDeclarationNode = TreeUtils.findParentOfType(
+      "type_declaration",
+      node,
+    );
+    if (!typeDeclarationNode) {
+      return undefined;
+    }
+    return TextEdit.del(
+      Range.create(
+        Position.create(
+          typeDeclarationNode.startPosition.row,
+          typeDeclarationNode.startPosition.column,
+        ),
+        Position.create(
+          typeDeclarationNode.endPosition.row,
+          typeDeclarationNode.endPosition.column,
+        ),
+      ),
+    );
+  }
+
   private static removeValueFromExposingList(
     exposedNodes: SyntaxNode[],
     valueName: string,


Test patch:
diff --git a/test/diagnosticTests/elmLsDiagnostics.test.ts b/test/diagnosticTests/elmLsDiagnostics.test.ts
index ed559c5..37be19b 100644
--- a/test/diagnosticTests/elmLsDiagnostics.test.ts
+++ b/test/diagnosticTests/elmLsDiagnostics.test.ts
@@ -26,17 +26,23 @@ describe("ElmLsDiagnostics", () => {
     await treeParser.init();
     elmDiagnostics = new ElmLsDiagnostics();
 
-    const program = await treeParser.getProgram(getSourceFiles(source));
+    const sources = getSourceFiles(source)
+    const program = await treeParser.getProgram(sources);
 
-    const sourceFile = program.getSourceFile(uri);
+    let diagnostics : Array<IDiagnostic> = []
+    for ( const fileName in sources) {
+      const filePath = URI.file(baseUri + fileName).toString();
+      const sourceFile = program.getSourceFile(filePath);
 
-    if (!sourceFile) {
-      fail();
-    }
+      if (!sourceFile) {
+        fail();
+      }
 
-    const diagnostics = elmDiagnostics
-      .createDiagnostics(sourceFile, program)
-      .filter((diagnostic) => diagnostic.data.code === code);
+      diagnostics = diagnostics.concat(elmDiagnostics
+        .createDiagnostics(sourceFile, program)
+        .filter((diagnostic) => diagnostic.data.code === code)
+      );
+    }
 
     const diagnosticsEqual = Utils.arrayEquals(
       diagnostics,
@@ -1576,7 +1582,7 @@ foo = Thing
 
     it("unused value constructor not exposed", async () => {
       const source = `
-module Bar exposing (foo, Some))
+module Bar exposing (Some)
 
 type Some = Thing | Other
 			`;
@@ -1656,5 +1662,30 @@ text en it language =
 
       await testDiagnostics(source, "unused_value_constructor", []);
     });
+
+    it("used in a different file", async () => {
+      const source =`
+--@ Main.elm
+module Main exposing (..)
+
+import B exposing (C(..))
+
+f : Int -> C
+f x =
+    if x > 1 then
+        Something
+    else
+        SomethingElse
+
+--@ B.elm
+module B exposing (..)
+
+type C
+    = Something
+    | SomethingElse
+			`;
+
+      await testDiagnostics(source, "unused_value_constructor", []);
+    });
   });
 });
diff --git a/test/referencesProviderTests/functionReferences.test.ts b/test/referencesProviderTests/functionReferences.test.ts
index afcb5ec..4b7ea66 100644
--- a/test/referencesProviderTests/functionReferences.test.ts
+++ b/test/referencesProviderTests/functionReferences.test.ts
@@ -32,6 +32,35 @@ func = foo
     await testBase.testReferences(source);
   });
 
+  it(`function with args references in other files`, async () => {
+    const source = `
+--@ Module.elm
+module Module exposing (foo)
+                       --X
+foo a b = 42
+--^
+
+--@ Bar.elm
+module Bar exposing (..)
+
+import Module exposing (foo)
+                       --X
+
+bar = foo 1 2
+     --X
+
+--@ FooExtra.elm
+module FooExtra exposing (..)
+
+import Module exposing (foo)
+                       --X
+
+func = foo 1 2
+      --X
+`;
+    await testBase.testReferences(source);
+  });
+
   it(`function references used with module prefix in other files`, async () => {
     const source = `
 --@ Module.elm
@@ -59,6 +88,33 @@ func = Module.foo
     await testBase.testReferences(source);
   });
 
+  it(`function with arg references used with module prefix in other files`, async () => {
+    const source = `
+--@ Module.elm
+module Module exposing (foo)
+                       --X
+foo a b = 42
+--^
+
+--@ Bar.elm
+module Bar exposing (..)
+
+import Module
+
+bar = Module.foo 1 2
+           --X
+
+--@ FooExtra.elm
+module FooExtra exposing (..)
+
+import Module
+
+func = Module.foo 1 2
+             --X
+`;
+    await testBase.testReferences(source);
+  });
+
   it(`function references used with module alias prefix in other files`, async () => {
     const source = `
 --@ Module.elm
@@ -100,6 +156,20 @@ bar = foo
     await testBase.testReferences(source);
   });
 
+  it(`local calls to a function with args`, async () => {
+    const source = `
+--@ Module.elm
+module Module exposing (foo)
+                        --X
+foo a = 42
+--^
+
+bar = foo 1
+     --X
+`;
+    await testBase.testReferences(source);
+  });
+
   it(`function annotation gets a reference`, async () => {
     const source = `
 --@ Module.elm
@@ -155,6 +225,37 @@ foo =
 test = 
     foo.field
    --X
+`;
+    await testBase.testReferences(source);
+  });
+
+  it(`function references used both with and without module prefix in other files`, async () => {
+    const source = `
+--@ Module.elm
+module Module exposing (foo)
+                       --X
+foo = 42
+--^
+
+--@ Bar.elm
+module Bar exposing (..)
+
+import Module
+
+bar = Module.foo
+           --X
+
+--@ FooExtra.elm
+module FooExtra exposing (..)
+
+import Module exposing (foo)
+                       --X
+
+func = Module.foo
+             --X
+
+func2 = foo
+       --X
 `;
     await testBase.testReferences(source);
   });
diff --git a/test/renameProvider.test.ts b/test/renameProvider.test.ts
index 7789631..d637cca 100644
--- a/test/renameProvider.test.ts
+++ b/test/renameProvider.test.ts
@@ -117,6 +117,50 @@ describe("renameProvider", () => {
     });
   }
 
+  it("renaming a function", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (func)
+
+func a =
+--^
+    15
+
+--@ Module/App.elm
+module Module.App exposing (foo)
+
+import Test exposing (func)
+
+foo : a -> Int
+foo =
+    func
+`;
+
+    const expectedResult = `
+--@ Test.elm
+module Test exposing (bar)
+
+bar a =
+    15
+
+--@ Module/App.elm
+module Module.App exposing (foo)
+
+import Test exposing (bar)
+
+foo : a -> Int
+foo =
+    bar
+`;
+
+    const renameRange = Range.create(
+      Position.create(2, 0),
+      Position.create(2, 4),
+    );
+    await testPrepareRename(source, renameRange);
+    await testRename(source, "bar", expectedResult);
+  });
+
   it("renaming a qualified value function part", async () => {
     const source = `
 --@ Test.elm