Repository: elm-tooling/elm-language-server (ts)

Issue:
Unused import diagnostics improvements
If there is an unused import alias and there are no exposing values, we can say the entire import is unused. Also make sure the code actions work for all cases.

Pull request description:
Improve unused import diagnostics
Cleans up unused import diagnostics and adds a code action to remove unused import. Closes #486. 

With this new way, if `import Html as H` is unused, it is just an unused import, and the whole thing can be removed. If the alias in `import Html as H exposing (..)` is unused, we report an unused alias, because the import is still used.

Patch:
diff --git a/src/providers/diagnostics/elmLsDiagnostics.ts b/src/providers/diagnostics/elmLsDiagnostics.ts
index c57ce4e..c85cb1a 100644
--- a/src/providers/diagnostics/elmLsDiagnostics.ts
+++ b/src/providers/diagnostics/elmLsDiagnostics.ts
@@ -8,6 +8,7 @@ import {
   Connection,
   DiagnosticSeverity,
   DiagnosticTag,
+  Position,
   Range,
   TextEdit,
 } from "vscode-languageserver";
@@ -529,6 +530,33 @@ export class ElmLsDiagnostics {
           }
         }
 
+        if (diagnostic.data.code === "unused_import") {
+          const node = TreeUtils.getNamedDescendantForPosition(
+            treeContainer.tree.rootNode,
+            diagnostic.range.end,
+          );
+
+          const moduleName = node.childForFieldName("moduleName");
+
+          result.push({
+            diagnostics: [diagnostic],
+            edit: {
+              changes: {
+                [uri]: [
+                  TextEdit.del(
+                    Range.create(
+                      diagnostic.range.start,
+                      Position.create(diagnostic.range.end.line + 1, 0),
+                    ),
+                  ),
+                ],
+              },
+            },
+            kind: CodeActionKind.QuickFix,
+            title: `Remove unused import \`${moduleName?.text ?? node.text}\``,
+          });
+        }
+
         if (diagnostic.data.code === "unused_alias") {
           const node = TreeUtils.getNamedDescendantForPosition(
             treeContainer.tree.rootNode,
@@ -576,11 +604,14 @@ export class ElmLsDiagnostics {
     const moduleImports = this.moduleImportsQuery
       .matches(tree.rootNode)
       .map((match) => match.captures[0].node)
-      .filter(
-        (node) =>
-          node.nextNamedSibling?.type !== "exposing_list" &&
-          node.nextNamedSibling?.type !== "as_clause",
-      );
+      .filter((node) => !node.parent?.childForFieldName("exposing"))
+      .map((node) => {
+        const alias = node.parent
+          ?.childForFieldName("asClause")
+          ?.childForFieldName("name");
+
+        return alias ? alias : node;
+      });
 
     // Would need to adjust tree-sitter (use fields) to get a better query
     moduleImports.forEach((moduleImport) => {
@@ -595,9 +626,13 @@ export class ElmLsDiagnostics {
         .map((match) => match.captures.map((n) => n.node.text).join("."))
         .filter((moduleReference) => moduleReference === moduleImport.text);
 
-      if (references.length === 0 && moduleImport.parent) {
+      const importNode =
+        moduleImport.parent?.type === "as_clause"
+          ? moduleImport.parent?.parent
+          : moduleImport.parent;
+      if (references.length === 0 && importNode) {
         diagnostics.push({
-          range: this.getNodeRange(moduleImport.parent),
+          range: this.getNodeRange(importNode),
           message: `Unused import \`${moduleImport.text}\``,
           severity: DiagnosticSeverity.Warning,
           source: "ElmLS",
@@ -666,6 +701,11 @@ export class ElmLsDiagnostics {
       .map((match) => match.captures[0].node);
 
     moduleAliases.forEach((moduleAlias) => {
+      // This case is handled by unused_import
+      if (!moduleAlias.parent?.parent?.childForFieldName("exposing")) {
+        return;
+      }
+
       const references = this.moduleAliasReferencesQuery
         .matches(tree.rootNode)
         .filter(Utils.notUndefined.bind(this))
diff --git a/src/util/types/typeChecker.ts b/src/util/types/typeChecker.ts
index d33e32a..9727891 100644
--- a/src/util/types/typeChecker.ts
+++ b/src/util/types/typeChecker.ts
@@ -495,6 +495,25 @@ export function createTypeChecker(workspace: IElmWorkspace): TypeChecker {
 
       let definitionFromOtherFile;
 
+      // Make sure the next node is a dot, or else it isn't a Module
+      if (TreeUtils.nextNode(nodeAtPosition)?.type === "dot") {
+        const endPos = upperCaseQidText.indexOf(nodeText) + nodeText.length;
+
+        const moduleNameOrAlias = nodeParent.text.substring(0, endPos);
+        const moduleName =
+          findImportModuleNameNode(moduleNameOrAlias, treeContainer)?.text ??
+          moduleNameOrAlias;
+
+        const definitionFromOtherFile = findImportOfType(
+          treeContainer,
+          moduleName,
+          "Module",
+        );
+        if (definitionFromOtherFile) {
+          return definitionFromOtherFile;
+        }
+      }
+
       if (isTypeUsage) {
         definitionFromOtherFile = findImportOfType(
           treeContainer,
@@ -524,25 +543,6 @@ export function createTypeChecker(workspace: IElmWorkspace): TypeChecker {
         return definitionFromOtherFile;
       }
 
-      // Make sure the next node is a dot, or else it isn't a Module
-      if (TreeUtils.nextNode(nodeAtPosition)?.type === "dot") {
-        const endPos = upperCaseQidText.indexOf(nodeText) + nodeText.length;
-
-        const moduleNameOrAlias = nodeParent.text.substring(0, endPos);
-        const moduleName =
-          findImportModuleNameNode(moduleNameOrAlias, treeContainer)?.text ??
-          moduleNameOrAlias;
-
-        const definitionFromOtherFile = findImportOfType(
-          treeContainer,
-          moduleName,
-          "Module",
-        );
-        if (definitionFromOtherFile) {
-          return definitionFromOtherFile;
-        }
-      }
-
       definitionFromOtherFile = findImportOfType(
         treeContainer,
         findImportModuleNameNode(upperCaseQidText, treeContainer)?.text ??
diff --git a/src/util/types/typeInference.ts b/src/util/types/typeInference.ts
index 401198e..500b4f7 100644
--- a/src/util/types/typeInference.ts
+++ b/src/util/types/typeInference.ts
@@ -409,7 +409,7 @@ function typeMismatchError(
     }
     if (diff.mismatched.size > 0) {
       s += `\nMismatched fields: `;
-      diff.mismatched.forEach(([found, expected], field) => {
+      diff.mismatched.forEach(([expected, found], field) => {
         s += `\n Field \`${field}\` expected \`${checker.typeToString(
           expected,
         )}\`, found \`${checker.typeToString(found)}\``;


Test patch:
diff --git a/test/definitionProviderTests/importDefinition.test.ts b/test/definitionProviderTests/importDefinition.test.ts
index 68ced10..48d4390 100644
--- a/test/definitionProviderTests/importDefinition.test.ts
+++ b/test/definitionProviderTests/importDefinition.test.ts
@@ -553,26 +553,6 @@ module App exposing (..)
 
 type Expression = Expression
                     --X
-`;
-    await testBase.testDefinition(source);
-  });
-
-  it(`test union constructor that is before a record accessor function`, async () => {
-    const source = `
---@ main.elm
-import App exposing (..)
-
-fooLens =
-    FieldLens .foo (\\newFoo r -> { r | foo = newFoo })
-    --^App.elm
-
---@ App.elm
-module App exposing (..)
-
-type alias FieldLens a b c d =
---X
-    { get : a -> b, set : c -> a -> d }
-                    
 `;
     await testBase.testDefinition(source);
   });
diff --git a/test/diagnosticTests/elmLsDiagnostics.test.ts b/test/diagnosticTests/elmLsDiagnostics.test.ts
index fd1ed0c..ed559c5 100644
--- a/test/diagnosticTests/elmLsDiagnostics.test.ts
+++ b/test/diagnosticTests/elmLsDiagnostics.test.ts
@@ -368,11 +368,11 @@ type alias Thing = { name : Bar.Name }
       await testDiagnostics(source, "unused_import", []);
     });
 
-    it("unused but has alias", async () => {
+    it("unused but has exposing", async () => {
       const source = `
 module Foo exposing (..)
 
-import Bar as B
+import Bar exposing (baz)
 
 foo = 1
 			`;
@@ -380,34 +380,42 @@ foo = 1
       await testDiagnostics(source, "unused_import", []);
     });
 
-    it("unused but has exposing", async () => {
+    it("unused import", async () => {
       const source = `
 module Foo exposing (..)
 
-import Bar exposing (baz)
+import Bar
 
 foo = 1
 			`;
 
-      await testDiagnostics(source, "unused_import", []);
+      await testDiagnostics(source, "unused_import", [
+        diagnosticWithRangeAndName(
+          {
+            start: { line: 3, character: 0 },
+            end: { line: 3, character: 10 },
+          },
+          "Bar",
+        ),
+      ]);
     });
 
-    it("unused import", async () => {
+    it("no usage for alias", async () => {
       const source = `
 module Foo exposing (..)
 
-import Bar
+import Bar as B
 
-foo = 1
+foo = (+) 1 2
 			`;
 
       await testDiagnostics(source, "unused_import", [
         diagnosticWithRangeAndName(
           {
             start: { line: 3, character: 0 },
-            end: { line: 3, character: 10 },
+            end: { line: 3, character: 15 },
           },
-          "Bar",
+          "B",
         ),
       ]);
     });
@@ -431,11 +439,11 @@ foo = 1
       };
     };
 
-    it("no usage for alias", async () => {
+    it("no usage for alias with exposing", async () => {
       const source = `
 module Foo exposing (..)
 
-import Bar as B
+import Bar as B exposing (..)
 
 foo = (+) 1 2
 			`;
@@ -451,12 +459,25 @@ foo = (+) 1 2
       ]);
     });
 
-    it("used as qualified", async () => {
+    // This case is handled by unused_import
+    it("unused alias but no exposing", async () => {
       const source = `
 module Foo exposing (..)
 
 import Bar as B
 
+foo = 1
+			`;
+
+      await testDiagnostics(source, "unused_alias", []);
+    });
+
+    it("used as qualified", async () => {
+      const source = `
+module Foo exposing (..)
+
+import Bar as B exposing (..)
+
 foo = B.add 1
 			`;
 
@@ -467,7 +488,7 @@ foo = B.add 1
       const source = `
 module Main exposing (..)
 
-import X as Y
+import X as Y exposing (..)
 
 z a =
     case a of
@@ -482,7 +503,7 @@ z a =
       const source = `
 module Foo exposing (..)
 
-import Bar as B
+import Bar as B exposing (..)
 
 foo : B.Thing
 foo = bar
@@ -495,7 +516,7 @@ foo = bar
       const source = `
 module Foo exposing (..)
 
-import Bar as B
+import Bar as B exposing (..)
 
 foo = B.math.add 1
 			`;
@@ -508,7 +529,7 @@ foo = B.math.add 1
 module Foo exposing (..)
 
 
-import Bar as B
+import Bar as B exposing (..)
 
 type alias Thing = { name : B.Name }
 			`;