Merge pull request #1253 from github/aeisenberg/codeSnippet-handling

Author: aeisenberg

extensions/ql-vscode/src/remote-queries/sarif-processing.ts

@@ -54,9 +54,9 @@ function extractResultAlerts(
   for (const location of result.locations ?? []) {
     const physicalLocation = location.physicalLocation!;
     const filePath = physicalLocation.artifactLocation!.uri!;
-    const codeSnippet = getCodeSnippet(physicalLocation.contextRegion!);
+    const codeSnippet = getCodeSnippet(physicalLocation.contextRegion, physicalLocation.region);
     const highlightedRegion = physicalLocation.region
-      ? getHighlightedRegion(physicalLocation.region!)
+      ? getHighlightedRegion(physicalLocation.region)
       : undefined;
 
     const analysisAlert: AnalysisAlert = {
@@ -156,15 +156,21 @@ export function tryGetRule(
   return undefined;
 }
 
-function getCodeSnippet(region: sarif.Region): CodeSnippet {
-  const text = region.snippet!.text!;
+function getCodeSnippet(region?: sarif.Region, alternateRegion?: sarif.Region): CodeSnippet | undefined {
+  region = region ?? alternateRegion;
+
+  if (!region) {
+    return undefined;
+  }
+
+  const text = region.snippet?.text || '';
   const { startLine, endLine } = parseSarifRegion(region);
 
   return {
     startLine,
     endLine,
     text
-  } as CodeSnippet;
+  };
 }
 
 function getHighlightedRegion(region: sarif.Region): HighlightedRegion {
@@ -175,7 +181,7 @@ function getHighlightedRegion(region: sarif.Region): HighlightedRegion {
     startColumn,
     endLine,
 
-    // parseSarifRegion currently shifts the end column by 1 to account 
+    // parseSarifRegion currently shifts the end column by 1 to account
     // for the way vscode counts columns so we need to shift it back.
     endColumn: endColumn + 1
   };
@@ -195,7 +201,7 @@ function getCodeFlows(
         for (const threadFlowLocation of threadFlow.locations) {
           const physicalLocation = threadFlowLocation!.location!.physicalLocation!;
           const filePath = physicalLocation!.artifactLocation!.uri!;
-          const codeSnippet = getCodeSnippet(physicalLocation.contextRegion!);
+          const codeSnippet = getCodeSnippet(physicalLocation.contextRegion, physicalLocation.region);
           const highlightedRegion = physicalLocation.region
             ? getHighlightedRegion(physicalLocation.region)
             : undefined;

extensions/ql-vscode/src/remote-queries/shared/analysis-result.ts

@@ -21,7 +21,7 @@ export interface AnalysisAlert {
   shortDescription: string;
   severity: ResultSeverity;
   fileLink: FileLink;
-  codeSnippet: CodeSnippet;
+  codeSnippet?: CodeSnippet;
   highlightedRegion?: HighlightedRegion;
   codeFlows: CodeFlow[];
 }

extensions/ql-vscode/src/remote-queries/view/FileCodeSnippet.tsx

@@ -180,23 +180,33 @@ const FileCodeSnippet = ({
   messageChildren,
 }: {
   fileLink: FileLink,
-  codeSnippet: CodeSnippet,
+  codeSnippet?: CodeSnippet,
   highlightedRegion?: HighlightedRegion,
   severity?: ResultSeverity,
   message?: AnalysisMessage,
   messageChildren?: React.ReactNode,
 }) => {
 
-  const code = codeSnippet.text.split('\n');
-
-  const startingLine = codeSnippet.startLine;
-  const endingLine = codeSnippet.endLine;
+  const startingLine = codeSnippet?.startLine || 0;
+  const endingLine = codeSnippet?.endLine || 0;
 
   const titleFileUri = createRemoteFileRef(
     fileLink,
     startingLine,
     endingLine);
 
+  if (!codeSnippet) {
+    return (
+      <Container>
+        <TitleContainer>
+          <Link href={titleFileUri}>{fileLink.filePath}</Link>
+        </TitleContainer>
+      </Container>
+    );
+  }
+
+  const code = codeSnippet.text.split('\n');
+
   return (
     <Container>
       <TitleContainer>

extensions/ql-vscode/test/pure-tests/sarif-processing.test.ts

@@ -419,15 +419,42 @@ describe('SARIF processing', () => {
       expectResultParsingError(result.errors[0]);
     });
 
-    it('should return errors for result locations with no context region', () => {
+    it('should not return errors for result locations with no snippet', () => {
+      const sarif = buildValidSarifLog();
+      sarif.runs![0]!.results![0]!.locations![0]!.physicalLocation!.contextRegion!.snippet = undefined;
+
+      const result = extractAnalysisAlerts(sarif, fakefileLinkPrefix);
+
+      const expectedCodeSnippet = {
+        startLine: result.alerts[0].codeSnippet!.startLine,
+        endLine: result.alerts[0].codeSnippet!.endLine,
+        text: ''
+      };
+
+      const actualCodeSnippet = result.alerts[0].codeSnippet;
+
+      expect(result).to.be.ok;
+      expectNoParsingError(result);
+      expect(actualCodeSnippet).to.deep.equal(expectedCodeSnippet);
+    });
+
+    it('should use highlightedRegion for result locations with no contextRegion', () => {
       const sarif = buildValidSarifLog();
       sarif.runs![0]!.results![0]!.locations![0]!.physicalLocation!.contextRegion = undefined;
 
       const result = extractAnalysisAlerts(sarif, fakefileLinkPrefix);
 
+      const expectedCodeSnippet = {
+        startLine: result.alerts[0].highlightedRegion!.startLine,
+        endLine: result.alerts[0].highlightedRegion!.endLine,
+        text: ''
+      };
+
+      const actualCodeSnippet = result.alerts[0].codeSnippet;
+
       expect(result).to.be.ok;
-      expect(result.errors.length).to.equal(1);
-      expectResultParsingError(result.errors[0]);
+      expectNoParsingError(result);
+      expect(actualCodeSnippet).to.deep.equal(expectedCodeSnippet);
     });
 
     it('should not return errors for result locations with no region', () => {
@@ -438,6 +465,7 @@ describe('SARIF processing', () => {
 
       expect(result).to.be.ok;
       expect(result.alerts.length).to.equal(1);
+      expectNoParsingError(result);
     });
 
     it('should return errors for result locations with no physical location', () => {
@@ -537,9 +565,9 @@ describe('SARIF processing', () => {
       expect(result).to.be.ok;
       expect(result.errors.length).to.equal(0);
       expect(result.alerts.length).to.equal(3);
-      expect(result.alerts.find(a => getMessageText(a.message) === 'msg1' && a.codeSnippet.text === 'foo')).to.be.ok;
-      expect(result.alerts.find(a => getMessageText(a.message) === 'msg1' && a.codeSnippet.text === 'bar')).to.be.ok;
-      expect(result.alerts.find(a => getMessageText(a.message) === 'msg2' && a.codeSnippet.text === 'baz')).to.be.ok;
+      expect(result.alerts.find(a => getMessageText(a.message) === 'msg1' && a.codeSnippet!.text === 'foo')).to.be.ok;
+      expect(result.alerts.find(a => getMessageText(a.message) === 'msg1' && a.codeSnippet!.text === 'bar')).to.be.ok;
+      expect(result.alerts.find(a => getMessageText(a.message) === 'msg2' && a.codeSnippet!.text === 'baz')).to.be.ok;
       expect(result.alerts.every(a => a.severity === 'Warning')).to.be.true;
     });
 
@@ -595,9 +623,14 @@ describe('SARIF processing', () => {
     expect(msg.startsWith('Error when processing SARIF result')).to.be.true;
   }
 
+  function expectNoParsingError(result: { errors: string[] }) {
+    const array = result.errors;
+    expect(array.length, array.join()).to.equal(0);
+  }
+
   function buildValidSarifLog(): sarif.Log {
     return {
-      version: '0.0.1' as sarif.Log.version,
+      version: '2.1.0',
       runs: [
         {
           results: [