Merge pull request #2157 from github/starcke/commands-registration

Commands registration

extensions/ql-vscode/src/common/app.ts

@@ -3,6 +3,7 @@ import { Disposable } from "../pure/disposable-object";
 import { AppEventEmitter } from "./events";
 import { Logger } from "./logging";
 import { Memento } from "./memento";
+import { AppCommandManager } from "./commands";
 
 export interface App {
   createEventEmitter<T>(): AppEventEmitter<T>;
@@ -15,6 +16,7 @@ export interface App {
   readonly workspaceStoragePath?: string;
   readonly workspaceState: Memento;
   readonly credentials: Credentials;
+  readonly commands: AppCommandManager;
 }
 
 export enum AppMode {

extensions/ql-vscode/src/common/commands.ts

@@ -0,0 +1,24 @@
+import { CommandManager } from "../packages/commands";
+
+/**
+ * Contains type definitions for all commands used by the extension.
+ *
+ * To add a new command first define its type here, then provide
+ * the implementation in the corresponding `getCommands` function.
+ */
+
+// Base commands not tied directly to a module like e.g. variant analysis.
+export type BaseCommands = {
+  "codeQL.openDocumentation": () => Promise<void>;
+};
+
+// Commands tied to variant analysis
+export type VariantAnalysisCommands = {
+  "codeQL.openVariantAnalysisLogs": (
+    variantAnalysisId: number,
+  ) => Promise<void>;
+};
+
+export type AllCommands = BaseCommands & VariantAnalysisCommands;
+
+export type AppCommandManager = CommandManager<AllCommands>;

extensions/ql-vscode/src/common/vscode/commands.ts

@@ -0,0 +1,32 @@
+import { commands } from "vscode";
+import { commandRunner } from "../../commandRunner";
+import { CommandFunction, CommandManager } from "../../packages/commands";
+
+/**
+ * Create a command manager for VSCode, wrapping the commandRunner
+ * and vscode.executeCommand.
+ */
+export function createVSCodeCommandManager<
+  Commands extends Record<string, CommandFunction>,
+>(): CommandManager<Commands> {
+  return new CommandManager(commandRunner, wrapExecuteCommand);
+}
+
+/**
+ * wrapExecuteCommand wraps commands.executeCommand to satisfy that the
+ * type is a Promise. Type script does not seem to be smart enough
+ * to figure out that `ReturnType<Commands[CommandName]>` is actually
+ * a Promise, so we need to add a second layer of wrapping and unwrapping
+ * (The `Promise<Awaited<` part) to get the right types.
+ */
+async function wrapExecuteCommand<
+  Commands extends Record<string, CommandFunction>,
+  CommandName extends keyof Commands & string = keyof Commands & string,
+>(
+  commandName: CommandName,
+  ...args: Parameters<Commands[CommandName]>
+): Promise<Awaited<ReturnType<Commands[CommandName]>>> {
+  return await commands.executeCommand<
+    Awaited<ReturnType<Commands[CommandName]>>
+  >(commandName, ...args);
+}

extensions/ql-vscode/src/common/vscode/vscode-app.ts

@@ -6,14 +6,19 @@ import { AppEventEmitter } from "../events";
 import { extLogger, Logger } from "../logging";
 import { Memento } from "../memento";
 import { VSCodeAppEventEmitter } from "./events";
+import { AppCommandManager } from "../commands";
+import { createVSCodeCommandManager } from "./commands";
 
 export class ExtensionApp implements App {
   public readonly credentials: VSCodeCredentials;
+  public readonly commands: AppCommandManager;
 
   public constructor(
     public readonly extensionContext: vscode.ExtensionContext,
   ) {
     this.credentials = new VSCodeCredentials();
+    this.commands = createVSCodeCommandManager();
+    extensionContext.subscriptions.push(this.commands);
   }
 
   public get extensionPath(): string {

extensions/ql-vscode/src/extension.ts

@@ -136,6 +136,7 @@ import { RepositoriesFilterSortStateWithIds } from "./pure/variant-analysis-filt
 import { DbModule } from "./databases/db-module";
 import { redactableError } from "./pure/errors";
 import { QueryHistoryDirs } from "./query-history/query-history-dirs";
+import { AllCommands, BaseCommands } from "./common/commands";
 
 /**
  * extension.ts
@@ -167,6 +168,17 @@ let isInstallingOrUpdatingDistribution = false;
 const extensionId = "GitHub.vscode-codeql";
 const extension = extensions.getExtension(extensionId);
 
+/**
+ * Return all commands that are not tied to the more specific managers.
+ */
+function getCommands(): BaseCommands {
+  return {
+    "codeQL.openDocumentation": async () => {
+      await env.openExternal(Uri.parse("https://codeql.github.com/docs/"));
+    },
+  };
+}
+
 /**
  * If the user tries to execute vscode commands after extension activation is failed, give
  * a sensible error message.
@@ -1191,14 +1203,14 @@ async function activateWithInstalledDistribution(
     ),
   );
 
-  ctx.subscriptions.push(
-    commandRunner(
-      "codeQL.openVariantAnalysisLogs",
-      async (variantAnalysisId: number) => {
-        await variantAnalysisManager.openVariantAnalysisLogs(variantAnalysisId);
-      },
-    ),
-  );
+  const allCommands: AllCommands = {
+    ...getCommands(),
+    ...variantAnalysisManager.getCommands(),
+  };
+
+  for (const [commandName, command] of Object.entries(allCommands)) {
+    app.commands.register(commandName as keyof AllCommands, command);
+  }
 
   ctx.subscriptions.push(
     commandRunner(
@@ -1410,12 +1422,6 @@ async function activateWithInstalledDistribution(
     ),
   );
 
-  ctx.subscriptions.push(
-    commandRunner("codeQL.openDocumentation", async () =>
-      env.openExternal(Uri.parse("https://codeql.github.com/docs/")),
-    ),
-  );
-
   ctx.subscriptions.push(
     commandRunner("codeQL.copyVersion", async () => {
       const text = `CodeQL extension version: ${

extensions/ql-vscode/src/packages/commands/CommandManager.ts

@@ -1 +1,70 @@
-export class CommandManager {}
+/**
+ * Contains a generic implementation of typed commands.
+ *
+ * This allows different parts of the extension to register commands with a certain type,
+ * and then allow other parts to call those commands in a well-typed manner.
+ */
+
+import { Disposable } from "./Disposable";
+
+/**
+ * A command function is a completely untyped command.
+ */
+export type CommandFunction = (...args: any[]) => Promise<unknown>;
+
+/**
+ * The command manager basically takes a single input, the type
+ * of all the known commands. The second parameter is provided by
+ * default (and should not be needed by the caller) it is a
+ * technicality to allow the type system to look up commands.
+ */
+export class CommandManager<
+  Commands extends Record<string, CommandFunction>,
+  CommandName extends keyof Commands & string = keyof Commands & string,
+> implements Disposable
+{
+  // TODO: should this be a map?
+  // TODO: handle multiple command names
+  private commands: Disposable[] = [];
+
+  constructor(
+    private readonly commandRegister: <T extends CommandName>(
+      commandName: T,
+      fn: Commands[T],
+    ) => Disposable,
+    private readonly commandExecute: <T extends CommandName>(
+      commandName: T,
+      ...args: Parameters<Commands[T]>
+    ) => Promise<Awaited<ReturnType<Commands[T]>>>,
+  ) {}
+
+  /**
+   * Register a command with the specified name and implementation.
+   */
+  register<T extends CommandName>(
+    commandName: T,
+    definition: Commands[T],
+  ): void {
+    this.commands.push(this.commandRegister(commandName, definition));
+  }
+
+  /**
+   * Execute a command with the specified name and the provided arguments.
+   */
+  execute<T extends CommandName>(
+    commandName: T,
+    ...args: Parameters<Commands[T]>
+  ): Promise<Awaited<ReturnType<Commands[T]>>> {
+    return this.commandExecute(commandName, ...args);
+  }
+
+  /**
+   * Dispose the manager, disposing all the registered commands.
+   */
+  dispose(): void {
+    this.commands.forEach((cmd) => {
+      cmd.dispose();
+    });
+    this.commands = [];
+  }
+}

extensions/ql-vscode/src/packages/commands/Disposable.ts

@@ -0,0 +1,7 @@
+/**
+ * This interface mirrors the vscode.Disaposable class, so that
+ * the command manager does not depend on vscode directly.
+ */
+export interface Disposable {
+  dispose(): void;
+}

extensions/ql-vscode/src/variant-analysis/variant-analysis-manager.ts

@@ -62,6 +62,7 @@ import { URLSearchParams } from "url";
 import { DbManager } from "../databases/db-manager";
 import { App } from "../common/app";
 import { redactableError } from "../pure/errors";
+import { AppCommandManager, VariantAnalysisCommands } from "../common/commands";
 
 export class VariantAnalysisManager
   extends DisposableObject
@@ -123,6 +124,18 @@ export class VariantAnalysisManager
     );
   }
 
+  getCommands(): VariantAnalysisCommands {
+    return {
+      "codeQL.openVariantAnalysisLogs": async (variantAnalysisId: number) => {
+        await this.openVariantAnalysisLogs(variantAnalysisId);
+      },
+    };
+  }
+
+  get commandManager(): AppCommandManager {
+    return this.app.commands;
+  }
+
   public async runVariantAnalysis(
     uri: Uri | undefined,
     progress: ProgressCallback,

extensions/ql-vscode/src/variant-analysis/variant-analysis-view-manager.ts

@@ -2,6 +2,7 @@ import {
   VariantAnalysis,
   VariantAnalysisScannedRepositoryState,
 } from "./shared/variant-analysis";
+import { AppCommandManager } from "../common/commands";
 
 export interface VariantAnalysisViewInterface {
   variantAnalysisId: number;
@@ -11,6 +12,8 @@ export interface VariantAnalysisViewInterface {
 export interface VariantAnalysisViewManager<
   T extends VariantAnalysisViewInterface,
 > {
+  commandManager: AppCommandManager;
+
   registerView(view: T): void;
   unregisterView(view: T): void;
   getView(variantAnalysisId: number): T | undefined;

extensions/ql-vscode/src/variant-analysis/variant-analysis-view.ts

@@ -145,7 +145,7 @@ export class VariantAnalysisView
         );
         break;
       case "openLogs":
-        await commands.executeCommand(
+        await this.manager.commandManager.execute(
           "codeQL.openVariantAnalysisLogs",
           this.variantAnalysisId,
         );

extensions/ql-vscode/test/__mocks__/appMock.ts

@@ -6,6 +6,8 @@ import { createMockLogger } from "./loggerMock";
 import { createMockMemento } from "../mock-memento";
 import { testCredentialsWithStub } from "../factories/authentication";
 import { Credentials } from "../../src/common/authentication";
+import { AppCommandManager } from "../../src/common/commands";
+import { createMockCommandManager } from "./commandsMock";
 
 export function createMockApp({
   extensionPath = "/mock/extension/path",
@@ -15,6 +17,7 @@ export function createMockApp({
   executeCommand = jest.fn(() => Promise.resolve()),
   workspaceState = createMockMemento(),
   credentials = testCredentialsWithStub(),
+  commands = createMockCommandManager(),
 }: {
   extensionPath?: string;
   workspaceStoragePath?: string;
@@ -23,6 +26,7 @@ export function createMockApp({
   executeCommand?: () => Promise<void>;
   workspaceState?: Memento;
   credentials?: Credentials;
+  commands?: AppCommandManager;
 }): App {
   return {
     mode: AppMode.Test,
@@ -35,6 +39,7 @@ export function createMockApp({
     createEventEmitter,
     executeCommand,
     credentials,
+    commands,
   };
 }
 

extensions/ql-vscode/test/__mocks__/commandsMock.ts

@@ -0,0 +1,13 @@
+import { AppCommandManager } from "../../src/common/commands";
+import { CommandFunction, CommandManager } from "../../src/packages/commands";
+import { Disposable } from "../../src/packages/commands/Disposable";
+
+export function createMockCommandManager({
+  registerCommand = jest.fn(),
+  executeCommand = jest.fn(),
+}: {
+  registerCommand?: (commandName: string, fn: CommandFunction) => Disposable;
+  executeCommand?: (commandName: string, ...args: any[]) => Promise<any>;
+} = {}): AppCommandManager {
+  return new CommandManager(registerCommand, executeCommand);
+}

extensions/ql-vscode/test/factories/extension-context.ts

@@ -19,5 +19,6 @@ export function createMockExtensionContext({
     globalStorageUri: vscode.Uri.file(globalStoragePath),
     storageUri: vscode.Uri.file(workspaceStoragePath),
     workspaceState: createMockMemento(),
+    subscriptions: [],
   } as any as vscode.ExtensionContext;
 }

extensions/ql-vscode/test/unit-tests/packages/commands/CommandManager.test.ts

@@ -1,8 +1,111 @@
-import { CommandManager } from "../../../../src/packages/commands";
+import {
+  CommandFunction,
+  CommandManager,
+} from "../../../../src/packages/commands";
 
-describe(CommandManager.name, () => {
-  it("can create a command manager", () => {
-    const commandManager = new CommandManager();
-    expect(commandManager).not.toBeUndefined();
+describe("CommandManager", () => {
+  it("can register a command", () => {
+    const commandRegister = jest.fn();
+    const commandManager = new CommandManager<Record<string, CommandFunction>>(
+      commandRegister,
+      jest.fn(),
+    );
+    const myCommand = jest.fn();
+    commandManager.register("abc", myCommand);
+    expect(commandRegister).toHaveBeenCalledTimes(1);
+    expect(commandRegister).toHaveBeenCalledWith("abc", myCommand);
+  });
+
+  it("can register typed commands", async () => {
+    const commands = {
+      "codeQL.openVariantAnalysisLogs": async (variantAnalysisId: number) => {
+        return variantAnalysisId * 10;
+      },
+    };
+    const commandManager = new CommandManager<typeof commands>(
+      jest.fn(),
+      jest.fn(),
+    );
+
+    // @ts-expect-error wrong command name should give a type error
+    commandManager.register("abc", jest.fn());
+
+    commandManager.register(
+      "codeQL.openVariantAnalysisLogs",
+      // @ts-expect-error wrong function parameter type should give a type error
+      async (variantAnalysisId: string): Promise<number> => 10,
+    );
+
+    commandManager.register(
+      "codeQL.openVariantAnalysisLogs",
+      // @ts-expect-error wrong function return type should give a type error
+      async (variantAnalysisId: number): Promise<string> => "hello",
+    );
+
+    // Working types
+    commandManager.register(
+      "codeQL.openVariantAnalysisLogs",
+      async (variantAnalysisId: number): Promise<number> =>
+        variantAnalysisId * 10,
+    );
+  });
+
+  it("can dispose of its commands", () => {
+    const dispose1 = jest.fn();
+    const dispose2 = jest.fn();
+    const commandRegister = jest
+      .fn()
+      .mockReturnValueOnce({ dispose: dispose1 })
+      .mockReturnValueOnce({ dispose: dispose2 });
+    const commandManager = new CommandManager<Record<string, CommandFunction>>(
+      commandRegister,
+      jest.fn(),
+    );
+    commandManager.register("abc", jest.fn());
+    commandManager.register("def", jest.fn());
+    expect(dispose1).not.toHaveBeenCalled();
+    expect(dispose2).not.toHaveBeenCalled();
+    commandManager.dispose();
+    expect(dispose1).toHaveBeenCalledTimes(1);
+    expect(dispose2).toHaveBeenCalledTimes(1);
+  });
+
+  it("can execute a command", async () => {
+    const commandExecute = jest.fn().mockReturnValue(7);
+    const commandManager = new CommandManager<Record<string, CommandFunction>>(
+      jest.fn(),
+      commandExecute,
+    );
+    const result = await commandManager.execute("abc", "hello", true);
+    expect(result).toEqual(7);
+    expect(commandExecute).toHaveBeenCalledTimes(1);
+    expect(commandExecute).toHaveBeenCalledWith("abc", "hello", true);
+  });
+
+  it("can execute typed commands", async () => {
+    const commands = {
+      "codeQL.openVariantAnalysisLogs": async (variantAnalysisId: number) => {
+        return variantAnalysisId * 10;
+      },
+    };
+    const commandManager = new CommandManager<typeof commands>(
+      jest.fn(),
+      jest.fn(),
+    );
+
+    // @ts-expect-error wrong command name should give a type error
+    await commandManager.execute("abc", 4);
+
+    await commandManager.execute(
+      "codeQL.openVariantAnalysisLogs",
+      // @ts-expect-error wrong argument type should give a type error
+      "xyz",
+    );
+
+    // @ts-expect-error wrong number of arguments should give a type error
+    await commandManager.execute("codeQL.openVariantAnalysisLogs", 2, 3);
+
+    // Working types
+    await commandManager.execute("codeQL.openVariantAnalysisLogs", 7);
   });
 });