Repository: elm-tooling/elm-language-server
Pull request #550 (2021-03-05 05:55:32)

Handle removing all import values better
Handle overlapping ranges when removing multiple import values at the same time. I also added special handling for the case when we remove all import values, it removes the exposing list. Still need to add tests and there is one case that still does not work.

- [x] When the last two (or more) values in a list are unused, an extra comma is left behind.
- [x] Add tests



Patch:
diff --git a/src/positionUtil.ts b/src/positionUtil.ts
index 6751e72..dd17375 100644
--- a/src/positionUtil.ts
+++ b/src/positionUtil.ts
@@ -30,14 +30,27 @@ export class PositionUtil {
   }
 }
 
-export function comparePosition(pos1: VSPosition, pos2: TSPosition): number {
-  if (pos1.line === pos2.row && pos1.character === pos2.column) {
+export function comparePosition(
+  _pos1: VSPosition | TSPosition,
+  _pos2: VSPosition | TSPosition,
+): number {
+  // Convert TSPosition to VSPosition
+  const pos1 =
+    "row" in _pos1
+      ? PositionUtil.FROM_TS_POSITION(_pos1).toVSPosition()
+      : _pos1;
+  const pos2 =
+    "row" in _pos2
+      ? PositionUtil.FROM_TS_POSITION(_pos2).toVSPosition()
+      : _pos2;
+
+  if (pos1.line === pos2.line && pos1.character === pos2.character) {
     return 0;
   }
 
   if (
-    pos1.line < pos2.row ||
-    (pos1.line === pos2.row && pos1.character < pos2.column)
+    pos1.line < pos2.line ||
+    (pos1.line === pos2.line && pos1.character < pos2.character)
   ) {
     return -1;
   }
diff --git a/src/providers/codeActionLs/removeUnusedCodeAction.ts b/src/providers/codeActionLs/removeUnusedCodeAction.ts
index a3fa9f9..47afd4a 100644
--- a/src/providers/codeActionLs/removeUnusedCodeAction.ts
+++ b/src/providers/codeActionLs/removeUnusedCodeAction.ts
@@ -35,6 +35,7 @@ CodeActionProvider.registerCodeAction({
       .filter(Utils.notUndefined.bind(this));
   },
   getFixAllCodeAction: (params) => {
+    const importsMap = new Map<string, Set<string>>();
     return CodeActionProvider.getFixAllCodeAction(
       "Remove all unused code",
       params,
@@ -45,6 +46,7 @@ CodeActionProvider.registerCodeAction({
           ...getEditsForDiagnostic(
             convertFromCompilerDiagnostic(diagnostic),
             params.sourceFile,
+            importsMap,
           ).edits,
         );
       },
@@ -55,7 +57,23 @@ CodeActionProvider.registerCodeAction({
 function getEditsForDiagnostic(
   diagnostic: IDiagnostic,
   sourceFile: ISourceFile,
+  importsMap?: Map<string, Set<string>>,
 ): { title?: string; edits: TextEdit[] } {
+  const addImportToSet = (module: string, value: string): void => {
+    if (!importsMap) {
+      return;
+    }
+
+    let existing = importsMap.get(module);
+
+    if (!existing) {
+      existing = new Set<string>();
+      importsMap.set(module, existing);
+    }
+
+    existing.add(value);
+  };
+
   switch (diagnostic.data.code) {
     case "unused_import": {
       const node = TreeUtils.getNamedDescendantForPosition(
@@ -150,6 +168,32 @@ function getEditsForDiagnostic(
         break;
       }
 
+      const allValues =
+        importClause
+          .childForFieldName("exposing")
+          ?.namedChildren.filter(
+            (n) => n.type === "exposed_value" || n.type === "exposed_type",
+          )
+          .map((n) => n.text) ?? [];
+
+      // This is only for the fix all code action
+      // If we are removing all import values, we need to remove the entire exposing
+      addImportToSet(moduleName.text, node.text);
+      if (
+        importsMap &&
+        allValues.every((val) => importsMap.get(moduleName.text)?.has(val))
+      ) {
+        const removeExposingExit = RefactorEditUtils.removeImportExposingList(
+          sourceFile.tree,
+          moduleName.text,
+        );
+        if (removeExposingExit) {
+          return {
+            edits: [removeExposingExit],
+          };
+        }
+      }
+
       const removeValueEdit = RefactorEditUtils.removeValueFromImport(
         sourceFile.tree,
         moduleName.text,
@@ -157,6 +201,31 @@ function getEditsForDiagnostic(
       );
 
       if (removeValueEdit) {
+        // Detect if there are 2 or more values at the end of a exposing list that are removed
+        // For example, if we are removing `foo` and `bar` in `(func, foo, bar)`
+        // We need to add another edit to remove the comma before `foo`
+        if (importsMap && allValues.length > 2) {
+          allValues.reverse();
+          const firstUsedIndex = allValues.findIndex(
+            (val) => !importsMap.get(moduleName.text)?.has(val),
+          );
+
+          if (firstUsedIndex >= 2) {
+            const val = allValues[firstUsedIndex - 1];
+
+            const removeValueWithCommaEdit = RefactorEditUtils.removeValueFromImport(
+              sourceFile.tree,
+              moduleName.text,
+              val,
+              /* forceRemovePrecedingComma */ true,
+            );
+
+            if (removeValueWithCommaEdit) {
+              return { edits: [removeValueWithCommaEdit, removeValueEdit] };
+            }
+          }
+        }
+
         return {
           title: `Remove unused ${
             node.type === "exposed_type" ? "type" : "value"
diff --git a/src/providers/codeActionProvider.ts b/src/providers/codeActionProvider.ts
index d4a55b0..05d9b3f 100644
--- a/src/providers/codeActionProvider.ts
+++ b/src/providers/codeActionProvider.ts
@@ -9,7 +9,6 @@ import {
   Command,
   Connection,
   Diagnostic as LspDiagnostic,
-  DiagnosticSeverity,
   Position,
   Range,
   TextEdit,
@@ -23,6 +22,7 @@ import { Settings } from "../util/settings";
 import { Diagnostic } from "../compiler/diagnostics";
 import {
   convertFromCompilerDiagnostic,
+  convertToCompilerDiagnostic,
   DiagnosticsProvider,
   IDiagnostic,
 } from "./diagnostics/diagnosticsProvider";
@@ -35,6 +35,7 @@ import { ExposeUnexposeHandler } from "./handlers/exposeUnexposeHandler";
 import { MoveRefactoringHandler } from "./handlers/moveRefactoringHandler";
 import { ICodeActionParams } from "./paramsExtensions";
 import { ElmPackageCache } from "../compiler/elmPackageCache";
+import { comparePosition } from "../positionUtil";
 
 export interface ICodeActionRegistration {
   errorCodes: string[];
@@ -145,15 +146,7 @@ export class CodeActionProvider {
       ...params.program.getSuggestionDiagnostics(params.sourceFile),
       ...diagnosticProvider
         .getCurrentDiagnostics(params.sourceFile.uri, DiagnosticKind.ElmLS)
-        .map((diag) => ({
-          message: diag.message,
-          source: diag.source,
-          severity: diag.severity ?? DiagnosticSeverity.Warning,
-          range: diag.range,
-          code: diag.data.code,
-          uri: diag.data.uri,
-          tags: diag.tags,
-        })),
+        .map(convertToCompilerDiagnostic),
     ];
   }
 
@@ -221,6 +214,29 @@ export class CodeActionProvider {
       }
     });
 
+    const sortedEdits = edits.sort((a, b) =>
+      comparePosition(a.range.start, b.range.start),
+    );
+
+    // Using object mutation here to fix the ranges
+    sortedEdits.forEach((edit, i) => {
+      const lastEditEnd = sortedEdits[i - 1]?.range.end;
+      const newEditStart = edit.range.start;
+
+      // Handle if the ranges overlap
+      if (
+        lastEditEnd &&
+        newEditStart &&
+        comparePosition(lastEditEnd, newEditStart) > 0
+      ) {
+        edit.range.start = lastEditEnd;
+
+        if (comparePosition(edit.range.end, edit.range.start) < 0) {
+          edit.range.end = edit.range.start;
+        }
+      }
+    });
+
     return {
       title,
       kind: CodeActionKind.QuickFix,
diff --git a/src/providers/diagnostics/diagnosticsProvider.ts b/src/providers/diagnostics/diagnosticsProvider.ts
index bb466dc..c4fffe0 100644
--- a/src/providers/diagnostics/diagnosticsProvider.ts
+++ b/src/providers/diagnostics/diagnosticsProvider.ts
@@ -3,6 +3,7 @@ import {
   CancellationToken,
   Connection,
   Diagnostic as LspDiagnostic,
+  DiagnosticSeverity,
   FileChangeType,
 } from "vscode-languageserver";
 import { TextDocument } from "vscode-languageserver-textdocument";
@@ -61,6 +62,18 @@ export function convertFromCompilerDiagnostic(diag: Diagnostic): IDiagnostic {
   };
 }
 
+export function convertToCompilerDiagnostic(diag: IDiagnostic): Diagnostic {
+  return {
+    message: diag.message,
+    source: diag.source,
+    severity: diag.severity ?? DiagnosticSeverity.Warning,
+    range: diag.range,
+    code: diag.data.code,
+    uri: diag.data.uri,
+    tags: diag.tags,
+  };
+}
+
 class PendingDiagnostics extends Map<string, number> {
   public getOrderedFiles(): string[] {
     return Array.from(this.entries())
@@ -230,6 +243,20 @@ export class DiagnosticsProvider {
     return this.currentDiagnostics.get(uri)?.get() ?? [];
   }
 
+  /**
+   * Used for tests only
+   */
+  public forceElmLsDiagnosticsUpdate(
+    sourceFile: ISourceFile,
+    program: IProgram,
+  ): void {
+    this.updateDiagnostics(
+      sourceFile.uri,
+      DiagnosticKind.ElmLS,
+      this.elmLsDiagnostics.createDiagnostics(sourceFile, program),
+    );
+  }
+
   private requestDiagnostics(uri: string): void {
     this.pendingDiagnostics.set(uri, Date.now());
     this.triggerDiagnostics();
diff --git a/src/providers/diagnostics/elmLsDiagnostics.ts b/src/providers/diagnostics/elmLsDiagnostics.ts
index 715933a..cadd01e 100644
--- a/src/providers/diagnostics/elmLsDiagnostics.ts
+++ b/src/providers/diagnostics/elmLsDiagnostics.ts
@@ -848,7 +848,7 @@ export class ElmLsDiagnostics {
     return diagnostics;
   }
 
-  getSingleFieldRecordDiagnostics(
+  private getSingleFieldRecordDiagnostics(
     tree: Tree,
     uri: string,
     program: IProgram,
diff --git a/src/util/refactorEditUtils.ts b/src/util/refactorEditUtils.ts
index 4ad0510..d57f011 100644
--- a/src/util/refactorEditUtils.ts
+++ b/src/util/refactorEditUtils.ts
@@ -127,6 +127,7 @@ export class RefactorEditUtils {
     tree: Tree,
     moduleName: string,
     valueName: string,
+    forceRemoveLastComma = false,
   ): TextEdit | undefined {
     const importClause = TreeUtils.findImportClauseByName(tree, moduleName);
 
@@ -163,11 +164,35 @@ export class RefactorEditUtils {
         return this.removeValueFromExposingList(
           exposedValuesAndTypes,
           valueName,
+          forceRemoveLastComma,
         );
       }
     }
   }
 
+  public static removeImportExposingList(
+    tree: Tree,
+    moduleName: string,
+  ): TextEdit | undefined {
+    const importClause = TreeUtils.findImportClauseByName(tree, moduleName);
+    const exposingList = importClause?.childForFieldName("exposing");
+
+    if (exposingList) {
+      return TextEdit.del(
+        Range.create(
+          Position.create(
+            exposingList.startPosition.row,
+            exposingList.startPosition.column - 1,
+          ),
+          Position.create(
+            exposingList.endPosition.row,
+            exposingList.endPosition.column,
+          ),
+        ),
+      );
+    }
+  }
+
   public static addImport(
     tree: Tree,
     moduleName: string,
@@ -374,6 +399,7 @@ export class RefactorEditUtils {
   private static removeValueFromExposingList(
     exposedNodes: SyntaxNode[],
     valueName: string,
+    forceRemoveLastComma = false,
   ): TextEdit | undefined {
     const exposedNode = exposedNodes.find(
       (node) => node.text === valueName || node.text === `${valueName}(..)`,
@@ -383,12 +409,14 @@ export class RefactorEditUtils {
       let startPosition = exposedNode.startPosition;
       let endPosition = exposedNode.endPosition;
 
-      if (exposedNode.previousSibling?.text === ",") {
+      if (
+        exposedNode.previousSibling?.text === "," &&
+        (exposedNode.nextSibling?.text === ")" || forceRemoveLastComma)
+      ) {
         startPosition = exposedNode.previousSibling.startPosition;
       }
 
       if (
-        exposedNode.previousSibling?.text !== "," &&
         exposedNode.nextSibling?.text === "," &&
         exposedNode.nextSibling?.nextSibling
       ) {


Test patch:
diff --git a/test/codeActionTests/codeActionTestBase.ts b/test/codeActionTests/codeActionTestBase.ts
index f98e2b2..76be0c0 100644
--- a/test/codeActionTests/codeActionTestBase.ts
+++ b/test/codeActionTests/codeActionTestBase.ts
@@ -5,7 +5,11 @@ import { IProgram } from "../../src/compiler/program";
 import {
   CodeActionProvider,
   convertFromCompilerDiagnostic,
+  convertToCompilerDiagnostic,
+  DiagnosticsProvider,
 } from "../../src/providers";
+import { ElmLsDiagnostics } from "../../src/providers/diagnostics/elmLsDiagnostics";
+import { DiagnosticKind } from "../../src/providers/diagnostics/fileDiagnostics";
 import { ICodeActionParams } from "../../src/providers/paramsExtensions";
 import { Utils } from "../../src/util/utils";
 import {
@@ -63,6 +67,7 @@ export async function testCodeAction(
   source: string,
   expectedCodeActions: CodeAction[],
   expectedResultAfterEdits?: string,
+  testFixAll = false,
 ): Promise<void> {
   const treeParser = new SourceTreeParser();
   await treeParser.init();
@@ -91,6 +96,15 @@ export async function testCodeAction(
   workspaces.splice(0, workspaces.length);
   workspaces.push(program);
 
+  container.register(DiagnosticsProvider, {
+    useValue: new DiagnosticsProvider(),
+  });
+
+  // Needed by codeActionProvider which uses these diagnostics
+  container
+    .resolve(DiagnosticsProvider)
+    .forceElmLsDiagnosticsUpdate(sourceFile, program);
+
   const range = { start: result.position, end: result.position };
   const codeActions =
     codeActionProvider.handleCodeAction({
@@ -103,6 +117,9 @@ export async function testCodeAction(
           ...program.getSyntacticDiagnostics(sourceFile),
           ...program.getSemanticDiagnostics(sourceFile),
           ...program.getSuggestionDiagnostics(sourceFile),
+          ...new ElmLsDiagnostics()
+            .createDiagnostics(sourceFile, program)
+            .map(convertToCompilerDiagnostic),
         ]
           .filter((diag) => Utils.rangeOverlaps(diag.range, range))
           .map(convertFromCompilerDiagnostic),
@@ -132,7 +149,9 @@ export async function testCodeAction(
       expect(
         applyEditsToSource(
           stripCommentLines(result.sources[uri]),
-          codeActions[0].edit!.changes![URI.file(baseUri + uri).toString()],
+          codeActions[testFixAll ? codeActions.length - 1 : 0].edit!.changes![
+            URI.file(baseUri + uri).toString()
+          ],
         ),
       ).toEqual(source);
     });
diff --git a/test/codeActionTests/removeUnusedCodeAction.test.ts b/test/codeActionTests/removeUnusedCodeAction.test.ts
new file mode 100644
index 0000000..cd49cc9
--- /dev/null
+++ b/test/codeActionTests/removeUnusedCodeAction.test.ts
@@ -0,0 +1,308 @@
+import { testCodeAction } from "./codeActionTestBase";
+
+describe("remove unused code action", () => {
+  it("removing an import at the start of exposing list", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo, bar, Maybe, Result(..))
+                    --^
+
+func = ""
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (bar, Maybe, Result(..))
+
+func = ""
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove unused value `foo`" }],
+      expectedSource,
+    );
+  });
+
+  it("removing an import in the middle of exposing list", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo, bar, Maybe, Result(..))
+                               --^
+
+func = ""
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo, bar, Result(..))
+
+func = ""
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove unused value `Maybe`" }],
+      expectedSource,
+    );
+  });
+
+  it("removing an import at the end of exposing list", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo, bar, Maybe, Result)
+                                      --^
+
+func = ""
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo, bar, Maybe)
+
+func = ""
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove unused value `Result`" }],
+      expectedSource,
+    );
+  });
+
+  it("removing all imports in exposing list", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo, bar, Maybe, Result)
+                    --^
+
+func = ""
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo
+
+func = ""
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove all unused code" }],
+      expectedSource,
+      true,
+    );
+  });
+
+  it("removing all imports in exposing list except the first", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo, bar, Maybe, Result)
+                         --^
+
+func = foo
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo)
+
+func = foo
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove all unused code" }],
+      expectedSource,
+      true,
+    );
+  });
+
+  it("removing all imports in exposing list except the middle", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (bar, Maybe, foo, Result)
+                    --^
+
+func = foo
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo)
+
+func = foo
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove all unused code" }],
+      expectedSource,
+      true,
+    );
+  });
+
+  it("removing all imports in exposing list except the end", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (bar, Maybe, Result, foo)
+                    --^
+
+func = foo
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo)
+
+func = foo
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove all unused code" }],
+      expectedSource,
+      true,
+    );
+  });
+
+  it("removing some imports in exposing list - 1st and 3rd", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (Maybe, foo, Result, bar)
+                    --^
+
+func = foo + bar
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (foo, bar)
+
+func = foo + bar
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove all unused code" }],
+      expectedSource,
+      true,
+    );
+  });
+
+  it("removing some imports in exposing list - 2nd and 4th", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (bar, Maybe, foo, Result)
+                          --^
+
+func = foo + bar
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (bar, foo)
+
+func = foo + bar
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove all unused code" }],
+      expectedSource,
+      true,
+    );
+  });
+
+  it("removing some imports in exposing list - last 2", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (bar, foo, Maybe, Result)
+                              --^
+
+func = foo + bar
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (bar, foo)
+
+func = foo + bar
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove all unused code" }],
+      expectedSource,
+      true,
+    );
+  });
+
+  it("removing some imports in exposing list - first 2", async () => {
+    const source = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (Maybe, Result, bar, foo)
+                            --^
+
+func = foo + bar
+`;
+
+    const expectedSource = `
+--@ Test.elm
+module Test exposing (..)
+
+import Foo exposing (bar, foo)
+
+func = foo + bar
+`;
+
+    await testCodeAction(
+      source,
+      [{ title: "Remove all unused code" }],
+      expectedSource,
+      true,
+    );
+  });
+});