_Correctly_ emulate VS Code's saveBeforeStart

github · dbartol · Jun 21, 2023 · Jun 21, 2023 · Jun 21, 2023 · Jun 21, 2023
commit 0bd0bf1944a42181cb3397f1366134d91dc84536
@@ -358,6 +358,8 @@ export class LocalQueries extends DisposableObject {
     range?: Range,
     templates?: Record<string, string>,
   ): Promise<CoreCompletedQuery> {
+    await saveBeforeStart();
+
     let queryPath: string;
     if (queryUri !== undefined) {
       // The query URI is provided by the command, most likely because the command was run from an
@@ -388,8 +390,6 @@ export class LocalQueries extends DisposableObject {
     const additionalPacks = getOnDiskWorkspaceFolders();
     const extensionPacks = await this.getDefaultExtensionPacks(additionalPacks);
 
-    await saveBeforeStart();
-
     const coreQueryRun = this.queryRunner.createQueryRun(
       databaseItem.databaseUri.fsPath,
       {

@@ -2,14 +2,7 @@ import * as messages from "./pure/messages-shared";
 import * as legacyMessages from "./pure/legacy-messages";
 import { DatabaseInfo, QueryMetadata } from "./common/interface-types";
 import { join, parse, dirname, basename } from "path";
-import {
-  Range,
-  TextDocument,
-  TextEditor,
-  Uri,
-  window,
-  workspace,
-} from "vscode";
+import { Range, TextEditor, Uri, window, workspace } from "vscode";
 import { isCanary, VSCODE_SAVE_BEFORE_START_SETTING } from "./config";
 import {
   pathExists,
@@ -503,61 +496,29 @@ export async function saveBeforeStart(): Promise<void> {
     (VSCODE_SAVE_BEFORE_START_SETTING.getValue<string>() as SaveBeforeStartMode) ??
     "nonUntitledEditorsInActiveGroup";
 
+  // Despite the names of the modes, the VS Code implementation doesn't restrict itself to the
+  // current tab group. It saves all dirty files in all groups. We'll do the same.
   switch (mode) {
     case "nonUntitledEditorsInActiveGroup":
-      await saveAllInGroup(false);
+      await workspace.saveAll(false);
       break;
 
     case "allEditorsInActiveGroup":
-      await saveAllInGroup(true);
+      // The VS Code implementation of this mode only saves an untitled file if it is the document
+      // in the active editor. That's too much work for us, so we'll just live with the inconsistency.
+      await workspace.saveAll(true);
       break;
 
     case "none":
       break;
 
     default:
       // Unexpected value. Fall back to the default behavior.
-      await saveAllInGroup(false);
+      await workspace.saveAll(false);
       break;
   }
 }
 
-// Used in tests
-export async function saveAllInGroup(includeUntitled: boolean): Promise<void> {
-  // There's no good way to get from a `Tab` to a `TextDocument`, so we'll collect all of the dirty
-  // documents indexed by their URI, and then compare those URIs against the URIs of the tabs.
-  const dirtyDocumentUris = new Map<string, TextDocument>();
-  for (const openDocument of workspace.textDocuments) {
-    if (openDocument.isDirty) {
-      console.warn(`${openDocument.uri.toString()} is dirty.`);
-      if (!openDocument.isUntitled || includeUntitled) {
-        dirtyDocumentUris.set(openDocument.uri.toString(), openDocument);
-      }
-    }
-  }
-  if (dirtyDocumentUris.size > 0) {
-    const tabGroup = window.tabGroups.activeTabGroup;
-    for (const tab of tabGroup.tabs) {
-      const input = tab.input;
-      // The `input` property can be of an arbitrary type, depending on the underlying tab type. For
-      // text editors (and potentially others), it's an object with a `uri` property. That's all we
-      // need to know to match it up with a dirty document.
-      if (typeof input === "object") {
-        const uri = (input as any).uri;
-        if (uri instanceof Uri) {
-          const document = dirtyDocumentUris.get(uri.toString());
-          if (document !== undefined) {
-            await document.save();
-            // Remove the URI from the dirty list so we don't wind up saving the same file twice
-            // if it's open in multiple editors.
-            dirtyDocumentUris.delete(uri.toString());
-          }
-        }
-      }
-    }
-  }
-}
-
 /**
  * @param filePath This needs to be equivalent to Java's `Path.toRealPath(NO_FOLLOW_LINKS)`
  */

diff --git a/extensions/ql-vscode/test/data/debugger/clean.ql b/extensions/ql-vscode/test/data/debugger/clean.ql
@@ -0,0 +1,20 @@
+---
+lockVersion: 1.0.0
+dependencies:
+  codeql/javascript-all:
+    version: 0.6.3
+  codeql/javascript-queries:
+    version: 0.6.3
+  codeql/regex:
+    version: 0.0.14
+  codeql/suite-helpers:
+    version: 0.5.3
+  codeql/tutorial:
+    version: 0.0.11
+  codeql/typos:
+    version: 0.0.18
+  codeql/util:
+    version: 0.0.11
+  codeql/yaml:
+    version: 0.0.3
+compiled: false
diff --git a/extensions/ql-vscode/test/data/debugger/dirty.ql b/extensions/ql-vscode/test/data/debugger/dirty.ql
diff --git a/extensions/ql-vscode/test/data/debugger/other-clean.ql b/extensions/ql-vscode/test/data/debugger/other-clean.ql
diff --git a/extensions/ql-vscode/test/data/debugger/other-dirty.ql b/extensions/ql-vscode/test/data/debugger/other-dirty.ql
@@ -0,0 +1,4 @@
+name: integration-test-debugger-javascript
+version: 0.0.0
+dependencies:
+  codeql/javascript-queries: "*"
@@ -3,6 +3,7 @@ import { join, resolve } from "path";
 import { CodeQLCliServer } from "../../../../src/codeql-cli/cli";
 import { getActivatedExtension } from "../../global.helper";
 import { tryGetQueryMetadata } from "../../../../src/codeql-cli/query-metadata";
+import { getDataFolderFilePath } from "../utils";
 
 describe("tryGetQueryMetadata", () => {
   const baseDir = resolve(__dirname, "..");
@@ -30,7 +31,7 @@ describe("tryGetQueryMetadata", () => {
     // Query with empty metadata
     const noMetadata = await tryGetQueryMetadata(
       cli,
-      join(baseDir, "data", "simple-query.ql"),
+      getDataFolderFilePath("debugger/simple-query.ql"),
     );
 
     expect(noMetadata).toEqual({});

@@ -1,5 +1,11 @@
-import { Selection, Uri, window, workspace } from "vscode";
-import { join } from "path";
+import {
+  Position,
+  Selection,
+  TextEditor,
+  Uri,
+  window,
+  workspace,
+} from "vscode";
 
 import { DatabaseManager } from "../../../../src/databases/local-databases";
 import {
@@ -13,17 +19,20 @@ import { CodeQLCliServer } from "../../../../src/codeql-cli/cli";
 import { QueryOutputDir } from "../../../../src/run-queries-shared";
 import { createVSCodeCommandManager } from "../../../../src/common/vscode/commands";
 import { AllCommands } from "../../../../src/common/commands";
+import { getDataFolderFilePath } from "../utils";
 
 async function selectForQuickEval(
   path: string,
   line: number,
   column: number,
   endLine: number,
   endColumn: number,
-): Promise<void> {
+): Promise<TextEditor> {
   const document = await workspace.openTextDocument(path);
   const editor = await window.showTextDocument(document);
   editor.selection = new Selection(line, column, endLine, endColumn);
+
+  return editor;
 }
 
 async function getResultCount(
@@ -42,9 +51,11 @@ describeWithCodeQL()("Debugger", () => {
   let databaseManager: DatabaseManager;
   let cli: CodeQLCliServer;
   const appCommands = createVSCodeCommandManager<AllCommands>();
-  const simpleQueryPath = join(__dirname, "..", "data", "simple-query.ql");
-  const quickEvalQueryPath = join(__dirname, "..", "data", "QuickEvalQuery.ql");
-  const quickEvalLibPath = join(__dirname, "..", "data", "QuickEvalLib.qll");
+  const simpleQueryPath = getDataFolderFilePath("debugger/simple-query.ql");
+  const quickEvalQueryPath = getDataFolderFilePath(
+    "debugger/QuickEvalQuery.ql",
+  );
+  const quickEvalLibPath = getDataFolderFilePath("debugger/QuickEvalLib.qll");
 
   beforeEach(async () => {
     const extension = await getActivatedExtension();
@@ -138,4 +149,35 @@ describeWithCodeQL()("Debugger", () => {
       await controller.expectStopped();
     });
   });
+
+  it("should save dirty documents before launching a debug session", async () => {
+    await withDebugController(appCommands, async (controller) => {
+      const editor = await selectForQuickEval(quickEvalLibPath, 4, 15, 4, 32);
+      await editor.edit((editBuilder) => {
+        editBuilder.insert(new Position(0, 0), "/* comment */");
+      });
+      expect(editor.document.isDirty).toBe(true);
+
+      await controller.startDebuggingSelection({
+        query: quickEvalQueryPath, // The query context. This query extends the abstract class.
+      });
+      await controller.expectLaunched();
+
+      // Should have saved the dirty document.
+      expect(editor.document.isDirty).toBe(false);
+
+      await controller.expectSucceeded();
+      await controller.expectStopped();
+
+      await editor.edit((editBuilder) => {
+        editBuilder.insert(new Position(0, 0), "/* another comment */");
+      });
+      expect(editor.document.isDirty).toBe(true);
+
+      await controller.continueDebuggingSelection();
+      await controller.expectSucceeded();
+      await controller.expectStopped();
+      expect(editor.document.isDirty).toBe(false);
+    });
+  });
 });
@@ -1,10 +1,17 @@
 const path = require("path");
+const fs = require("fs");
 
 const {
   config: baseConfig,
+  tmpDir,
   rootDir,
 } = require("../jest-runner-vscode.config.base");
 
+const tmpDataDir = path.join(tmpDir.name, "data");
+fs.cpSync(path.resolve(rootDir, "test/data"), tmpDataDir, {
+  recursive: true,
+});
+
 /** @type import("jest-runner-vscode").RunnerOptions */
 const config = {
   ...baseConfig,
@@ -17,7 +24,7 @@ const config = {
     "github.codespaces",
     "--disable-extension",
     "github.copilot",
-    path.resolve(rootDir, "test/data"),
+    tmpDataDir,
     path.resolve(rootDir, "test/data-extensions"), // folder containing the extension packs and packs that are targeted by the extension pack
     // CLI integration tests requires a multi-root workspace so that the data and the QL sources are accessible.
     ...(process.env.TEST_CODEQL_PATH ? [process.env.TEST_CODEQL_PATH] : []),

@@ -38,6 +38,9 @@ import {
 } from "../../../src/common/commands";
 import { ProgressCallback } from "../../../src/common/vscode/progress";
 import { withDebugController } from "./debugger/debug-controller";
+import { getDataFolderFilePath } from "./utils";
+
+const simpleQueryPath = getDataFolderFilePath("debugger/simple-query.ql");
 
 type DebugMode = "localQueries" | "debug";
 
@@ -213,13 +216,12 @@ describeWithCodeQL()("Queries", () => {
 
   describe.each(MODES)("running queries (%s)", (mode) => {
     it("should run a query", async () => {
-      const queryPath = join(__dirname, "data", "simple-query.ql");
       const result = await compileAndRunQuery(
         mode,
         appCommandManager,
         localQueries,
         QuickEvalType.None,
-        Uri.file(queryPath),
+        Uri.file(simpleQueryPath),
         progress,
         token,
         dbItem,
@@ -233,13 +235,12 @@ describeWithCodeQL()("Queries", () => {
     // Asserts a fix for bug https://github.com/github/vscode-codeql/issues/733
     it("should restart the database and run a query", async () => {
       await appCommandManager.execute("codeQL.restartQueryServer");
-      const queryPath = join(__dirname, "data", "simple-query.ql");
       const result = await compileAndRunQuery(
         mode,
         appCommandManager,
         localQueries,
         QuickEvalType.None,
-        Uri.file(queryPath),
+        Uri.file(simpleQueryPath),
         progress,
         token,
         dbItem,

@@ -0,0 +1,9 @@
+import { workspace } from "vscode";
+import { join } from "path";
+
+/**
+ * Get the absolute path to a file in the temporary copy of the `data` folder.
+ */
+export function getDataFolderFilePath(path: string): string {
+  return join(workspace.workspaceFolders![0].uri.fsPath, path);
+}