Better comments around splat and slurp functions

Also, address other small PR comments.

Author: aeisenberg

extensions/ql-vscode/src/interface.ts

@@ -316,7 +316,7 @@ export class InterfaceManager extends DisposableObject {
               // sortedResultsInfo doesn't have an entry for the current
               // result set. Use this to determine whether or not we use
               // the sorted bqrs file.
-              !!this._displayedQuery?.completedQuery.sortedResultsInfo[msg.selectedTable] || false
+              !!this._displayedQuery?.completedQuery.sortedResultsInfo[msg.selectedTable]
             );
           }
           break;

extensions/ql-vscode/src/query-results.ts

@@ -64,6 +64,10 @@ export class CompletedQueryInfo implements QueryWithResults {
    */
   interpretedResultsSortState: InterpretedResultsSortState | undefined;
 
+  /**
+   * Note that in the {@link FullQueryInfo.slurp} method, we create a CompletedQueryInfo instance
+   * by explicitly setting the prototype in order to avoid calling this constructor.
+   */
   constructor(
     evaluation: QueryWithResults,
   ) {
@@ -191,7 +195,8 @@ export class FullQueryInfo {
       return queries.map((q: FullQueryInfo) => {
 
         // Need to explicitly set prototype since reading in from JSON will not
-        // do this automatically.
+        // do this automatically. Note that we can't call the constructor here since
+        // the constructor invokes extra logic that we don't want to do.
         Object.setPrototypeOf(q, FullQueryInfo.prototype);
 
         // The config object is a global, se we need to set it explicitly
@@ -218,32 +223,43 @@ export class FullQueryInfo {
     }
   }
 
+  /**
+   * Save the query history to disk. It is not necessary that the parent directory
+   * exists, but if it does, it must be writable. An existing file will be overwritten.
+   *
+   * Any errors will be rethrown.
+   *
+   * @param queries the list of queries to save.
+   * @param fsPath the path to save the queries to.
+   */
   static async splat(queries: FullQueryInfo[], fsPath: string): Promise<void> {
     try {
       const data = JSON.stringify(queries, null, 2);
       await fs.mkdirp(path.dirname(fsPath));
       await fs.writeFile(fsPath, data);
     } catch (e) {
-      void showAndLogErrorMessage('Error saving query history.', {
-        fullMessage: ['Error saving query history.', e.stack].join('\n'),
-      });
+      throw new Error(`Error saving query history to ${fsPath}: ${e.message}`);
     }
   }
 
   public failureReason: string | undefined;
   public completedQuery: CompletedQueryInfo | undefined;
   private config: QueryHistoryConfig | undefined;
 
+  /**
+   * Note that in the {@link FullQueryInfo.slurp} method, we create a FullQueryInfo instance
+   * by explicitly setting the prototype in order to avoid calling this constructor.
+   */
   constructor(
     public readonly initialInfo: InitialQueryInfo,
     config: QueryHistoryConfig,
-    private readonly source: CancellationTokenSource
+    private readonly source?: CancellationTokenSource
   ) {
     this.setConfig(config);
   }
 
   cancel() {
-    this.source.cancel();
+    this.source?.cancel();
   }
 
   get startTime() {

extensions/ql-vscode/src/run-queries.ts

@@ -59,6 +59,10 @@ export const queriesDir = path.join(tmpDir.name, 'queries');
 export class QueryEvaluationInfo {
   readonly querySaveDir: string;
 
+  /**
+   * Note that in the {@link FullQueryInfo.slurp} method, we create a QueryEvaluationInfo instance
+   * by explicitly setting the prototype in order to avoid calling this constructor.
+   */
   constructor(
     public readonly id: string,
     public readonly dbItemPath: string,
@@ -190,16 +194,19 @@ export class QueryEvaluationInfo {
   canHaveInterpretedResults(): boolean {
     if (!this.databaseHasMetadataFile) {
       void logger.log('Cannot produce interpreted results since the database does not have a .dbinfo or codeql-database.yml file.');
+      return false;
     }
 
     const hasKind = !!this.metadata?.kind;
     if (!hasKind) {
       void logger.log('Cannot produce interpreted results since the query does not have @kind metadata.');
+      return false;
     }
 
+    // table is the default query kind. It does not produce interpreted results.
+    // any query kind that is not table can, in principle, produce interpreted results.
     const isTable = hasKind && this.metadata?.kind === 'table';
-
-    return this.databaseHasMetadataFile && hasKind && !isTable;
+    return !isTable;
   }
 
   /**
@@ -388,15 +395,13 @@ async function checkDbschemeCompatibility(
 
     // At this point, we have learned about three dbschemes:
 
-    // query.program.dbschemePath is the dbscheme of the actual
-    // database we're querying.
+    // the dbscheme of the actual database we're querying.
     const dbschemeOfDb = await hash(dbItem.contents.dbSchemeUri.fsPath);
 
-    // query.queryDbScheme is the dbscheme of the query we're
-    // running, including the library we've resolved it to use.
+    // the dbscheme of the query we're running, including the library we've resolved it to use.
     const dbschemeOfLib = await hash(query.queryDbscheme);
 
-    // info.finalDbscheme is which database we're able to upgrade to
+    // the database we're able to upgrade to
     const upgradableTo = await hash(finalDbscheme);
 
     if (upgradableTo != dbschemeOfLib) {
@@ -533,14 +538,13 @@ export async function determineSelectedQuery(selectedResourceUri: Uri | undefine
   if (queryUri.scheme !== 'file') {
     throw new Error('Can only run queries that are on disk.');
   }
-  const queryPath = queryUri.fsPath || '';
+  const queryPath = queryUri.fsPath;
 
   if (quickEval) {
     if (!(queryPath.endsWith('.ql') || queryPath.endsWith('.qll'))) {
       throw new Error('The selected resource is not a CodeQL file; It should have the extension ".ql" or ".qll".');
     }
-  }
-  else {
+  } else {
     if (!(queryPath.endsWith('.ql'))) {
       throw new Error('The selected resource is not a CodeQL query file; It should have the extension ".ql".');
     }
@@ -645,10 +649,11 @@ export async function compileAndRunQueryAgainstDatabase(
     }
   }
 
+  const hasMetadataFile = (await dbItem.hasMetadataFile());
   const query = new QueryEvaluationInfo(
     initialInfo.id,
     dbItem.databaseUri.fsPath,
-    (await dbItem.hasMetadataFile()),
+    hasMetadataFile,
     packConfig.dbscheme,
     initialInfo.quickEvalPosition,
     metadata,

extensions/ql-vscode/src/vscode-tests/no-workspace/run-queries.test.ts

@@ -6,7 +6,7 @@ import * as sinon from 'sinon';
 import * as chaiAsPromised from 'chai-as-promised';
 
 import { QueryEvaluationInfo, queriesDir } from '../../run-queries';
-import { QlProgram, Severity, compileQuery } from '../../pure/messages';
+import { Severity, compileQuery } from '../../pure/messages';
 import { Uri } from 'vscode';
 
 chai.use(chaiAsPromised);
@@ -21,7 +21,7 @@ describe('run-queries', () => {
     expect(info.dilPath).to.eq(path.join(queriesDir, queryId, 'results.dil'));
     expect(info.resultsPaths.resultsPath).to.eq(path.join(queriesDir, queryId, 'results.bqrs'));
     expect(info.resultsPaths.interpretedResultsPath).to.eq(path.join(queriesDir, queryId, 'interpretedResults.sarif'));
-    expect(info.dbItemPath).to.eq('file:///abc');
+    expect(info.dbItemPath).to.eq(Uri.file('/abc').fsPath);
   });
 
   it('should check if interpreted results can be created', async () => {
@@ -47,8 +47,10 @@ describe('run-queries', () => {
       const mockProgress = 'progress-monitor';
       const mockCancel = 'cancel-token';
       const mockQlProgram = {
-        mock: 'program'
-      } as unknown as QlProgram;
+        dbschemePath: '',
+        libraryPath: [],
+        queryPath: ''
+      };
 
       const results = await info.compile(
         qs as any,