Repository: elm-tooling/elm-language-server
Pull request #567 (2021-03-26 17:02:16)

Explicitly set expose function code action to preferred
Don't default to preferred = true for all codeActions


Patch:
diff --git a/src/providers/codeAction/addMissingCaseBranchesCodeAction.ts b/src/providers/codeAction/addMissingCaseBranchesCodeAction.ts
index 4eb5800..790dc4b 100644
--- a/src/providers/codeAction/addMissingCaseBranchesCodeAction.ts
+++ b/src/providers/codeAction/addMissingCaseBranchesCodeAction.ts
@@ -1,5 +1,5 @@
-import { CodeAction, Range, TextEdit } from "vscode-languageserver";
-import { CodeActionProvider } from "..";
+import { Range, TextEdit } from "vscode-languageserver";
+import { CodeActionProvider, ICodeAction } from "..";
 import { getSpaces } from "../../util/refactorEditUtils";
 import { TreeUtils } from "../../util/treeUtils";
 import { Diagnostics } from "../../compiler/diagnostics";
@@ -25,7 +25,7 @@ CodeActionProvider.registerCodeAction({
       ),
     ];
   },
-  getFixAllCodeAction: (params: ICodeActionParams): CodeAction | undefined => {
+  getFixAllCodeAction: (params: ICodeActionParams): ICodeAction | undefined => {
     return CodeActionProvider.getFixAllCodeAction(
       "Add all missing case branches",
       params,
diff --git a/src/providers/codeAction/addMissingRecordFieldCodeAction.ts b/src/providers/codeAction/addMissingRecordFieldCodeAction.ts
index 6eeba85..c73375d 100644
--- a/src/providers/codeAction/addMissingRecordFieldCodeAction.ts
+++ b/src/providers/codeAction/addMissingRecordFieldCodeAction.ts
@@ -1,6 +1,6 @@
-import { CodeAction, Range, TextEdit } from "vscode-languageserver";
+import { Range, TextEdit } from "vscode-languageserver";
 import { SyntaxNode } from "web-tree-sitter";
-import { CodeActionProvider } from "..";
+import { CodeActionProvider, ICodeAction } from "..";
 import { ISourceFile } from "../../compiler/forest";
 import { getSpaces } from "../../util/refactorEditUtils";
 import { TreeUtils } from "../../util/treeUtils";
@@ -32,7 +32,7 @@ CodeActionProvider.registerCodeAction({
 
     return [];
   },
-  getFixAllCodeAction: (params: ICodeActionParams): CodeAction | undefined => {
+  getFixAllCodeAction: (params: ICodeActionParams): ICodeAction | undefined => {
     return CodeActionProvider.getFixAllCodeAction(
       "Create all missing record fields",
       params,
diff --git a/src/providers/codeAction/addTypeAnnotationCodeAction.ts b/src/providers/codeAction/addTypeAnnotationCodeAction.ts
index 60473d8..5ae527f 100644
--- a/src/providers/codeAction/addTypeAnnotationCodeAction.ts
+++ b/src/providers/codeAction/addTypeAnnotationCodeAction.ts
@@ -10,6 +10,7 @@ import { TreeUtils } from "../../util/treeUtils";
 import { Diagnostics } from "../../compiler/diagnostics";
 import {
   CodeActionProvider,
+  ICodeAction,
   IRefactorCodeAction,
   IRefactorEdit,
 } from "../codeActionProvider";
@@ -21,6 +22,9 @@ const fixId = "add_type_annotation";
 CodeActionProvider.registerCodeAction({
   errorCodes,
   fixId,
+  preferredAction: {
+    priority: 3,
+  },
   getCodeActions: (params: ICodeActionParams): CodeAction[] => {
     const edits = getEdits(params, params.range);
 
@@ -36,7 +40,7 @@ CodeActionProvider.registerCodeAction({
 
     return [];
   },
-  getFixAllCodeAction: (params: ICodeActionParams): CodeAction | undefined => {
+  getFixAllCodeAction: (params: ICodeActionParams): ICodeAction | undefined => {
     return CodeActionProvider.getFixAllCodeAction(
       "Add all missing type annotations",
       params,
diff --git a/src/providers/codeAction/exposeUnexposeCodeAction.ts b/src/providers/codeAction/exposeUnexposeCodeAction.ts
index 5d0454a..f2384e3 100644
--- a/src/providers/codeAction/exposeUnexposeCodeAction.ts
+++ b/src/providers/codeAction/exposeUnexposeCodeAction.ts
@@ -10,6 +10,9 @@ import { ICodeActionParams } from "../paramsExtensions";
 
 const refactorName = "expose_unexpose";
 CodeActionProvider.registerRefactorAction(refactorName, {
+  preferredAction: {
+    priority: 2,
+  },
   getAvailableActions: (params: ICodeActionParams): IRefactorCodeAction[] => {
     const result: IRefactorCodeAction[] = [];
     const nodeAtPosition = TreeUtils.getNamedDescendantForPosition(
diff --git a/src/providers/codeAction/importCodeAction.ts b/src/providers/codeAction/importCodeAction.ts
index 060d34d..446baae 100644
--- a/src/providers/codeAction/importCodeAction.ts
+++ b/src/providers/codeAction/importCodeAction.ts
@@ -15,6 +15,10 @@ const fixId = "import";
 CodeActionProvider.registerCodeAction({
   errorCodes,
   fixId,
+  preferredAction: {
+    priority: 1,
+    thereCanOnlyBeOne: true,
+  },
   getCodeActions: (params: ICodeActionParams): CodeAction[] | undefined => {
     const valueNode = TreeUtils.getNamedDescendantForRange(
       params.sourceFile,
diff --git a/src/providers/codeAction/makeDeclarationFromUsageCodeAction.ts b/src/providers/codeAction/makeDeclarationFromUsageCodeAction.ts
index 028b88c..0e6d7ba 100644
--- a/src/providers/codeAction/makeDeclarationFromUsageCodeAction.ts
+++ b/src/providers/codeAction/makeDeclarationFromUsageCodeAction.ts
@@ -1,9 +1,9 @@
-import { CodeAction, Range } from "vscode-languageserver";
+import { Range } from "vscode-languageserver";
 import { TextEdit } from "vscode-languageserver-textdocument";
 import { RefactorEditUtils } from "../../util/refactorEditUtils";
 import { TreeUtils } from "../../util/treeUtils";
 import { Diagnostics } from "../../compiler/diagnostics";
-import { CodeActionProvider } from "../codeActionProvider";
+import { CodeActionProvider, ICodeAction } from "../codeActionProvider";
 import { ICodeActionParams } from "../paramsExtensions";
 
 const errorCodes = [Diagnostics.MissingValue.code];
@@ -27,7 +27,7 @@ CodeActionProvider.registerCodeAction({
 
     return [];
   },
-  getFixAllCodeAction: (params: ICodeActionParams): CodeAction | undefined => {
+  getFixAllCodeAction: (params: ICodeActionParams): ICodeAction | undefined => {
     return CodeActionProvider.getFixAllCodeAction(
       "Create all missing local functions",
       params,
diff --git a/src/providers/codeActionProvider.ts b/src/providers/codeActionProvider.ts
index 05d9b3f..8805050 100644
--- a/src/providers/codeActionProvider.ts
+++ b/src/providers/codeActionProvider.ts
@@ -37,14 +37,27 @@ import { ICodeActionParams } from "./paramsExtensions";
 import { ElmPackageCache } from "../compiler/elmPackageCache";
 import { comparePosition } from "../positionUtil";
 
+interface IPreferredAction {
+  priority: number;
+  thereCanOnlyBeOne?: boolean; // Only marked as preferred if there is only a single fix
+}
+
 export interface ICodeActionRegistration {
   errorCodes: string[];
   fixId: string;
+  preferredAction?: IPreferredAction;
   getCodeActions(params: ICodeActionParams): CodeAction[] | undefined;
-  getFixAllCodeAction(params: ICodeActionParams): CodeAction | undefined;
+  getFixAllCodeAction(params: ICodeActionParams): ICodeAction | undefined;
+}
+
+export interface ICodeAction extends CodeAction {
+  data: {
+    fixId: string;
+    isFixAll?: boolean;
+  };
 }
 
-export interface IRefactorCodeAction extends CodeAction {
+export interface IRefactorCodeAction extends Omit<CodeAction, "isPreferred"> {
   data: {
     uri: string;
     refactorName: string;
@@ -59,6 +72,7 @@ export interface IRefactorEdit {
   renamePosition?: Position;
 }
 export interface IRefactorRegistration {
+  preferredAction?: IPreferredAction;
   getAvailableActions(params: ICodeActionParams): IRefactorCodeAction[];
   getEditsForAction(
     params: ICodeActionParams,
@@ -81,6 +95,8 @@ export class CodeActionProvider {
     IRefactorRegistration
   >();
 
+  private static preferredActions = new Map<string, IPreferredAction>();
+
   constructor() {
     this.settings = container.resolve("Settings");
     this.elmMake = container.resolve(ElmMakeDiagnostics);
@@ -127,6 +143,13 @@ export class CodeActionProvider {
     registration.errorCodes.forEach((code) => {
       CodeActionProvider.errorCodeToRegistrationMap.set(code, registration);
     });
+
+    if (registration.preferredAction) {
+      CodeActionProvider.preferredActions.set(
+        registration.fixId,
+        registration.preferredAction,
+      );
+    }
   }
 
   public static registerRefactorAction(
@@ -134,6 +157,13 @@ export class CodeActionProvider {
     registration: IRefactorRegistration,
   ): void {
     this.refactorRegistrations.set(name, registration);
+
+    if (registration.preferredAction) {
+      CodeActionProvider.preferredActions.set(
+        name,
+        registration.preferredAction,
+      );
+    }
   }
 
   private static getDiagnostics(
@@ -181,7 +211,6 @@ export class CodeActionProvider {
       title,
       kind: CodeActionKind.QuickFix,
       edit: { changes },
-      isPreferred: true,
       command,
     };
   }
@@ -196,7 +225,7 @@ export class CodeActionProvider {
       edits: { [uri: string]: TextEdit[] },
       diagnostic: Diagnostic,
     ) => void,
-  ): CodeAction {
+  ): ICodeAction {
     const edits: TextEdit[] = [];
     const changes = callbackChanges
       ? {}
@@ -242,7 +271,7 @@ export class CodeActionProvider {
       kind: CodeActionKind.QuickFix,
       diagnostics,
       edit: { changes },
-      data: fixId,
+      data: { fixId, isFixAll: true },
     };
   }
 
@@ -250,7 +279,7 @@ export class CodeActionProvider {
     this.connection.console.info("A code action was requested");
     const make = this.elmMake.onCodeAction(params);
 
-    const results: CodeAction[] = [];
+    const results: (ICodeAction | IRefactorCodeAction)[] = [];
 
     // For each diagnostic in the context, get the code action registration that
     // handles the diagnostic error code and ask for the code actions for that error
@@ -272,14 +301,14 @@ export class CodeActionProvider {
                 .getCodeActions(params)
                 ?.map((codeAction) =>
                   this.addDiagnosticToCodeAction(codeAction, diagnostic),
-                ) ?? [];
+                )
+                .map((codeAction) => ({
+                  ...codeAction,
+                  data: { fixId: reg.fixId },
+                })) ?? [];
 
             if (
               codeActions.length > 0 &&
-              !results.some(
-                // Check if there is already a "fix all" code action for this fix
-                (codeAction) => /* fixId */ codeAction.data === reg.fixId,
-              ) &&
               CodeActionProvider.getDiagnostics(
                 params,
                 this.diagnosticsProvider,
@@ -310,7 +339,13 @@ export class CodeActionProvider {
       ).flatMap((registration) => registration.getAvailableActions(params)),
     );
 
-    return [...results, ...make];
+    return [
+      ...results.map((codeAction, _, allActions) => ({
+        ...codeAction,
+        isPreferred: this.isPreferredFix(codeAction, allActions),
+      })),
+      ...make,
+    ];
   }
 
   private onCodeActionResolve(
@@ -351,4 +386,65 @@ export class CodeActionProvider {
     codeAction.diagnostics = [diagnostic];
     return codeAction;
   }
+
+  protected isPreferredFix(
+    action: ICodeAction | IRefactorCodeAction,
+    allActions: readonly (ICodeAction | IRefactorCodeAction)[],
+  ): boolean {
+    if ("isFixAll" in action.data && action.data.isFixAll) {
+      return false;
+    }
+
+    const fixIdOrRefactorName =
+      "fixId" in action.data ? action.data.fixId : action.data.refactorName;
+    const actionPriority = CodeActionProvider.preferredActions.get(
+      fixIdOrRefactorName,
+    );
+    if (!actionPriority) {
+      return false;
+    }
+
+    return allActions.every((otherAction) => {
+      if (otherAction === action) {
+        return true;
+      }
+
+      if ("isFixAll" in otherAction.data && otherAction.data.isFixAll) {
+        return true;
+      }
+
+      const otherFixIdOrRefactorName =
+        "fixId" in otherAction.data
+          ? otherAction.data.fixId
+          : otherAction.data.refactorName;
+
+      const otherActionPriority = CodeActionProvider.preferredActions.get(
+        otherFixIdOrRefactorName,
+      );
+      if (
+        !otherActionPriority ||
+        otherActionPriority.priority < actionPriority.priority
+      ) {
+        return true;
+      } else if (otherActionPriority.priority > actionPriority.priority) {
+        return false;
+      }
+
+      if (
+        actionPriority.thereCanOnlyBeOne &&
+        fixIdOrRefactorName === otherFixIdOrRefactorName
+      ) {
+        return false;
+      }
+
+      return true;
+    });
+  }
+
+  /**
+   * Only used for testing
+   */
+  protected static clearPreferredActions(): void {
+    CodeActionProvider.preferredActions.clear();
+  }
 }


Test patch:
diff --git a/test/codeActionTests/preferredActions.test.ts b/test/codeActionTests/preferredActions.test.ts
new file mode 100644
index 0000000..db3edb4
--- /dev/null
+++ b/test/codeActionTests/preferredActions.test.ts
@@ -0,0 +1,169 @@
+import { mockDeep } from "jest-mock-extended";
+import { container } from "tsyringe";
+import {
+  CodeActionProvider,
+  ICodeAction,
+  IRefactorCodeAction,
+} from "../../src/providers/codeActionProvider";
+import Parser from "web-tree-sitter";
+
+container.register<Parser>("Parser", {
+  useValue: mockDeep<Parser>({
+    getLanguage: () => ({
+      query: () => {
+        //
+      },
+    }),
+  }),
+});
+
+class MockCodeActionsProvider extends CodeActionProvider {
+  public isPreferredFix(
+    action: ICodeAction | IRefactorCodeAction,
+    allActions: (ICodeAction | IRefactorCodeAction)[],
+  ): boolean {
+    return super.isPreferredFix(action, allActions);
+  }
+
+  public static clearPreferredActions() {
+    super.clearPreferredActions();
+  }
+}
+
+describe("preferred code action tests", () => {
+  beforeEach(() => MockCodeActionsProvider.clearPreferredActions());
+
+  it("highest priority is set to preferred with code fixes", () => {
+    MockCodeActionsProvider.registerCodeAction({
+      fixId: "fix1",
+      errorCodes: [],
+      preferredAction: {
+        priority: 1,
+      },
+      getCodeActions: () => [],
+      getFixAllCodeAction: () => {
+        return undefined;
+      },
+    });
+
+    MockCodeActionsProvider.registerCodeAction({
+      fixId: "fix2",
+      errorCodes: [],
+      preferredAction: {
+        priority: 2,
+      },
+      getCodeActions: () => [],
+      getFixAllCodeAction: () => {
+        return undefined;
+      },
+    });
+
+    MockCodeActionsProvider.registerCodeAction({
+      fixId: "fix3",
+      errorCodes: [],
+      getCodeActions: () => [],
+      getFixAllCodeAction: () => {
+        return undefined;
+      },
+    });
+
+    const mock = new MockCodeActionsProvider();
+
+    const allActions: (ICodeAction | IRefactorCodeAction)[] = [
+      { title: "Fix 1", data: { fixId: "fix1" } },
+      { title: "Fix 2", data: { fixId: "fix2" } },
+      { title: "Fix 3", data: { fixId: "fix3" } },
+    ];
+
+    expect(mock.isPreferredFix(allActions[0], allActions)).toBe(false);
+    expect(mock.isPreferredFix(allActions[1], allActions)).toBe(true);
+    expect(mock.isPreferredFix(allActions[2], allActions)).toBe(false);
+  });
+
+  it("highest priority is set to preferred with refactor actions", () => {
+    MockCodeActionsProvider.registerCodeAction({
+      fixId: "fix1",
+      errorCodes: [],
+      preferredAction: {
+        priority: 0,
+      },
+      getCodeActions: () => [],
+      getFixAllCodeAction: () => {
+        return undefined;
+      },
+    });
+
+    MockCodeActionsProvider.registerRefactorAction("refactor1", {
+      preferredAction: {
+        priority: 1,
+      },
+      getAvailableActions: () => [],
+      getEditsForAction: () => ({}),
+    });
+
+    const mock = new MockCodeActionsProvider();
+
+    const allActions: (ICodeAction | IRefactorCodeAction)[] = [
+      { title: "Fix 1", data: { fixId: "fix1" } },
+      { title: "Refactor 1", data: { fixId: "refactor1" } },
+    ];
+
+    expect(mock.isPreferredFix(allActions[0], allActions)).toBe(false);
+    expect(mock.isPreferredFix(allActions[1], allActions)).toBe(true);
+  });
+
+  it("not preferred if no preference", () => {
+    MockCodeActionsProvider.registerCodeAction({
+      fixId: "fix1",
+      errorCodes: [],
+      getCodeActions: () => [],
+      getFixAllCodeAction: () => {
+        return undefined;
+      },
+    });
+
+    MockCodeActionsProvider.registerCodeAction({
+      fixId: "fix2",
+      errorCodes: [],
+      getCodeActions: () => [],
+      getFixAllCodeAction: () => {
+        return undefined;
+      },
+    });
+
+    const mock = new MockCodeActionsProvider();
+
+    const allActions: (ICodeAction | IRefactorCodeAction)[] = [
+      { title: "Fix 1", data: { fixId: "fix1" } },
+      { title: "Fix 2", data: { fixId: "fix2" } },
+    ];
+
+    expect(mock.isPreferredFix(allActions[0], allActions)).toBe(false);
+    expect(mock.isPreferredFix(allActions[1], allActions)).toBe(false);
+  });
+
+  it("not marked as preferred when there can only be one", () => {
+    MockCodeActionsProvider.registerCodeAction({
+      fixId: "fix1",
+      errorCodes: [],
+      preferredAction: {
+        priority: 1,
+        thereCanOnlyBeOne: true,
+      },
+      getCodeActions: () => [],
+      getFixAllCodeAction: () => {
+        return undefined;
+      },
+    });
+
+    const mock = new MockCodeActionsProvider();
+
+    const allActions: (ICodeAction | IRefactorCodeAction)[] = [
+      { title: "Fix 1", data: { fixId: "fix1" } },
+      { title: "Fix 1 - Another", data: { fixId: "fix1" } },
+    ];
+
+    expect(mock.isPreferredFix(allActions[0], allActions)).toBe(false);
+    expect(mock.isPreferredFix(allActions[1], allActions)).toBe(false);
+  });
+});