Fix AST viewer for refactored language packs #939
Conversation
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
added
bug
|
@aeisenberg I've tested all of the relevant CLI/language pack combinations manually. Is there an easy way to automate such testing? |
| @@ -775,11 +775,15 @@ export class CodeQLCliServer implements Disposable { | |||
| * the default CLI search path is used. | |||
| * @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[]> { | |||
There was a problem hiding this comment.
Why wasn't this async before?
There was a problem hiding this comment.
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.
| include: { | ||
| kind: kindOfKeyType(keyType), | ||
| 'tags contain': tagOfKeyType(keyType) | ||
| const queries = await cli.resolveQueriesInSuite(suiteFile, helpers.getOnDiskWorkspaceFolders()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| let blameCli: boolean; | ||
|
|
||
| if (cliCanHandleLibraryPack) { | ||
| // The CLI can handle both library packs and query packs, so search both packs in order. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
See above. Even the two-call case is only for a specific combination of CLI/pack versions.
|
Hmmm...getting some test failures. Looks like there's a new field that should be mocked. Let me see what the fix is. |
aeisenberg
force-pushed
the
dbartol/ast-viewer-refactored-packs
branch
from
efc0d8b
to
4c1689f
Compare
Test that old CLIs properly ignore the library packs.
aeisenberg
force-pushed
the
dbartol/ast-viewer-refactored-packs
branch
from
4c1689f
to
f3136a0
Compare
|
Fixed the unit test and added a new one. Feel free to merge when all workflows pass. |
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 querieswhen the extension tries to search the library pack for contextual queries. This change makes two related fixes:This change depends on CLI 2.6.1, which is being released soon, adding the
--allow-library-packsoption tocodeql resolve queries. That PR is already open against the CLI.Replace this with a description of the changes your pull request makes.
Checklist
@github/docs-content-codeqlhas been cc'd in all issues for UI or other user-facing changes made by this pull request.