Merge pull request #2406 from Marcono1234/location-url-column

robertbrignull · web-flow · commit f2c7c4111757 · 2023-07-19T11:01:20.000+01:00
Include column numbers in location URLs
diff --git a/extensions/ql-vscode/CHANGELOG.md b/extensions/ql-vscode/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## [UNRELEASED]
 
+- Links to code on GitHub now include column numbers as well as line numbers. [#2406](https://github.com/github/vscode-codeql/pull/2406)
 - No longer highlight trailing commas for jump to definition.  [#2615](https://github.com/github/vscode-codeql/pull/2615)
 
 ## 1.8.8 - 17 July 2023
diff --git a/extensions/ql-vscode/src/common/bqrs-utils.ts b/extensions/ql-vscode/src/common/bqrs-utils.ts
@@ -142,5 +142,7 @@ export function tryGetRemoteLocation(
     fileLink,
     resolvableLocation.startLine,
     resolvableLocation.endLine,
+    resolvableLocation.startColumn,
+    resolvableLocation.endColumn,
   );
 }
diff --git a/extensions/ql-vscode/src/common/location-link-utils.ts b/extensions/ql-vscode/src/common/location-link-utils.ts
@@ -4,8 +4,20 @@ export function createRemoteFileRef(
   fileLink: FileLink,
   startLine?: number,
   endLine?: number,
+  startColumn?: number,
+  endColumn?: number,
 ): string {
-  if (startLine && endLine && startLine !== endLine) {
+  if (
+    startColumn &&
+    endColumn &&
+    startLine &&
+    endLine &&
+    // Verify that location information is valid; otherwise highlighting might be broken
+    ((startLine === endLine && startColumn < endColumn) || startLine < endLine)
+  ) {
+    // This relies on column highlighting of new code view on GitHub
+    return `${fileLink.fileLinkPrefix}/${fileLink.filePath}#L${startLine}C${startColumn}-L${endLine}C${endColumn}`;
+  } else if (startLine && endLine && startLine < endLine) {
     return `${fileLink.fileLinkPrefix}/${fileLink.filePath}#L${startLine}-L${endLine}`;
   } else if (startLine) {
     return `${fileLink.fileLinkPrefix}/${fileLink.filePath}#L${startLine}`;
diff --git a/extensions/ql-vscode/src/variant-analysis/markdown-generation.ts b/extensions/ql-vscode/src/variant-analysis/markdown-generation.ts
@@ -148,8 +148,7 @@ function generateMarkdownForInterpretedResult(
   lines.push(
     createMarkdownRemoteFileRef(
       interpretedResult.fileLink,
-      interpretedResult.highlightedRegion?.startLine,
-      interpretedResult.highlightedRegion?.endLine,
+      interpretedResult.highlightedRegion,
     ),
   );
   lines.push("");
@@ -250,8 +249,7 @@ function generateMarkdownForAlertMessage(
     } else if (token.t === "location") {
       alertMessage += createMarkdownRemoteFileRef(
         token.location.fileLink,
-        token.location.highlightedRegion?.startLine,
-        token.location.highlightedRegion?.endLine,
+        token.location.highlightedRegion,
         token.text,
       );
     }
@@ -275,8 +273,7 @@ function generateMarkdownForPathResults(
       const threadFlow = codeFlow.threadFlows[i];
       const link = createMarkdownRemoteFileRef(
         threadFlow.fileLink,
-        threadFlow.highlightedRegion?.startLine,
-        threadFlow.highlightedRegion?.endLine,
+        threadFlow.highlightedRegion,
       );
       pathLines.push(`${listNumber}. ${link}`);
 
@@ -361,13 +358,18 @@ function generateMarkdownForRawTableCell(
  */
 export function createMarkdownRemoteFileRef(
   fileLink: FileLink,
-  startLine?: number,
-  endLine?: number,
+  region?: HighlightedRegion,
   linkText?: string,
 ): string {
   const markdownLink = `[${
     linkText || fileLink.filePath
-  }](${createRemoteFileRef(fileLink, startLine, endLine)})`;
+  }](${createRemoteFileRef(
+    fileLink,
+    region?.startLine,
+    region?.endLine,
+    region?.startColumn,
+    region?.endColumn,
+  )})`;
   return markdownLink;
 }
 
diff --git a/extensions/ql-vscode/src/view/common/FileCodeSnippet/CodeSnippetMessage.tsx b/extensions/ql-vscode/src/view/common/FileCodeSnippet/CodeSnippetMessage.tsx
@@ -73,6 +73,8 @@ export const CodeSnippetMessage = ({
                     token.location.fileLink,
                     token.location.highlightedRegion?.startLine,
                     token.location.highlightedRegion?.endLine,
+                    token.location.highlightedRegion?.startColumn,
+                    token.location.highlightedRegion?.endColumn,
                   )}
                 >
                   {token.text}
diff --git a/extensions/ql-vscode/src/view/common/FileCodeSnippet/FileCodeSnippet.tsx b/extensions/ql-vscode/src/view/common/FileCodeSnippet/FileCodeSnippet.tsx
@@ -65,6 +65,8 @@ export const FileCodeSnippet = ({
     fileLink,
     highlightedRegion?.startLine || startingLine,
     highlightedRegion?.endLine || endingLine,
+    highlightedRegion?.startColumn,
+    highlightedRegion?.endColumn,
   );
 
   if (!codeSnippet) {
diff --git a/extensions/ql-vscode/test/unit-tests/common/location.test.ts b/extensions/ql-vscode/test/unit-tests/common/location.test.ts
@@ -87,7 +87,7 @@ describe("getting links to remote (GitHub) locations", () => {
     );
 
     expect(link).toEqual(
-      "https://github.com/owner/repo/blob/sha1234/path/to/file.ext#L194-L237",
+      "https://github.com/owner/repo/blob/sha1234/path/to/file.ext#L194C18-L237C1",
     );
   });
 
@@ -129,7 +129,7 @@ describe("getting links to remote (GitHub) locations", () => {
     );
 
     expect(link).toEqual(
-      "https://github.com/owner/repo/blob/sha1234/path/to/file.ext#L194-L237",
+      "https://github.com/owner/repo/blob/sha1234/path/to/file.ext#L194C18-L237C1",
     );
   });
 });
diff --git a/extensions/ql-vscode/test/unit-tests/data/markdown-generation/interpreted-results/path-problem/expected/result-1-github-codeql.md b/extensions/ql-vscode/test/unit-tests/data/markdown-generation/interpreted-results/path-problem/expected/result-1-github-codeql.md
diff --git a/extensions/ql-vscode/test/unit-tests/data/markdown-generation/interpreted-results/path-problem/expected/result-2-meteor-meteor.md b/extensions/ql-vscode/test/unit-tests/data/markdown-generation/interpreted-results/path-problem/expected/result-2-meteor-meteor.md
@@ -1,6 +1,6 @@
 ### meteor/meteor
 
-[npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259)
+[npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259C28-L259C62)
 
 <pre><code class="javascript">  if (isWindows()) {
     //set for the current session and beyond
@@ -9,46 +9,46 @@
   }
 </code></pre>
 
-*This shell command depends on an uncontrolled [absolute path](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L39).*
+*This shell command depends on an uncontrolled [absolute path](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L39C20-L39C61).*
 
 #### Paths
 
 <details>
 <summary>Path with 11 steps</summary>
 
-1. [npm-packages/meteor-installer/config.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L39)
+1. [npm-packages/meteor-installer/config.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L39C20-L39C61)
    <pre><code class="javascript">
    const meteorLocalFolder = '.meteor';
    const meteorPath = <strong>path.resolve(rootPath, meteorLocalFolder)</strong>;
 
    module.exports = {
    </code></pre>
 
-2. [npm-packages/meteor-installer/config.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L39)
+2. [npm-packages/meteor-installer/config.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L39C7-L39C61)
    <pre><code class="javascript">
    const meteorLocalFolder = '.meteor';
    const <strong>meteorPath = path.resolve(rootPath, meteorLocalFolder)</strong>;
 
    module.exports = {
    </code></pre>
 
-3. [npm-packages/meteor-installer/config.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L44)
+3. [npm-packages/meteor-installer/config.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L44C3-L44C13)
    <pre><code class="javascript">  METEOR_LATEST_VERSION,
      extractPath: rootPath,
      <strong>meteorPath</strong>,
      release: process.env.INSTALL_METEOR_VERSION || METEOR_LATEST_VERSION,
      rootPath,
    </code></pre>
 
-4. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L12)
+4. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L12C3-L12C13)
    <pre><code class="javascript">const os = require('os');
    const {
      <strong>meteorPath</strong>,
      release,
      startedPath,
    </code></pre>
 
-5. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L11-L23)
+5. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L11C7-L23C27)
    <pre><code class="javascript">const tmp = require('tmp');
    const os = require('os');
    const <strong>{</strong>
@@ -68,47 +68,47 @@
    const {
    </code></pre>
 
-6. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259)
+6. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259C42-L259C52)
    <pre><code class="javascript">  if (isWindows()) {
        //set for the current session and beyond
        child_process.execSync(`setx path "${<strong>meteorPath</strong>}/;%path%`);
        return;
      }
    </code></pre>
 
-7. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259)
+7. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259C42-L259C52)
    <pre><code class="javascript">  if (isWindows()) {
        //set for the current session and beyond
        child_process.execSync(`setx path "${<strong>meteorPath</strong>}/;%path%`);
        return;
      }
    </code></pre>
 
-8. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259)
+8. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259C42-L259C52)
    <pre><code class="javascript">  if (isWindows()) {
        //set for the current session and beyond
        child_process.execSync(`setx path "${<strong>meteorPath</strong>}/;%path%`);
        return;
      }
    </code></pre>
 
-9. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259)
+9. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259C42-L259C52)
    <pre><code class="javascript">  if (isWindows()) {
        //set for the current session and beyond
        child_process.execSync(`setx path "${<strong>meteorPath</strong>}/;%path%`);
        return;
      }
    </code></pre>
 
-10. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259)
+10. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259C42-L259C52)
     <pre><code class="javascript">  if (isWindows()) {
         //set for the current session and beyond
         child_process.execSync(`setx path "${<strong>meteorPath</strong>}/;%path%`);
         return;
       }
     </code></pre>
 
-11. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259)
+11. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259C28-L259C62)
     <pre><code class="javascript">  if (isWindows()) {
         //set for the current session and beyond
         child_process.execSync(<strong>`setx path "${meteorPath}/;%path%`</strong>);
@@ -121,15 +121,15 @@
 <details>
 <summary>Path with 2 steps</summary>
 
-1. [npm-packages/meteor-installer/config.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L39)
+1. [npm-packages/meteor-installer/config.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/config.js#L39C20-L39C61)
    <pre><code class="javascript">
    const meteorLocalFolder = '.meteor';
    const meteorPath = <strong>path.resolve(rootPath, meteorLocalFolder)</strong>;
 
    module.exports = {
    </code></pre>
 
-2. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259)
+2. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L259C28-L259C62)
    <pre><code class="javascript">  if (isWindows()) {
        //set for the current session and beyond
        child_process.execSync(<strong>`setx path "${meteorPath}/;%path%`</strong>);
diff --git a/extensions/ql-vscode/test/unit-tests/data/markdown-generation/interpreted-results/problem/expected/result-1-github-codeql.md b/extensions/ql-vscode/test/unit-tests/data/markdown-generation/interpreted-results/problem/expected/result-1-github-codeql.md
@@ -1,6 +1,6 @@
 ### github/codeql
 
-[javascript/extractor/tests/regexp/input/multipart.js](https://github.com/github/codeql/blob/d094bbc06d063d0da8d0303676943c345e61de53/javascript/extractor/tests/regexp/input/multipart.js#L17-L20)
+[javascript/extractor/tests/regexp/input/multipart.js](https://github.com/github/codeql/blob/d094bbc06d063d0da8d0303676943c345e61de53/javascript/extractor/tests/regexp/input/multipart.js#L17C6-L20C6)
 
 <pre><code class="javascript">
 var bad95 = new RegExp(
diff --git a/extensions/ql-vscode/test/unit-tests/data/markdown-generation/interpreted-results/problem/expected/result-2-meteor-meteor.md b/extensions/ql-vscode/test/unit-tests/data/markdown-generation/interpreted-results/problem/expected/result-2-meteor-meteor.md
@@ -1,6 +1,6 @@
 ### meteor/meteor
 
-[packages/deprecated/markdown/showdown.js](https://github.com/meteor/meteor/blob/53f3c4442d3542d3d2a012a854472a0d1bef9d12/packages/deprecated/markdown/showdown.js#L415)
+[packages/deprecated/markdown/showdown.js](https://github.com/meteor/meteor/blob/53f3c4442d3542d3d2a012a854472a0d1bef9d12/packages/deprecated/markdown/showdown.js#L415C41-L415C48)
 
 <pre><code class="javascript">		/g,hashElement);
 	*/
@@ -13,7 +13,7 @@
 
 ----------------------------------------
 
-[packages/deprecated/markdown/showdown.js](https://github.com/meteor/meteor/blob/53f3c4442d3542d3d2a012a854472a0d1bef9d12/packages/deprecated/markdown/showdown.js#L523)
+[packages/deprecated/markdown/showdown.js](https://github.com/meteor/meteor/blob/53f3c4442d3542d3d2a012a854472a0d1bef9d12/packages/deprecated/markdown/showdown.js#L523C58-L523C61)
 
 <pre><code class="javascript">	// Build a regex to find HTML tags and comments.  See Friedl's
 	// "Mastering Regular Expressions", 2nd Ed., pp. 200-201.
@@ -26,7 +26,7 @@
 
 ----------------------------------------
 
-[tools/tests/apps/modules/imports/links/acorn/src/parseutil.js](https://github.com/meteor/meteor/blob/53f3c4442d3542d3d2a012a854472a0d1bef9d12/tools/tests/apps/modules/imports/links/acorn/src/parseutil.js#L9)
+[tools/tests/apps/modules/imports/links/acorn/src/parseutil.js](https://github.com/meteor/meteor/blob/53f3c4442d3542d3d2a012a854472a0d1bef9d12/tools/tests/apps/modules/imports/links/acorn/src/parseutil.js#L9C24-L9C38)
 
 <pre><code class="javascript">// ## Parser utilities
 
@@ -39,15 +39,15 @@ pp.strictDirective = function(start) {
 
 ----------------------------------------
 
-[tools/tests/apps/modules/imports/links/acorn/src/parseutil.js](https://github.com/meteor/meteor/blob/53f3c4442d3542d3d2a012a854472a0d1bef9d12/tools/tests/apps/modules/imports/links/acorn/src/parseutil.js#L9)
+[tools/tests/apps/modules/imports/links/acorn/src/parseutil.js](https://github.com/meteor/meteor/blob/53f3c4442d3542d3d2a012a854472a0d1bef9d12/tools/tests/apps/modules/imports/links/acorn/src/parseutil.js#L9C43-L9C57)
 
 <pre><code class="javascript">const literal = /^(?:'((?:\\.|[^'])*?)'|"(<strong>(?:\\.|[^"])*?</strong>)")/</code></pre>
 
 *This part of the regular expression may cause exponential backtracking on strings starting with '"' and containing many repetitions of '\!'.*
 
 ----------------------------------------
 
-[app/src/main/AndroidManifest.xml](https://github.com/AlexRogalskiy/android-nrf-toolbox/blob/034cf3aa7d2a3a4145177de32546ca518a462a66/app/src/main/AndroidManifest.xml#L239-L249)
+[app/src/main/AndroidManifest.xml](https://github.com/AlexRogalskiy/android-nrf-toolbox/blob/034cf3aa7d2a3a4145177de32546ca518a462a66/app/src/main/AndroidManifest.xml#L239C3-L249C15)
 
 <pre><code class="javascript">		&lt;/service&gt;
 
diff --git a/extensions/ql-vscode/test/unit-tests/data/markdown-generation/raw-results/expected/result-1-github-codeql.md b/extensions/ql-vscode/test/unit-tests/data/markdown-generation/raw-results/expected/result-1-github-codeql.md
diff --git a/extensions/ql-vscode/test/unit-tests/data/markdown-generation/raw-results/expected/result-2-meteor-meteor.md b/extensions/ql-vscode/test/unit-tests/data/markdown-generation/raw-results/expected/result-2-meteor-meteor.md

Original file line number	Diff line number	Diff line change
`@@ -142,5 +142,7 @@ export function tryGetRemoteLocation(`
`142`	`142`	`fileLink,`
`143`	`143`	`resolvableLocation.startLine,`
`144`	`144`	`resolvableLocation.endLine,`
	`145`	`+ resolvableLocation.startColumn,`
	`146`	`+ resolvableLocation.endColumn,`
`145`	`147`	`);`
`146`	`148`	`}`
Original file line number	Diff line number	Diff line change
`@@ -73,6 +73,8 @@ export const CodeSnippetMessage = ({`
`73`	`73`	`token.location.fileLink,`
`74`	`74`	`token.location.highlightedRegion?.startLine,`
`75`	`75`	`token.location.highlightedRegion?.endLine,`
	`76`	`+ token.location.highlightedRegion?.startColumn,`
	`77`	`+ token.location.highlightedRegion?.endColumn,`
`76`	`78`	`)}`
`77`	`79`	`>`
`78`	`80`	`{token.text}`