Fix AST viewer for refactored language packs by dbartol · Pull Request #939 · github/vscode-codeql

dbartol · 2021-09-01T23:25:47Z

Most of the languages have recently been refactored into separate library and query packs, with the contextual queries defined in the query pack. In the near future, these contextual queries will move to the library pack.

Current CLI releases throw an error in codeql resolve queries when the extension tries to search the library pack for contextual queries. This change makes two related fixes:

If the queries are not found in the library pack, it then scans the corresponding standard query pack as a fallback.
It detects the problematic combination of CLI and packs, and avoids scanning the library pack at all in those cases. If no queries are found in the problematic scenario, the error message instructs the user to upgrade to the latest CLI version, instead of claiming that the language simply doesn't support the contextual queries yet.

This change depends on CLI 2.6.1, which is being released soon, adding the --allow-library-packs option to codeql resolve queries. That PR is already open against the CLI.

Replace this with a description of the changes your pull request makes.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
@github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.

Most of the languages have recently been refactored into separate library and query packs, with the contextual queries defined in the query pack. In the near future, these contextual queries will move to the library pack. Current CLI releases throw an error in `codeql resolve queries` when the extension tries to search the library pack for contextual queries. This change makes two related fixes: 1. If the queries are not found in the library pack, it then scans the corresponding standard query pack as a fallback. 2. It detects the problematic combination of CLI and packs, and avoids scanning the library pack at all in those cases. If no queries are found in the problematic scenario, the error message instructs the user to upgrade to the latest CLI version, instead of claiming that the language simply doesn't support the contextual queries yet. This change depends on CLI 2.6.1, which is being released soon, adding the `--allow-library-packs` option to `codeql resolve queries`. That PR is already open against the CLI.

dbartol · 2021-09-01T23:29:21Z

@aeisenberg I've tested all of the relevant CLI/language pack combinations manually. Is there an easy way to automate such testing?

aeisenberg

Nice and clever. I have a handful of comments, but none of them are really blocking this from going in. I am not really concerned about calling the CLI multiple times since it is only atmost twice.

extensions/ql-vscode/CHANGELOG.md

aeisenberg · 2021-09-02T00:49:34Z

extensions/ql-vscode/src/cli.ts

   * @returns A list of query files found.
   */
-  resolveQueriesInSuite(suite: string, additionalPacks: string[], searchPath?: string[]): Promise<string[]> {
+  async resolveQueriesInSuite(suite: string, additionalPacks: string[], searchPath?: string[]): Promise<string[]> {


Why wasn't this async before?

Previously, its only return statement was returning an appropriately-typed promise from a function call, so there was no need to use await inside the implementation.

aeisenberg · 2021-09-02T00:56:48Z

extensions/ql-vscode/src/contextual/queryResolver.ts

-    include: {
-      kind: kindOfKeyType(keyType),
-      'tags contain': tagOfKeyType(keyType)
+    const queries = await cli.resolveQueriesInSuite(suiteFile, helpers.getOnDiskWorkspaceFolders());


I'm concerned about calling the CLI in a loop. Each call is a few seconds since it needs to restart the vm. If it's only going to be called 2 or 3 times for each AST Viewer call, maybe that's fine.

Alternatively, could you combine all the qlpacks into one suite? It would look like this:

const allPacks = [ { qlpack: 'codeql/cpp-queries', include: { ... } }, { qlpack: 'codeql/cpp-all', include: { ... } } ];

And then you can just send this suite directly to the CLI. I think it would work.

We could use the CodeQL CLI server (codeql execute cli-server) . Probably not in this pull request, but it could be something to consider in a future refactoring.

Actually, did a little bit of digging here and we are using the cli server for all calls to the cli. The queries are handled by the query server and other commands by the cli server.

This means that it is less important to combine these pack references into a single suite.

Once the packs are all properly refactored, we'll always find the queries in the dbscheme library pack, so we'll never search the query pack. The only situation where we'll search both packs is with a >=2.6.1 CLI and packs that were released at the same time as 2.6.1. By the next dist upgrade, the contextual queries will already be in the library pack.

aeisenberg · 2021-09-02T00:58:08Z

extensions/ql-vscode/src/contextual/queryResolver.ts

+  let blameCli: boolean;
+
+  if (cliCanHandleLibraryPack) {
+    // The CLI can handle both library packs and query packs, so search both packs in order.


Ah...so it's only ever 2 calls to the CLI for resolve queries. Still might make sense to combine them into a single suite.

See above. Even the two-call case is only for a specific combination of CLI/pack versions.

extensions/ql-vscode/src/helpers.ts

aeisenberg · 2021-09-02T15:48:09Z

Hmmm...getting some test failures. Looks like there's a new field that should be mocked.

Let me see what the fix is.

Test that old CLIs properly ignore the library packs.

aeisenberg · 2021-09-02T17:22:32Z

Fixed the unit test and added a new one. Feel free to merge when all workflows pass.

dbartol added bug Something isn't working Complexity: Low A good task for newcomers to learn, or experienced team members to complete quickly. labels Sep 1, 2021

dbartol requested a review from aeisenberg September 1, 2021 23:25

dbartol requested a review from a team as a code owner September 1, 2021 23:25

Add changenote for AST viewer failure fix

6c73170

aeisenberg approved these changes Sep 2, 2021

View reviewed changes

Fix PR feedback

7b7849a

aeisenberg force-pushed the dbartol/ast-viewer-refactored-packs branch from efc0d8b to 4c1689f Compare September 2, 2021 17:00

Fix unit test and add new test

f3136a0

Test that old CLIs properly ignore the library packs.

aeisenberg force-pushed the dbartol/ast-viewer-refactored-packs branch from 4c1689f to f3136a0 Compare September 2, 2021 17:13

aeisenberg enabled auto-merge (rebase) September 2, 2021 18:05

Merge branch 'main' into dbartol/ast-viewer-refactored-packs

1fc2592

aeisenberg merged commit 71f374d into main Sep 2, 2021

aeisenberg deleted the dbartol/ast-viewer-refactored-packs branch September 2, 2021 18:14

Conversation

dbartol commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

dbartol commented Sep 1, 2021

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeisenberg commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeisenberg commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dbartol commented Sep 1, 2021 •

edited

Loading

aeisenberg commented Sep 2, 2021 •

edited

Loading