QL debug in VSCode: switching dbs has no effect.

[aschackmull]

Describe the bug
If I enter debug mode for a query in order to be able to quick-eval inside a shared qlpack and I then select a different db then querying still happens on the prior db

Version
CodeQL extension version: 1.8.8
CodeQL CLI version: 2.14.0
Platform: darwin x64

To reproduce

1. Go to a .ql file
2. Right-click -> "CodeQL: Debug Query"
3. Switch to a different database
4. Right-click -> "CodeQL: Debug Selection" on any predicate

Expected behavior
I'd expect querying to always happen on the currently selected database.

Additional context

[aeisenberg]

@dbartol, is this something that you've seen before?

[hvitved]

I have experienced the same thing, and I think this is a pretty big footgun.

[dbartol]

This is expected in the current design. However, I see how this is confusing, so let's discuss how we could change this. I'll give some background and ideas below:

The original idea was that a "debug session" consists of running a specific query on a specific database. Both the query and the database are specified in launch.json, but if not explicitly set there, the query defaults to the currently active editor, and the database defaults to the currently selected database. Once the session start, though, neither of those change. Part of the rationale for that was that it would make it easier to implement more advanced stateful debugging features, like "stepping through" QL code. Another part of the rationale was just that that's how debugging for other languages tends to work, at least if you think of a QL query as a program with a command-line argument to specify the input database.

I'm not sure either of those justifications holds up, though. First, it's only the path to the query file that is immutable during the debug session. You can change the contents of the query file and/or libraries, and we'll pick them up. Other languages sometimes support changing code while the debug session is active ("Edit and Continue", "Hot Reload", etc.), and it's even easier for QL to support this because we effectively re-compile and re-evaluate the query every time anyway, relying on caches to speed everything up. Dealing with changes to the QL code will make single-stepping harder to implement, but we wouldn't want to give up the ability to tweak the code without having to start stepping from the root of the query again. And clearly, if we can handle the query code itself changing, you would expect us to be able to handle the database changing too.

So, there's no reason we shouldn't support changing the database on the fly, so let's talk about the UX. As I mentioned above, the launch.json file allows the user to specify a specific database to use via a file path, or they can specify ${currentDatabase} (the default). What should happen if the user changes the current database selection while a debug session is active? Options include:

1. Silently ignore the change. This is today's behavior, and it's confusing.
2. Ignore the change, but warn the user. I guess this would be slightly better, because at least the user would know why their results weren't changing. It's still probably not what the user wanted, though.
3. Silently switch the database for all future evaluations in the current session. This is probably what you were expecting to happen.
4. Switch the database for all future evaluations in the current session, but warn the user (with a "Don't show this again" checkbox).
5. Ask the user whether to switch the database for future evaluations in the current session (with a "Don't show this again" checkbox).

If the launch.json file specified ${currentDatabase}, I think any of the above options are viable, but I think I'd we'd prefer either 3, 4, or 5. I assume the vast majority of users would want to switch databases for the active session, because there's not much of a reason to change the current database in the UI otherwise, barring some bizarre scenario involvling multiple active debug sessions.

If the launch.json file specified a specific database path, I'm less sure of the right behavior. It could be the same or different behavior as the ${currentDatabase} case. I think it's more important to have a prompt or warning here than in the ${currentDatabase} case, though.

[dbartol]

Update: Immediately after writing the above comment, I think I realized that I'm thinking about this all wrong. New proposal:

• The "current database" selection in the UI should always match the target database of the active debug session.
• When starting a new debug session whose launch.json specifies ${currentDatabase} (the default), the launched session should use whatever database is currently selected.
• When starting a new debug session whose launch.json specifies a specific database, the current database selection should automatically switch to the specified database.
• Changing the current database selection when a debug session is active should always silently change the target database of the debug session.
• When the last debug session exits, the current database selection should remain on the database that was used by the last debug session when it exited.

Does this sound right?

[aeisenberg]

• Changing the current database selection when a debug session is active should always silently change the target database of the debug session.

I wonder if it there should be a warning that pops up before doing this. I'm not really sure what the expectations around debugging are. If there is any database-specific state that would get lost if you change databases, then issuing a warning makes sense.

[aschackmull]

Debugging in QL currently means running some query in some context. When we're debugging, the query to run is most often selected as a quick-eval selection and the most important piece of context for running that query is the db, which we choose in the database selection (the fact that it can also be chosen via launch.json is mostly irrelevant and I doubt anyone uses that). Now there's one additional piece of context that often needs to be chosen in order to run certain quick-eval queries and that's the ql file that provides module-instantiation context. This is currently the main reason for using the debug feature - and as a side-note it's quite annoying that this causes the quick-eval menu option to be temporarily renamed. There's other pieces of context that we'd like to be able to specify as well: a ql-file only goes so far in choosing module-instantiations, we'd like to be more precise to avoid evaluating all of the instantiations in scope (https://github.com/github/codeql-core/issues/3850). Another note, is that the ql-file is mostly only necessary context when we want to quick-eval inside parameterised modules, so the query that initially runs (the ql-file) is always the wrong query to run, and all debug-sessions therefore start by cancelling the running query as that wasn't what we wanted. I think we should view debugging more as an extension of the current quick-eval, which means that we choose several pieces of context to surround a query (a quick-eval selection), this always includes the db (chosen from the database selection) and sometimes includes a query-scope to define abstract class extensions and module parameterisations.

[hvitved]

Does this sound right?

Sounds good to me.