From 84028434e0088815b4d03fa9a496b4854b7d761f Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Thu, 6 Aug 2020 09:49:34 -0700 Subject: [PATCH] Refactor: Remove duplicated BQRS types This refactoring combines the types in `bqrs-types.ts` and `bqrs-cli-types.ts`. Historically, the former was used for BQRS files parsed by the extension and the latter for BQRS files parsed by the cli. They describe the same file types, but using different property and type names. We have moved to parsing all BQRS files by the cli. This refactoring removes the `bqrs-types.ts` file and replaces all BQRS references to use types in `bqrs-cli-types.ts`. Additionally, the `adapt.ts` file has been deleted since its purpose was to convert between extension and cli BQRS types. Some one type and one function from `adapt.ts` has been moved from `adapt.ts` to `bqrs-types.ts`. It's possible that we want to do a further refactoring to simply remove them both. --- extensions/ql-vscode/src/adapt.ts | 113 ------------------ extensions/ql-vscode/src/astViewer.ts | 21 +++- extensions/ql-vscode/src/bqrs-cli-types.ts | 31 ++--- extensions/ql-vscode/src/bqrs-types.ts | 102 ---------------- extensions/ql-vscode/src/bqrs-utils.ts | 60 ++++++---- extensions/ql-vscode/src/cli.ts | 1 - .../src/compare/compare-interface.ts | 6 +- .../ql-vscode/src/compare/resultsDiff.ts | 2 +- .../src/compare/view/CompareTable.tsx | 2 +- extensions/ql-vscode/src/databases.ts | 4 +- extensions/ql-vscode/src/interface-types.ts | 18 +-- extensions/ql-vscode/src/interface-utils.ts | 41 +++---- extensions/ql-vscode/src/interface.ts | 22 ++-- extensions/ql-vscode/src/sarif-utils.ts | 76 +++++++----- .../ql-vscode/src/view/RawTableHeader.tsx | 12 +- extensions/ql-vscode/src/view/RawTableRow.tsx | 4 +- .../ql-vscode/src/view/RawTableValue.tsx | 24 ++-- extensions/ql-vscode/src/view/alert-table.tsx | 59 ++++----- .../ql-vscode/src/view/raw-results-table.tsx | 2 +- .../ql-vscode/src/view/result-table-utils.tsx | 4 +- .../ql-vscode/src/view/result-tables.tsx | 10 +- extensions/ql-vscode/src/view/results.tsx | 15 ++- .../no-workspace/interface-utils.test.ts | 49 +++----- .../test/pure-tests/location.test.ts | 47 +++----- 24 files changed, 261 insertions(+), 464 deletions(-) delete mode 100644 extensions/ql-vscode/src/adapt.ts delete mode 100644 extensions/ql-vscode/src/bqrs-types.ts diff --git a/extensions/ql-vscode/src/adapt.ts b/extensions/ql-vscode/src/adapt.ts deleted file mode 100644 index e351f98a6..000000000 --- a/extensions/ql-vscode/src/adapt.ts +++ /dev/null @@ -1,113 +0,0 @@ -import { DecodedBqrsChunk, ResultSetSchema, ColumnKind, Column, ColumnValue } from './bqrs-cli-types'; -import { LocationValue, ResultSetSchema as AdaptedSchema, ColumnSchema, ColumnType, LocationStyle } from './bqrs-types'; -import { ResultSet } from './interface-types'; - -// FIXME: This is a temporary bit of impedance matching to convert -// from the types provided by ./bqrs-cli-types, to the types used by -// the view layer. -// -// The reason that it is benign for now is that it is only used by -// feature-flag-guarded codepaths that won't be encountered by normal -// users. It is not yet guaranteed to produce correct output for raw -// results. -// -// Eventually, the view layer should be refactored to directly accept data -// of types coming from bqrs-cli-types, and this file can be deleted. - -export type ResultRow = ResultValue[]; - -export interface ResultElement { - label: string; - location?: LocationValue; -} - -export interface ResultUri { - uri: string; -} - -export type ResultValue = ResultElement | ResultUri | string; - -export interface RawResultSet { - readonly schema: AdaptedSchema; - readonly rows: readonly ResultRow[]; -} - -function adaptKind(kind: ColumnKind): ColumnType { - // XXX what about 'u'? - if (kind === 'e') { - return { type: 'e', primitiveType: 's', locationStyle: LocationStyle.FivePart, hasLabel: true }; - } - else { - return { type: kind }; - } -} - -function adaptColumn(col: Column): ColumnSchema { - return { name: col.name!, type: adaptKind(col.kind) }; -} - -export function adaptSchema(schema: ResultSetSchema): AdaptedSchema { - return { - columns: schema.columns.map(adaptColumn), - name: schema.name, - tupleCount: schema.rows, - version: 0, - }; -} - -export function adaptValue(val: ColumnValue): ResultValue { - // XXX taking a lot of incorrect shortcuts here - - if (typeof val === 'string') { - return val; - } - - if (typeof val === 'number' || typeof val === 'boolean') { - return val + ''; - } - - const url = val.url; - - if (typeof url === 'string') { - return url; - } - - if (url === undefined) { - return val.label || ''; - } - - return { - label: val.label || '', - location: { - t: LocationStyle.FivePart, - lineStart: url.startLine, - lineEnd: url.endLine, - colStart: url.startColumn, - colEnd: url.endColumn, - // FIXME: This seems definitely wrong. Should we be using - // something like the code in sarif-utils.ts? - file: url.uri.replace(/file:/, ''), - } - }; - -} - -export function adaptRow(row: ColumnValue[]): ResultRow { - return row.map(adaptValue); -} - -export function adaptBqrs(schema: AdaptedSchema, page: DecodedBqrsChunk): RawResultSet { - return { - schema, - rows: page.tuples.map(adaptRow), - }; -} - -export interface ParsedResultSets { - pageNumber: number; - numPages: number; - numInterpretedPages: number; - selectedTable?: string; // when undefined, means 'show default table' - resultSetNames: string[]; - resultSet: ResultSet; -} diff --git a/extensions/ql-vscode/src/astViewer.ts b/extensions/ql-vscode/src/astViewer.ts index 99fa8039c..592c89f9e 100644 --- a/extensions/ql-vscode/src/astViewer.ts +++ b/extensions/ql-vscode/src/astViewer.ts @@ -4,6 +4,7 @@ import { DatabaseItem } from './databases'; import { UrlValue, BqrsId } from './bqrs-cli-types'; import fileRangeFromURI from './contextual/fileRangeFromURI'; import { showLocation } from './interface-utils'; +import { isStringLoc, isWholeFileLoc, isLineColumnLoc } from './bqrs-utils'; export interface AstItem { id: BqrsId; @@ -33,7 +34,7 @@ class AstViewerDataProvider implements vscode.TreeDataProvider 1 && matches[1]) { if (isWholeFileMatch(matches)) { return { - t: LocationStyle.WholeFile, - file: matches[1], - }; + uri: matches[1], + } as WholeFileLocation; } else { return { - t: LocationStyle.FivePart, - file: matches[1], - lineStart: Number(matches[2]), - colStart: Number(matches[3]), - lineEnd: Number(matches[4]), - colEnd: Number(matches[5]), + uri: matches[1], + startLine: Number(matches[2]), + startColumn: Number(matches[3]), + endLine: Number(matches[4]), + endColumn: Number(matches[5]), }; } } else { @@ -75,8 +67,26 @@ function isWholeFileMatch(matches: RegExpExecArray): boolean { * the file path is empty. If so, we do not want to render this location * as a link. * - * @param path A file path + * @param uri A file uri */ -function isEmptyPath(path: string) { - return !path || path === '/'; +function isEmptyPath(uriStr: string) { + return !uriStr || uriStr === 'file:/'; +} + +export function isLineColumnLoc(loc: UrlValue): loc is LineColumnLocation { + return typeof loc !== 'string' + && !isEmptyPath(loc.uri) + && 'startLine' in loc + && 'startColumn' in loc + && 'endLine' in loc + && 'endColumn' in loc + && loc.endColumn > 0; +} + +export function isWholeFileLoc(loc: UrlValue): loc is WholeFileLocation { + return typeof loc !== 'string' && !isEmptyPath(loc.uri) && !isLineColumnLoc(loc); +} + +export function isStringLoc(loc: UrlValue): loc is string { + return typeof loc === 'string'; } diff --git a/extensions/ql-vscode/src/cli.ts b/extensions/ql-vscode/src/cli.ts index 9fc9a59d3..25b9ff104 100644 --- a/extensions/ql-vscode/src/cli.ts +++ b/extensions/ql-vscode/src/cli.ts @@ -525,7 +525,6 @@ export class CodeQLCliServer implements Disposable { return await this.runJsonCodeQlCliCommand(['bqrs', 'decode'], subcommandArgs, 'Reading bqrs data'); } - async interpretBqrs(metadata: { kind: string; id: string }, resultsPath: string, interpretedResultsPath: string, sourceInfo?: SourceInfo): Promise { const args = [ `-t=kind=${metadata.kind}`, diff --git a/extensions/ql-vscode/src/compare/compare-interface.ts b/extensions/ql-vscode/src/compare/compare-interface.ts index da506afa8..71877f3be 100644 --- a/extensions/ql-vscode/src/compare/compare-interface.ts +++ b/extensions/ql-vscode/src/compare/compare-interface.ts @@ -19,8 +19,7 @@ import { Logger } from '../logging'; import { CodeQLCliServer } from '../cli'; import { DatabaseManager } from '../databases'; import { getHtmlForWebview, jumpToLocation } from '../interface-utils'; -import { adaptSchema, adaptBqrs, RawResultSet } from '../adapt'; -import { BQRSInfo } from '../bqrs-cli-types'; +import { adaptBqrs, RawResultSet, BQRSInfo } from '../bqrs-cli-types'; import resultsDiff from './resultsDiff'; interface ComparePair { @@ -257,8 +256,7 @@ export class CompareInterfaceManager extends DisposableObject { resultsPath, resultSetName ); - const adaptedSchema = adaptSchema(schema); - return adaptBqrs(adaptedSchema, chunk); + return adaptBqrs(schema, chunk); } private compareResults( diff --git a/extensions/ql-vscode/src/compare/resultsDiff.ts b/extensions/ql-vscode/src/compare/resultsDiff.ts index 7f9f4f06c..60b685abd 100644 --- a/extensions/ql-vscode/src/compare/resultsDiff.ts +++ b/extensions/ql-vscode/src/compare/resultsDiff.ts @@ -1,4 +1,4 @@ -import { RawResultSet } from '../adapt'; +import { RawResultSet } from '../bqrs-cli-types'; import { QueryCompareResult } from '../interface-types'; /** diff --git a/extensions/ql-vscode/src/compare/view/CompareTable.tsx b/extensions/ql-vscode/src/compare/view/CompareTable.tsx index c26d15b5f..a7dff6ec0 100644 --- a/extensions/ql-vscode/src/compare/view/CompareTable.tsx +++ b/extensions/ql-vscode/src/compare/view/CompareTable.tsx @@ -3,7 +3,7 @@ import * as React from 'react'; import { SetComparisonsMessage } from '../../interface-types'; import RawTableHeader from '../../view/RawTableHeader'; import { className } from '../../view/result-table-utils'; -import { ResultRow } from '../../adapt'; +import { ResultRow } from '../../bqrs-cli-types'; import RawTableRow from '../../view/RawTableRow'; import { vscode } from '../../view/vscode-api'; diff --git a/extensions/ql-vscode/src/databases.ts b/extensions/ql-vscode/src/databases.ts index 47892157e..61a97a176 100644 --- a/extensions/ql-vscode/src/databases.ts +++ b/extensions/ql-vscode/src/databases.ts @@ -316,8 +316,10 @@ class DatabaseItemImpl implements DatabaseItem { } } - public resolveSourceFile(file: string | undefined): vscode.Uri { + public resolveSourceFile(uri: string | undefined): vscode.Uri { const sourceArchive = this.sourceArchive; + // FIXME: This is wrong. Should parse the uri properly first + const file = uri?.replace(/file:/, ''); if (sourceArchive === undefined) { if (file !== undefined) { // Treat it as an absolute path. diff --git a/extensions/ql-vscode/src/interface-types.ts b/extensions/ql-vscode/src/interface-types.ts index 48d40c5c4..100484613 100644 --- a/extensions/ql-vscode/src/interface-types.ts +++ b/extensions/ql-vscode/src/interface-types.ts @@ -1,10 +1,5 @@ import * as sarif from 'sarif'; -import { - ResolvableLocationValue, - ColumnSchema, - ResultSetSchema, -} from './bqrs-types'; -import { ResultRow, ParsedResultSets, RawResultSet } from './adapt'; +import { RawResultSet, ResultRow, ResultSetSchema, Column, ResolvableLocationValue } from './bqrs-cli-types'; /** * This module contains types and code that are shared between @@ -243,7 +238,7 @@ export interface SetComparisonsMessage { time: string; }; }; - readonly columns: readonly ColumnSchema[]; + readonly columns: readonly Column[]; readonly commonResultSetNames: string[]; readonly currentResultSetName: string; readonly rows: QueryCompareResult | undefined; @@ -293,3 +288,12 @@ export function getDefaultResultSetName( resultSetNames[0], ].filter((resultSetName) => resultSetNames.includes(resultSetName))[0]; } + +export interface ParsedResultSets { + pageNumber: number; + numPages: number; + numInterpretedPages: number; + selectedTable?: string; // when undefined, means 'show default table' + resultSetNames: string[]; + resultSet: ResultSet; +} diff --git a/extensions/ql-vscode/src/interface-utils.ts b/extensions/ql-vscode/src/interface-utils.ts index c5273563b..ef02aff9b 100644 --- a/extensions/ql-vscode/src/interface-utils.ts +++ b/extensions/ql-vscode/src/interface-utils.ts @@ -12,19 +12,14 @@ import { TextEditorRevealType, ThemeColor, } from 'vscode'; -import { - FivePartLocation, - LocationStyle, - LocationValue, - WholeFileLocation, - ResolvableLocationValue, -} from './bqrs-types'; import { tryGetResolvableLocation, + isLineColumnLoc } from './bqrs-utils'; import { DatabaseItem, DatabaseManager } from './databases'; import { ViewSourceFileMsg } from './interface-types'; import { Logger } from './logging'; +import { LineColumnLocation, WholeFileLocation, UrlValue, ResolvableLocationValue } from './bqrs-cli-types'; /** * This module contains functions and types that are sharedd between @@ -61,19 +56,19 @@ export function fileUriToWebviewUri( * @param databaseItem Database in which to resolve the file location. */ function resolveFivePartLocation( - loc: FivePartLocation, + loc: LineColumnLocation, databaseItem: DatabaseItem ): Location { // `Range` is a half-open interval, and is zero-based. CodeQL locations are closed intervals, and // are one-based. Adjust accordingly. const range = new Range( - Math.max(0, loc.lineStart - 1), - Math.max(0, loc.colStart - 1), - Math.max(0, loc.lineEnd - 1), - Math.max(0, loc.colEnd) + Math.max(0, loc.startLine - 1), + Math.max(0, loc.startColumn - 1), + Math.max(0, loc.endLine - 1), + Math.max(0, loc.endColumn) ); - return new Location(databaseItem.resolveSourceFile(loc.file), range); + return new Location(databaseItem.resolveSourceFile(loc.uri), range); } /** @@ -87,7 +82,7 @@ function resolveWholeFileLocation( ): Location { // A location corresponding to the start of the file. const range = new Range(0, 0, 0, 0); - return new Location(databaseItem.resolveSourceFile(loc.file), range); + return new Location(databaseItem.resolveSourceFile(loc.uri), range); } /** @@ -97,20 +92,16 @@ function resolveWholeFileLocation( * @param databaseItem Database in which to resolve the file location. */ export function tryResolveLocation( - loc: LocationValue | undefined, + loc: UrlValue | undefined, databaseItem: DatabaseItem ): Location | undefined { const resolvableLoc = tryGetResolvableLocation(loc); - if (resolvableLoc === undefined) { - return undefined; - } - switch (resolvableLoc.t) { - case LocationStyle.FivePart: - return resolveFivePartLocation(resolvableLoc, databaseItem); - case LocationStyle.WholeFile: - return resolveWholeFileLocation(resolvableLoc, databaseItem); - default: - return undefined; + if (!resolvableLoc || typeof resolvableLoc === 'string') { + return; + } else if (isLineColumnLoc(resolvableLoc)) { + return resolveFivePartLocation(resolvableLoc, databaseItem); + } else { + return resolveWholeFileLocation(resolvableLoc, databaseItem); } } diff --git a/extensions/ql-vscode/src/interface.ts b/extensions/ql-vscode/src/interface.ts index 288308ba5..18b03ad9f 100644 --- a/extensions/ql-vscode/src/interface.ts +++ b/extensions/ql-vscode/src/interface.ts @@ -36,12 +36,6 @@ import * as messages from './messages'; import { CompletedQuery, interpretResults } from './query-results'; import { QueryInfo, tmpDir } from './run-queries'; import { parseSarifLocation, parseSarifPlainTextMessage } from './sarif-utils'; -import { - adaptSchema, - adaptBqrs, - ParsedResultSets, - RawResultSet, -} from './adapt'; import { WebviewReveal, fileUriToWebviewUri, @@ -51,8 +45,8 @@ import { shownLocationLineDecoration, jumpToLocation, } from './interface-utils'; -import { getDefaultResultSetName } from './interface-types'; -import { ResultSetSchema } from './bqrs-cli-types'; +import { getDefaultResultSetName, ParsedResultSets } from './interface-types'; +import { RawResultSet, adaptBqrs, ResultSetSchema } from './bqrs-cli-types'; /** * interface.ts @@ -94,7 +88,7 @@ function sortInterpretedResults( } function numPagesOfResultSet(resultSet: RawResultSet): number { - return Math.ceil(resultSet.schema.tupleCount / RAW_RESULTS_PAGE_SIZE); + return Math.ceil(resultSet.schema.rows / RAW_RESULTS_PAGE_SIZE); } function numInterpretedPages(interpretation: Interpretation | undefined): number { @@ -378,8 +372,7 @@ export class InterfaceManager extends DisposableObject { pageSize: RAW_RESULTS_PAGE_SIZE } ); - const adaptedSchema = adaptSchema(schema); - const resultSet = adaptBqrs(adaptedSchema, chunk); + const resultSet = adaptBqrs(schema, chunk); return { pageNumber: 0, numPages: numPagesOfResultSet(resultSet), @@ -492,8 +485,7 @@ export class InterfaceManager extends DisposableObject { pageSize: RAW_RESULTS_PAGE_SIZE } ); - const adaptedSchema = adaptSchema(schema); - const resultSet = adaptBqrs(adaptedSchema, chunk); + const resultSet = adaptBqrs(schema, chunk); const parsedResultSets: ParsedResultSets = { pageNumber, @@ -684,7 +676,7 @@ export class InterfaceManager extends DisposableObject { result.locations[0], sourceLocationPrefix ); - if (sarifLoc.t == 'NoLocation') { + if ('hint' in sarifLoc) { continue; } const resultLocation = tryResolveLocation(sarifLoc, databaseItem); @@ -709,7 +701,7 @@ export class InterfaceManager extends DisposableObject { relatedLocationsById[section.dest], sourceLocationPrefix ); - if (sarifChunkLoc.t == 'NoLocation') { + if ('hint' in sarifChunkLoc) { continue; } const referenceLocation = tryResolveLocation( diff --git a/extensions/ql-vscode/src/sarif-utils.ts b/extensions/ql-vscode/src/sarif-utils.ts index a39d570d4..4a8467da4 100644 --- a/extensions/ql-vscode/src/sarif-utils.ts +++ b/extensions/ql-vscode/src/sarif-utils.ts @@ -1,21 +1,23 @@ import * as Sarif from 'sarif'; import * as path from 'path'; -import { LocationStyle, ResolvableLocationValue } from './bqrs-types'; +import { ResolvableLocationValue } from './bqrs-cli-types'; export interface SarifLink { dest: number; text: string; } +interface NoLocation { + hint: string; +} type ParsedSarifLocation = - | ResolvableLocationValue - // Resolvable locations have a `file` field, but it will sometimes include + | (ResolvableLocationValue & { userVisibleFile: string }) + // Resolvable locations have a `uri` field, but it will sometimes include // a source location prefix, which contains build-specific information the user // doesn't really need to see. We ensure that `userVisibleFile` will not contain // that, and is appropriate for display in the UI. - & { userVisibleFile: string } - | { t: 'NoLocation'; hint: string }; + | NoLocation; export type SarifMessageComponent = string | SarifLink @@ -57,68 +59,78 @@ export function parseSarifPlainTextMessage(message: string): SarifMessageCompone * @returns A string that is valid for the `.file` field of a `FivePartLocation`: * directory separators are normalized, but drive letters `C:` may appear. */ -export function getPathRelativeToSourceLocationPrefix(sourceLocationPrefix: string, sarifRelativeUui: string) { +export function getPathRelativeToSourceLocationPrefix( + sourceLocationPrefix: string, + sarifRelativeUui: string +) { const normalizedSourceLocationPrefix = sourceLocationPrefix.replace(/\\/g, '/'); - return path.join(normalizedSourceLocationPrefix, decodeURIComponent(sarifRelativeUui)); + return `file:${ + path.join(normalizedSourceLocationPrefix, decodeURIComponent(sarifRelativeUui)) + }`; } -export function parseSarifLocation(loc: Sarif.Location, sourceLocationPrefix: string): ParsedSarifLocation { +export function parseSarifLocation( + loc: Sarif.Location, + sourceLocationPrefix: string +): ParsedSarifLocation { const physicalLocation = loc.physicalLocation; if (physicalLocation === undefined) - return { t: 'NoLocation', hint: 'no physical location' }; + return { hint: 'no physical location' }; if (physicalLocation.artifactLocation === undefined) - return { t: 'NoLocation', hint: 'no artifact location' }; + return { hint: 'no artifact location' }; if (physicalLocation.artifactLocation.uri === undefined) - return { t: 'NoLocation', hint: 'artifact location has no uri' }; + return { hint: 'artifact location has no uri' }; // This is not necessarily really an absolute uri; it could either be a // file uri or a relative uri. const uri = physicalLocation.artifactLocation.uri; + // FIXME: This is probably wrong const fileUriRegex = /^file:/; - const effectiveLocation = uri.match(fileUriRegex) ? - decodeURIComponent(uri.replace(fileUriRegex, '')) : - getPathRelativeToSourceLocationPrefix(sourceLocationPrefix, uri); - const userVisibleFile = uri.match(fileUriRegex) ? - decodeURIComponent(uri.replace(fileUriRegex, '')) : - uri; + const effectiveLocation = uri.match(fileUriRegex) + ? uri + : getPathRelativeToSourceLocationPrefix(sourceLocationPrefix, uri); + const userVisibleFile = uri.match(fileUriRegex) + ? decodeURIComponent(uri.replace(fileUriRegex, '')) + : uri; if (physicalLocation.region === undefined) { // If the region property is absent, the physicalLocation object refers to the entire file. // Source: https://docs.oasis-open.org/sarif/sarif/v2.1.0/cs01/sarif-v2.1.0-cs01.html#_Toc16012638. - // TODO: Do we get here if we provide a non-filesystem URL? return { - t: LocationStyle.WholeFile, - file: effectiveLocation, - userVisibleFile, - }; + uri: effectiveLocation, + userVisibleFile + } as ParsedSarifLocation; } else { const region = physicalLocation.region; // We assume that the SARIF we're given always has startLine // This is not mandated by the SARIF spec, but should be true of // SARIF output by our own tools. - const lineStart = region.startLine!; + const startLine = region.startLine!; // These defaults are from SARIF 2.1.0 spec, section 3.30.2, "Text Regions" // https://docs.oasis-open.org/sarif/sarif/v2.1.0/cs01/sarif-v2.1.0-cs01.html#_Ref493492556 - const lineEnd = region.endLine === undefined ? lineStart : region.endLine; - const colStart = region.startColumn === undefined ? 1 : region.startColumn; + const endLine = region.endLine === undefined ? startLine : region.endLine; + const startColumn = region.startColumn === undefined ? 1 : region.startColumn; // We also assume that our tools will always supply `endColumn` field, which is // fortunate, since the SARIF spec says that it defaults to the end of the line, whose // length we don't know at this point in the code. // // It is off by one with respect to the way vscode counts columns in selections. - const colEnd = region.endColumn! - 1; + const endColumn = region.endColumn! - 1; return { - t: LocationStyle.FivePart, - file: effectiveLocation, + uri: effectiveLocation, userVisibleFile, - lineStart, - colStart, - lineEnd, - colEnd, + startLine, + startColumn, + endLine, + endColumn, }; } } + +export function isNoLocation(loc: ParsedSarifLocation): loc is NoLocation { + return 'hint' in loc; +} diff --git a/extensions/ql-vscode/src/view/RawTableHeader.tsx b/extensions/ql-vscode/src/view/RawTableHeader.tsx index ac187561b..885d5e37e 100644 --- a/extensions/ql-vscode/src/view/RawTableHeader.tsx +++ b/extensions/ql-vscode/src/view/RawTableHeader.tsx @@ -3,10 +3,10 @@ import * as React from 'react'; import { vscode } from './vscode-api'; import { RawResultsSortState, SortDirection } from '../interface-types'; import { nextSortDirection } from './result-table-utils'; -import { ColumnSchema } from '../bqrs-types'; +import { Column } from '../bqrs-cli-types'; interface Props { - readonly columns: readonly ColumnSchema[]; + readonly columns: readonly Column[]; readonly schemaName: string; readonly sortState?: RawResultsSortState; readonly preventSort?: boolean; @@ -31,9 +31,9 @@ function toggleSortStateForColumn( nextDirection === undefined ? undefined : { - columnIndex: index, - sortDirection: nextDirection, - }; + columnIndex: index, + sortDirection: nextDirection, + }; vscode.postMessage({ t: 'changeSort', resultSetName: schemaName, @@ -46,7 +46,7 @@ export default function RawTableHeader(props: Props) { {[ - ( + ( # diff --git a/extensions/ql-vscode/src/view/RawTableRow.tsx b/extensions/ql-vscode/src/view/RawTableRow.tsx index 24f4f8442..cea36c474 100644 --- a/extensions/ql-vscode/src/view/RawTableRow.tsx +++ b/extensions/ql-vscode/src/view/RawTableRow.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { ResultRow } from '../adapt'; +import { ResultRow } from '../bqrs-cli-types'; import { zebraStripe } from './result-table-utils'; import RawTableValue from './RawTableValue'; @@ -23,6 +23,6 @@ export default function RawTableRow(props: Props) { /> ))} - + ); } diff --git a/extensions/ql-vscode/src/view/RawTableValue.tsx b/extensions/ql-vscode/src/view/RawTableValue.tsx index 8f45982eb..b4ce7b341 100644 --- a/extensions/ql-vscode/src/view/RawTableValue.tsx +++ b/extensions/ql-vscode/src/view/RawTableValue.tsx @@ -1,22 +1,30 @@ import * as React from 'react'; -import { ResultValue } from '../adapt'; import { renderLocation } from './result-table-utils'; +import { ColumnValue } from '../bqrs-cli-types'; +import { isStringLoc, isWholeFileLoc, isLineColumnLoc } from '../bqrs-utils'; interface Props { - value: ResultValue; + value: ColumnValue; databaseUri: string; } export default function RawTableValue(props: Props): JSX.Element { const v = props.value; - if (typeof v === 'string') { + if (typeof v === 'string' + || typeof v === 'number' + || typeof v === 'boolean') { return {v}; } - else if ('uri' in v) { - return {v.uri}; - } - else { - return renderLocation(v.location, v.label, props.databaseUri); + + const loc = v.url; + if (!loc) { + return ; + } else if (isStringLoc(loc)) { + return {loc}; + } else if (isWholeFileLoc(loc) || isLineColumnLoc(loc)) { + return renderLocation(loc, v.label, props.databaseUri); + } else { + return ; } } diff --git a/extensions/ql-vscode/src/view/alert-table.tsx b/extensions/ql-vscode/src/view/alert-table.tsx index 1c59a84de..32cf6a348 100644 --- a/extensions/ql-vscode/src/view/alert-table.tsx +++ b/extensions/ql-vscode/src/view/alert-table.tsx @@ -2,14 +2,14 @@ import * as path from 'path'; import * as React from 'react'; import * as Sarif from 'sarif'; import * as Keys from '../result-keys'; -import { LocationStyle } from '../bqrs-types'; import * as octicons from './octicons'; import { className, renderLocation, ResultTableProps, zebraStripe, selectableZebraStripe, jumpToLocation, nextSortDirection } from './result-table-utils'; import { onNavigation, NavigationEvent } from './results'; import { PathTableResultSet } from '../interface-types'; -import { parseSarifPlainTextMessage, parseSarifLocation } from '../sarif-utils'; +import { parseSarifPlainTextMessage, parseSarifLocation, isNoLocation } from '../sarif-utils'; import { InterpretedResultsSortColumn, SortDirection, InterpretedResultsSortState } from '../interface-types'; import { vscode } from './vscode-api'; +import { isWholeFileLoc, isLineColumnLoc } from '../bqrs-utils'; export type PathTableProps = ResultTableProps & { resultSet: PathTableResultSet }; export interface PathTableState { @@ -98,20 +98,18 @@ export class PathTable extends React.Component { relatedLocationsById[loc.id!] = loc; } - const result: JSX.Element[] = []; // match things like `[link-text](related-location-id)` const parts = parseSarifPlainTextMessage(msg); - - for (const part of parts) { + return parts.map((part, i) => { if (typeof part === 'string') { - result.push({part} ); + return {part}; } else { const renderedLocation = renderSarifLocationWithText(part.text, relatedLocationsById[part.dest], undefined); - result.push({renderedLocation} ); + return {renderedLocation}; } - } return result; + }); } function renderNonLocation(msg: string | undefined, locationHint: string): JSX.Element | undefined { @@ -131,14 +129,13 @@ export class PathTable extends React.Component { function renderSarifLocationWithText(text: string | undefined, loc: Sarif.Location, pathNodeKey: Keys.PathNode | undefined): JSX.Element | undefined { const parsedLoc = parseSarifLocation(loc, sourceLocationPrefix); - switch (parsedLoc.t) { - case 'NoLocation': - return renderNonLocation(text, parsedLoc.hint); - case LocationStyle.FivePart: - case LocationStyle.WholeFile: - return renderLocation(parsedLoc, text, databaseUri, undefined, updateSelectionCallback(pathNodeKey)); + if ('hint' in parsedLoc) { + return renderNonLocation(text, parsedLoc.hint); + } else if (isWholeFileLoc(parsedLoc) || isLineColumnLoc(parsedLoc)) { + return renderLocation(parsedLoc, text, databaseUri, undefined, updateSelectionCallback(pathNodeKey)); + } else { + return undefined; } - return undefined; } /** @@ -147,18 +144,18 @@ export class PathTable extends React.Component { */ function renderSarifLocation(loc: Sarif.Location, pathNodeKey: Keys.PathNode | undefined): JSX.Element | undefined { const parsedLoc = parseSarifLocation(loc, sourceLocationPrefix); - let shortLocation, longLocation: string; - switch (parsedLoc.t) { - case 'NoLocation': - return renderNonLocation('[no location]', parsedLoc.hint); - case LocationStyle.WholeFile: - shortLocation = `${path.basename(parsedLoc.userVisibleFile)}`; - longLocation = `${parsedLoc.userVisibleFile}`; - return renderLocation(parsedLoc, shortLocation, databaseUri, longLocation, updateSelectionCallback(pathNodeKey)); - case LocationStyle.FivePart: - shortLocation = `${path.basename(parsedLoc.userVisibleFile)}:${parsedLoc.lineStart}:${parsedLoc.colStart}`; - longLocation = `${parsedLoc.userVisibleFile}`; - return renderLocation(parsedLoc, shortLocation, databaseUri, longLocation, updateSelectionCallback(pathNodeKey)); + if ('hint' in parsedLoc) { + return renderNonLocation('[no location]', parsedLoc.hint); + } else if (isWholeFileLoc(parsedLoc)) { + const shortLocation = `${path.basename(parsedLoc.userVisibleFile)}`; + const longLocation = `${parsedLoc.userVisibleFile}`; + return renderLocation(parsedLoc, shortLocation, databaseUri, longLocation, updateSelectionCallback(pathNodeKey)); + } else if (isLineColumnLoc(parsedLoc)) { + const shortLocation = `${path.basename(parsedLoc.userVisibleFile)}:${parsedLoc.startLine}:${parsedLoc.startColumn}`; + const longLocation = `${parsedLoc.userVisibleFile}`; + return renderLocation(parsedLoc, shortLocation, databaseUri, longLocation, updateSelectionCallback(pathNodeKey)); + } else { + return undefined; } } @@ -290,10 +287,14 @@ export class PathTable extends React.Component { if (nextIndex < 0 || nextIndex >= path.locations.length) return prevState; const sarifLoc = path.locations[nextIndex].location; - if (sarifLoc === undefined) return prevState; + if (sarifLoc === undefined) { + return prevState; + } const loc = parseSarifLocation(sarifLoc, this.props.resultSet.sourceLocationPrefix); - if (loc.t === 'NoLocation') return prevState; + if (isNoLocation(loc)) { + return prevState; + } jumpToLocation(loc, this.props.databaseUri); const newSelection = { ...selectedPathNode, pathNodeIndex: nextIndex }; diff --git a/extensions/ql-vscode/src/view/raw-results-table.tsx b/extensions/ql-vscode/src/view/raw-results-table.tsx index 27b22092d..d2cce964e 100644 --- a/extensions/ql-vscode/src/view/raw-results-table.tsx +++ b/extensions/ql-vscode/src/view/raw-results-table.tsx @@ -4,7 +4,7 @@ import { RAW_RESULTS_LIMIT, RawResultsSortState } from '../interface-types'; import { RawTableResultSet } from '../interface-types'; import RawTableHeader from './RawTableHeader'; import RawTableRow from './RawTableRow'; -import { ResultRow } from '../adapt'; +import { ResultRow } from '../bqrs-cli-types'; export type RawTableProps = ResultTableProps & { resultSet: RawTableResultSet; diff --git a/extensions/ql-vscode/src/view/result-table-utils.tsx b/extensions/ql-vscode/src/view/result-table-utils.tsx index 023cac670..d92e0a5f1 100644 --- a/extensions/ql-vscode/src/view/result-table-utils.tsx +++ b/extensions/ql-vscode/src/view/result-table-utils.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { LocationValue, ResolvableLocationValue } from '../bqrs-types'; +import { UrlValue, ResolvableLocationValue } from '../bqrs-cli-types'; import { tryGetResolvableLocation } from '../bqrs-utils'; import { RawResultsSortState, QueryMetadata, SortDirection } from '../interface-types'; import { assertNever } from '../helpers-pure'; @@ -60,7 +60,7 @@ export function jumpToLocation(loc: ResolvableLocationValue, databaseUri: string /** * Render a location as a link which when clicked displays the original location. */ -export function renderLocation(loc: LocationValue | undefined, label: string | undefined, +export function renderLocation(loc: UrlValue | undefined, label: string | undefined, databaseUri: string, title?: string, callback?: () => void): JSX.Element { // If the label was empty, use a placeholder instead, so the link is still clickable. diff --git a/extensions/ql-vscode/src/view/result-tables.tsx b/extensions/ql-vscode/src/view/result-tables.tsx index 8422a56d7..23daa0276 100644 --- a/extensions/ql-vscode/src/view/result-tables.tsx +++ b/extensions/ql-vscode/src/view/result-tables.tsx @@ -11,11 +11,11 @@ import { ALERTS_TABLE_NAME, SELECT_TABLE_NAME, getDefaultResultSetName, + ParsedResultSets } from '../interface-types'; import { PathTable } from './alert-table'; import { RawTable } from './raw-results-table'; import { ResultTableProps, tableSelectionHeaderClassName, toggleDiagnosticsClassName, alertExtrasClassName } from './result-table-utils'; -import { ParsedResultSets } from '../adapt'; import { vscode } from './vscode-api'; @@ -48,7 +48,7 @@ const UPDATING_RESULTS_TEXT_CLASS_NAME = 'vscode-codeql__result-tables-updating- function getResultCount(resultSet: ResultSet): number { switch (resultSet.t) { case 'RawResultSet': - return resultSet.schema.tupleCount; + return resultSet.schema.rows; case 'SarifResultSet': return resultSet.numTotalResults; } @@ -81,7 +81,11 @@ export class ResultTables // unused stubs because a SarifResultSet schema isn't used the // same way as a RawResultSet. Probably should pull `name` field // out. - schema: { name: ALERTS_TABLE_NAME, version: 0, columns: [], tupleCount: 1 }, + schema: { + name: ALERTS_TABLE_NAME, + rows: 1, + columns: [] + }, name: ALERTS_TABLE_NAME, ...this.props.interpretation, }); diff --git a/extensions/ql-vscode/src/view/results.tsx b/extensions/ql-vscode/src/view/results.tsx index 0c2b85c3a..bd53eef07 100644 --- a/extensions/ql-vscode/src/view/results.tsx +++ b/extensions/ql-vscode/src/view/results.tsx @@ -11,12 +11,10 @@ import { QueryMetadata, ResultsPaths, ALERTS_TABLE_NAME, + ParsedResultSets, } from '../interface-types'; import { EventHandlers as EventHandlerList } from './event-handler-list'; import { ResultTables } from './result-tables'; -import { - ParsedResultSets, -} from '../adapt'; import { ResultSet } from '../interface-types'; import { vscode } from './vscode-api'; @@ -113,7 +111,11 @@ class App extends React.Component<{}, ResultsViewState> { resultSet: { t: 'SarifResultSet', name: ALERTS_TABLE_NAME, - schema: { name: ALERTS_TABLE_NAME, version: 0, columns: [], tupleCount: 1 }, + schema: { + name: ALERTS_TABLE_NAME, + rows: 1, + columns: [] + }, ...msg.interpretation, }, selectedTable: ALERTS_TABLE_NAME, @@ -174,7 +176,10 @@ class App extends React.Component<{}, ResultsViewState> { resultsInfo: ResultsInfo ): Promise { const parsedResultSets = resultsInfo.parsedResultSets; - return [{ t: 'RawResultSet', ...parsedResultSets.resultSet }]; + return [{ + ...parsedResultSets.resultSet, + t: (parsedResultSets.resultSet.t ?? 'RawResultSet') as any + }]; } private async loadResults(): Promise { diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/interface-utils.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/interface-utils.test.ts index de4b58d45..fa9ca3a45 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/interface-utils.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/interface-utils.test.ts @@ -9,7 +9,6 @@ import { tryResolveLocation, } from '../../interface-utils'; import { getDefaultResultSetName } from '../../interface-types'; -import { LocationStyle } from '../../bqrs-types'; import { DatabaseItem } from '../../databases'; describe('interface-utils', () => { @@ -73,20 +72,17 @@ describe('interface-utils', () => { describe('resolveWholeFileLocation', () => { it('should resolve a whole file location', () => { const mockDatabaseItem: DatabaseItem = ({ - resolveSourceFile: sinon.stub().returns(vscode.Uri.parse('abc')), + resolveSourceFile: sinon.stub().returns(vscode.Uri.file('abc')), } as unknown) as DatabaseItem; expect( tryResolveLocation( - { - t: LocationStyle.WholeFile, - file: 'hucairz', - }, + 'file://hucairz:1:1:1:1', mockDatabaseItem ) ).to.deep.equal( new vscode.Location( - vscode.Uri.parse('abc'), - new vscode.Range(0, 0, 0, 0) + vscode.Uri.file('abc'), + new vscode.Range(0, 0, 0, 1) ) ); }); @@ -99,12 +95,11 @@ describe('interface-utils', () => { expect( tryResolveLocation( { - t: LocationStyle.FivePart, - colStart: 1, - colEnd: 3, - lineStart: 4, - lineEnd: 5, - file: 'hucairz', + startColumn: 1, + endColumn: 3, + startLine: 4, + endLine: 5, + uri: 'hucairz', }, mockDatabaseItem ) @@ -127,12 +122,11 @@ describe('interface-utils', () => { expect( tryResolveLocation( { - t: LocationStyle.FivePart, - colStart: 1, - colEnd: 3, - lineStart: 4, - lineEnd: 5, - file: '', + startColumn: 1, + endColumn: 3, + startLine: 4, + endLine: 5, + uri: '', }, mockDatabaseItem ) @@ -146,10 +140,7 @@ describe('interface-utils', () => { expect( tryResolveLocation( - { - t: LocationStyle.String, - loc: 'file://hucairz:0:0:0:0' - }, + 'file://hucairz:0:0:0:0', mockDatabaseItem ) ).to.deep.equal( @@ -170,10 +161,7 @@ describe('interface-utils', () => { expect( tryResolveLocation( - { - t: LocationStyle.String, - loc: 'file://hucairz:5:4:3:2' - }, + 'file://hucairz:5:4:3:2', mockDatabaseItem ) ).to.deep.equal( @@ -194,10 +182,7 @@ describe('interface-utils', () => { expect( tryResolveLocation( - { - t: LocationStyle.String, - loc: 'file://hucairz:x:y:z:a' - }, + 'file://hucairz:x:y:z:a', mockDatabaseItem ) ).to.be.undefined; diff --git a/extensions/ql-vscode/test/pure-tests/location.test.ts b/extensions/ql-vscode/test/pure-tests/location.test.ts index aecf3352e..992eb9d02 100644 --- a/extensions/ql-vscode/test/pure-tests/location.test.ts +++ b/extensions/ql-vscode/test/pure-tests/location.test.ts @@ -1,46 +1,33 @@ import { expect } from 'chai'; import 'mocha'; -import { LocationStyle, StringLocation } from '../../src/bqrs-types'; import { tryGetResolvableLocation } from '../../src/bqrs-utils'; -describe('processing string locations', function () { - it('should detect Windows whole-file locations', function () { - const loc: StringLocation = { - t: LocationStyle.String, - loc: 'file://C:/path/to/file.ext:0:0:0:0' - }; +describe('processing string locations', function() { + it('should detect Windows whole-file locations', function() { + const loc = 'file://C:/path/to/file.ext:0:0:0:0'; const wholeFileLoc = tryGetResolvableLocation(loc); - expect(wholeFileLoc).to.eql({t: LocationStyle.WholeFile, file: 'C:/path/to/file.ext'}); + expect(wholeFileLoc).to.eql({ uri: 'C:/path/to/file.ext' }); }); - it('should detect Unix whole-file locations', function () { - const loc: StringLocation = { - t: LocationStyle.String, - loc: 'file:///path/to/file.ext:0:0:0:0' - }; + it('should detect Unix whole-file locations', function() { + const loc = 'file:///path/to/file.ext:0:0:0:0'; const wholeFileLoc = tryGetResolvableLocation(loc); - expect(wholeFileLoc).to.eql({t: LocationStyle.WholeFile, file: '/path/to/file.ext'}); + expect(wholeFileLoc).to.eql({ uri: '/path/to/file.ext' }); }); - it('should detect Unix 5-part locations', function () { - const loc: StringLocation = { - t: LocationStyle.String, - loc: 'file:///path/to/file.ext:1:2:3:4' - }; + + it('should detect Unix 5-part locations', function() { + const loc = 'file:///path/to/file.ext:1:2:3:4'; const wholeFileLoc = tryGetResolvableLocation(loc); expect(wholeFileLoc).to.eql({ - t: LocationStyle.FivePart, - file: '/path/to/file.ext', - lineStart: 1, - colStart: 2, - lineEnd: 3, - colEnd: 4 + uri: '/path/to/file.ext', + startLine: 1, + startColumn: 2, + endLine: 3, + endColumn: 4 }); }); - it('should ignore other string locations', function () { + it('should ignore other string locations', function() { for (const loc of ['file:///path/to/file.ext', 'I am not a location']) { - const wholeFileLoc = tryGetResolvableLocation({ - t: LocationStyle.String, - loc: loc - }); + const wholeFileLoc = tryGetResolvableLocation(loc); expect(wholeFileLoc).to.be.undefined; } });