Implement basic CodeQL debug adapter

[dbartol]

This PR is the initial implementation of a Debug Adapter Protocol-based debug engine for CodeQL. Implementing the DAP allows us to take advantage of all of VS Code's existing UI around debugging, including configuration via launch.json, the "Start Debugging" and "Start Without Debugging" commands, and the Debug Console. Later, we'll build further UI on top of this work, and perhaps add some advanced debugging features.

For now, our DAP implementation runs in the extension process, and invokes the query server via the same code that we were already using for "Run Query" and other local query commands. The existing commands do not go through the debug adapter yet. Instead, I've added a few new commands under the canary flag, and I've enabled the regular VS Code "Start Debugging" (F5) and "Start Without Debugging" (Ctrl-F5) commands to work for CodeQL.

Configuration

In "launch.json", there is a new available debug configuration type, named "codeql". Its configuration options are as follows:

• query - The path to the query to debug. This query will be treated as the root query of the compilation, even when doing quick eval. Thus, the quick eval selection can now be in a different file from the actual query, allowing debugging of queries that extend abstract class, instantiate parameterized modules, etc. If query is undefined, the currently active ".ql" file will be used, just like the "Run Query" command.
• database - The path to the target database directory. If database is undefined, the currently selected database from the database panel will be used, just like the "Run Query" command.
• extensionPacks - An array of the pack names (not paths) of the extension packs to use. Each of these packs must be findable along the additionalPacks path. An empty array specifies that no extension packs are to be used. If extensionPacks is undefined, the default depends on the codeQL.useExtensionPacks setting: If true, then all extension packs in the current workspace will be used. If false, no extension packs will be used.
• additionalPacks - An array of paths to search for additional library packs. This is equivalent to the --additional-packs CLI option. If additionalPacks is undefined, the default is to use the list of root folders in the current workspace, as we already do for the other commands.
• quickEval - A boolean value to specify whether to perform a quickeval of the current selection instead of a regular query evaluation. Users will rarely set this explicitly in "launch.json". It exists primarily so that the new "CodeQL: Debug Selection" command can force it to true.

Commands

The "CodeQL: Run Query" and "CodeQL: Quick Evaluation" commands work exactly as before. They ignore any debug configuration, and do not start a debug session. Later, once we're more confident in the debugger UX, we'll make all of those commands go through the debug configuration.

If a "codeql" debug configuration is selected on the Debug panel, hitting F5 ("Start Debugging") will launch a debug session with the active debug configuration. This will bring up the VS Code debug toolbar, and will evaluate the configured query against the configured database. Results are displayed in the results view as before, and the query history panel is updated. The most visible change is that all of the query server log output will now go to the Debug Console. The Debug Console is specific to that debug session, so you don't have to search through the query server output pane to make sure you're looking at the right query run.

The debug session will stay active after evaluation is complete. It can be terminated by clicking the stop (red square) button on the debug toolbar, or hitting Shift-F5.

Hitting Ctrl-F5 ("Start Without Debuggin") with a "codeql" debug configuration active will evaluate the query, display the results, and immediately terminate the debug session. This is most similar to the behavior of the existing "Run Query" command.

While a "codeql" debug session is active, the "CodeQL: Debug Selection" command will perform a quickeval of the current selection, in the context of the active debug session. If no debug session is active, but a "codeql" debug configuration is selected in the Debug panel, then this command launches a new debug session with that debug configuration to perform the quickeval.

While a "codeql" debug session is active, the "Continue" (F5) command will re-evaluate the entire query.

The "Step Into", "Steo Out", and "Step Over" commands don't do anything (yet).

Code Changes

package.json has been updated to provide the "codeql" debug configuration type. Note that this is available even without the canary flag, but any attempt to start a CodeQL debug session will display an error stating that the feature is not yet available. It also adds the new commands.

debug-configuration.ts resolves the debug configuration from "launch.json" to the actual launch parameters needed by the debug engine. VS Code calls the resolveDebugConfiguration() function to fill in defaults before any ${...} variables are resolved, and then calls resolveDebugConfigurationWithSubstitutedVariables() to give us a chance to finalize the launch configuration after variable substitution. It is in the latter function that we determine the quickeval context if needed, and validate that the user has specified a query and a database.

debug-protocol.ts defines a few custom messages we've added beyond the standard DAP messages. We've added one new request (to run a quickeval with a specific selection), and two events (one at the start of evaluation that reports the output directory and quickeval context, and one at the end of evaluation that reports the evaluation time and any error message).

debug-session.ts is the actual implementation of the DAP, using a helper library provided by VS Code. This is actually pretty simple, since it just invokes the query server to do the heavy lifting.

debugger-factory.ts implements the factory that creates DebugSession objects when VS Code starts a new debug session.

debugger-ui.ts implements the new debugger commands, and handles the display of results after each successful evaluation.

local-databases-ui.ts doesn't really do anything new, but I did have to update how it handles progress when prompting the user to select a database. Before, it assumed that it always had a progress callback provided by the invoking command. However, when invoked during debug launch to fill in a missing database property on the configuration, it needs to create its own progress bar if it has to load a database.

local-databases.ts now separates the creation of a DatabaseItem from the registration of a DatabaseItem in the list of available databases. This allows a debug configuration to specify a path to a database that might not be present in the database manager for the current workspace.

local-queries.ts and run-queries-shared.ts got a bit of refactoring around how we compute query configuration. I've split up determineSelectedQuery() so that each configuration property (query, quickeval, etc.) can be computed independently. I also removed the assumption that the quickeval selection is in the same document as the actual query being compiled.

Tests

The new debugger tests I've added exercise the new commands, and validate that we evaluation the correct files and selections, and get the correct number of results. This is all done via a DebugController class that turns the event-driven style of the debug adapter into an async/await based sequential style. The test issues debugger commands, then asserts that they caused the expected events.

I've also added new flavors to the existing query tests to perform the same evaluation while going through the debugger instead of localQueries. This also uses the DebugController when using the debugger.

[aeisenberg]

That's a lot of code. I went through it commit by commit, so some of my comments may be outdated because of changes, but they might still be relevant.

I haven't tried this out, but will shortly.

[aeisenberg]

Also resolved from the package cache.

[aeisenberg]

This?

Suggested change

	"when": "editorLangId == ql && resourceExtname == .ql && !inDebugMode"
	"when": "config.codeQL.canary && editorLangId == ql && resourceExtname == .ql && !inDebugMode"

[koesie10]

This is an existing command, so wouldn't we still want to keep this available?

[dbartol]

Yeah, that's the existing "CodeQL: Run Query" command. I just changed the when clause to disable it when we're already debugging. Otherwise, the user is likely to select "Run Query", which will run the query outside the debugger and without debug context, rather than just "Continue".

Once we have the basic debugger support merged, we'll have to take a careful look at the UX around the commands, configuration, etc. What I've got now is intended to be the easiest-to-implement way to enable the new features, not necessarily the best UX.

[aeisenberg]

Minor: this is the canonical way to do the same thing:

Suggested change

	...qlConfiguration,
	query: qlConfiguration.query ?? "${file}",
	database: qlConfiguration.database ?? "${command:currentDatabase}",
	query: "${file}",
	database: "${command:currentDatabase}",
	...qlConfiguration,

[dbartol]

I think that has subtly different behavior. If query was specifically set to undefined, then your suggestion will preserve the undefined value, but mine will overwrite the undefined value with the "${file}" value. I'm pretty sure the current implementation is what we want for this particular case.

[aeisenberg]

Are these lines doing anything?

[dbartol]

They're copying the query and database paths from the input configuration to the result configuration, right?

[aeisenberg]

I think those lines are superfluous since you already have ...qlConfiguration.

$ node
Welcome to Node.js v16.18.1.
Type ".help" for more information.
> const qlConfiguration = { query: "a", database: "b" }
undefined
> newConfig = {
...       ...qlConfiguration,
...       query: qlConfiguration.query,
...       database: qlConfiguration.database,
... }
{ query: 'a', database: 'b' }
> newConfig = { ... qlConfiguration }
{ query: 'a', database: 'b' }

[dbartol]

I think you're looking at an older version of this file. The current version doesn't have the ...qlConfiguration.

[aeisenberg]

I guess here, ! is unavoidable.

[dbartol]

I realized that I was only saving lastResult for the exit code (resultType), so I changed it to just save resultType with a default value.

[aeisenberg]

I see that you're not inheriting and cancellation token. I haven't looked closely, but I would have expected this to be the case.

[dbartol]

The ProgressContext contains both the progress callback and the token from the parent. If there's a parent context, we'll inherit its token. Otherwise, withProgress will create a new one if needed.

[aeisenberg]

We're generally using this style for type casts.

Suggested change

	<CodeQLDebugProtocol.QuickEvalRequest["arguments"]>args,
	args as CodeQLDebugProtocol.QuickEvalRequest["arguments"],

[dbartol]

Fixed

[aeisenberg]

Trying to run locally, but getting this error on startup:

[GitHub.vscode-codeql]: Menu item references a command codeQL.debug.quickEvalContextEditor which is not defined in the 'commands' section.

[aeisenberg]

Fixed by adding this to package.json.

      {
        "command": "codeQL.debug.quickEval",
        "title": "CodeQL: Debug Quick Evaluation"
      },
      {
        "command": "codeQL.debug.quickEvalContextEditor",
        "title": "CodeQL: Debug Quick Evaluation"
      },

But these commands are not yet hooked up to any handlers.

[aeisenberg]

Nice. I have it working. Do you have some easy examples of things that work differently in debug mode than with regular evaluation.

Minor thing: It would be nice if I saw some evidence of which launch configuration gave rise to a query result in the history view. Right now, there's no way to tell if something was debugged or if it was a regular launch. This is something to work on later.

[koesie10]

This is an existing command, so wouldn't we still want to keep this available?

[koesie10]

By using await here, we're saying that we're still in this function (and resolving the configuration) until the user closes the error message. We can use void to show the message and return from this function.

Suggested change

	await showAndLogErrorMessage(
	void showAndLogErrorMessage(

This should probably also be done for all the other calls to showAndLogErrorMessage unless there is a specific reason we need to wait for the user to close the message before continuing.

[dbartol]

Fixed both places in this file, plus one from my previous PR in run-queries.ts that had the same problem. Searching for await showAndLog in the rest of our codebase returns only one more hit, which appears to await it on purpose to consume the result.

[koesie10]

Is there a way to avoid nested ternaries here? I find that these are quite hard to read.

[dbartol]

Done.

[koesie10]

This means that the user needs to restart VS Code when enabling the canary flag. Should we be handling changes to the canary flag here or is this a short-lived feature flag?

[dbartol]

It turns out that I can just remove the canary check here. There's no way to conditionally register a new debugger type, so the "codeql" debugger type is always registered (by package.json). Instead, there's code to test the canary flag and show an error if the user manages to create a "codeql" debug configuration and try to launch it. Since the debug session will fail to start, there's no harm in registering the debug adapter tracker unconditionally.

[koesie10]

Can we move the debugger integration tests to a debugger subdirectory to make the separation clear?

[dbartol]

Done

[dbartol]

Since there were a lot of comments about the various !s needed in the debug session code to handle properties that are only defined in certain states, I took another approach and factored that evaluation and related resource management out into a RunningQuery class, similar in spirit to the CoreQueryRun class except that it also handles the debugger-specific cancellation token source, logging, and progress. Now, the only state-dependent property on the session is runningQuery, which seems to make the code a bit simpler. I've resolved all of the !-related comments as no longer relevant, but of course please do comment on the new approach.

[dbartol]

Nice. I have it working. Do you have some easy examples of things that work differently in debug mode than with regular evaluation.

Everything should work the same for now, except that using the debugger mode will respect configuration from launch.json. So, if you edit your launch.json to add, say, database: or query: paths, you'll use those instead of the current database and current file.

The main thing that works differently because of configuration is QuickEval in a .qll file. If a query adds additional overrides of an abstract class, the QuickEval will pick them up in debug mode. One of the debugger tests tests this case specifically.

Oh, and the query server output will be routed to the Debug Console, rather than having to find the query server output pane in the dropdown of the Output tab.

[dbartol]

OK, I think I've addressed all of the feedback from both @aeisenberg and @koesie10.

[aeisenberg]

I think those lines are superfluous since you already have ...qlConfiguration.

$ node
Welcome to Node.js v16.18.1.
Type ".help" for more information.
> const qlConfiguration = { query: "a", database: "b" }
undefined
> newConfig = {
...       ...qlConfiguration,
...       query: qlConfiguration.query,
...       database: qlConfiguration.database,
... }
{ query: 'a', database: 'b' }
> newConfig = { ... qlConfiguration }
{ query: 'a', database: 'b' }

[aeisenberg]

Curious...why extends Exclude<any, any[]>?

[dbartol]

Otherwise makeArray<number[]>([ 5, 6, 7 ]) wouldn't know whether to return the input or wrap it in an array without some deeper runtime type inspection.

[aeisenberg]

Do you think we should be canceling the RunningQuery when it is disposed? It might be that the RunningQuery is disposed, but the query itself is still running. This can cause flakes in tests.

[dbartol]

See below

[aeisenberg]

Oh....I see you did here exactly what I suggested above. Can you change this so this.runningQuery is disposed here and in the RunningQuery class itself, the dispose method calls cancel?

[dbartol]

I'm not sure it's OK to dispose the token source until we're sure the cancellation has actually been observed by the running query. The way I've implemented it, the running query (and its token source) are disposed after query completes (even if cancelled), and the debug session just "lets go" of the running query.

[dbartol]

@aeisenberg I've fixed the zombie evaluation issue we discussed in a151ade.

I just null out this.runningQuery when we do a forcible disconnect, and then stop sending progress and log output events when this.runningQuery is undefined.

[aeisenberg]

Nice...let's 🚢