Skip to content

Refactor interpreted-data interface in preparation for other interpretations than SARIF #811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hvitved wants to merge 2 commits into from
Closed

Refactor interpreted-data interface in preparation for other interpretations than SARIF #811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb239da
Refactor interpreted-data interface in preparation for other interpre…
hvitved Mar 25, 2021
6ef287a
Address review comments
hvitved Apr 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions extensions/ql-vscode/src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,20 +674,13 @@ export class CodeQLCliServer implements Disposable {
return await this.runJsonCodeQlCliCommand<DecodedBqrsChunk>(['bqrs', 'decode'], subcommandArgs, 'Reading bqrs data');
}

async runInterpretCommand(format: string, metadata: QueryMetadata, resultsPath: string, interpretedResultsPath: string, sourceInfo?: SourceInfo) {
async runInterpretCommand(format: string, additonalArgs: string[], metadata: QueryMetadata, resultsPath: string, interpretedResultsPath: string, sourceInfo?: SourceInfo) {
const args = [
'--output', interpretedResultsPath,
'--format', format,
// Forward all of the query metadata.
...Object.entries(metadata).map(([key, value]) => `-t=${key}=${value}`)
];
if (format == SARIF_FORMAT) {
// TODO: This flag means that we don't group interpreted results
// by primary location. We may want to revisit whether we call
// interpretation with and without this flag, or do some
// grouping client-side.
args.push('--no-group-results');
}
].concat(additonalArgs);
if (sourceInfo !== undefined) {
args.push(
'--source-archive', sourceInfo.sourceArchive,
Expand All @@ -709,13 +702,21 @@ export class CodeQLCliServer implements Disposable {
await this.runCodeQlCliCommand(['bqrs', 'interpret'], args, 'Interpreting query results');
}

async interpretBqrs(metadata: QueryMetadata, resultsPath: string, interpretedResultsPath: string, sourceInfo?: SourceInfo): Promise<sarif.Log> {
await this.runInterpretCommand(SARIF_FORMAT, metadata, resultsPath, interpretedResultsPath, sourceInfo);
async interpretBqrsSarif(metadata: QueryMetadata, resultsPath: string, interpretedResultsPath: string, sourceInfo?: SourceInfo): Promise<sarif.Log> {
const additionalArgs = [
// TODO: This flag means that we don't group interpreted results
// by primary location. We may want to revisit whether we call
// interpretation with and without this flag, or do some
// grouping client-side.
'--no-group-results'
];

await this.runInterpretCommand(SARIF_FORMAT, additionalArgs, metadata, resultsPath, interpretedResultsPath, sourceInfo);
return await sarifParser(interpretedResultsPath);
}

async generateResultsCsv(metadata: QueryMetadata, resultsPath: string, csvPath: string, sourceInfo?: SourceInfo): Promise<void> {
await this.runInterpretCommand(CSV_FORMAT, metadata, resultsPath, csvPath, sourceInfo);
await this.runInterpretCommand(CSV_FORMAT, [], metadata, resultsPath, csvPath, sourceInfo);
}

async sortBqrs(resultsPath: string, sortedResultsPath: string, resultSet: string, sortKeys: number[], sortDirections: SortDirection[]): Promise<void> {
Expand Down Expand Up @@ -1031,11 +1032,11 @@ class SplitBuffer {

/**
* A version of startsWith that isn't overriden by a broken version of ms-python.
*
*
* The definition comes from
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith
* which is CC0/public domain
*
*
* See https://github.com/github/vscode-codeql/issues/802 for more context as to why we need it.
*/
private static startsWith(s: string, searchString: string, position: number): boolean {
Expand Down
45 changes: 26 additions & 19 deletions extensions/ql-vscode/src/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
import { Logger } from './logging';
import * as messages from './pure/messages';
import { commandRunner } from './commandRunner';
import { CompletedQuery, interpretResults } from './query-results';
import { CompletedQuery, interpretResultsSarif } from './query-results';
import { QueryInfo, tmpDir } from './run-queries';
import { parseSarifLocation, parseSarifPlainTextMessage } from './pure/sarif-utils';
import {
Expand Down Expand Up @@ -92,7 +92,7 @@ function numPagesOfResultSet(resultSet: RawResultSet): number {
}

function numInterpretedPages(interpretation: Interpretation | undefined): number {
return Math.ceil((interpretation?.sarif.runs[0].results?.length || 0) / PAGE_SIZE.getValue<number>());
return Math.ceil((interpretation?.data.runs[0].results?.length || 0) / PAGE_SIZE.getValue<number>());
}

export class InterfaceManager extends DisposableObject {
Expand Down Expand Up @@ -139,9 +139,9 @@ export class InterfaceManager extends DisposableObject {
this._diagnosticCollection.clear();
if (this.isShowingPanel()) {
void this.postMessage({
t: 'untoggleShowProblems'
});
}
t: 'untoggleShowProblems'
});
}
}
})
);
Expand Down Expand Up @@ -470,7 +470,7 @@ export class InterfaceManager extends DisposableObject {
if (this._interpretation === undefined) {
throw new Error('Trying to show interpreted results but interpretation was undefined');
}
if (this._interpretation.sarif.runs[0].results === undefined) {
if (this._interpretation.data.t === 'SarifInterpretationData' && this._interpretation.data.runs[0].results === undefined) {
throw new Error('Trying to show interpreted results but results were undefined');
}

Expand Down Expand Up @@ -588,7 +588,7 @@ export class InterfaceManager extends DisposableObject {
return undefined;
}

const sarif = await interpretResults(
const sarif = await interpretResultsSarif(
this.cliServer,
metadata,
resultsPaths,
Expand All @@ -603,12 +603,12 @@ export class InterfaceManager extends DisposableObject {

const numTotalResults = sarif.runs[0]?.results?.length || 0;

sarif.sortState = sortState;
const interpretation: Interpretation = {
sarif,
data: sarif,
sourceLocationPrefix,
numTruncatedResults: 0,
numTotalResults,
sortState,
numTotalResults
};
this._interpretation = interpretation;
return interpretation;
Expand All @@ -617,7 +617,6 @@ export class InterfaceManager extends DisposableObject {
private getPageOfInterpretedResults(
pageNumber: number
): Interpretation {

function getPageOfRun(run: Sarif.Run): Sarif.Run {
return {
...run, results: run.results?.slice(
Expand All @@ -627,16 +626,21 @@ export class InterfaceManager extends DisposableObject {
};
}

if (this._interpretation === undefined) {
const interp = this._interpretation;
if (interp === undefined) {
throw new Error('Tried to get interpreted results before interpretation finished');
}
if (this._interpretation.sarif.runs.length !== 1) {
void this.logger.log(`Warning: SARIF file had ${this._interpretation.sarif.runs.length} runs, expected 1`);

if (interp.data.t !== 'SarifInterpretationData')
return interp;

if (interp.data.runs.length !== 1) {
void this.logger.log(`Warning: SARIF file had ${interp.data.runs.length} runs, expected 1`);
}
const interp = this._interpretation;

return {
...interp,
sarif: { ...interp.sarif, runs: [getPageOfRun(interp.sarif.runs[0])] },
data: { ...interp.data, runs: [getPageOfRun(interp.data.runs[0])] },
};
}

Expand Down Expand Up @@ -722,9 +726,12 @@ export class InterfaceManager extends DisposableObject {
interpretation: Interpretation,
databaseItem: DatabaseItem
): Promise<void> {
const { sarif, sourceLocationPrefix } = interpretation;
const { data, sourceLocationPrefix } = interpretation;

if (data.t !== 'SarifInterpretationData')
return;

if (!sarif.runs || !sarif.runs[0].results) {
if (!data.runs || !data.runs[0].results) {
void this.logger.log(
'Didn\'t find a run in the sarif results. Error processing sarif?'
);
Expand All @@ -733,7 +740,7 @@ export class InterfaceManager extends DisposableObject {

const diagnostics: [Uri, ReadonlyArray<Diagnostic>][] = [];

for (const result of sarif.runs[0].results) {
for (const result of data.runs[0].results) {
const message = result.message.text;
if (message === undefined) {
void this.logger.log('Sarif had result without plaintext message');
Expand Down
30 changes: 20 additions & 10 deletions extensions/ql-vscode/src/pure/interface-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@ export const SELECT_TABLE_NAME = '#select';
export const ALERTS_TABLE_NAME = 'alerts';

export type RawTableResultSet = { t: 'RawResultSet' } & RawResultSet;
export type PathTableResultSet = {
t: 'SarifResultSet';
export type InterpretedResultSet<T> = {
t: 'InterpretedResultSet';
readonly schema: ResultSetSchema;
name: string;
} & Interpretation;
interpretation: InterpretationT<T>;

@aeisenberg aeisenberg Mar 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
interpretation: InterpretationT<T>;
interpretation: Interpretation<T>;

};

export type ResultSet = RawTableResultSet | PathTableResultSet;
export type ResultSet = RawTableResultSet | InterpretedResultSet<InterpretationData>;

/**
* Only ever show this many rows in a raw result table.
Expand Down Expand Up @@ -46,18 +47,27 @@ export interface PreviousExecution {
durationSeconds: number;
}

export interface Interpretation {
sourceLocationPrefix: string;
numTruncatedResults: number;
numTotalResults: number;
export type SarifInterpretationData = {
t: 'SarifInterpretationData';

@aeisenberg aeisenberg Mar 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, you are added a nested t field. This seems to complicate things somewhat. Would it make sense to keep only one t field at the top level, and specify RawResultSet, SarifResultSet, and (eventually) GraphResultSet?

I don't remember your other PR, so maybe there's a good reason to keep this.

@hvitved hvitved Apr 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in preparation for

export type GraphInterpretationData = {
  t: 'GraphInterpretationData';
  dot: string[];
};

(from the other PR) so we can do dispatch on the types (this is how to do tagged unions in Typescript, right?)

/**
* sortState being undefined means don't sort, just present results in the order
* they appear in the sarif file.
*/
sortState?: InterpretedResultsSortState;
sarif: sarif.Log;
} & sarif.Log;

// Add more interpretation data kinds when needed (e.g., graph data)
export type InterpretationData = SarifInterpretationData;

export interface InterpretationT<T> {

@aeisenberg aeisenberg Mar 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export interface InterpretationT<T> {
export interface Interpretation<T> {

sourceLocationPrefix: string;
numTruncatedResults: number;
numTotalResults: number;
data: T;
}

export type Interpretation = InterpretationT<InterpretationData>;

@aeisenberg aeisenberg Mar 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type Interpretation = InterpretationT<InterpretationData>;
export type Interpretation = Interpretation<InterpretationData>;

@hvitved hvitved Apr 7, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to work, I guess you can't have both a generic type and a non-generic type with the same name?


export interface ResultsPaths {
resultsPath: string;
interpretedResultsPath: string;
Expand Down Expand Up @@ -358,7 +368,7 @@ export function getDefaultResultSetName(
return [
ALERTS_TABLE_NAME,
SELECT_TABLE_NAME,
resultSetNames[0],
resultSetNames[0]
].filter((resultSetName) => resultSetNames.includes(resultSetName))[0];
}

Expand Down
14 changes: 7 additions & 7 deletions extensions/ql-vscode/src/query-results.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@ import { env } from 'vscode';
import { QueryWithResults, tmpDir, QueryInfo } from './run-queries';
import * as messages from './pure/messages';
import * as cli from './cli';
import * as sarif from 'sarif';
import * as fs from 'fs-extra';
import * as path from 'path';
import { RawResultsSortState, SortedResultSetInfo, DatabaseInfo, QueryMetadata, InterpretedResultsSortState, ResultsPaths } from './pure/interface-types';
import { RawResultsSortState, SortedResultSetInfo, DatabaseInfo, QueryMetadata, InterpretedResultsSortState, ResultsPaths, SarifInterpretationData } from './pure/interface-types';
import { QueryHistoryConfig } from './config';
import { QueryHistoryItemOptions } from './query-history';

Expand Down Expand Up @@ -182,19 +181,20 @@ export function getQueryFileName(query: QueryInfo) {


/**
* Call cli command to interpret results.
* Call cli command to interpret SARIF results.
*/
export async function interpretResults(
export async function interpretResultsSarif(
server: cli.CodeQLCliServer,
metadata: QueryMetadata | undefined,
resultsPaths: ResultsPaths,
sourceInfo?: cli.SourceInfo
): Promise<sarif.Log> {
): Promise<SarifInterpretationData> {
const { resultsPath, interpretedResultsPath } = resultsPaths;
if (await fs.pathExists(interpretedResultsPath)) {
return JSON.parse(await fs.readFile(interpretedResultsPath, 'utf8'));
return { ...JSON.parse(await fs.readFile(interpretedResultsPath, 'utf8')), t: 'SarifInterpretationData' };
}
return await server.interpretBqrs(ensureMetadataIsComplete(metadata), resultsPath, interpretedResultsPath, sourceInfo);
const res = await server.interpretBqrsSarif(ensureMetadataIsComplete(metadata), resultsPath, interpretedResultsPath, sourceInfo);
return { ...res, t: 'SarifInterpretationData' };
}

export function ensureMetadataIsComplete(metadata: QueryMetadata | undefined) {
Expand Down
18 changes: 9 additions & 9 deletions extensions/ql-vscode/src/view/alert-table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as Keys from '../pure/result-keys';
import * as octicons from './octicons';
import { className, renderLocation, ResultTableProps, zebraStripe, selectableZebraStripe, jumpToLocation, nextSortDirection, emptyQueryResultsMessage } from './result-table-utils';
import { onNavigation, NavigationEvent } from './results';
import { PathTableResultSet } from '../pure/interface-types';
import { InterpretedResultSet, SarifInterpretationData } from '../pure/interface-types';
import {
parseSarifPlainTextMessage,
parseSarifLocation,
Expand All @@ -15,7 +15,7 @@ import { InterpretedResultsSortColumn, SortDirection, InterpretedResultsSortStat
import { vscode } from './vscode-api';
import { isWholeFileLoc, isLineColumnLoc } from '../pure/bqrs-utils';

export type PathTableProps = ResultTableProps & { resultSet: PathTableResultSet };
export type PathTableProps = ResultTableProps & { resultSet: InterpretedResultSet<SarifInterpretationData> };
export interface PathTableState {
expanded: { [k: string]: boolean };
selectedPathNode: undefined | Keys.PathNode;
Expand Down Expand Up @@ -51,7 +51,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
}

sortClass(column: InterpretedResultsSortColumn): string {
const sortState = this.props.resultSet.sortState;
const sortState = this.props.resultSet.interpretation.data.sortState;
if (sortState !== undefined && sortState.sortBy === column) {
return sortState.sortDirection === SortDirection.asc ? 'sort-asc' : 'sort-desc';
}
Expand All @@ -61,7 +61,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
}

getNextSortState(column: InterpretedResultsSortColumn): InterpretedResultsSortState | undefined {
const oldSortState = this.props.resultSet.sortState;
const oldSortState = this.props.resultSet.interpretation.data.sortState;
const prevDirection = oldSortState && oldSortState.sortBy === column ? oldSortState.sortDirection : undefined;
const nextDirection = nextSortDirection(prevDirection, true);
return nextDirection === undefined ? undefined :
Expand Down Expand Up @@ -94,7 +94,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
</thead>;

const rows: JSX.Element[] = [];
const { numTruncatedResults, sourceLocationPrefix } = resultSet;
const { numTruncatedResults, sourceLocationPrefix } = resultSet.interpretation;

function renderRelatedLocations(msg: string, relatedLocations: Sarif.Location[]): JSX.Element[] {
const relatedLocationsById: { [k: string]: Sarif.Location } = {};
Expand Down Expand Up @@ -188,13 +188,13 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
return (e) => this.toggle(e, indices);
};

if (!resultSet.sarif.runs?.[0]?.results?.length) {
if (!resultSet.interpretation.data.runs?.[0]?.results?.length) {
return this.renderNoResults();
}

let expansionIndex = 0;

resultSet.sarif.runs[0].results.forEach((result, resultIndex) => {
resultSet.interpretation.data.runs[0].results.forEach((result, resultIndex) => {
const text = result.message.text || '[no text]';
const msg: JSX.Element[] =
result.relatedLocations === undefined ?
Expand Down Expand Up @@ -307,7 +307,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
const { selectedPathNode } = prevState;
if (selectedPathNode === undefined) return prevState;

const path = Keys.getPath(this.props.resultSet.sarif, selectedPathNode);
const path = Keys.getPath(this.props.resultSet.interpretation.data, selectedPathNode);
if (path === undefined) return prevState;

const nextIndex = selectedPathNode.pathNodeIndex + event.direction;
Expand All @@ -318,7 +318,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
return prevState;
}

const loc = parseSarifLocation(sarifLoc, this.props.resultSet.sourceLocationPrefix);
const loc = parseSarifLocation(sarifLoc, this.props.resultSet.interpretation.sourceLocationPrefix);
if (isNoLocation(loc)) {
return prevState;
}
Expand Down
Loading